From da7f07ffd60b10953b8a9d475754d4ec7b74f8de Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Mon, 15 Jan 2024 11:34:40 -0500 Subject: [PATCH] Fix occasional `AttributeError` in `apps.grafana_plugin.tasks.sync.sync_organization_async` task (#3687) # Which issue(s) this PR fixes Fix this issue I came across in a celery task retry exception log: ![Screenshot 2024-01-15 at 11 21 13](https://github.com/grafana/oncall/assets/9406895/ed08f2f1-dc7d-4ad3-88a0-dc02cd740582) ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- CHANGELOG.md | 8 +++++++ engine/apps/grafana_plugin/helpers/client.py | 2 +- engine/apps/user_management/sync.py | 2 +- .../apps/user_management/tests/test_sync.py | 24 +++++++++++++------ 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb9f025a..5b372914 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +## v1.3.87 (2024-01-15) + +### Fixed + +- Fix occasional `AttributeError` in `apps.grafana_plugin.tasks.sync.sync_organization_async` task by @joeyorlando ([#3687](https://github.com/grafana/oncall/pull/3687)) + ## v1.3.86 (2024-01-12) ### Fixed diff --git a/engine/apps/grafana_plugin/helpers/client.py b/engine/apps/grafana_plugin/helpers/client.py index 4544f5b9..142a28ed 100644 --- a/engine/apps/grafana_plugin/helpers/client.py +++ b/engine/apps/grafana_plugin/helpers/client.py @@ -196,7 +196,7 @@ class GrafanaAPIClient(APIClient): class PluginSettings(typing.TypedDict): enabled: bool - jsonData: typing.Dict[str, str] + jsonData: typing.NotRequired[typing.Dict[str, str]] class TeamsResponse(_BaseGrafanaAPIResponse): teams: typing.List["GrafanaAPIClient.Types.GrafanaTeam"] diff --git a/engine/apps/user_management/sync.py b/engine/apps/user_management/sync.py index 94be2ebc..d3bd1d28 100644 --- a/engine/apps/user_management/sync.py +++ b/engine/apps/user_management/sync.py @@ -54,7 +54,7 @@ def _sync_organization(organization: Organization) -> None: grafana_incident_settings, _ = grafana_api_client.get_grafana_incident_plugin_settings() if grafana_incident_settings is not None: organization.is_grafana_incident_enabled = grafana_incident_settings["enabled"] - organization.grafana_incident_backend_url = grafana_incident_settings["jsonData"].get( + organization.grafana_incident_backend_url = grafana_incident_settings.get("jsonData", {}).get( GrafanaAPIClient.GRAFANA_INCIDENT_PLUGIN_BACKEND_URL_KEY ) else: diff --git a/engine/apps/user_management/tests/test_sync.py b/engine/apps/user_management/tests/test_sync.py index 20013ba9..6c0a6d92 100644 --- a/engine/apps/user_management/tests/test_sync.py +++ b/engine/apps/user_management/tests/test_sync.py @@ -177,6 +177,14 @@ def test_sync_users_for_team(make_organization, make_user_for_organization, make @pytest.mark.django_db +@pytest.mark.parametrize( + "get_grafana_incident_plugin_settings_return_value", + [ + ({"enabled": True, "jsonData": {"backendUrl": MOCK_GRAFANA_INCIDENT_BACKEND_URL}}, None), + # missing jsonData (sometimes this is what we get back from the Grafana API) + ({"enabled": True}, None), + ], +) @patch.object(GrafanaAPIClient, "is_rbac_enabled_for_organization", return_value=False) @patch.object( GrafanaAPIClient, @@ -212,21 +220,20 @@ def test_sync_users_for_team(make_organization, make_user_for_organization, make ), ) @patch.object(GrafanaAPIClient, "check_token", return_value=(None, {"connected": True})) -@patch.object( - GrafanaAPIClient, - "get_grafana_incident_plugin_settings", - return_value=({"enabled": True, "jsonData": {"backendUrl": MOCK_GRAFANA_INCIDENT_BACKEND_URL}}, None), -) +@patch.object(GrafanaAPIClient, "get_grafana_incident_plugin_settings") @patch("apps.user_management.sync.org_sync_signal") def test_sync_organization( mocked_org_sync_signal, - _mock_get_grafana_incident_plugin_settings, + mock_get_grafana_incident_plugin_settings, _mock_check_token, _mock_get_teams, _mock_get_users, _mock_is_rbac_enabled_for_organization, + get_grafana_incident_plugin_settings_return_value, make_organization, ): + mock_get_grafana_incident_plugin_settings.return_value = get_grafana_incident_plugin_settings_return_value + organization = make_organization() api_members_response = ( @@ -259,7 +266,10 @@ def test_sync_organization( # check that is_grafana_incident_enabled flag is set assert organization.is_grafana_incident_enabled is True - assert organization.grafana_incident_backend_url == MOCK_GRAFANA_INCIDENT_BACKEND_URL + if get_grafana_incident_plugin_settings_return_value[0].get("jsonData"): + assert organization.grafana_incident_backend_url == MOCK_GRAFANA_INCIDENT_BACKEND_URL + else: + assert organization.grafana_incident_backend_url is None mocked_org_sync_signal.send.assert_called_once_with(sender=None, organization=organization)