diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f0449f4..08152a99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Handle slack metadata limit when creating paging command payload ([#2007](https://github.com/grafana/oncall/pull/2007)) - Fix issue with sometimes cached final schedule not being refreshed after an update ([#2004](https://github.com/grafana/oncall/pull/2004)) ## v1.2.28 (2023-05-24) diff --git a/engine/apps/slack/constants.py b/engine/apps/slack/constants.py index 24572538..34f7a3c6 100644 --- a/engine/apps/slack/constants.py +++ b/engine/apps/slack/constants.py @@ -9,3 +9,5 @@ SLACK_WRONG_TEAM_NAMES = [SLACK_INVALID_AUTH_RESPONSE, PLACEHOLDER] 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 9b9272a2..ce9891dc 100644 --- a/engine/apps/slack/scenarios/paging.py +++ b/engine/apps/slack/scenarios/paging.py @@ -10,6 +10,7 @@ from apps.alerts.paging import ( 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,10 @@ USERS_DATA_KEY = "users" 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) + 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 +217,13 @@ class OnPagingUserChange(scenario_step.ScenarioStep): ) 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 +242,17 @@ class OnPagingItemActionChange(scenario_step.ScenarioStep): 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 +283,15 @@ class OnPagingConfirmUserChange(scenario_step.ScenarioStep): } # 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 +312,13 @@ class OnPagingScheduleChange(scenario_step.ScenarioStep): 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 +332,7 @@ class OnPagingScheduleChange(scenario_step.ScenarioStep): 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 +371,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 caf7a315..bb7b7cac 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()