-
Notifications
You must be signed in to change notification settings - Fork 299
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
Notify user when their shift swap request is taken #2992
Conversation
…om:grafana/oncall into jorlando/notify-beneficiary-when-ssr-taken
@@ -59,8 +59,6 @@ def _do_take(self, benefactor: User) -> dict: | |||
new_state=shift_swap.insight_logs_serialized, | |||
) | |||
|
|||
update_shift_swap_request_message.apply_async((shift_swap.pk,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to ShiftSwapRequest.take
(as this needs to be done in two spots, incoming API requests as well as Slack events when clicking "Accept")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
"text": ( | ||
f"{shift_swap_request.beneficiary.get_username_with_slack_verbal(True)} your teammate " | ||
f"{shift_swap_request.benefactor.get_username_with_slack_verbal()} has taken the shift swap request" | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe just having <user> has taken the shift swap request
could work better here?
I feel like addressing the SSR beneficiary directly would work well in DMs, but the thread message is visible to multiple people, so maybe it'd make sense to rephrase this so it's not directed at a single person?
|
||
message: Message = mock_send_push_notification.call_args.args[1] | ||
assert message.data["type"] == "oncall.info" | ||
assert message.data["title"] == "Your shift swap request has been taken" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe add a test to check that no one is getting push notifications except the beneficiary?
What this PR does
Closes #2868
Slack thread message
Push notification
Clicking on the push notification goes to the SSR detail view
Checklist
pr:no public docs
PR label added if not required)CHANGELOG.md
updated (orpr:no changelog
PR label added if not required)