Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): Add getUTCMonday, fix tests in U.S. timezone #879

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

donmccurdy
Copy link
Member

@donmccurdy donmccurdy commented Jun 13, 2024

Description

Currently the tests do not pass when running locally from a U.S. time zone, fixed in this PR. Existing code in groupByDate.js used UTC time for all groupings, and TimeSeriesWidget uses local time, but both call into getMonday which cannot do both ... so I think it's likely that a UTC-specific version of getMonday is required.

Changes:

  • getMonday: Returns midnight (local time) on the Monday preceeding a given date, in milliseconds since the UNIX epoch.
  • getUTCMonday: Returns midnight (UTC) on the Monday preceeding a given date, in milliseconds since the UNIX epoch.

Copy link

Pull Request Test Coverage Report for Build 9505380995

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 71.245%

Totals Coverage Status
Change from base Build 9485566432: 0.04%
Covered Lines: 2811
Relevant Lines: 3635

💛 - Coveralls

1 similar comment
Copy link

Pull Request Test Coverage Report for Build 9505380995

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 71.245%

Totals Coverage Status
Change from base Build 9485566432: 0.04%
Covered Lines: 2811
Relevant Lines: 3635

💛 - Coveralls

Copy link

github-actions bot commented Jun 13, 2024

Visit the preview URL for this PR (updated for commit 0e81c14):

https://cartodb-fb-storybook-react-dev--pr879-fix-timezone-agn-tzpdwud3.web.app

(expires Fri, 21 Jun 2024 13:34:42 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c

Copy link

Pull Request Test Coverage Report for Build 9505380995

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 71.245%

Totals Coverage Status
Change from base Build 9485566432: 0.04%
Covered Lines: 2811
Relevant Lines: 3635

💛 - Coveralls

Copy link

Pull Request Test Coverage Report for Build 9505453641

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 71.245%

Totals Coverage Status
Change from base Build 9485566432: 0.04%
Covered Lines: 2811
Relevant Lines: 3635

💛 - Coveralls

2 similar comments
Copy link

Pull Request Test Coverage Report for Build 9505453641

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 71.245%

Totals Coverage Status
Change from base Build 9485566432: 0.04%
Covered Lines: 2811
Relevant Lines: 3635

💛 - Coveralls

Copy link

Pull Request Test Coverage Report for Build 9505453641

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 71.245%

Totals Coverage Status
Change from base Build 9485566432: 0.04%
Covered Lines: 2811
Relevant Lines: 3635

💛 - Coveralls

@donmccurdy donmccurdy requested a review from a team June 13, 2024 19:19
Copy link
Contributor

@vmilan vmilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add an entry to the changelog on every PR 🙏

I added for you the last one that was missing, here: https://github.com/CartoDB/carto-react/pull/881/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4ed

Copy link
Contributor

@jmgaya jmgaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May the time be with you, now... 🕐

@donmccurdy donmccurdy force-pushed the fix/timezone-agnostic-week-counts branch from f960e9c to 0e81c14 Compare June 14, 2024 13:29
@donmccurdy
Copy link
Member Author

donmccurdy commented Jun 14, 2024

@vmilan changelog updated, thank you!

Copy link

Pull Request Test Coverage Report for Build 9516971163

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 71.245%

Totals Coverage Status
Change from base Build 9515608873: 0.04%
Covered Lines: 2811
Relevant Lines: 3635

💛 - Coveralls

2 similar comments
Copy link

Pull Request Test Coverage Report for Build 9516971163

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 71.245%

Totals Coverage Status
Change from base Build 9515608873: 0.04%
Covered Lines: 2811
Relevant Lines: 3635

💛 - Coveralls

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 9516971163

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 71.245%

Totals Coverage Status
Change from base Build 9515608873: 0.04%
Covered Lines: 2811
Relevant Lines: 3635

💛 - Coveralls

@donmccurdy donmccurdy merged commit 6a3bcf1 into master Jun 17, 2024
2 checks passed
@donmccurdy donmccurdy deleted the fix/timezone-agnostic-week-counts branch June 17, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants