Add openAPI schema for some internal endpoints #5037

Ferril · 2024-09-18T12:06:50Z

What this PR does

Adds openAPI schema for following endpoints:

/escalation_chain
/escalation_policy
/channel_filter
/user_notification_policy

Which issue(s) this PR closes

Related to https://github.com/grafana/oncall-private/issues/2457

Checklist

Unit, integration, and e2e (if applicable) tests updated
Documentation added (or pr:no public docs PR label added if not required)
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.

- /escalation_chain - /escalation_policy - /channel_filter - /user_notification_policy

vadimkerr

great job 👍

vadimkerr · 2024-09-19T16:04:21Z

engine/apps/api/serializers/channel_filter.py

+    def _get_slack_channel(self, obj) -> SlackChannelDetails | None:
+        if obj.slack_channel_id is not None:
            slack_team_identity = self.context["request"].auth.organization.slack_team_identity
            if slack_team_identity is not None:
                slack_channel = slack_team_identity.get_cached_channels(slack_id=obj.slack_channel_id).first()
                if slack_channel:
-                    result["slack_channel"] = {
+                    return {
                        "display_name": slack_channel.name,
-                        "slack_id": obj.slack_channel_id,
+                        "slack_id": slack_channel.slack_id,
                        "id": slack_channel.public_primary_key,
                    }
+        return None


nit: invert IFs to reduce nesting

#5037 (comment)

vadimkerr · 2024-09-19T16:20:37Z

engine/apps/api/views/escalation_policy.py

-        user_id = self.request.query_params.get("user")
-        slack_channel_id = self.request.query_params.get("slack_channel")
-        channel_filter_id = self.request.query_params.get("channel_filter")


I think these are no longer used, can we remove them?

They have been already removed

https://github.com/grafana/oncall/pull/5037/files#diff-232bde1ff9834bf2d4fea690e3c5c94f77acd148ac0579b4081a8b2f43c37831R46-R56

I mean that frontend/mobile app don't seem to use these filters, can we remove them?

engine/apps/api/views/user_notification_policy.py

vadimkerr · 2024-09-19T16:39:12Z

engine/common/api_helpers/filters.py

@@ -66,6 +67,13 @@ class MultipleChoiceCharFilter(filters.ModelMultipleChoiceFilter):
    pass


+@extend_schema_field(serializers.CharField)
+class ModelChoiceCharFilter(filters.ModelChoiceFilter):


maybe call it ModelChoicePublicPrimaryKeyFilter so it's easier to understand what it's used for? can also add a default to_field_name="public_primary_key" in there

vadimkerr · 2024-09-19T16:45:57Z

engine/apps/api/views/channel_filter.py

+
+@extend_schema_view(
+    list=extend_schema(responses=ChannelFilterSerializer),
+    retrieve=extend_schema(responses=ChannelFilterRetrieveResponseSerializer),


can this be just ChannelFilterSerializer?

vadimkerr · 2024-09-19T16:47:44Z

engine/apps/api/serializers/channel_filter.py

@@ -218,3 +220,15 @@ def update(self, instance, validated_data):
            raise BadRequest(detail="Filtering term of default channel filter cannot be changed")

        return super().update(instance, validated_data)
+
+
+class ChannelFilterRetrieveResponseSerializer(ChannelFilterUpdateSerializer):


maybe ChannelFilterUpdateResponseSerializer would be a better name here? this way it indicates that it's a response from update/create, not retrieve

vadimkerr · 2024-09-19T16:51:08Z

engine/settings/base.py

+    "/channel_filters",
+    "/escalation_chains",
+    "/escalation_policies",
+    "/notification_policies",


might be a good idea to re-generate frontend types and commit them in this PR as well

vadimkerr · 2024-09-20T13:07:16Z

engine/apps/api/serializers/channel_filter.py

+    def _get_slack_channel(self, obj) -> SlackChannelDetails | None:
+        if obj.slack_channel_id is None:
+            return None
+        slack_team_identity = self.context["request"].auth.organization.slack_team_identity
+        if slack_team_identity is not None:
+            slack_channel = slack_team_identity.get_cached_channels(slack_id=obj.slack_channel_id).first()
+            if slack_channel:
+                return {
+                    "display_name": slack_channel.name,
+                    "slack_id": slack_channel.slack_id,
+                    "id": slack_channel.public_primary_key,
+                }
+        return None


Suggested change

def _get_slack_channel(self, obj) -> SlackChannelDetails | None:

if obj.slack_channel_id is None:

return None

slack_team_identity = self.context["request"].auth.organization.slack_team_identity

if slack_team_identity is not None:

slack_channel = slack_team_identity.get_cached_channels(slack_id=obj.slack_channel_id).first()

if slack_channel:

return {

"display_name": slack_channel.name,

"slack_id": slack_channel.slack_id,

"id": slack_channel.public_primary_key,

}

return None

def _get_slack_channel(self, obj) -> SlackChannelDetails | None:

if obj.slack_channel_id is None:

return None

slack_team_identity = self.context["request"].auth.organization.slack_team_identity

if slack_team_identity is None:

return None

slack_channel = slack_team_identity.get_cached_channels(slack_id=obj.slack_channel_id).first()

if slack_channel is None:

return None

return {

"display_name": slack_channel.name,

"slack_id": slack_channel.slack_id,

"id": slack_channel.public_primary_key,

}

check out https://youtu.be/CFRhGnuXG-4?si=WhR8vxKXXN0t7a22 for inspiration 😄

joeyorlando

I think we should revert the pnpm generate-types commit and run that in the packages/grafana-oncall-app directory of grafana/irm instead.

cc: @brojd

Add openAPI schema for some internal endpoints

742272b

- /escalation_chain - /escalation_policy - /channel_filter - /user_notification_policy

Ferril added pr:no public docs Added to a PR that does not require public documentation updates release:ignore PR will not be added to release notes labels Sep 18, 2024

Ferril requested a review from a team as a code owner September 18, 2024 12:06

Ferril temporarily deployed to github-pages September 18, 2024 12:07 — with GitHub Actions Inactive

mypy fix

cf71d6d

Ferril temporarily deployed to github-pages September 18, 2024 12:55 — with GitHub Actions Inactive

vadimkerr reviewed Sep 19, 2024

View reviewed changes

Small openAPI schema fixes

938d81d

Ferril temporarily deployed to github-pages September 20, 2024 10:13 — with GitHub Actions Inactive

Ferril had a problem deploying to github-pages September 20, 2024 10:13 — with GitHub Actions Failure

Fix typo

5e91fe8

Ferril had a problem deploying to github-pages September 20, 2024 10:37 — with GitHub Actions Failure

Ferril temporarily deployed to github-pages September 20, 2024 10:37 — with GitHub Actions Inactive

vadimkerr reviewed Sep 20, 2024

View reviewed changes

Fix after review

589e76c

Ferril temporarily deployed to github-pages September 23, 2024 10:16 — with GitHub Actions Inactive

pnpm generate-types

2acad03

vadimkerr requested a review from a team as a code owner September 27, 2024 12:55

vadimkerr had a problem deploying to github-pages September 27, 2024 12:55 — with GitHub Actions Failure

joeyorlando requested changes Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add openAPI schema for some internal endpoints #5037

Add openAPI schema for some internal endpoints #5037

Ferril commented Sep 18, 2024

vadimkerr left a comment

vadimkerr Sep 19, 2024

vadimkerr Sep 20, 2024

vadimkerr Sep 19, 2024

Ferril Sep 20, 2024

vadimkerr Sep 20, 2024

vadimkerr Sep 19, 2024

vadimkerr Sep 19, 2024

vadimkerr Sep 19, 2024

vadimkerr Sep 19, 2024

vadimkerr Sep 20, 2024

vadimkerr Sep 20, 2024

joeyorlando left a comment

Add openAPI schema for some internal endpoints #5037

Are you sure you want to change the base?

Add openAPI schema for some internal endpoints #5037

Conversation

Ferril commented Sep 18, 2024

What this PR does

Which issue(s) this PR closes

Checklist

vadimkerr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyorlando left a comment

Choose a reason for hiding this comment