Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
…m/hawk.api.nodejs into feature/affected-users
src/models/eventsFactory.js
Outdated
| const dayMidnight = getUTCMidnight(day) / 1000; | ||
| const groupedEvents = groupedData[`groupingTimestamp:${dayMidnight}`]; | ||
|
|
||
| now.setHours(0, 0, 0, 0); |
There was a problem hiding this comment.
why to not use getUTCMidnight instead?
There was a problem hiding this comment.
getUTCMidnight sounds like a pure function but in fact it implicitly change passed date. So I think explicit set is better here
There was a problem hiding this comment.
so you can rename it with setUTCMidnight. Using a separate method will improve the readability of this code
src/models/eventsFactory.js
Outdated
| dailyEvents = dailyEvents.map((item) => { | ||
| const groupingTimestamp = new Date(item.groupingTimestamp * 1000); | ||
|
|
||
| groupingTimestamp.setTime(groupingTimestamp.getTime() + timezoneOffset * 60 * 1000); |
There was a problem hiding this comment.
why the getMidnightWithTimezoneOffset util is replaced with such a line?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
3 participants
No description provided.