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

Add affected users chart #383

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add affected users chart #383

wants to merge 5 commits into from

Conversation

gohabereg
Copy link
Member

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2021

Codecov Report

Merging #383 (199c8fa) into master (0e19681) will decrease coverage by 0.60%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #383      +/-   ##
==========================================
- Coverage   54.70%   54.09%   -0.61%     
==========================================
  Files           9        9              
  Lines         117      122       +5     
  Branches        8        7       -1     
==========================================
+ Hits           64       66       +2     
- Misses         53       56       +3     
Impacted Files Coverage Δ
src/utils/dates.ts 27.27% <33.33%> (+3.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e19681...199c8fa. Read the comment docs.

const dayMidnight = getUTCMidnight(day) / 1000;
const groupedEvents = groupedData[`groupingTimestamp:${dayMidnight}`];

now.setHours(0, 0, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

why to not use getUTCMidnight instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

getUTCMidnight sounds like a pure function but in fact it implicitly change passed date. So I think explicit set is better here

Copy link
Member

Choose a reason for hiding this comment

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

so you can rename it with setUTCMidnight. Using a separate method will improve the readability of this code

@@ -296,16 +300,32 @@ class EventsFactory extends Factory {
* Convert UTC midnight to midnights in user's timezone
*/
dailyEvents = dailyEvents.map((item) => {
const groupingTimestamp = new Date(item.groupingTimestamp * 1000);

groupingTimestamp.setTime(groupingTimestamp.getTime() + timezoneOffset * 60 * 1000);
Copy link
Member

Choose a reason for hiding this comment

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

why the getMidnightWithTimezoneOffset util is replaced with such a line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it didn't work as expected and is too complicated.

groupingTimestamp is already UTC midnight, so we just need to recalculate it with timezone offset

Copy link
Member

Choose a reason for hiding this comment

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

looks suspicious. It was working fine and handled edge points with errors that happened near midnight.

Why do you add a timezoneOffset to the Midnight? It won't be midnight after that.

As I can see, the getMidnightWithTimezoneOffset has a comment describing a problem. What exactly does not work as expected?

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.

3 participants