ref(slack): Refactor Link Identity View (getsentry#72792)

iamrajjoshi · web-flow · commit 12bb4a95adaa · 2024-06-18T12:36:16.000-04:00
https://www.notion.so/sentry/Slack-ff5b8ab4fd1e4760ab829438ce97ce15 Addresses `bot-3` task. Here, I refactored the view to have two separate methods for get and post. I added bunch of logging and metrics & improved tests.
diff --git a/src/sentry/integrations/slack/views/__init__.py b/src/sentry/integrations/slack/views/__init__.py
@@ -1,6 +1,6 @@
 from typing import Any
 
-from django.http import HttpResponse
+from django.http import HttpRequest, HttpResponse
 from django.urls import reverse
 from django.views.decorators.cache import never_cache as django_never_cache
 from rest_framework.request import Request
@@ -23,7 +23,7 @@ def build_linking_url(endpoint: str, **kwargs: Any) -> str:
     return url
 
 
-def render_error_page(request: Request, status: int, body_text: str) -> HttpResponse:
+def render_error_page(request: Request | HttpRequest, status: int, body_text: str) -> HttpResponse:
     return render_to_response(
         "sentry/integrations/slack/link-team-error.html",
         request=request,
diff --git a/src/sentry/integrations/slack/views/link_identity.py b/src/sentry/integrations/slack/views/link_identity.py
@@ -1,14 +1,21 @@
+import logging
+
 from django.core.signing import BadSignature, SignatureExpired
-from django.http import HttpResponse
+from django.db import IntegrityError
+from django.http import Http404, HttpRequest, HttpResponse
+from django.http.response import HttpResponseBase
 from django.utils.decorators import method_decorator
 from rest_framework.request import Request
 
+from sentry.integrations.slack.views import render_error_page
+from sentry.integrations.slack.views.types import IdentityParams
 from sentry.integrations.types import ExternalProviderEnum, ExternalProviders
 from sentry.integrations.utils import get_identity_or_404
 from sentry.models.identity import Identity
 from sentry.notifications.notificationcontroller import NotificationController
 from sentry.notifications.notifications.integration_nudge import IntegrationNudgeNotification
 from sentry.services.hybrid_cloud.integration.model import RpcIntegration
+from sentry.utils import metrics
 from sentry.utils.signing import unsign
 from sentry.web.frontend.base import BaseView, control_silo_view
 from sentry.web.helpers import render_to_response
@@ -17,6 +24,8 @@
 from . import build_linking_url as base_build_linking_url
 from . import never_cache
 
+_logger = logging.getLogger(__name__)
+
 SUCCESS_LINKED_MESSAGE = (
     "Your Slack identity has been linked to your Sentry account. You're good to go!"
 )
@@ -40,47 +49,112 @@ class SlackLinkIdentityView(BaseView):
     Django view for linking user to slack account. Creates an entry on Identity table.
     """
 
+    _METRICS_SUCCESS_KEY = "sentry.integrations.slack.link_identity_view.success"
+    _METRICS_FAILURE_KEY = "sentry.integrations.slack.link_identity_view.failure"
+
     @method_decorator(never_cache)
-    def handle(self, request: Request, signed_params: str) -> HttpResponse:
+    def dispatch(self, request: HttpRequest, signed_params: str) -> HttpResponseBase:
         try:
             params = unsign(signed_params)
-        except (SignatureExpired, BadSignature):
+        except (SignatureExpired, BadSignature) as e:
+            _logger.warning("dispatch.signature_error", exc_info=e)
+            metrics.incr(self._METRICS_FAILURE_KEY, tags={"error": str(e)}, sample_rate=1.0)
             return render_to_response(
                 "sentry/integrations/slack/expired-link.html",
                 request=request,
             )
 
-        organization, integration, idp = get_identity_or_404(
-            ExternalProviders.SLACK,
-            request.user,
-            integration_id=params["integration_id"],
+        try:
+            organization, integration, idp = get_identity_or_404(
+                ExternalProviders.SLACK,
+                request.user,
+                integration_id=params["integration_id"],
+            )
+        except Http404:
+            _logger.exception(
+                "get_identity_error", extra={"integration_id": params["integration_id"]}
+            )
+            metrics.incr(self._METRICS_FAILURE_KEY + ".get_identity", sample_rate=1.0)
+            return render_error_page(
+                request,
+                status=404,
+                body_text="HTTP 404: Could not find the Slack identity.",
+            )
+
+        _logger.info("get_identity_success", extra={"integration_id": params["integration_id"]})
+        metrics.incr(self._METRICS_SUCCESS_KEY + ".get_identity", sample_rate=1.0)
+        params.update({"organization": organization, "integration": integration, "idp": idp})
+        return super().dispatch(
+            request, organization=organization, integration=integration, idp=idp, params=params
         )
 
-        if request.method != "POST":
-            return render_to_response(
-                "sentry/auth-link-identity.html",
-                request=request,
-                context={"organization": organization, "provider": integration.get_provider()},
+    def get(self, request: Request, *args, **kwargs) -> HttpResponse:
+        params = kwargs["params"]
+        organization, integration = params["organization"], params["integration"]
+
+        return render_to_response(
+            "sentry/auth-link-identity.html",
+            request=request,
+            context={"organization": organization, "provider": integration.get_provider()},
+        )
+
+    def post(self, request: Request, *args, **kwargs) -> HttpResponse:
+        try:
+            params_dict = kwargs["params"]
+            params = IdentityParams(
+                organization=kwargs["organization"],
+                integration=kwargs["integration"],
+                idp=kwargs["idp"],
+                slack_id=params_dict["slack_id"],
+                channel_id=params_dict["channel_id"],
+            )
+        except KeyError as e:
+            _logger.exception("slack.link.missing_params")
+            metrics.incr(
+                self._METRICS_FAILURE_KEY + ".post.missing_params",
+                tags={"error": str(e)},
+                sample_rate=1.0,
+            )
+            return render_error_page(
+                request,
+                status=400,
+                body_text="HTTP 400: Missing required parameters.",
             )
 
-        Identity.objects.link_identity(user=request.user, idp=idp, external_id=params["slack_id"])
+        try:
+            Identity.objects.link_identity(
+                user=request.user, idp=params.idp, external_id=params.slack_id
+            )
+        except IntegrityError:
+            _logger.exception("slack.link.integrity_error")
+            metrics.incr(
+                self._METRICS_FAILURE_KEY + ".post.identity.integrity_error",
+                sample_rate=1.0,
+            )
+            raise Http404
+
+        # TODO: We should use use the dataclass to send the slack response
+        send_slack_response(
+            params.integration, SUCCESS_LINKED_MESSAGE, params.__dict__, command="link"
+        )
 
-        send_slack_response(integration, SUCCESS_LINKED_MESSAGE, params, command="link")
-        has_slack_settings = None
         controller = NotificationController(
             recipients=[request.user],
-            organization_id=organization.id,
+            organization_id=params.organization.id,
             provider=ExternalProviderEnum.SLACK,
         )
         has_slack_settings = controller.user_has_any_provider_settings(ExternalProviderEnum.SLACK)
 
         if not has_slack_settings:
-            IntegrationNudgeNotification(organization, request.user, ExternalProviders.SLACK).send()
+            IntegrationNudgeNotification(
+                params.organization, request.user, ExternalProviders.SLACK
+            ).send()
+
+        _logger.info("link_identity_success", extra={"slack_id": params.slack_id})
+        metrics.incr(self._METRICS_SUCCESS_KEY + ".post.link_identity", sample_rate=1.0)
 
-        # TODO(epurkhiser): We could do some fancy slack querying here to
-        #  render a nice linking page with info about the user their linking.
         return render_to_response(
             "sentry/integrations/slack/linked.html",
             request=request,
-            context={"channel_id": params["channel_id"], "team_id": integration.external_id},
+            context={"channel_id": params.channel_id, "team_id": params.integration.external_id},
         )
diff --git a/src/sentry/integrations/slack/views/types.py b/src/sentry/integrations/slack/views/types.py
@@ -0,0 +1,23 @@
+from dataclasses import dataclass
+
+from sentry.models.identity import IdentityProvider
+from sentry.models.integrations.integration import Integration
+from sentry.services.hybrid_cloud.organization import RpcOrganization
+
+
+@dataclass
+class IdentityParams:
+    organization: RpcOrganization
+    integration: Integration
+    idp: IdentityProvider
+    slack_id: str
+    channel_id: str
+    response_url: str | None = None
+
+    def __init__(self, organization, integration, idp, slack_id, channel_id, response_url=None):
+        self.organization = organization
+        self.integration = integration
+        self.idp = idp
+        self.slack_id = slack_id
+        self.channel_id = channel_id
+        self.response_url = response_url
diff --git a/src/sentry/integrations/slack/views/unlink_identity.py b/src/sentry/integrations/slack/views/unlink_identity.py
@@ -5,10 +5,12 @@
 from django.http import Http404, HttpRequest, HttpResponse
 from django.http.response import HttpResponseBase
 from django.utils.decorators import method_decorator
+from rest_framework.request import Request
 
 from sentry.integrations.slack.utils.notifications import send_slack_response
 from sentry.integrations.slack.views import build_linking_url as base_build_linking_url
-from sentry.integrations.slack.views import never_cache
+from sentry.integrations.slack.views import never_cache, render_error_page
+from sentry.integrations.slack.views.types import IdentityParams
 from sentry.integrations.types import ExternalProviders
 from sentry.integrations.utils import get_identity_or_404
 from sentry.models.identity import Identity
@@ -66,14 +68,20 @@ def dispatch(self, request: HttpRequest, signed_params: str) -> HttpResponseBase
                 "get_identity_error", extra={"integration_id": params["integration_id"]}
             )
             metrics.incr(self._METRICS_FAILURE_KEY + ".get_identity", sample_rate=1.0)
-            raise
+            return render_error_page(
+                request,
+                status=404,
+                body_text="HTTP 404: Could not find the Slack identity.",
+            )
 
         _logger.info("get_identity_success", extra={"integration_id": params["integration_id"]})
         metrics.incr(self._METRICS_SUCCESS_KEY + ".get_identity", sample_rate=1.0)
         params.update({"organization": organization, "integration": integration, "idp": idp})
-        return super().dispatch(request, params=params)
+        return super().dispatch(
+            request, organization=organization, integration=integration, idp=idp, params=params
+        )
 
-    def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse:
+    def get(self, request: Request, *args, **kwargs) -> HttpResponse:
         params = kwargs["params"]
         organization, integration = params["organization"], params["integration"]
 
@@ -83,27 +91,45 @@ def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse:
             context={"organization": organization, "provider": integration.get_provider()},
         )
 
-    def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse:
-        params = kwargs["params"]
-        integration, idp = params["integration"], params["idp"]
+    def post(self, request: Request, *args, **kwargs) -> HttpResponse:
+        try:
+            params_dict = kwargs["params"]
+            params = IdentityParams(
+                organization=kwargs["organization"],
+                integration=kwargs["integration"],
+                idp=kwargs["idp"],
+                slack_id=params_dict["slack_id"],
+                channel_id=params_dict["channel_id"],
+            )
+        except KeyError as e:
+            _logger.exception("slack.unlink.missing_params", extra={"error": str(e)})
+            metrics.incr(self._METRICS_FAILURE_KEY + ".post.missing_params", sample_rate=1.0)
+            return render_error_page(
+                request,
+                status=400,
+                body_text="HTTP 400: Missing required parameters.",
+            )
 
         try:
-            Identity.objects.filter(idp_id=idp.id, external_id=params["slack_id"]).delete()
+            Identity.objects.filter(idp_id=params.idp, external_id=params.slack_id).delete()
         except IntegrityError:
-            _logger.exception("slack.unlink.integrity-error")
+            _logger.exception("slack.unlink.integrity_error")
             metrics.incr(
                 self._METRICS_FAILURE_KEY + ".post.identity.integrity_error",
                 sample_rate=1.0,
             )
             raise Http404
 
-        send_slack_response(integration, SUCCESS_UNLINKED_MESSAGE, params, command="unlink")
+        # TODO: We should use use the dataclass to send the slack response
+        send_slack_response(
+            params.integration, SUCCESS_UNLINKED_MESSAGE, params.__dict__, command="unlink"
+        )
 
-        _logger.info("unlink_identity_success", extra={"slack_id": params["slack_id"]})
+        _logger.info("unlink_identity_success", extra={"slack_id": params.slack_id})
         metrics.incr(self._METRICS_SUCCESS_KEY + ".post.unlink_identity", sample_rate=1.0)
 
         return render_to_response(
             "sentry/integrations/slack/unlinked.html",
             request=request,
-            context={"channel_id": params["channel_id"], "team_id": integration.external_id},
+            context={"channel_id": params.channel_id, "team_id": params.integration.external_id},
         )
