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 Business hour logic to routing and triage #365

Merged
merged 26 commits into from
Dec 14, 2022

Conversation

hubertdeng123
Copy link
Member

@hubertdeng123 hubertdeng123 commented Dec 8, 2022

This PR adds two functions allows for calculating timeToTriageBy and timeToRouteBy in business hours instead of business days.

@hubertdeng123
Copy link
Member Author

hmm flaky tests that are on main, I'll try to fix that up tomorrow

src/config/index.ts Outdated Show resolved Hide resolved
src/config/index.ts Outdated Show resolved Hide resolved
@@ -182,10 +182,9 @@ describe('issueNotifier Tests', function () {
expect(say).lastCalledWith(
'This channel is set to receive notifications for: Team: Test (no office specified)'
);
expect(await getLabelsTable().where({ channel_id })).toEqual([
expect(await getLabelsTable().where({ channel_id })).toMatchObject([
Copy link
Member Author

Choose a reason for hiding this comment

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

this fixes flaky tests, id here isn't what we care about

Copy link
Contributor

Choose a reason for hiding this comment

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

Good find!

@hubertdeng123 hubertdeng123 marked this pull request as ready for review December 9, 2022 18:18
@hubertdeng123 hubertdeng123 requested a review from a team December 9, 2022 18:18
Copy link
Contributor

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

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

Couple of suggestions for clarifying comments and a suggestion to rename things to be more consistent but overall, well done! Dealing with dates and date math in JS is a pain so I'm impressed!

Copy link
Member

@chadwhitacre chadwhitacre left a comment

Choose a reason for hiding this comment

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

Picked over for understanding, suggested some naming clarifications and logic tighten-ups.

Copy link
Member

@chadwhitacre chadwhitacre left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@chadwhitacre chadwhitacre left a comment

Choose a reason for hiding this comment

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

Ship it!

@hubertdeng123 hubertdeng123 merged commit c0de7a3 into main Dec 14, 2022
@hubertdeng123 hubertdeng123 deleted the hubertdeng123/add-business-hours branch December 14, 2022 17:28
@chadwhitacre chadwhitacre mentioned this pull request Dec 20, 2022
14 tasks
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