adding time in calculations of yesterday and today #66
Conversation
Signed-off-by: Vedansh Saini <[email protected]>
Reviewer's Guide by SourceryThis pull request addresses issue #65 by incorporating time information into the date calculations for 'yesterday' and 'today'. The changes ensure that date ranges are accurately calculated by setting specific times (00:00:00 for the start date and 23:59:59 for the end date). Additionally, a filtering mechanism has been added to apply these date ranges to the fetched GitHub data. Sequence diagram for fetching and filtering data by datesequenceDiagram
participant User
participant scrumHelper.js
participant GitHub API
participant filterDataByDate
User->>scrumHelper.js: Initiates data fetch
scrumHelper.js->>GitHub API: Fetches data (issues, PRs, user data)
activate GitHub API
GitHub API-->>scrumHelper.js: Returns data
deactivate GitHub API
scrumHelper.js->>filterDataByDate: Filters data by date range
activate filterDataByDate
filterDataByDate-->>scrumHelper.js: Returns filtered data
deactivate filterDataByDate
scrumHelper.js->>scrumHelper.js: Stores filtered data
scrumHelper.js-->>User: Updates UI with filtered data
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vedansh-5 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a library like Moment.js or date-fns to simplify date manipulation and formatting.
- The duplication in date formatting logic between
getLastWeekandgetTodaycould be extracted into a separate helper function.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Since this is a very small change a helper function is not necessary. |
|
@hongquan This would fix your issue. |
src/scripts/scrumHelper.js
Outdated
| var WeekYear = Week.getFullYear(); | ||
| today.setHours(23, 59, 59, 999); | ||
| // var Week = new Date(today.getFullYear(), today.getMonth(), today.getDate()); | ||
| var WeekMonth = today.getMonth() + 1; |
There was a problem hiding this comment.
Please update the variables name to make them follow the same style (camelCase).
Signed-off-by: Vedansh Saini <[email protected]>
|
@hongquan In the latest commit, I have worked on the changes you requested, implementing camelCase, better readability in functions and date calculations, also I am using a dual approach in time addition, we are adding time at the end of string for the github API to have the complete information and we are using |
There was a problem hiding this comment.
Have you tested?
It seems that your change still have the same bug as before.
The bug comes from ignoring timezone difference.
The new Date() returns the datetime at user timezone, but when you constructing the API request, you assume that the datetime is in UTC.
src/scripts/scrumHelper.js
Outdated
| var WeekDisplayPadded = | ||
| ('0000' + WeekYear.toString()).slice(-4) + | ||
| today.setHours(23, 59, 59, 999); | ||
| var week = new Date(today.getFullYear(), today.getMonth(), today.getDate()); |
There was a problem hiding this comment.
Your variable name is confusing. Its name is week, but the value seems to be a single date.
Thanks for pointing that out, I now see the issue comes from assuming the local timezone while constructing the API req, I will update the implementation to properly convet the datetime. |
Signed-off-by: Vedansh Saini <[email protected]>
|
In the latest changes I have implemented to convert the local time to UTC, but this doesn't work either, because it is checked against GitHub timestamps which are stored in UTC so now, i'll add the code to convert github timestamps to local time |
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
|
@hongquan I made some changes in the code, can you please review it again as of now, the extended UTC+- 14 ensures no data is missed, but it might have overlapping data, I'll refine my filtering logic more, I'd appreciate any feedback from you. Thanks. |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @vedansh-5 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider consolidating the date calculation logic in
src/scripts/scrumHelper.jsandsrc/scripts/main.jsto avoid duplication. - The date filtering logic seems to be duplicated, consider creating a shared utility function.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| today.setHours(23, 59, 59, 999); | ||
|
|
||
| const utc = new Date(today.getTime() + (14 * 60 * 60 * 1000)); |
There was a problem hiding this comment.
suggestion: Review the repetitive pattern of applying a timezone offset.
Both getLastWeek() and getToday() apply the same 14-hour offset adjustment. It might be worthwhile to refactor this repeated logic into a shared helper function for clarity and maintainability.
Suggested implementation:
// Helper function to apply a 14-hour timezone offset and format the date as ISO string
function applyTimezoneOffset(date) {
const offsetInMilliseconds = 14 * 60 * 60 * 1000;
const utcDate = new Date(date.getTime() + offsetInMilliseconds);
return utcDate.toISOString().slice(0, 19) + "Z";
} today.setHours(23, 59, 59, 999);
return applyTimezoneOffset(today);
} You should also update the getLastWeek() function (if it uses the same pattern) in a similar fashion by replacing its 14-hour offset logic with a call to applyTimezoneOffset().
| '-' + | ||
| ('00' + WeekDay.toString()).slice(-2); | ||
| return WeekDisplayPadded; | ||
| var today = new Date(today.getFullYear(), today.getMonth(), today.getDate()); |
There was a problem hiding this comment.
suggestion: Reconsider duplicated date formatting logic in getToday().
getToday() in main.js manually formats the date after adjusting with the offset, while scrumHelper.js uses toISOString slicing. Aligning these approaches (potentially via a common utility function) could improve consistency and reduce potential formatting discrepancies.
|
|
||
| const utc = new Date(lastWeek.getTime() + (14 * 60* 60* 1000)); | ||
|
|
||
| return utc.toISOString().slice(0,19) + "Z"; |
There was a problem hiding this comment.
issue (complexity): Consider extracting the date transformation logic into a shared helper function to avoid repetition and improve code maintainability
Extract a shared helper to handle the date transformation so you don’t repeat the setHours and offset logic. For example:
function formatDateWithOffset(date, isStart) {
if (isStart) {
date.setHours(0, 0, 0, 0);
} else {
date.setHours(23, 59, 59, 999);
}
const offsetHours = 14;
const utc = new Date(date.getTime() + (offsetHours * 60 * 60 * 1000));
return utc.toISOString().slice(0, 19) + "Z";
}Then update your functions as follows:
function getLastWeek() {
const today = new Date();
const daysBack = gsoc === 0 ? 7 : 1;
const lastWeek = new Date(today.getFullYear(), today.getMonth(), today.getDate() - daysBack);
return formatDateWithOffset(lastWeek, true);
}
function getToday() {
const today = new Date();
return formatDateWithOffset(today, false);
}This refactoring reduces duplication while keeping the functionality identical.
There was a problem hiding this comment.
it is used in only two functions, also its a single line, I don't think a function is necessary.
There was a problem hiding this comment.
I'll respond to the comment by the developer:
While I understand your perspective that the code is used in only two functions and appears to be a single line, creating a helper function offers several benefits:
-
Code Readability: The extracted function makes the date transformation logic more explicit and easier to understand at a glance.
-
Maintainability: If the date transformation logic needs to change in the future, you'll only need to modify it in one place.
-
Reusability: Even if it's currently used in just two functions, having a dedicated helper makes it easier to reuse the logic elsewhere if needed.
-
Consistency: The helper ensures that date transformations are handled consistently across different parts of the code.
The proposed refactoring is lightweight and doesn't add unnecessary complexity. It actually simplifies the existing code by:
- Removing manual string formatting
- Centralizing the time zone and offset logic
- Making the intent of the code clearer
I recommend considering the proposed refactoring, but if you feel strongly against it, that's okay too. The most important thing is maintaining clean, understandable code.
Would you like to discuss this further or share your thoughts on the suggested approach?
|
@hongquan I'd really appreciate your review on the changes. Thanks. |
Fixes issue #65
Changes: Added time in calculation logic of yesterday and today, earlier it was not given setting it to default at 00:00 causing the issue.
Summary by Sourcery
Improve date calculation logic to correctly handle time zones and date ranges in GitHub data retrieval
Bug Fixes:
Enhancements: