From 686ebbfb379889c0f454e5bbead118078b31343d Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 5 Nov 2024 04:51:04 -0500 Subject: [PATCH] chore: fix some minor issues with recent `slack_channel` changes (#5228) # What this PR does Follow up PR to https://github.com/grafana/oncall/pull/5199 and https://github.com/grafana/oncall/pull/5224, addresses a few issues I noticed on dev while testing the feature ## 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] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes. --- engine/apps/api/serializers/channel_filter.py | 28 +++++- engine/apps/api/serializers/organization.py | 2 +- engine/apps/api/serializers/schedule_base.py | 8 +- .../apps/api/serializers/schedule_calendar.py | 1 + engine/apps/api/serializers/schedule_ical.py | 13 +-- .../api/serializers/schedule_polymorphic.py | 2 +- engine/apps/api/serializers/schedule_web.py | 1 + engine/apps/api/serializers/slack_channel.py | 8 ++ engine/apps/api/tests/test_channel_filter.py | 6 +- engine/apps/api/tests/test_schedules.py | 87 ++++++++++++++++++- .../test_set_org_default_slack_channel.py | 51 +++++++++++ engine/apps/api/views/channel_filter.py | 11 ++- 12 files changed, 194 insertions(+), 24 deletions(-) diff --git a/engine/apps/api/serializers/channel_filter.py b/engine/apps/api/serializers/channel_filter.py index a4bae8e3..a1c796ad 100644 --- a/engine/apps/api/serializers/channel_filter.py +++ b/engine/apps/api/serializers/channel_filter.py @@ -2,7 +2,7 @@ from rest_framework import serializers from apps.alerts.models import AlertReceiveChannel, ChannelFilter, EscalationChain from apps.api.serializers.labels import LabelPairSerializer -from apps.api.serializers.slack_channel import SlackChannelSerializer +from apps.api.serializers.slack_channel import SlackChannelDetails, SlackChannelSerializer from apps.base.messaging import get_messaging_backend_from_id from apps.telegram.models import TelegramToOrganizationConnector from common.api_helpers.custom_fields import ( @@ -29,7 +29,7 @@ class ChannelFilterSerializer(EagerLoadingMixin, serializers.ModelSerializer): allow_null=True, required=False, ) - slack_channel = SlackChannelSerializer(read_only=True, allow_null=True) + slack_channel = SlackChannelSerializer(read_only=True) # TODO: we probably don't need both telegram_channel and telegram_channel_details, research which one isn't needed # and get rid of it @@ -143,7 +143,7 @@ class ChannelFilterSerializer(EagerLoadingMixin, serializers.ModelSerializer): class ChannelFilterCreateSerializer(ChannelFilterSerializer): - slack_channel_id = SlackChannelsFilteredByOrganizationSlackWorkspaceField( + slack_channel = SlackChannelsFilteredByOrganizationSlackWorkspaceField( allow_null=True, required=False, write_only=True, @@ -156,7 +156,6 @@ class ChannelFilterCreateSerializer(ChannelFilterSerializer): "alert_receive_channel", "escalation_chain", "slack_channel", - "slack_channel_id", "created_at", "filtering_labels", "filtering_term", @@ -169,6 +168,15 @@ class ChannelFilterCreateSerializer(ChannelFilterSerializer): ] read_only_fields = ["created_at", "is_default"] + def to_representation(self, obj): + """ + This feels hacky.. it's because the UI currently POST/PUTs using "slack_channel", which is the SLACK ID of + the slack channel that we'd like to set it to, whereas what we return is an object with more details + """ + result = super().to_representation(obj) + result["slack_channel"] = SlackChannelSerializer(obj.slack_channel).data if obj.slack_channel else None + return result + def create(self, validated_data): instance = super().create(validated_data) instance.to_index(0) # the new route should be the first one @@ -188,3 +196,15 @@ class ChannelFilterUpdateSerializer(ChannelFilterCreateSerializer): raise BadRequest(detail="Filtering term of default channel filter cannot be changed") return super().update(instance, validated_data) + + +class ChannelFilterUpdateResponseSerializer(ChannelFilterUpdateSerializer): + """ + This serializer is used in OpenAPI schema to show proper response structure, + as `slack_channel` field expects string on create/update and returns dict on response + """ + + slack_channel = serializers.SerializerMethodField() + + def get_slack_channel(self, obj) -> SlackChannelDetails | None: + ... diff --git a/engine/apps/api/serializers/organization.py b/engine/apps/api/serializers/organization.py index a305b2e9..163c80df 100644 --- a/engine/apps/api/serializers/organization.py +++ b/engine/apps/api/serializers/organization.py @@ -22,7 +22,7 @@ class OrganizationSerializer(EagerLoadingMixin, serializers.ModelSerializer): slack_team_identity = FastSlackTeamIdentitySerializer(read_only=True) name = serializers.CharField(required=False, allow_null=True, allow_blank=True, source="org_title") - slack_channel = SlackChannelSerializer(read_only=True, allow_null=True, required=False) + slack_channel = SlackChannelSerializer(read_only=True, source="default_slack_channel") rbac_enabled = serializers.BooleanField(read_only=True, source="is_rbac_permissions_enabled") grafana_incident_enabled = serializers.BooleanField(read_only=True, source="is_grafana_incident_enabled") diff --git a/engine/apps/api/serializers/schedule_base.py b/engine/apps/api/serializers/schedule_base.py index 7f781bf9..242d6b1a 100644 --- a/engine/apps/api/serializers/schedule_base.py +++ b/engine/apps/api/serializers/schedule_base.py @@ -13,7 +13,7 @@ class ScheduleBaseSerializer(EagerLoadingMixin, serializers.ModelSerializer): id = serializers.CharField(read_only=True, source="public_primary_key") organization = serializers.HiddenField(default=CurrentOrganizationDefault()) team = TeamPrimaryKeyRelatedField(allow_null=True, required=False) - slack_channel = SlackChannelSerializer(read_only=True, allow_null=True, required=False) + slack_channel = SlackChannelSerializer(read_only=True) user_group = UserGroupSerializer() warnings = serializers.SerializerMethodField() on_call_now = serializers.SerializerMethodField() @@ -75,10 +75,8 @@ class ScheduleBaseSerializer(EagerLoadingMixin, serializers.ModelSerializer): def validate(self, attrs): if "slack_channel_id" in attrs: - # this is set by OrganizationFilteredPrimaryKeyRelatedField in the serializer classes - # that subclass ScheduleBaseSerializer - slack_channel = attrs.pop("slack_channel_id", None) - attrs["slack_channel"] = slack_channel + # this is set in the serializer classes which subclass ScheduleBaseSerializer + attrs["slack_channel"] = attrs.pop("slack_channel_id", None) return attrs def create(self, validated_data): diff --git a/engine/apps/api/serializers/schedule_calendar.py b/engine/apps/api/serializers/schedule_calendar.py index 005c4142..d6c35ea4 100644 --- a/engine/apps/api/serializers/schedule_calendar.py +++ b/engine/apps/api/serializers/schedule_calendar.py @@ -26,6 +26,7 @@ class ScheduleCalendarCreateSerializer(ScheduleCalendarSerializer): queryset=SlackChannel.objects, required=False, allow_null=True, + write_only=True, ) user_group = OrganizationFilteredPrimaryKeyRelatedField( filter_field="slack_team_identity__organizations", diff --git a/engine/apps/api/serializers/schedule_ical.py b/engine/apps/api/serializers/schedule_ical.py index 4b912274..4fb1b9f2 100644 --- a/engine/apps/api/serializers/schedule_ical.py +++ b/engine/apps/api/serializers/schedule_ical.py @@ -33,6 +33,7 @@ class ScheduleICalCreateSerializer(ScheduleICalSerializer): queryset=SlackChannel.objects, required=False, allow_null=True, + write_only=True, ) user_group = OrganizationFilteredPrimaryKeyRelatedField( filter_field="slack_team_identity__organizations", @@ -41,12 +42,6 @@ class ScheduleICalCreateSerializer(ScheduleICalSerializer): allow_null=True, ) - def create(self, validated_data): - created_schedule = super().create(validated_data) - # for iCal-based schedules we need to refresh final schedule information - refresh_ical_final_schedule.apply_async((created_schedule.pk,)) - return created_schedule - class Meta: model = OnCallScheduleICal fields = [ @@ -60,6 +55,12 @@ class ScheduleICalCreateSerializer(ScheduleICalSerializer): "ical_url_overrides": {"required": False, "allow_null": True}, } + def create(self, validated_data): + created_schedule = super().create(validated_data) + # for iCal-based schedules we need to refresh final schedule information + refresh_ical_final_schedule.apply_async((created_schedule.pk,)) + return created_schedule + class ScheduleICalUpdateSerializer(ScheduleICalCreateSerializer): class Meta: diff --git a/engine/apps/api/serializers/schedule_polymorphic.py b/engine/apps/api/serializers/schedule_polymorphic.py index e6588652..e8c2db99 100644 --- a/engine/apps/api/serializers/schedule_polymorphic.py +++ b/engine/apps/api/serializers/schedule_polymorphic.py @@ -12,7 +12,7 @@ from common.api_helpers.mixins import EagerLoadingMixin class PolymorphicScheduleSerializer(EagerLoadingMixin, PolymorphicSerializer): - SELECT_RELATED = ["organization", "user_group", "team"] + SELECT_RELATED = ["organization", "team", "user_group", "slack_channel"] resource_type_field_name = "type" diff --git a/engine/apps/api/serializers/schedule_web.py b/engine/apps/api/serializers/schedule_web.py index b8b45388..3a503041 100644 --- a/engine/apps/api/serializers/schedule_web.py +++ b/engine/apps/api/serializers/schedule_web.py @@ -22,6 +22,7 @@ class ScheduleWebCreateSerializer(ScheduleWebSerializer): queryset=SlackChannel.objects, required=False, allow_null=True, + write_only=True, ) user_group = OrganizationFilteredPrimaryKeyRelatedField( filter_field="slack_team_identity__organizations", diff --git a/engine/apps/api/serializers/slack_channel.py b/engine/apps/api/serializers/slack_channel.py index 4447dd8a..9169c226 100644 --- a/engine/apps/api/serializers/slack_channel.py +++ b/engine/apps/api/serializers/slack_channel.py @@ -1,8 +1,16 @@ +import typing + from rest_framework import serializers from apps.slack.models import SlackChannel +class SlackChannelDetails(typing.TypedDict): + display_name: str + slack_id: str + id: str + + class SlackChannelSerializer(serializers.ModelSerializer): id = serializers.CharField(read_only=True, source="public_primary_key") display_name = serializers.CharField(source="name") diff --git a/engine/apps/api/tests/test_channel_filter.py b/engine/apps/api/tests/test_channel_filter.py index ee7d06be..5dd03524 100644 --- a/engine/apps/api/tests/test_channel_filter.py +++ b/engine/apps/api/tests/test_channel_filter.py @@ -715,7 +715,7 @@ def test_channel_filter_with_slack_channel_crud( reverse("api-internal:channel_filter-list"), data={ "alert_receive_channel": alert_receive_channel.public_primary_key, - "slack_channel_id": slack_channel1.slack_id, + "slack_channel": slack_channel1.slack_id, }, format="json", **auth_headers, @@ -732,7 +732,7 @@ def test_channel_filter_with_slack_channel_crud( # update the slack channel url = reverse("api-internal:channel_filter-detail", kwargs={"pk": created_channel_filter["id"]}) - response = client.patch(url, data={"slack_channel_id": slack_channel2.slack_id}, format="json", **auth_headers) + response = client.patch(url, data={"slack_channel": slack_channel2.slack_id}, format="json", **auth_headers) assert response.status_code == status.HTTP_200_OK assert response.json()["slack_channel"] == { @@ -742,7 +742,7 @@ def test_channel_filter_with_slack_channel_crud( } # remove the slack channel - response = client.patch(url, data={"slack_channel_id": None}, format="json", **auth_headers) + response = client.patch(url, data={"slack_channel": None}, format="json", **auth_headers) assert response.status_code == status.HTTP_200_OK assert response.json()["slack_channel"] is None diff --git a/engine/apps/api/tests/test_schedules.py b/engine/apps/api/tests/test_schedules.py index 3a80ac10..4a29dc9d 100644 --- a/engine/apps/api/tests/test_schedules.py +++ b/engine/apps/api/tests/test_schedules.py @@ -640,7 +640,6 @@ def test_create_calendar_schedule(schedule_internal_api_setup, make_user_auth_he "type": 0, "name": "created_calendar_schedule", "time_zone": "UTC", - "slack_channel_id": None, "user_group": None, "team": None, "warnings": [], @@ -671,7 +670,6 @@ def test_create_ical_schedule(schedule_internal_api_setup, make_user_auth_header "ical_url_overrides": None, "name": "created_ical_schedule", "type": 1, - "slack_channel_id": None, "user_group": None, "team": None, "warnings": [], @@ -706,7 +704,6 @@ def test_create_web_schedule(schedule_internal_api_setup, make_user_auth_headers "name": "created_web_schedule", "type": 2, "time_zone": "UTC", - "slack_channel_id": None, "user_group": None, "team": None, "warnings": [], @@ -2459,6 +2456,90 @@ def test_team_not_updated_if_not_in_data( assert schedule.team == team +# we don't need to validate the ical URL when creating an ical schedule.. so just patch that functionality +@patch("apps.api.serializers.schedule_ical.ScheduleICalSerializer.validate_ical_url_primary", return_value=ICAL_URL) +@pytest.mark.parametrize( + "schedule_type,other_create_data", + [ + (0, {}), + (1, {"ical_url_primary": ICAL_URL}), + (2, {}), + ], +) +@pytest.mark.django_db +def test_can_update_slack_channel( + _mock_validate_ical_url_primary, + make_organization_and_user_with_plugin_token, + make_slack_team_identity, + make_slack_channel, + make_user_auth_headers, + schedule_type, + other_create_data, +): + organization, user, token = make_organization_and_user_with_plugin_token() + auth_headers = make_user_auth_headers(user, token) + slack_team_identity = make_slack_team_identity() + organization.slack_team_identity = slack_team_identity + organization.save() + + slack_channel1 = make_slack_channel(slack_team_identity) + slack_channel2 = make_slack_channel(slack_team_identity) + + client = APIClient() + + # we can set it when creating + response = client.post( + reverse("api-internal:schedule-list"), + { + "name": "created_schedule", + "type": schedule_type, + "slack_channel_id": slack_channel1.public_primary_key, + **other_create_data, + }, + format="json", + **auth_headers, + ) + + assert response.status_code == status.HTTP_201_CREATED + + response_data = response.json() + schedule_id = response_data["id"] + url = reverse("api-internal:schedule-detail", kwargs={"pk": schedule_id}) + + # NOTE: the response returned by the POST/PUT endpoint currently doesn't include slack_channel_id + # as it's not used by the UI.. additionally, there was already a bug in it that despite specifying it, it + # would return null.. the proper way to refactor this is to change the name of slack_channel_id used in the + # request (as this clashes with the slack_channel_id db column) + def _assert_slack_channel_updated(new_slack_channel): + response = client.get(url, **auth_headers) + + assert response.status_code == status.HTTP_200_OK + assert response.json()["slack_channel"] == new_slack_channel + + # we can update it + response = client.patch( + url, + data={ + "slack_channel_id": slack_channel2.public_primary_key, + }, + format="json", + **auth_headers, + ) + assert response.status_code == status.HTTP_200_OK + _assert_slack_channel_updated( + { + "id": slack_channel2.public_primary_key, + "display_name": slack_channel2.name, + "slack_id": slack_channel2.slack_id, + } + ) + + # we can unset it + response = client.patch(url, data={"slack_channel_id": None}, format="json", **auth_headers) + assert response.status_code == status.HTTP_200_OK + _assert_slack_channel_updated(None) + + @patch.object(SlackUserGroup, "can_be_updated", new_callable=PropertyMock) @pytest.mark.django_db def test_can_update_user_groups( diff --git a/engine/apps/api/tests/test_set_org_default_slack_channel.py b/engine/apps/api/tests/test_set_org_default_slack_channel.py index 59b0ce9e..b44508e1 100644 --- a/engine/apps/api/tests/test_set_org_default_slack_channel.py +++ b/engine/apps/api/tests/test_set_org_default_slack_channel.py @@ -36,3 +36,54 @@ def test_set_org_default_slack_channel_permissions( response = client.post(url, format="json", **make_user_auth_headers(user, token)) assert response.status_code == expected_status + + +@pytest.mark.django_db +def test_set_organization_slack_default_channel( + make_organization_and_user_with_plugin_token, + make_slack_team_identity, + make_slack_channel, + make_user_auth_headers, +): + slack_team_identity = make_slack_team_identity() + slack_channel = make_slack_channel(slack_team_identity) + + organization, user, token = make_organization_and_user_with_plugin_token() + organization.slack_team_identity = slack_team_identity + organization.save() + + auth_headers = make_user_auth_headers(user, token) + + assert organization.default_slack_channel is None + + client = APIClient() + + def _update_default_slack_channel(slack_channel_id): + # this endpoint doesn't return any data.. + response = client.post( + reverse("api-internal:set-default-slack-channel"), + data={ + "id": slack_channel_id, + }, + format="json", + **auth_headers, + ) + assert response.status_code == status.HTTP_200_OK + + def _assert_default_slack_channel_is_updated(slack_channel_id): + response = client.get(reverse("api-internal:api-organization"), format="json", **auth_headers) + assert response.status_code == status.HTTP_200_OK + assert response.json()["slack_channel"] == slack_channel_id + + _update_default_slack_channel(slack_channel.public_primary_key) + _assert_default_slack_channel_is_updated( + { + "id": slack_channel.public_primary_key, + "display_name": slack_channel.name, + "slack_id": slack_channel.slack_id, + } + ) + + # NOTE: currently the endpoint doesn't allow to remove default slack channel, if and when it does, uncomment this + # _update_default_slack_channel(None) + # _assert_default_slack_channel_is_updated(None) diff --git a/engine/apps/api/views/channel_filter.py b/engine/apps/api/views/channel_filter.py index 5c2ec9d9..f3bc46e8 100644 --- a/engine/apps/api/views/channel_filter.py +++ b/engine/apps/api/views/channel_filter.py @@ -1,5 +1,5 @@ from django_filters import rest_framework as filters -from drf_spectacular.utils import extend_schema +from drf_spectacular.utils import extend_schema, extend_schema_view from rest_framework import status from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticated @@ -10,6 +10,7 @@ from apps.api.permissions import RBACPermission from apps.api.serializers.channel_filter import ( ChannelFilterCreateSerializer, ChannelFilterSerializer, + ChannelFilterUpdateResponseSerializer, ChannelFilterUpdateSerializer, ) from apps.auth_token.auth import PluginAuthentication @@ -33,6 +34,14 @@ class ChannelFilterFilter(ModelFieldFilterMixin, filters.FilterSet): ) +@extend_schema_view( + list=extend_schema(responses=ChannelFilterSerializer), + create=extend_schema(request=ChannelFilterCreateSerializer, responses=ChannelFilterUpdateResponseSerializer), + update=extend_schema(request=ChannelFilterUpdateSerializer, responses=ChannelFilterUpdateResponseSerializer), + partial_update=extend_schema( + request=ChannelFilterUpdateSerializer, responses=ChannelFilterUpdateResponseSerializer + ), +) class ChannelFilterView( TeamFilteringMixin, PublicPrimaryKeyMixin[ChannelFilter],