Browse Source

feat(discord): Parse url for discord channel id in alert (#60490)

Closes https://github.com/getsentry/sentry/issues/54207

Before:
![Screenshot 2023-11-22 at 1 12
20 PM](https://github.com/getsentry/sentry/assets/22582037/bf95b06d-02bf-4add-a627-ba802b2848c0)

After:
<img width="977" alt="Screenshot 2023-11-27 at 1 03 40 PM"
src="https://github.com/getsentry/sentry/assets/22582037/f1d600f7-5d19-4e9f-b509-bfbd57c3dda6">
Julia Hoge 1 year ago
parent
commit
8eb477b9f1

+ 5 - 4
src/sentry/integrations/discord/actions/issue_alert/form.py

@@ -7,6 +7,7 @@ from django.core.exceptions import ValidationError
 from django.forms.fields import ChoiceField
 from django.forms.fields import ChoiceField
 
 
 from sentry.integrations.discord.utils.channel import validate_channel_id
 from sentry.integrations.discord.utils.channel import validate_channel_id
+from sentry.integrations.discord.utils.channel_from_url import get_channel_id_from_url
 from sentry.services.hybrid_cloud.integration import integration_service
 from sentry.services.hybrid_cloud.integration import integration_service
 from sentry.shared_integrations.exceptions import ApiTimeoutError, IntegrationError
 from sentry.shared_integrations.exceptions import ApiTimeoutError, IntegrationError
 
 
@@ -14,7 +15,7 @@ from sentry.shared_integrations.exceptions import ApiTimeoutError, IntegrationEr
 class DiscordNotifyServiceForm(forms.Form):
 class DiscordNotifyServiceForm(forms.Form):
     # NOTE: server (guild id) maps directly to the integration ID
     # NOTE: server (guild id) maps directly to the integration ID
     server = forms.ChoiceField(choices=(), widget=forms.Select())
     server = forms.ChoiceField(choices=(), widget=forms.Select())
-    channel_id = forms.CharField(widget=forms.TextInput())
+    channel_url = forms.CharField(widget=forms.TextInput())
     tags = forms.CharField(required=False, widget=forms.TextInput())
     tags = forms.CharField(required=False, widget=forms.TextInput())
 
 
     def __init__(self, *args: Any, **kwargs: Any) -> None:
     def __init__(self, *args: Any, **kwargs: Any) -> None:
@@ -33,7 +34,7 @@ class DiscordNotifyServiceForm(forms.Form):
 
 
     def clean(self) -> dict[str, object] | None:
     def clean(self) -> dict[str, object] | None:
         cleaned_data: dict[str, object] = super().clean() or {}
         cleaned_data: dict[str, object] = super().clean() or {}
-        channel_id = cleaned_data.get("channel_id")
+        channel_url = cleaned_data.get("channel_url")
         server = cleaned_data.get("server")
         server = cleaned_data.get("server")
         integration = integration_service.get_integration(integration_id=server)
         integration = integration_service.get_integration(integration_id=server)
 
 
@@ -43,10 +44,10 @@ class DiscordNotifyServiceForm(forms.Form):
                 code="invalid",
                 code="invalid",
             )
             )
 
 
-        if channel_id and isinstance(channel_id, str):
+        if channel_url and isinstance(channel_url, str):
             try:
             try:
                 validate_channel_id(
                 validate_channel_id(
-                    channel_id=channel_id,
+                    channel_id=get_channel_id_from_url(channel_url),
                     guild_id=integration.external_id,
                     guild_id=integration.external_id,
                     integration_id=integration.id,
                     integration_id=integration.id,
                     guild_name=integration.name,
                     guild_name=integration.name,

+ 3 - 3
src/sentry/integrations/discord/actions/issue_alert/notification.py

@@ -14,7 +14,7 @@ from sentry.utils import metrics
 class DiscordNotifyServiceAction(IntegrationEventAction):
 class DiscordNotifyServiceAction(IntegrationEventAction):
     id = "sentry.integrations.discord.notify_action.DiscordNotifyServiceAction"
     id = "sentry.integrations.discord.notify_action.DiscordNotifyServiceAction"
     form_cls = DiscordNotifyServiceForm
     form_cls = DiscordNotifyServiceForm
-    label = "Send a notification to the {server} Discord server in the channel with ID: {channel_id} and show tags {tags} in the notification."
+    label = "Send a notification to the {server} Discord server in the channel with ID or URL: {channel_url} and show tags {tags} in the notification."
     prompt = "Send a Discord notification"
     prompt = "Send a Discord notification"
     provider = "discord"
     provider = "discord"
     integration_key = "server"
     integration_key = "server"
@@ -26,7 +26,7 @@ class DiscordNotifyServiceAction(IntegrationEventAction):
                 "type": "choice",
                 "type": "choice",
                 "choices": [(i.id, i.name) for i in self.get_integrations()],
                 "choices": [(i.id, i.name) for i in self.get_integrations()],
             },
             },
-            "channel_id": {"type": "string", "placeholder": "e.g., 1134274732116676679"},
+            "channel_url": {"type": "string", "placeholder": "Paste ID or URL here"},
             "tags": {"type": "string", "placeholder": "e.g., environment,user,my_tag"},
             "tags": {"type": "string", "placeholder": "e.g., environment,user,my_tag"},
         }
         }
 
 
@@ -72,7 +72,7 @@ class DiscordNotifyServiceAction(IntegrationEventAction):
 
 
         return self.label.format(
         return self.label.format(
             server=self.get_integration_name(),
             server=self.get_integration_name(),
-            channel_id=self.get_option("channel_id"),
+            channel_url=self.get_option("channel_url"),
             tags="[{}]".format(", ".join(tags)),
             tags="[{}]".format(", ".join(tags)),
         )
         )
 
 

+ 36 - 0
src/sentry/integrations/discord/utils/channel_from_url.py

@@ -0,0 +1,36 @@
+from venv import logger
+
+from django.core.exceptions import ValidationError
+
+
+def get_channel_id_from_url(channel_url: str) -> str:
+    # https://discord.com/channels/1146873646112059423/1157018214425956502
+    prefix = "https://discord.com/channels/"
+
+    if channel_url.isdigit():
+        return channel_url
+
+    if not channel_url.startswith(prefix):
+        logger.info(
+            "rule.discord.bad_channel_url",
+            extra={
+                "channel_url": channel_url,
+                "reason": "channel URL missing or malformed",
+            },
+        )
+        raise ValidationError("Discord channel URL is missing or formatted incorrectly")
+
+    id_string = channel_url[len(prefix) :]
+    if "/" in id_string:
+        channel_id = id_string.split("/")[1]
+        if channel_id:
+            return channel_id
+
+    logger.info(
+        "rule.discord.bad_channel_url.missing_id",
+        extra={
+            "channel_url": channel_url,
+            "reason": "channel ID missing from URL",
+        },
+    )
+    raise ValidationError("Discord channel id is missing from provided URL")

+ 25 - 44
tests/sentry/integrations/discord/test_issue_alert.py

@@ -29,6 +29,7 @@ class DiscordIssueAlertTest(RuleTestCase):
     def setUp(self):
     def setUp(self):
         self.guild_id = "guild-id"
         self.guild_id = "guild-id"
         self.channel_id = "channel-id"
         self.channel_id = "channel-id"
+        self.channel_url = "https://discord.com/channels/guild-id/channel-id"
         self.discord_user_id = "user1234"
         self.discord_user_id = "user1234"
         self.discord_integration = self.create_integration(
         self.discord_integration = self.create_integration(
             provider="discord",
             provider="discord",
@@ -53,6 +54,7 @@ class DiscordIssueAlertTest(RuleTestCase):
             data={
             data={
                 "server": self.discord_integration.id,
                 "server": self.discord_integration.id,
                 "channel_id": self.channel_id,
                 "channel_id": self.channel_id,
+                "channel_url": self.channel_url,
                 "tags": self.tags,
                 "tags": self.tags,
             }
             }
         )
         )
@@ -233,7 +235,7 @@ class DiscordIssueAlertTest(RuleTestCase):
         form.full_clean()
         form.full_clean()
         assert form.is_valid()
         assert form.is_valid()
         assert int(form.cleaned_data["server"]) == self.discord_integration.id
         assert int(form.cleaned_data["server"]) == self.discord_integration.id
-        assert form.cleaned_data["channel_id"] == self.channel_id
+        assert form.cleaned_data["channel_url"] == self.channel_url
         assert form.cleaned_data["tags"] == self.tags
         assert form.cleaned_data["tags"] == self.tags
         assert mock_validate_channel_id.call_count == 1
         assert mock_validate_channel_id.call_count == 1
 
 
@@ -242,7 +244,7 @@ class DiscordIssueAlertTest(RuleTestCase):
         label = self.rule.render_label()
         label = self.rule.render_label()
         assert (
         assert (
             label
             label
-            == "Send a notification to the Cool server Discord server in the channel with ID: channel-id and show tags [environment, user] in the notification."
+            == f"Send a notification to the Cool server Discord server in the channel with ID or URL: {self.channel_url} and show tags [{self.tags}] in the notification."
         )
         )
 
 
 
 
@@ -250,6 +252,7 @@ class DiscordNotifyServiceFormTest(TestCase):
     def setUp(self):
     def setUp(self):
         self.guild_id = "guild-id"
         self.guild_id = "guild-id"
         self.channel_id = "channel-id"
         self.channel_id = "channel-id"
+        self.channel_url = "https://discord.com/channels/guild-id/channel-id"
         self.discord_integration = self.create_integration(
         self.discord_integration = self.create_integration(
             provider="discord",
             provider="discord",
             name="Cool server",
             name="Cool server",
@@ -264,9 +267,16 @@ class DiscordNotifyServiceFormTest(TestCase):
         )
         )
         self.integrations = [self.discord_integration, self.other_integration]
         self.integrations = [self.discord_integration, self.other_integration]
 
 
+        self.form = DiscordNotifyServiceForm(
+            data={
+                "server": self.discord_integration.id,
+                "channel_url": self.channel_url,
+            },
+            integrations=self.integrations,
+        )
+
     def test_has_choices(self):
     def test_has_choices(self):
-        form = DiscordNotifyServiceForm(integrations=self.integrations)
-        assert form.fields["server"].choices == [  # type: ignore
+        assert self.form.fields["server"].choices == [  # type: ignore
             (self.discord_integration.id, self.discord_integration.name),
             (self.discord_integration.id, self.discord_integration.name),
             (self.other_integration.id, self.other_integration.name),
             (self.other_integration.id, self.other_integration.name),
         ]
         ]
@@ -276,26 +286,13 @@ class DiscordNotifyServiceFormTest(TestCase):
         return_value=None,
         return_value=None,
     )
     )
     def test_valid(self, mock_validate_channel_id):
     def test_valid(self, mock_validate_channel_id):
-        form = DiscordNotifyServiceForm(
-            data={
-                "server": self.discord_integration.id,
-                "channel_id": self.channel_id,
-                "tags": "environment",
-            },
-            integrations=self.integrations,
-        )
-
-        form.full_clean()
-        assert form.is_valid()
+        self.form.full_clean()
+        assert self.form.is_valid()
         assert mock_validate_channel_id.call_count == 1
         assert mock_validate_channel_id.call_count == 1
 
 
     def test_no_channel_id(self):
     def test_no_channel_id(self):
-        form = DiscordNotifyServiceForm(
-            data={"server": self.discord_integration.id},
-            integrations=self.integrations,
-        )
-        form.full_clean()
-        assert not form.is_valid()
+        self.form.full_clean()
+        assert not self.form.is_valid()
 
 
     def test_no_server(self):
     def test_no_server(self):
         form = DiscordNotifyServiceForm(integrations=self.integrations)
         form = DiscordNotifyServiceForm(integrations=self.integrations)
@@ -306,11 +303,12 @@ class DiscordNotifyServiceFormTest(TestCase):
         "sentry.integrations.discord.actions.issue_alert.form.validate_channel_id",
         "sentry.integrations.discord.actions.issue_alert.form.validate_channel_id",
         return_value=None,
         return_value=None,
     )
     )
-    def test_no_tags(self, mock_validate_channel_id):
+    def test_tags(self, mock_validate_channel_id):
         form = DiscordNotifyServiceForm(
         form = DiscordNotifyServiceForm(
             data={
             data={
                 "server": self.discord_integration.id,
                 "server": self.discord_integration.id,
-                "channel_id": self.channel_id,
+                "channel_url": self.channel_url,
+                "tags": "environment",
             },
             },
             integrations=self.integrations,
             integrations=self.integrations,
         )
         )
@@ -324,16 +322,8 @@ class DiscordNotifyServiceFormTest(TestCase):
         side_effect=ValidationError("bad"),
         side_effect=ValidationError("bad"),
     )
     )
     def test_invalid_channel_id(self, mock_validate_channel_id):
     def test_invalid_channel_id(self, mock_validate_channel_id):
-        form = DiscordNotifyServiceForm(
-            data={
-                "server": self.discord_integration.id,
-                "channel_id": self.channel_id,
-            },
-            integrations=self.integrations,
-        )
-
-        form.full_clean()
-        assert not form.is_valid()
+        self.form.full_clean()
+        assert not self.form.is_valid()
         assert mock_validate_channel_id.call_count == 1
         assert mock_validate_channel_id.call_count == 1
 
 
     @mock.patch(
     @mock.patch(
@@ -341,15 +331,6 @@ class DiscordNotifyServiceFormTest(TestCase):
         side_effect=ApiTimeoutError("Discord channel lookup timed out"),
         side_effect=ApiTimeoutError("Discord channel lookup timed out"),
     )
     )
     def test_channel_id_lookup_timeout(self, mock_validate_channel_id):
     def test_channel_id_lookup_timeout(self, mock_validate_channel_id):
-        form = DiscordNotifyServiceForm(
-            data={
-                "server": self.discord_integration.id,
-                "channel_id": self.channel_id,
-                "tags": "environment",
-            },
-            integrations=self.integrations,
-        )
-
-        form.full_clean()
-        assert not form.is_valid()
+        self.form.full_clean()
+        assert not self.form.is_valid()
         assert mock_validate_channel_id.call_count == 1
         assert mock_validate_channel_id.call_count == 1

+ 35 - 0
tests/sentry/integrations/discord/test_utils.py

@@ -7,6 +7,7 @@ from requests.exceptions import Timeout
 
 
 from sentry.integrations.discord.utils.auth import verify_signature
 from sentry.integrations.discord.utils.auth import verify_signature
 from sentry.integrations.discord.utils.channel import ChannelType, validate_channel_id
 from sentry.integrations.discord.utils.channel import ChannelType, validate_channel_id
+from sentry.integrations.discord.utils.channel_from_url import get_channel_id_from_url
 from sentry.shared_integrations.exceptions import ApiTimeoutError, IntegrationError
 from sentry.shared_integrations.exceptions import ApiTimeoutError, IntegrationError
 from sentry.shared_integrations.exceptions.base import ApiError
 from sentry.shared_integrations.exceptions.base import ApiError
 from sentry.testutils.cases import TestCase
 from sentry.testutils.cases import TestCase
@@ -106,3 +107,37 @@ class ValidateChannelTest(TestCase):
             validate_channel_id(
             validate_channel_id(
                 self.channel_id, self.guild_id, self.integration_id, self.guild_name
                 self.channel_id, self.guild_id, self.integration_id, self.guild_name
             )
             )
+
+
+class GetChannelIdFromUrl(TestCase):
+    def test_happy_path(self):
+        channel_id = get_channel_id_from_url("https://discord.com/channels/guild-id/channel-id")
+        assert channel_id == "channel-id"
+
+    def test_happy_path_with_extra_slash(self):
+        channel_id = get_channel_id_from_url("https://discord.com/channels/guild-id/channel-id/")
+        assert channel_id == "channel-id"
+
+    def test_missing_channel_id_with_slash(self):
+        with raises(ValidationError):
+            get_channel_id_from_url("https://discord.com/channels/guild-id/")
+
+    def test_missing_channel_id_no_slash(self):
+        with raises(ValidationError):
+            get_channel_id_from_url("https://discord.com/channels/guild-id")
+
+    def test_missing_guild_and_channel_with_slash(self):
+        with raises(ValidationError):
+            get_channel_id_from_url("https://discord.com/channels/")
+
+    def test_missing_guild_and_channel_no_slash(self):
+        with raises(ValidationError):
+            get_channel_id_from_url("https://discord.com/channels")
+
+    def test_different_link(self):
+        with raises(ValidationError):
+            get_channel_id_from_url("https://different.com")
+
+    def test_just_channel_id(self):
+        channel_id = get_channel_id_from_url("123455")
+        assert channel_id == "123455"