From eb849678a6ede27be18488495c678130e2a5afbe Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Thu, 16 Nov 2023 10:41:42 -0300 Subject: [PATCH] Update slack user group update not to retry on some errors (#3363) --- CHANGELOG.md | 1 + engine/apps/slack/errors.py | 6 ++- engine/apps/slack/models/slack_usergroup.py | 21 ++++++--- engine/apps/slack/tests/test_user_group.py | 47 +++++++++++++++++++-- 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d07fa261..7fd5965a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Populate `users` field of the public Shift GET API with `rolling_users` from the type override created from web UI([#3303](https://github.com/grafana/oncall/pull/3303)) +- Do not retry to update slack user group on every API error ([#3363](https://github.com/grafana/oncall/pull/3363)) ## v1.3.58 (2023-11-14) diff --git a/engine/apps/slack/errors.py b/engine/apps/slack/errors.py index 844c8b7f..ac750ac6 100644 --- a/engine/apps/slack/errors.py +++ b/engine/apps/slack/errors.py @@ -53,7 +53,11 @@ class SlackAPIInvalidAuthError(SlackAPIError): class SlackAPIUsergroupNotFoundError(SlackAPIError): - errors = ("no_such_subteam",) + errors = ("no_such_subteam", "subteam_not_found") + + +class SlackAPIInvalidUsersError(SlackAPIError): + errors = ("invalid_users",) class SlackAPIChannelNotFoundError(SlackAPIError): diff --git a/engine/apps/slack/models/slack_usergroup.py b/engine/apps/slack/models/slack_usergroup.py index 0bd45f27..a99c9ee8 100644 --- a/engine/apps/slack/models/slack_usergroup.py +++ b/engine/apps/slack/models/slack_usergroup.py @@ -10,7 +10,13 @@ from django.utils import timezone from apps.api.permissions import RBACPermission from apps.slack.client import SlackClient -from apps.slack.errors import SlackAPIError, SlackAPIPermissionDeniedError +from apps.slack.errors import ( + SlackAPIError, + SlackAPIInvalidUsersError, + SlackAPIPermissionDeniedError, + SlackAPITokenError, + SlackAPIUsergroupNotFoundError, +) from apps.slack.models import SlackTeamIdentity from apps.user_management.models.user import User from common.public_primary_keys import generate_public_primary_key, increase_public_primary_key_length @@ -112,10 +118,15 @@ class SlackUserGroup(models.Model): def update_members(self, slack_ids): sc = SlackClient(self.slack_team_identity) - sc.usergroups_users_update(usergroup=self.slack_id, users=slack_ids) - - self.members = slack_ids - self.save(update_fields=("members",)) + try: + sc.usergroups_users_update(usergroup=self.slack_id, users=slack_ids) + except (SlackAPITokenError, SlackAPIUsergroupNotFoundError, SlackAPIInvalidUsersError) as err: + logger.warning(f"Slack usergroup update failed: {err}") + except SlackAPIError: + raise + else: + self.members = slack_ids + self.save(update_fields=("members",)) def get_users_from_members_for_organization(self, organization): return organization.users.filter( diff --git a/engine/apps/slack/tests/test_user_group.py b/engine/apps/slack/tests/test_user_group.py index 0af070f2..c1f9c09c 100644 --- a/engine/apps/slack/tests/test_user_group.py +++ b/engine/apps/slack/tests/test_user_group.py @@ -4,6 +4,12 @@ import pytest from apps.schedules.models.on_call_schedule import OnCallScheduleQuerySet, OnCallScheduleWeb from apps.slack.client import SlackClient +from apps.slack.errors import ( + SlackAPIError, + SlackAPIInvalidUsersError, + SlackAPITokenError, + SlackAPIUsergroupNotFoundError, +) from apps.slack.models import SlackUserGroup from apps.slack.tasks import ( populate_slack_usergroups_for_team, @@ -20,14 +26,49 @@ def test_update_members(make_organization_with_slack_team_identity, make_slack_u user_group = make_slack_user_group(slack_team_identity) slack_ids = ["slack_id_1", "slack_id_2"] - - with patch.object(SlackClient, "api_call") as mock: + with patch("apps.slack.client.SlackClient.usergroups_users_update") as mock_update: user_group.update_members(slack_ids) - mock.assert_called() + mock_update.assert_called_once() assert user_group.members == slack_ids +@pytest.mark.django_db +@pytest.mark.parametrize("exception", [SlackAPITokenError, SlackAPIUsergroupNotFoundError, SlackAPIInvalidUsersError]) +def test_slack_user_group_update_errors( + make_organization_with_slack_team_identity, + make_slack_user_group, + exception, +): + organization, slack_team_identity = make_organization_with_slack_team_identity() + user_group = make_slack_user_group(slack_team_identity=slack_team_identity) + + slack_ids = ["slack_id_1", "slack_id_2"] + with patch("apps.slack.client.SlackClient.usergroups_users_update", side_effect=exception("Error")) as mock_update: + user_group.update_members(slack_ids) + + mock_update.assert_called_once() + assert user_group.members is None + + +@pytest.mark.django_db +def test_slack_user_group_update_errors_raise( + make_organization_with_slack_team_identity, + make_slack_user_group, +): + organization, slack_team_identity = make_organization_with_slack_team_identity() + user_group = make_slack_user_group(slack_team_identity=slack_team_identity) + + slack_ids = ["slack_id_1", "slack_id_2"] + with patch( + "apps.slack.client.SlackClient.usergroups_users_update", side_effect=SlackAPIError("Error") + ) as mock_update: + with pytest.raises(SlackAPIError): + user_group.update_members(slack_ids) + + mock_update.assert_called_once() + + @pytest.mark.django_db def test_oncall_slack_user_identities( make_organization_with_slack_team_identity,