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

@@ -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:
+        ...

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")

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):

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",

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:

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"
 

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",

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")

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

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(

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)

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],