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

Handle slack payload metadata limit in paging command #2007

Merged
merged 3 commits into from
May 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions engine/apps/slack/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
58 changes: 48 additions & 10 deletions engine/apps/slack/scenarios/paging.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -47,7 +48,10 @@
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


Expand Down Expand Up @@ -213,8 +217,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"],
Expand All @@ -233,12 +242,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"],
Expand Down Expand Up @@ -269,8 +283,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"],
Expand All @@ -291,8 +312,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"],
Expand All @@ -306,7 +332,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:
Expand Down Expand Up @@ -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)
Expand Down
89 changes: 89 additions & 0 deletions engine/apps/slack/tests/test_scenario_steps/test_paging.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down