From e81931c3d06960c4e87678c5c3e69342351f8413 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Wed, 24 May 2023 18:03:00 -0300 Subject: [PATCH 1/2] Handle slack payload metadata limit in paging command --- CHANGELOG.md | 4 + engine/apps/slack/constants.py | 2 + engine/apps/slack/scenarios/paging.py | 59 +++++++++--- .../tests/test_scenario_steps/test_paging.py | 89 +++++++++++++++++++ 4 files changed, 144 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 796b3afab5..9f2604df91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Phone provider refactoring [#1713](https://github.com/grafana/oncall/pull/1713) +### Fixed + +- Handle slack metadata limit when creating paging command payload + ## v1.2.28 (2023-05-24) ### Fixed diff --git a/engine/apps/slack/constants.py b/engine/apps/slack/constants.py index 24572538f1..34f7a3c611 100644 --- a/engine/apps/slack/constants.py +++ b/engine/apps/slack/constants.py @@ -9,3 +9,5 @@ SLACK_RATE_LIMIT_TIMEOUT = timezone.timedelta(minutes=5) SLACK_RATE_LIMIT_DELAY = 10 CACHE_UPDATE_INCIDENT_SLACK_MESSAGE_LIFETIME = 60 * 10 + +PRIVATE_METADATA_MAX_LENGTH = 3000 diff --git a/engine/apps/slack/scenarios/paging.py b/engine/apps/slack/scenarios/paging.py index 9b9272a2f4..542d4e00c6 100644 --- a/engine/apps/slack/scenarios/paging.py +++ b/engine/apps/slack/scenarios/paging.py @@ -10,6 +10,7 @@ check_user_availability, direct_paging, ) +from apps.slack.constants import PRIVATE_METADATA_MAX_LENGTH from apps.slack.models import SlackChannel from apps.slack.scenarios import scenario_step from apps.slack.slack_client.exceptions import SlackAPIException @@ -47,7 +48,11 @@ def add_or_update_item(payload, key, item_pk, policy): metadata = json.loads(payload["view"]["private_metadata"]) metadata[key][item_pk] = policy - payload["view"]["private_metadata"] = json.dumps(metadata) + updated_metadata = json.dumps(metadata) + print(updated_metadata) + if len(updated_metadata) > PRIVATE_METADATA_MAX_LENGTH: + raise ValueError("Cannot add entry, maximum exceeded") + payload["view"]["private_metadata"] = updated_metadata return payload @@ -213,8 +218,13 @@ def process_scenario(self, slack_user_identity, slack_team_identity, payload): ) else: # user is available to be paged - updated_payload = add_or_update_item(payload, USERS_DATA_KEY, selected_user.pk, DEFAULT_POLICY) - view = render_dialog(slack_user_identity, slack_team_identity, updated_payload) + error_msg = None + try: + updated_payload = add_or_update_item(payload, USERS_DATA_KEY, selected_user.pk, DEFAULT_POLICY) + except ValueError: + updated_payload = payload + error_msg = "Cannot add user, maximum responders exceeded" + view = render_dialog(slack_user_identity, slack_team_identity, updated_payload, error_msg=error_msg) self._slack_client.api_call( "views.update", trigger_id=payload["trigger_id"], @@ -233,12 +243,17 @@ def _parse_action(self, payload): def process_scenario(self, slack_user_identity, slack_team_identity, payload, policy=None): policy, key, user_pk = self._parse_action(payload) + error_msg = None if policy == REMOVE_ACTION: updated_payload = remove_item(payload, key, user_pk) else: - updated_payload = add_or_update_item(payload, key, user_pk, policy) + try: + updated_payload = add_or_update_item(payload, key, user_pk, policy) + except ValueError: + updated_payload = payload + error_msg = "Cannot update policy, maximum responders exceeded" - view = render_dialog(slack_user_identity, slack_team_identity, updated_payload) + view = render_dialog(slack_user_identity, slack_team_identity, updated_payload, error_msg=error_msg) self._slack_client.api_call( "views.update", trigger_id=payload["trigger_id"], @@ -269,8 +284,15 @@ def process_scenario(self, slack_user_identity, slack_team_identity, payload): } # add selected user selected_user = _get_selected_user_from_payload(previous_view_payload, private_metadata["input_id_prefix"]) - updated_payload = add_or_update_item(previous_view_payload, USERS_DATA_KEY, selected_user.pk, DEFAULT_POLICY) - view = render_dialog(slack_user_identity, slack_team_identity, updated_payload) + error_msg = None + try: + updated_payload = add_or_update_item( + previous_view_payload, USERS_DATA_KEY, selected_user.pk, DEFAULT_POLICY + ) + except ValueError: + updated_payload = payload + error_msg = "Cannot add user, maximum responders exceeded" + view = render_dialog(slack_user_identity, slack_team_identity, updated_payload, error_msg=error_msg) self._slack_client.api_call( "views.update", trigger_id=payload["trigger_id"], @@ -291,8 +313,13 @@ def process_scenario(self, slack_user_identity, slack_team_identity, payload, ac if selected_schedule is None: return - updated_payload = add_or_update_item(payload, SCHEDULES_DATA_KEY, selected_schedule.pk, DEFAULT_POLICY) - view = render_dialog(slack_user_identity, slack_team_identity, updated_payload) + error_msg = None + try: + updated_payload = add_or_update_item(payload, SCHEDULES_DATA_KEY, selected_schedule.pk, DEFAULT_POLICY) + except ValueError: + updated_payload = payload + error_msg = "Cannot add schedule, maximum responders exceeded" + view = render_dialog(slack_user_identity, slack_team_identity, updated_payload, error_msg=error_msg) self._slack_client.api_call( "views.update", trigger_id=payload["trigger_id"], @@ -306,7 +333,7 @@ def process_scenario(self, slack_user_identity, slack_team_identity, payload, ac DIVIDER_BLOCK = {"type": "divider"} -def render_dialog(slack_user_identity, slack_team_identity, payload, initial=False): +def render_dialog(slack_user_identity, slack_team_identity, payload, initial=False, error_msg=None): private_metadata = json.loads(payload["view"]["private_metadata"]) submit_routing_uid = private_metadata.get("submit_routing_uid") if initial: @@ -345,6 +372,18 @@ def render_dialog(slack_user_identity, slack_team_identity, payload, initial=Fal # blocks blocks = [organization_select, team_select, escalation_select, users_select, schedules_select] + if error_msg: + blocks += [ + { + "type": "section", + "block_id": "error_message", + "text": { + "type": "mrkdwn", + "text": f":warning: {error_msg}", + }, + } + ] + # selected items selected_users = get_current_items(payload, USERS_DATA_KEY, selected_organization.users) selected_schedules = get_current_items(payload, SCHEDULES_DATA_KEY, selected_organization.oncall_schedules) diff --git a/engine/apps/slack/tests/test_scenario_steps/test_paging.py b/engine/apps/slack/tests/test_scenario_steps/test_paging.py index caf7a3157f..bb7b7cac6b 100644 --- a/engine/apps/slack/tests/test_scenario_steps/test_paging.py +++ b/engine/apps/slack/tests/test_scenario_steps/test_paging.py @@ -138,6 +138,63 @@ def test_add_user_no_warning( assert metadata[USERS_DATA_KEY] == {str(user.pk): DEFAULT_POLICY} +@pytest.mark.django_db +def test_add_user_maximum_exceeded( + make_organization_and_user_with_slack_identities, make_schedule, make_on_call_shift, make_user_notification_policy +): + organization, user, slack_team_identity, slack_user_identity = make_organization_and_user_with_slack_identities() + # set up schedule: user is on call + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + team=None, + ) + now = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + start_date = now - timezone.timedelta(days=7) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": timezone.timedelta(hours=23, minutes=59, seconds=59), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user]]) + schedule.refresh_ical_file() + # setup notification policy + make_user_notification_policy( + user=user, + step=UserNotificationPolicy.Step.NOTIFY, + notify_by=UserNotificationPolicy.NotificationChannel.SMS, + ) + + payload = make_slack_payload(organization=organization, user=user) + + step = OnPagingUserChange(slack_team_identity) + with patch("apps.slack.scenarios.paging.PRIVATE_METADATA_MAX_LENGTH", 100): + with patch.object(step._slack_client, "api_call") as mock_slack_api_call: + step.process_scenario(slack_user_identity, slack_team_identity, payload) + + assert mock_slack_api_call.call_args.args == ("views.update",) + view_data = mock_slack_api_call.call_args.kwargs["view"] + metadata = json.loads(view_data["private_metadata"]) + # metadata unchanged, ignoring the prefix + original_metadata = json.loads(payload["view"]["private_metadata"]) + metadata.pop("input_id_prefix") + original_metadata.pop("input_id_prefix") + assert metadata == original_metadata + # error message is displayed + error_block = { + "type": "section", + "block_id": "error_message", + "text": {"type": "mrkdwn", "text": ":warning: Cannot add user, maximum responders exceeded"}, + } + assert error_block in view_data["blocks"] + + @pytest.mark.django_db def test_add_user_raise_warning(make_organization_and_user_with_slack_identities): organization, user, slack_team_identity, slack_user_identity = make_organization_and_user_with_slack_identities() @@ -247,6 +304,38 @@ def test_add_schedule(make_organization_and_user_with_slack_identities, make_sch assert metadata[USERS_DATA_KEY] == {str(user.pk): IMPORTANT_POLICY} +@pytest.mark.django_db +def test_add_schedule_responders_exceeded(make_organization_and_user_with_slack_identities, make_schedule): + organization, user, slack_team_identity, slack_user_identity = make_organization_and_user_with_slack_identities() + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb, team=None) + payload = make_slack_payload( + organization=organization, + schedule=schedule, + current_users={str(user.pk): IMPORTANT_POLICY}, + ) + + step = OnPagingScheduleChange(slack_team_identity) + with patch("apps.slack.scenarios.paging.PRIVATE_METADATA_MAX_LENGTH", 100): + with patch.object(step._slack_client, "api_call") as mock_slack_api_call: + step.process_scenario(slack_user_identity, slack_team_identity, payload) + + assert mock_slack_api_call.call_args.args == ("views.update",) + view_data = mock_slack_api_call.call_args.kwargs["view"] + metadata = json.loads(view_data["private_metadata"]) + # metadata unchanged, ignoring the prefix + original_metadata = json.loads(payload["view"]["private_metadata"]) + metadata.pop("input_id_prefix") + original_metadata.pop("input_id_prefix") + assert metadata == original_metadata + # error message is displayed + error_block = { + "type": "section", + "block_id": "error_message", + "text": {"type": "mrkdwn", "text": ":warning: Cannot add schedule, maximum responders exceeded"}, + } + assert error_block in view_data["blocks"] + + @pytest.mark.django_db def test_change_schedule_policy(make_organization_and_user_with_slack_identities, make_schedule): organization, user, slack_team_identity, slack_user_identity = make_organization_and_user_with_slack_identities() From 183332d0daec86784952fa55983be727b8ba177b Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Wed, 24 May 2023 21:33:30 -0300 Subject: [PATCH 2/2] Remove print --- engine/apps/slack/scenarios/paging.py | 1 - 1 file changed, 1 deletion(-) diff --git a/engine/apps/slack/scenarios/paging.py b/engine/apps/slack/scenarios/paging.py index 542d4e00c6..ce9891dccb 100644 --- a/engine/apps/slack/scenarios/paging.py +++ b/engine/apps/slack/scenarios/paging.py @@ -49,7 +49,6 @@ def add_or_update_item(payload, key, item_pk, policy): metadata = json.loads(payload["view"]["private_metadata"]) metadata[key][item_pk] = policy updated_metadata = json.dumps(metadata) - print(updated_metadata) if len(updated_metadata) > PRIVATE_METADATA_MAX_LENGTH: raise ValueError("Cannot add entry, maximum exceeded") payload["view"]["private_metadata"] = updated_metadata