diff --git a/CHANGELOG.md b/CHANGELOG.md index 27fbb6d5..0f57225a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Added + +- Notify user via Slack/mobile push-notification when their shift swap request is taken by @joeyorlando ([#2992](https://github.com/grafana/oncall/pull/2992)) + ## v1.3.36 (2023-09-07) ### Added diff --git a/engine/apps/api/tests/test_shift_swaps.py b/engine/apps/api/tests/test_shift_swaps.py index 1eb2731b..741bacdc 100644 --- a/engine/apps/api/tests/test_shift_swaps.py +++ b/engine/apps/api/tests/test_shift_swaps.py @@ -693,9 +693,8 @@ def test_delete_others_ssr_permissions(ssr_setup, make_user_auth_headers): assert response.status_code == status.HTTP_403_FORBIDDEN -@patch("apps.api.views.shift_swap.update_shift_swap_request_message") @pytest.mark.django_db -def test_take(mock_update_shift_swap_request_message, ssr_setup, make_user_auth_headers): +def test_take(ssr_setup, make_user_auth_headers): ssr, _, token, benefactor = ssr_setup() client = APIClient() url = reverse("api-internal:shift_swap-take", kwargs={"pk": ssr.public_primary_key}) @@ -721,14 +720,9 @@ def test_take(mock_update_shift_swap_request_message, ssr_setup, make_user_auth_ assert response.status_code == status.HTTP_200_OK assert response_json == expected_response - mock_update_shift_swap_request_message.apply_async.assert_called_once_with((ssr.pk,)) - -@patch("apps.api.views.shift_swap.update_shift_swap_request_message") @pytest.mark.django_db -def test_benficiary_tries_to_take_their_own_ssr( - mock_update_shift_swap_request_message, ssr_setup, make_user_auth_headers -): +def test_benficiary_tries_to_take_their_own_ssr(ssr_setup, make_user_auth_headers): ssr, beneficiary, token, _ = ssr_setup() client = APIClient() url = reverse("api-internal:shift_swap-take", kwargs={"pk": ssr.public_primary_key}) @@ -736,8 +730,6 @@ def test_benficiary_tries_to_take_their_own_ssr( response = client.post(url, format="json", **make_user_auth_headers(beneficiary, token)) assert response.status_code == status.HTTP_400_BAD_REQUEST - mock_update_shift_swap_request_message.apply_async.assert_not_called() - @pytest.mark.django_db def test_take_already_taken_ssr(ssr_setup, make_user_auth_headers): @@ -745,22 +737,16 @@ def test_take_already_taken_ssr(ssr_setup, make_user_auth_headers): client = APIClient() url = reverse("api-internal:shift_swap-take", kwargs={"pk": ssr.public_primary_key}) - with patch("apps.api.views.shift_swap.update_shift_swap_request_message") as mock_update_shift_swap_request_message: - response = client.post(url, format="json", **make_user_auth_headers(benefactor, token)) - assert response.status_code == status.HTTP_200_OK + response = client.post(url, format="json", **make_user_auth_headers(benefactor, token)) + assert response.status_code == status.HTTP_200_OK - mock_update_shift_swap_request_message.apply_async.assert_called_once_with((ssr.pk,)) - - with patch("apps.api.views.shift_swap.update_shift_swap_request_message") as mock_update_shift_swap_request_message: - response = client.post(url, format="json", **make_user_auth_headers(benefactor, token)) - assert response.status_code == status.HTTP_400_BAD_REQUEST - - mock_update_shift_swap_request_message.apply_async.assert_not_called() + # try to take the SSR again + response = client.post(url, format="json", **make_user_auth_headers(benefactor, token)) + assert response.status_code == status.HTTP_400_BAD_REQUEST -@patch("apps.api.views.shift_swap.update_shift_swap_request_message") @pytest.mark.django_db -def test_take_past_due_ssr(mock_update_shift_swap_request_message, ssr_setup, make_user_auth_headers): +def test_take_past_due_ssr(ssr_setup, make_user_auth_headers): ssr, _, token, benefactor = ssr_setup() client = APIClient() url = reverse("api-internal:shift_swap-take", kwargs={"pk": ssr.public_primary_key}) @@ -771,12 +757,9 @@ def test_take_past_due_ssr(mock_update_shift_swap_request_message, ssr_setup, ma response = client.post(url, format="json", **make_user_auth_headers(benefactor, token)) assert response.status_code == status.HTTP_400_BAD_REQUEST - mock_update_shift_swap_request_message.apply_async.assert_not_called() - -@patch("apps.api.views.shift_swap.update_shift_swap_request_message") @pytest.mark.django_db -def test_take_deleted_ssr(mock_update_shift_swap_request_message, ssr_setup, make_user_auth_headers): +def test_take_deleted_ssr(ssr_setup, make_user_auth_headers): ssr, _, token, benefactor = ssr_setup() client = APIClient() url = reverse("api-internal:shift_swap-take", kwargs={"pk": ssr.public_primary_key}) @@ -786,8 +769,6 @@ def test_take_deleted_ssr(mock_update_shift_swap_request_message, ssr_setup, mak response = client.post(url, format="json", **make_user_auth_headers(benefactor, token)) assert response.status_code == status.HTTP_404_NOT_FOUND - mock_update_shift_swap_request_message.apply_async.assert_not_called() - @patch("apps.api.views.shift_swap.ShiftSwapViewSet.take", return_value=mock_success_response) @pytest.mark.django_db diff --git a/engine/apps/api/views/shift_swap.py b/engine/apps/api/views/shift_swap.py index 9fbdf0fd..b6b94a71 100644 --- a/engine/apps/api/views/shift_swap.py +++ b/engine/apps/api/views/shift_swap.py @@ -59,8 +59,6 @@ class BaseShiftSwapViewSet(ModelViewSet): new_state=shift_swap.insight_logs_serialized, ) - update_shift_swap_request_message.apply_async((shift_swap.pk,)) - return ShiftSwapRequestSerializer(shift_swap).data def get_serializer_class(self): @@ -78,8 +76,6 @@ class BaseShiftSwapViewSet(ModelViewSet): return self.serializer_class.setup_eager_loading(queryset) def perform_destroy(self, instance: ShiftSwapRequest) -> None: - # TODO: should we allow deleting a taken request? - super().perform_destroy(instance) write_resource_insight_log(instance=instance, author=self.request.user, event=EntityEvent.DELETED) diff --git a/engine/apps/mobile_app/tasks/__init__.py b/engine/apps/mobile_app/tasks/__init__.py index 608a639e..39381b86 100644 --- a/engine/apps/mobile_app/tasks/__init__.py +++ b/engine/apps/mobile_app/tasks/__init__.py @@ -4,6 +4,7 @@ from .going_oncall_notification import ( # noqa:F401 ) from .new_alert_group import notify_user_about_new_alert_group # noqa:F401 from .new_shift_swap_request import ( # noqa:F401 + notify_beneficiary_about_taken_shift_swap_request, notify_shift_swap_request, notify_shift_swap_requests, notify_user_about_shift_swap_request, diff --git a/engine/apps/mobile_app/tasks/new_shift_swap_request.py b/engine/apps/mobile_app/tasks/new_shift_swap_request.py index 691f4244..f7f4e736 100644 --- a/engine/apps/mobile_app/tasks/new_shift_swap_request.py +++ b/engine/apps/mobile_app/tasks/new_shift_swap_request.py @@ -85,6 +85,23 @@ def _should_notify_user_about_shift_swap_request( ) +def _get_notification_title_and_subtitle(shift_swap_request: ShiftSwapRequest) -> typing.Tuple[str, str]: + notification_title: str + notification_subtitle: str + + beneficiary_name = shift_swap_request.beneficiary.name or shift_swap_request.beneficiary.username + schedule_name = shift_swap_request.schedule.name + + if shift_swap_request.is_taken: + notification_title = "Your shift swap request has been taken" + notification_subtitle = schedule_name + else: + notification_title = "New shift swap request" + notification_subtitle = f"{beneficiary_name}, {schedule_name}" + + return (notification_title, notification_subtitle) + + def _get_fcm_message( shift_swap_request: ShiftSwapRequest, user: User, @@ -92,9 +109,8 @@ def _get_fcm_message( mobile_app_user_settings: "MobileAppUserSettings", ) -> Message: thread_id = f"{shift_swap_request.public_primary_key}:{user.public_primary_key}:ssr" - notification_title = "New shift swap request" - beneficiary_name = shift_swap_request.beneficiary.name or shift_swap_request.beneficiary.username - notification_subtitle = f"{beneficiary_name}, {shift_swap_request.schedule.name}" + + notification_title, notification_subtitle = _get_notification_title_and_subtitle(shift_swap_request) # The mobile app will use this route to open the shift swap request route = f"/schedules/{shift_swap_request.schedule.public_primary_key}/ssrs/{shift_swap_request.public_primary_key}" @@ -128,20 +144,18 @@ def _get_fcm_message( return construct_fcm_message(MessageType.INFO, device_to_notify, thread_id, data, apns_payload) -@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) -def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: int) -> None: - """ - Send a push notification about a shift swap request to an individual user. - """ - # avoid circular import - from apps.mobile_app.models import FCMDevice, MobileAppUserSettings - +def _get_shift_swap_request(shift_swap_request_pk: int) -> typing.Optional[ShiftSwapRequest]: try: - shift_swap_request = ShiftSwapRequest.objects.get(pk=shift_swap_request_pk) + return ShiftSwapRequest.objects.get(pk=shift_swap_request_pk) except ShiftSwapRequest.DoesNotExist: logger.info(f"ShiftSwapRequest {shift_swap_request_pk} does not exist") return + +def _get_user_and_device(user_pk: int) -> typing.Optional[typing.Tuple[User, "FCMDevice", "MobileAppUserSettings"]]: + # avoid circular import + from apps.mobile_app.models import FCMDevice, MobileAppUserSettings + try: user = User.objects.get(pk=user_pk) except User.DoesNotExist: @@ -163,6 +177,24 @@ def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: in logger.info(f"Info notifications are not enabled for user {user_pk}") return + return (user, device_to_notify, mobile_app_user_settings) + + +@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) +def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: int) -> None: + """ + Send a push notification about a shift swap request to an individual user. + """ + shift_swap_request = _get_shift_swap_request(shift_swap_request_pk) + if not shift_swap_request: + return + + user_and_device = _get_user_and_device(user_pk) + if not user_and_device: + return + + user, device_to_notify, mobile_app_user_settings = user_and_device + if not shift_swap_request.is_open: logger.info(f"Shift swap request {shift_swap_request_pk} is not open anymore") return @@ -196,3 +228,18 @@ def notify_shift_swap_requests() -> None: """ for shift_swap_request, timeout in _get_shift_swap_requests_to_notify(timezone.now()): notify_shift_swap_request.delay(shift_swap_request.pk, timeout) + + +@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) +def notify_beneficiary_about_taken_shift_swap_request(shift_swap_request_pk: int) -> None: + shift_swap_request = _get_shift_swap_request(shift_swap_request_pk) + if not shift_swap_request: + return + + user_and_device = _get_user_and_device(shift_swap_request.beneficiary.pk) + if not user_and_device: + return + + user, device_to_notify, mobile_app_user_settings = user_and_device + message = _get_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings) + send_push_notification(device_to_notify, message) diff --git a/engine/apps/mobile_app/tests/tasks/test_new_shift_swap_request.py b/engine/apps/mobile_app/tests/tasks/test_new_shift_swap_request.py index 9b2303be..2baed054 100644 --- a/engine/apps/mobile_app/tests/tasks/test_new_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/tasks/test_new_shift_swap_request.py @@ -7,10 +7,14 @@ from firebase_admin.messaging import Message from apps.mobile_app.models import FCMDevice, MobileAppUserSettings from apps.mobile_app.tasks.new_shift_swap_request import ( + _get_notification_title_and_subtitle, + _get_shift_swap_request, _get_shift_swap_requests_to_notify, + _get_user_and_device, _has_user_been_notified_for_shift_swap_request, _mark_shift_swap_request_notified_for_user, _should_notify_user_about_shift_swap_request, + notify_beneficiary_about_taken_shift_swap_request, notify_shift_swap_request, notify_shift_swap_requests, notify_user_about_shift_swap_request, @@ -221,15 +225,24 @@ def test_notify_shift_swap_request_success( mock_notify_user_about_shift_swap_request.assert_called_once_with(shift_swap_request.pk, benefactor.pk) +@patch("apps.mobile_app.tasks.new_shift_swap_request._get_user_and_device") +@patch("apps.mobile_app.tasks.new_shift_swap_request.send_push_notification") @pytest.mark.django_db -def test_notify_user_about_shift_swap_request(make_organization, make_user, make_schedule, make_shift_swap_request): +def test_notify_user_about_shift_swap_request( + mock_send_push_notification, + mock_get_user_and_device, + make_organization, + make_user, + make_schedule, + make_shift_swap_request, +): organization = make_organization() beneficiary = make_user(organization=organization, name="John Doe", username="john.doe") benefactor = make_user(organization=organization) schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb, name="Test Schedule") device_to_notify = FCMDevice.objects.create(user=benefactor, registration_id="test_device_id") - MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=True) + maus = MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=True) now = timezone.now() swap_start = now + timezone.timedelta(days=100) @@ -239,10 +252,18 @@ def test_notify_user_about_shift_swap_request(make_organization, make_user, make schedule, beneficiary, swap_start=swap_start, swap_end=swap_end, created_at=now ) - with patch("apps.mobile_app.tasks.new_shift_swap_request.send_push_notification") as mock_send_push_notification: - notify_user_about_shift_swap_request(shift_swap_request.pk, benefactor.pk) + mock_get_user_and_device.return_value = None + notify_user_about_shift_swap_request(shift_swap_request.pk, benefactor.pk) + mock_get_user_and_device.assert_called_once_with(benefactor.pk) + mock_send_push_notification.assert_not_called() + mock_get_user_and_device.reset_mock() + + mock_get_user_and_device.return_value = (benefactor, device_to_notify, maus) + notify_user_about_shift_swap_request(shift_swap_request.pk, benefactor.pk) + mock_get_user_and_device.assert_called_once_with(benefactor.pk) mock_send_push_notification.assert_called_once() + assert mock_send_push_notification.call_args.args[0] == device_to_notify message: Message = mock_send_push_notification.call_args.args[1] @@ -256,32 +277,6 @@ def test_notify_user_about_shift_swap_request(make_organization, make_user, make assert message.apns.payload.aps.sound.critical is False -@pytest.mark.django_db -def test_notify_user_about_shift_swap_request_info_notifications_disabled( - make_organization, make_user, make_schedule, make_shift_swap_request -): - organization = make_organization() - beneficiary = make_user(organization=organization) - benefactor = make_user(organization=organization) - schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) - - FCMDevice.objects.create(user=benefactor, registration_id="test_device_id") - MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=False) - - now = timezone.now() - swap_start = now + timezone.timedelta(days=100) - swap_end = swap_start + timezone.timedelta(days=1) - - shift_swap_request = make_shift_swap_request( - schedule, beneficiary, swap_start=swap_start, swap_end=swap_end, created_at=now - ) - - with patch("apps.mobile_app.tasks.new_shift_swap_request.send_push_notification") as mock_send_push_notification: - notify_user_about_shift_swap_request(shift_swap_request.pk, benefactor.pk) - - mock_send_push_notification.assert_not_called() - - @pytest.mark.django_db def test_should_notify_user(make_organization, make_user, make_schedule, make_shift_swap_request): organization = make_organization() @@ -334,6 +329,74 @@ def test_should_notify_user(make_organization, make_user, make_schedule, make_sh assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is True +@pytest.mark.django_db +def test_get_notification_title_and_subtitle(make_organization, make_user, make_schedule, make_shift_swap_request): + organization = make_organization() + beneficiary_name = "hello" + beneficiary = make_user(organization=organization, name=beneficiary_name) + benefactor = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + ssr = make_shift_swap_request(schedule, beneficiary, swap_start=swap_start, swap_end=swap_end, created_at=now) + + title, subtitle = _get_notification_title_and_subtitle(ssr) + assert title == "New shift swap request" + assert subtitle == f"{beneficiary_name}, {schedule.name}" + + ssr.benefactor = benefactor + ssr.save() + ssr.refresh_from_db() + + title, subtitle = _get_notification_title_and_subtitle(ssr) + assert title == "Your shift swap request has been taken" + assert subtitle == schedule.name + + +@pytest.mark.django_db +def test_get_shift_swap_request(make_organization, make_user, make_schedule, make_shift_swap_request): + organization = make_organization() + beneficiary = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + ssr = make_shift_swap_request(schedule, beneficiary, swap_start=swap_start, swap_end=swap_end, created_at=now) + + assert _get_shift_swap_request(1234) is None + assert _get_shift_swap_request(ssr.pk) == ssr + + +@pytest.mark.django_db +def test_get_user_and_device(make_organization, make_user): + organization = make_organization() + user = make_user(organization=organization) + + # no user found + assert _get_user_and_device(1234) is None + + # no device found + assert _get_user_and_device(user.pk) is None + + # no mobile app user settings found + device = FCMDevice.objects.create(user=user, registration_id="test_device_id") + assert _get_user_and_device(user.pk) is None + + # info notifications disabled + mobile_app_settings = MobileAppUserSettings.objects.create(user=user, info_notifications_enabled=False) + assert _get_user_and_device(user.pk) is None + + mobile_app_settings.info_notifications_enabled = True + mobile_app_settings.save() + + assert _get_user_and_device(user.pk) == (user, device, mobile_app_settings) + + @pytest.mark.django_db def test_mark_notified(make_organization, make_user, make_schedule, make_shift_swap_request): organization = make_organization() @@ -357,3 +420,57 @@ def test_mark_notified(make_organization, make_user, make_schedule, make_shift_s with patch.object(cache, "set") as mock_cache_set: _mark_shift_swap_request_notified_for_user(shift_swap_request, benefactor, TIMEOUT) assert mock_cache_set.call_args.kwargs["timeout"] == TIMEOUT + + +@patch("apps.mobile_app.tasks.new_shift_swap_request._get_user_and_device") +@patch("apps.mobile_app.tasks.new_shift_swap_request.send_push_notification") +@pytest.mark.django_db +def test_notify_beneficiary_about_taken_shift_swap_request( + mock_send_push_notification, + mock_get_user_and_device, + make_organization, + make_user, + make_schedule, + make_shift_swap_request, +): + organization = make_organization() + beneficiary = make_user(organization=organization) + benefactor = make_user(organization=organization) + schedule_name = "Test Schedule" + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb, name=schedule_name) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, beneficiary, benefactor=benefactor, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + device_to_notify = FCMDevice.objects.create(user=beneficiary, registration_id="test_device_id") + maus = MobileAppUserSettings.objects.create(user=beneficiary, info_notifications_enabled=True) + + # no user, device, or mobile app settings + mock_get_user_and_device.return_value = None + notify_beneficiary_about_taken_shift_swap_request(shift_swap_request.pk) + mock_get_user_and_device.assert_called_once_with(beneficiary.pk) + mock_send_push_notification.assert_not_called() + + mock_get_user_and_device.reset_mock() + + mock_get_user_and_device.return_value = (beneficiary, device_to_notify, maus) + notify_beneficiary_about_taken_shift_swap_request(shift_swap_request.pk) + mock_get_user_and_device.assert_called_once_with(beneficiary.pk) + mock_send_push_notification.assert_called_once() + + assert mock_send_push_notification.call_args.args[0] == device_to_notify + + 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" + assert message.data["subtitle"] == schedule_name + assert ( + message.data["route"] + == f"/schedules/{schedule.public_primary_key}/ssrs/{shift_swap_request.public_primary_key}" + ) + assert message.apns.payload.aps.sound.critical is False diff --git a/engine/apps/public_api/tests/test_shift_swap.py b/engine/apps/public_api/tests/test_shift_swap.py index a4593320..44ca053b 100644 --- a/engine/apps/public_api/tests/test_shift_swap.py +++ b/engine/apps/public_api/tests/test_shift_swap.py @@ -154,7 +154,7 @@ def test_create_requires_beneficiary( make_organization_and_user_with_token, make_schedule, ): - organization, user, token = make_organization_and_user_with_token() + organization, _, token = make_organization_and_user_with_token() schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) today = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) @@ -330,15 +330,9 @@ def test_delete( mock_update_shift_swap_request_message.apply_async.assert_called_once_with((swap.pk,)) -@patch("apps.api.views.shift_swap.update_shift_swap_request_message") @pytest.mark.django_db -def test_take( - mock_update_shift_swap_request_message, - make_organization_and_user_with_token, - make_user_for_organization, - setup_swap, -): - organization, user, token = make_organization_and_user_with_token() +def test_take(make_organization_and_user_with_token, make_user_for_organization, setup_swap): + organization, _, token = make_organization_and_user_with_token() another_user = make_user_for_organization(organization) swap = setup_swap(organization) @@ -354,17 +348,10 @@ def test_take( assert swap.status == ShiftSwapRequest.Statuses.TAKEN assert swap.benefactor == another_user - mock_update_shift_swap_request_message.apply_async.assert_called_once_with((swap.pk,)) - -@patch("apps.api.views.shift_swap.update_shift_swap_request_message") @pytest.mark.django_db -def test_take_requires_benefactor( - mock_update_shift_swap_request_message, - make_organization_and_user_with_token, - setup_swap, -): - organization, user, token = make_organization_and_user_with_token() +def test_take_requires_benefactor(make_organization_and_user_with_token, setup_swap): + organization, _, token = make_organization_and_user_with_token() swap = setup_swap(organization) client = APIClient() @@ -378,18 +365,10 @@ def test_take_requires_benefactor( assert swap.status == ShiftSwapRequest.Statuses.OPEN assert swap.benefactor is None - mock_update_shift_swap_request_message.apply_async.assert_not_called() - -@patch("apps.api.views.shift_swap.update_shift_swap_request_message") @pytest.mark.django_db -def test_take_errors( - mock_update_shift_swap_request_message, - make_organization_and_user_with_token, - make_user_for_organization, - setup_swap, -): - organization, user, token = make_organization_and_user_with_token() +def test_take_errors(make_organization_and_user_with_token, make_user_for_organization, setup_swap): + organization, _, token = make_organization_and_user_with_token() another_user = make_user_for_organization(organization) swap = setup_swap(organization) @@ -421,5 +400,3 @@ def test_take_errors( data = {"benefactor": another_user.public_primary_key} response = client.post(url, data, format="json", HTTP_AUTHORIZATION=f"{token}") assert response.status_code == status.HTTP_400_BAD_REQUEST - - mock_update_shift_swap_request_message.apply_async.assert_not_called() diff --git a/engine/apps/schedules/models/shift_swap_request.py b/engine/apps/schedules/models/shift_swap_request.py index ad6b0406..586a4950 100644 --- a/engine/apps/schedules/models/shift_swap_request.py +++ b/engine/apps/schedules/models/shift_swap_request.py @@ -166,6 +166,10 @@ class ShiftSwapRequest(models.Model): """ return self.schedule.channel + @property + def schedule_slack_url(self) -> str: + return f"<{self.schedule.web_detail_page_link}|{self.schedule.name}>" + @property def organization(self) -> "Organization": return self.schedule.organization @@ -202,6 +206,11 @@ class ShiftSwapRequest(models.Model): return related_shifts def take(self, benefactor: "User") -> None: + from apps.schedules.tasks.shift_swaps import ( + notify_beneficiary_about_taken_shift_swap_request, + update_shift_swap_request_message, + ) + if benefactor == self.beneficiary: raise exceptions.BeneficiaryCannotTakeOwnShiftSwapRequest() if self.status != self.Statuses.OPEN: @@ -210,6 +219,9 @@ class ShiftSwapRequest(models.Model): self.benefactor = benefactor self.save() + update_shift_swap_request_message.apply_async((self.pk,)) + notify_beneficiary_about_taken_shift_swap_request.apply_async((self.pk,)) + # make sure final schedule ical representation is updated refresh_ical_final_schedule.apply_async((self.schedule.pk,)) diff --git a/engine/apps/schedules/tasks/shift_swaps/__init__.py b/engine/apps/schedules/tasks/shift_swaps/__init__.py index bde1521d..b9687492 100644 --- a/engine/apps/schedules/tasks/shift_swaps/__init__.py +++ b/engine/apps/schedules/tasks/shift_swaps/__init__.py @@ -1,2 +1,3 @@ +from .notify_when_taken import notify_beneficiary_about_taken_shift_swap_request # noqa: F401 from .slack_followups import send_shift_swap_request_slack_followups # noqa: F401 from .slack_messages import create_shift_swap_request_message, update_shift_swap_request_message # noqa: F401 diff --git a/engine/apps/schedules/tasks/shift_swaps/notify_when_taken.py b/engine/apps/schedules/tasks/shift_swaps/notify_when_taken.py new file mode 100644 index 00000000..2243a965 --- /dev/null +++ b/engine/apps/schedules/tasks/shift_swaps/notify_when_taken.py @@ -0,0 +1,36 @@ +from celery.utils.log import get_task_logger + +from apps.mobile_app.tasks import ( + notify_beneficiary_about_taken_shift_swap_request as notify_beneficiary_about_taken_shift_swap_request_via_push_notification, +) +from common.custom_celery_tasks import shared_dedicated_queue_retry_task + +task_logger = get_task_logger(__name__) + + +@shared_dedicated_queue_retry_task() +def notify_beneficiary_about_taken_shift_swap_request(shift_swap_request_pk: str) -> None: + from apps.schedules.models import ShiftSwapRequest + from apps.slack.scenarios.shift_swap_requests import AcceptShiftSwapRequestStep + + task_logger.info(f"Start notify_beneficiary_about_taken_shift_swap_request: pk = {shift_swap_request_pk}") + + try: + shift_swap_request = ShiftSwapRequest.objects.get(pk=shift_swap_request_pk) + except ShiftSwapRequest.DoesNotExist: + task_logger.info( + f"Tried to notify_beneficiary_about_taken_shift_swap_request for non-existing shift swap request {shift_swap_request_pk}" + ) + return + + notify_beneficiary_about_taken_shift_swap_request_via_push_notification.apply_async((shift_swap_request_pk,)) + + if shift_swap_request.slack_channel_id is None: + task_logger.info( + f"Skipping notify_beneficiary_about_taken_shift_swap_request for shift_swap_request {shift_swap_request_pk} because channel_id is None" + ) + return + + organization = shift_swap_request.organization + step = AcceptShiftSwapRequestStep(organization.slack_team_identity, organization) + step.post_request_taken_message_to_thread(shift_swap_request) diff --git a/engine/apps/schedules/tests/tasks/shift_swaps/test_notify_when_taken.py b/engine/apps/schedules/tests/tasks/shift_swaps/test_notify_when_taken.py new file mode 100644 index 00000000..70f58c8e --- /dev/null +++ b/engine/apps/schedules/tests/tasks/shift_swaps/test_notify_when_taken.py @@ -0,0 +1,80 @@ +from unittest.mock import patch + +import pytest + +from apps.schedules.tasks.shift_swaps import notify_beneficiary_about_taken_shift_swap_request + + +@patch( + "apps.schedules.tasks.shift_swaps.notify_when_taken.notify_beneficiary_about_taken_shift_swap_request_via_push_notification" +) +@patch("apps.slack.scenarios.shift_swap_requests.AcceptShiftSwapRequestStep") +@pytest.mark.django_db +def test_notify_beneficiary_about_taken_shift_swap_request_not_found( + MockAcceptShiftSwapRequestStep, + mock_notify_beneficiary_about_taken_shift_swap_request_via_push_notification, +): + notify_beneficiary_about_taken_shift_swap_request("12345") + + MockAcceptShiftSwapRequestStep.assert_not_called() + MockAcceptShiftSwapRequestStep.return_value.post_request_taken_message_to_thread.assert_not_called() + + mock_notify_beneficiary_about_taken_shift_swap_request_via_push_notification.apply_async.assert_not_called() + + +@patch( + "apps.schedules.tasks.shift_swaps.notify_when_taken.notify_beneficiary_about_taken_shift_swap_request_via_push_notification" +) +@patch("apps.slack.scenarios.shift_swap_requests.AcceptShiftSwapRequestStep") +@pytest.mark.django_db +def test_notify_beneficiary_about_taken_shift_swap_request_no_configured_slack_channel_for_schedule( + MockAcceptShiftSwapRequestStep, + mock_notify_beneficiary_about_taken_shift_swap_request_via_push_notification, + shift_swap_request_setup, +): + ssr, _, _ = shift_swap_request_setup() + assert ssr.schedule.channel is None + + notify_beneficiary_about_taken_shift_swap_request(ssr.pk) + + mock_notify_beneficiary_about_taken_shift_swap_request_via_push_notification.apply_async.assert_called_once_with( + (ssr.pk,) + ) + + MockAcceptShiftSwapRequestStep.assert_not_called() + MockAcceptShiftSwapRequestStep.return_value.post_request_taken_message_to_thread.assert_not_called() + + +@patch( + "apps.schedules.tasks.shift_swaps.notify_when_taken.notify_beneficiary_about_taken_shift_swap_request_via_push_notification" +) +@patch("apps.slack.scenarios.shift_swap_requests.AcceptShiftSwapRequestStep") +@pytest.mark.django_db +def test_notify_beneficiary_about_taken_shift_swap_request_post_message_to_channel_called( + MockAcceptShiftSwapRequestStep, + mock_notify_beneficiary_about_taken_shift_swap_request_via_push_notification, + shift_swap_request_setup, + make_slack_team_identity, +): + slack_channel_id = "C1234ASDFJ" + + ssr, _, _ = shift_swap_request_setup() + schedule = ssr.schedule + organization = schedule.organization + + slack_team_identity = make_slack_team_identity() + + schedule.channel = slack_channel_id + schedule.save() + + organization.slack_team_identity = slack_team_identity + organization.save() + + notify_beneficiary_about_taken_shift_swap_request(ssr.pk) + + MockAcceptShiftSwapRequestStep.assert_called_once_with(slack_team_identity, organization) + MockAcceptShiftSwapRequestStep.return_value.post_request_taken_message_to_thread.assert_called_once_with(ssr) + + mock_notify_beneficiary_about_taken_shift_swap_request_via_push_notification.apply_async.assert_called_once_with( + (ssr.pk,) + ) diff --git a/engine/apps/schedules/tests/test_shift_swap_request.py b/engine/apps/schedules/tests/test_shift_swap_request.py index 63de2b75..ab1d87c3 100644 --- a/engine/apps/schedules/tests/test_shift_swap_request.py +++ b/engine/apps/schedules/tests/test_shift_swap_request.py @@ -78,16 +78,27 @@ def test_status_deleted(shift_swap_request_setup) -> None: assert ssr.is_deleted is True +@patch("apps.schedules.tasks.shift_swaps.update_shift_swap_request_message") +@patch("apps.schedules.tasks.shift_swaps.notify_beneficiary_about_taken_shift_swap_request") +@patch("apps.schedules.models.shift_swap_request.refresh_ical_final_schedule") @pytest.mark.django_db -def test_take(shift_swap_request_setup) -> None: +def test_take( + mock_refresh_final, + mock_notify_beneficiary_about_taken_shift_swap_request, + mock_update_shift_swap_request_message, + shift_swap_request_setup, +) -> None: ssr, _, benefactor = shift_swap_request_setup() original_updated_at = ssr.updated_at - with patch("apps.schedules.models.shift_swap_request.refresh_ical_final_schedule") as mock_refresh_final: - ssr.take(benefactor) + ssr.take(benefactor) assert ssr.benefactor == benefactor assert ssr.updated_at != original_updated_at + + mock_update_shift_swap_request_message.apply_async.assert_called_once_with((ssr.pk,)) + mock_notify_beneficiary_about_taken_shift_swap_request.apply_async.assert_called_once_with((ssr.pk,)) + # final schedule refresh was triggered assert mock_refresh_final.apply_async.called_with((ssr.schedule.pk,)) diff --git a/engine/apps/slack/scenarios/shift_swap_requests.py b/engine/apps/slack/scenarios/shift_swap_requests.py index db131dc5..b7ffcf86 100644 --- a/engine/apps/slack/scenarios/shift_swap_requests.py +++ b/engine/apps/slack/scenarios/shift_swap_requests.py @@ -20,18 +20,14 @@ logger.setLevel(logging.DEBUG) SHIFT_SWAP_PK_ACTION_KEY = "shift_swap_request_pk" -def _schedule_slack_url(shift_swap_request) -> str: - schedule = shift_swap_request.schedule - return f"<{schedule.web_detail_page_link}|{schedule.name}>" - - class BaseShiftSwapRequestStep(scenario_step.ScenarioStep): def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyBlocks: pk = shift_swap_request.pk main_message_text = ( - f"*New shift swap request for {_schedule_slack_url(shift_swap_request)}*\n" - f"Your teammate {shift_swap_request.beneficiary.get_username_with_slack_verbal()} has submitted a shift swap request." + f"*New shift swap request for {shift_swap_request.schedule_slack_url}*\n" + f"Your teammate {shift_swap_request.beneficiary.get_username_with_slack_verbal(True)} has submitted " + "a shift swap request." ) datetime_format = SlackDateFormat.DATE_LONG_PRETTY @@ -117,7 +113,10 @@ class BaseShiftSwapRequestStep(scenario_step.ScenarioStep): "type": "section", "text": { "type": "mrkdwn", - "text": f"✅ {shift_swap_request.benefactor.get_username_with_slack_verbal()} has accepted the shift swap request", + "text": ( + f"✅ {shift_swap_request.benefactor.get_username_with_slack_verbal()} has " + "accepted the shift swap request" + ), }, }, ), @@ -172,6 +171,19 @@ class BaseShiftSwapRequestStep(scenario_step.ScenarioStep): blocks=self._generate_blocks(shift_swap_request), ) + def post_message_to_thread( + self, shift_swap_request: "ShiftSwapRequest", blocks: Block.AnyBlocks, reply_broadcast=False + ) -> None: + if not shift_swap_request.slack_message: + return + + self._slack_client.chat_postMessage( + channel=shift_swap_request.slack_message.channel_id, + thread_ts=shift_swap_request.slack_message.slack_id, + reply_broadcast=reply_broadcast, + blocks=blocks, + ) + class AcceptShiftSwapRequestStep(BaseShiftSwapRequestStep): def process_scenario( @@ -202,8 +214,28 @@ class AcceptShiftSwapRequestStep(BaseShiftSwapRequestStep): self.update_message(shift_swap_request) + def post_request_taken_message_to_thread(self, shift_swap_request: "ShiftSwapRequest") -> None: + self.post_message_to_thread( + shift_swap_request, + [ + typing.cast( + Block.Section, + { + "type": "section", + "text": { + "type": "mrkdwn", + "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" + ), + }, + }, + ) + ], + ) -class ShiftSwapRequestFollowUp(scenario_step.ScenarioStep): + +class ShiftSwapRequestFollowUp(BaseShiftSwapRequestStep): @staticmethod def _generate_blocks(shift_swap_request: "ShiftSwapRequest") -> Block.AnyBlocks: # Time until shift swap starts (example: "14 days", "2 hours") @@ -217,7 +249,7 @@ class ShiftSwapRequestFollowUp(scenario_step.ScenarioStep): "text": { "type": "mrkdwn", "text": ( - f"⚠️ This shift swap request for {_schedule_slack_url(shift_swap_request)} is " + f"⚠️ This shift swap request for {shift_swap_request.schedule_slack_url} is " f"still open and will start in {delta}. Jump back into the thread and accept it if " "you're available!" ), @@ -227,12 +259,7 @@ class ShiftSwapRequestFollowUp(scenario_step.ScenarioStep): ] def post_message(self, shift_swap_request: "ShiftSwapRequest") -> None: - self._slack_client.chat_postMessage( - channel=shift_swap_request.slack_message.channel_id, - thread_ts=shift_swap_request.slack_message.slack_id, - reply_broadcast=True, - blocks=self._generate_blocks(shift_swap_request), - ) + self.post_message_to_thread(shift_swap_request, self._generate_blocks(shift_swap_request), True) STEPS_ROUTING: ScenarioRoute.RoutingSteps = [ diff --git a/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py b/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py index 99cc0a76..df91ae47 100644 --- a/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py +++ b/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py @@ -183,6 +183,41 @@ class TestBaseShiftSwapRequestStep: channel=ssr.slack_channel_id, ts=ts, blocks=mock_generate_blocks.return_value ) + @pytest.mark.django_db + def test_post_message_to_thread(self, setup, make_slack_message) -> None: + ts = "12345.67" + blocks = [{"foo": "bar"}] + + ssr, _, _, _ = setup() + channel_id = "asdfadf" + organization = ssr.organization + slack_team_identity = organization.slack_team_identity + + slack_message = make_slack_message( + alert_group=None, organization=organization, slack_id=ts, channel_id=channel_id + ) + + step = scenarios.BaseShiftSwapRequestStep(slack_team_identity, organization) + + with patch.object(step, "_slack_client") as mock_slack_client: + step.post_message_to_thread(ssr, blocks) + + mock_slack_client.chat_postMessage.assert_not_called() + + ssr.slack_message = slack_message + ssr.save() + ssr.refresh_from_db() + + with patch.object(step, "_slack_client") as mock_slack_client: + step.post_message_to_thread(ssr, blocks, True) + + mock_slack_client.chat_postMessage.assert_called_once_with( + channel=channel_id, + thread_ts=ts, + reply_broadcast=True, + blocks=blocks, + ) + class TestAcceptShiftSwapRequestStep: @pytest.mark.django_db @@ -271,3 +306,32 @@ class TestAcceptShiftSwapRequestStep: event_payload, "The shift swap request is not in a state which allows it to be taken" ) mock_update_message.assert_not_called() + + @patch("apps.slack.scenarios.shift_swap_requests.AcceptShiftSwapRequestStep.post_message_to_thread") + @pytest.mark.django_db + def test_post_request_taken_message_to_thread(self, mock_post_message_to_thread, setup) -> None: + ssr, _, benefactor, _ = setup() + + organization = ssr.organization + slack_team_identity = organization.slack_team_identity + ssr.benefactor = benefactor + ssr.save() + + step = scenarios.AcceptShiftSwapRequestStep(slack_team_identity, organization, benefactor) + step.post_request_taken_message_to_thread(ssr) + + mock_post_message_to_thread.assert_called_once_with( + ssr, + [ + { + "type": "section", + "text": { + "type": "mrkdwn", + "text": ( + f"{ssr.beneficiary.get_username_with_slack_verbal(True)} your teammate " + f"{ssr.benefactor.get_username_with_slack_verbal()} has taken the shift swap request" + ), + }, + }, + ], + ) diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index e3969431..2043f0f9 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -256,7 +256,7 @@ class User(models.Model): def is_notification_allowed(self): return user_is_authorized(self, [RBACPermission.Permissions.NOTIFICATIONS_READ]) - def get_username_with_slack_verbal(self, mention=False): + def get_username_with_slack_verbal(self, mention=False) -> str: slack_verbal = None if self.slack_user_identity: diff --git a/engine/settings/celery_task_routes.py b/engine/settings/celery_task_routes.py index cedbd8ec..af0e72da 100644 --- a/engine/settings/celery_task_routes.py +++ b/engine/settings/celery_task_routes.py @@ -19,6 +19,9 @@ CELERY_TASK_ROUTES = { "apps.mobile_app.tasks.new_shift_swap_request.notify_shift_swap_requests": {"queue": "default"}, "apps.mobile_app.tasks.new_shift_swap_request.notify_shift_swap_request": {"queue": "default"}, "apps.mobile_app.tasks.new_shift_swap_request.notify_user_about_shift_swap_request": {"queue": "default"}, + "apps.mobile_app.tasks.new_shift_swap_request.notify_beneficiary_about_taken_shift_swap_request": { + "queue": "default" + }, "apps.schedules.tasks.refresh_ical_files.refresh_ical_file": {"queue": "default"}, "apps.schedules.tasks.refresh_ical_files.start_refresh_ical_files": {"queue": "default"}, "apps.schedules.tasks.refresh_ical_files.refresh_ical_final_schedule": {"queue": "default"}, @@ -48,6 +51,9 @@ CELERY_TASK_ROUTES = { }, "apps.schedules.tasks.shift_swaps.slack_messages.create_shift_swap_request_message": {"queue": "default"}, "apps.schedules.tasks.shift_swaps.slack_messages.update_shift_swap_request_message": {"queue": "default"}, + "apps.schedules.tasks.shift_swaps.notify_when_taken.notify_beneficiary_about_taken_shift_swap_request": { + "queue": "default" + }, "apps.schedules.tasks.shift_swaps.slack_followups.send_shift_swap_request_slack_followups": {"queue": "default"}, "apps.schedules.tasks.shift_swaps.slack_followups.send_shift_swap_request_slack_followup": {"queue": "default"}, "apps.migration_tool.tasks.start_migration_from_old_amixr": {"queue": "default"},