For slack triage notifications, use business hours left to notify hourly #735
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #735 +/- ##
==========================================
+ Coverage 86.13% 86.19% +0.06%
==========================================
Files 116 116
Lines 3217 3231 +14
Branches 655 656 +1
==========================================
+ Hits 2771 2785 +14
Misses 432 432
Partials 14 14 ☔ View full report in Codecov by Sentry. |
src/utils/businessHours.test.ts
Outdated
| const triageBy = '2023-12-22T18:00:00.000Z'; | ||
| const now = moment('2023-12-22T01:00:00.000Z'); | ||
| expect( | ||
| getBusinessHoursLeft( |
There was a problem hiding this comment.
When a function signature is more than ~3 args, I think named parameters are the way to go. In particular, the last 3 arguments here are all the same type (strings), and as someone unfamiliar with the codebase, it's quite hard to figure out what corresponds to what just by reading the invocation.
What if we did:
// At definition-site:
interface BusinessHoursLeft {
triageBy: string,
now: moment.Moment,
repo: string,
org: string,
productArea: string
}
function getBusinessHoursLeft({ triageBy, now, repo, org, productArea }: BusinessHoursLeft {
...
}
// At call-site:
getBusinessHoursLeft({
triageBy,
now,
repo: 'test-ttt-simple',
org: 'getsentry',
productArea: 'Other'
})
Now I know what's what, and can catch errors that the type system can't (ex, swapped string args) more easily when scanning.
There was a problem hiding this comment.
The reason why I did not do this is because it seemed explicit enough what was going on where the function is being called:
const businessHoursLeft = getBusinessHoursLeft(
triageBy,
now,
repo,
org,
productArea
);
This ambiguity would only exist for testing. This is not a super widely used function, so I'd rather not declare an interface for it. I'll clear up what the strings are by giving them names before passing them in if that's helpful.
There was a problem hiding this comment.
Sure, but that relies on the call-site naming its variables properly. If you use an interface, this is required, rather than optional, enforcing more readable code. The language even encourages this practice if you use an interface (you can just type the var name without stuttering), so it feels like a best practice.
In any case, I trust your judgment, just a small nit as a rather unfamiliar reader of the code.
There was a problem hiding this comment.
There are other functions in the file that could use cleaning up too. Creating an issue to get those resolved in a later PR:
#737
| ) | ||
| ).toEqual(1); | ||
| }); | ||
| it('should correctly calculate business hours over multiple days', function () { |
There was a problem hiding this comment.
Maybe add a test for holidays as well?
| ).toEqual(9); | ||
| }); | ||
| it('should correctly account for weekends when calculating business hours', function () { | ||
| const triageBy = '2023-01-17T18:00:00.000Z'; |
There was a problem hiding this comment.
Just curious what happens when we don't include milliseconds (or do include microseconds) in an ISO8601 timestamp. I know that Python's deserializer sometimes barfed on that, which caused trouble and test flakes in the past. Maybe JS's is smarter though.
There was a problem hiding this comment.
this will be fine, since the moment library will parse the timestamp without milliseconds properly
| repo, | ||
| org | ||
| ); | ||
| // Two cases to handle. |
hubertdeng123
deleted the
hubertdeng123/act-fast-issues-use-business-hours
branch
It's a bit annoying when in the morning, a team gets surprised with a slack notification of an issue that is due in 24 minutes. This is because at the end of the day, we're not necessarily notifying teams of issues due the next morning since it is technically over 12 hours away.
Also cleans up some unneeded async/await declarations
Closes this: #719