[WIP][Slack] Catch All - Code cleanup #70169

ykamo001 · 2024-05-02T19:30:50Z

While we were trying to add and make changes for features or bug fixes, we noticed a lot of complexity and low hanging issues that could be fixed to help make our lives and this code area easier and safer to work with.

In that vain, we're using this branch as a way to capture and check potential changes

ykamo001 · 2024-05-02T19:35:11Z

src/sentry/notifications/utils/actions.py

-    # Optional label. This falls back to name.
-    label: str | None = None
+@dataclass(kw_only=True)
+class _BaseMessageAction:


Created a base class to reduce repeated fields and help ensure changes are consistent. Also helps understand what the difference is between the block kit variant and the original class

ykamo001 · 2024-05-02T21:50:59Z

src/sentry/integrations/slack/message_builder/base/base.py

-from sentry.notifications.utils.actions import MessageAction
-
-
-def get_slack_button(action: MessageAction) -> Mapping[str, Any]:


remove a bunch of unused methods and functions

codecov · 2024-05-02T22:32:24Z

Codecov Report

Attention: Patch coverage is 98.21429% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 79.87%. Comparing base (0c692d5) to head (c4cc46e).
Report is 985 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #70169      +/-   ##
==========================================
+ Coverage   79.84%   79.87%   +0.03%     
==========================================
  Files        6509     6507       -2     
  Lines      289811   289998     +187     
  Branches    49928    49930       +2     
==========================================
+ Hits       231406   231649     +243     
+ Misses      57992    57936      -56     
  Partials      413      413

Files	Coverage Δ
src/sentry/integrations/slack/__init__.py	`100.00% <ø> (ø)`
...entry/integrations/slack/actions/message_action.py	`100.00% <100.00%> (ø)`
...ry/integrations/slack/message_builder/base/base.py	`100.00% <100.00%> (+35.71%)`	⬆️
...entry/integrations/slack/message_builder/issues.py	`89.50% <100.00%> (-0.42%)`	⬇️
src/sentry/integrations/slack/webhooks/action.py	`89.76% <100.00%> (+0.26%)`	⬆️
src/sentry/notifications/utils/actions.py	`100.00% <100.00%> (ø)`
...ations/slack/message_builder/notifications/base.py	`95.08% <93.54%> (-2.65%)`	⬇️

... and 79 files with indirect coverage changes

ykamo001 · 2024-05-03T00:05:18Z

src/sentry/notifications/utils/actions.py

+    def _get_button_text(self) -> str:
+        return self.name

-    # TODO(mgaeta): Refactor this to be provider-agnostic
-    selected_options: Sequence[Mapping[str, Any]] | None = None
-    option_groups: Sequence[Mapping[str, Any]] | None = None
-    block_id: str | None = None
-    elements: Sequence[Mapping[str, Any]] | None = None
+    def get_button(self) -> Mapping[str, Any]:


this doesn't need to be at the base class, but everything is so hard typed to MessageAction that we can't simply move this to the block kit class, because this is really just needed for slack flows

ykamo001 · 2024-05-03T00:14:47Z

src/sentry/integrations/slack/message_builder/base/block.py

-    @staticmethod
-    def get_link_button(action: MessageAction) -> SlackBlock:
-        return {
-            "type": "section",
-            "text": {
-                "type": "mrkdwn",
-                "text": action.label,
-            },
-            "accessory": {
-                "type": "button",
-                "text": {"type": "plain_text", "text": action.name, "emoji": True},
-                "style": action.style,
-                "value": action.value,
-                "url": action.url,
-            },
-        }


this is needed in getsentry, will need to move slowly

ykamo001 · 2024-05-03T20:39:16Z

src/sentry/integrations/slack/actions/message_action.py

+
+
+@dataclass
+class SlackMessageAction(BaseMessageAction):


Moved this over from BlockKitMessageAction to be more accurate to Slack specific namespace, and added helper functions so it's easier to enact on the object, and add tests to verify functionality and contracts

ykamo001 · 2024-05-03T20:39:40Z

src/sentry/integrations/slack/actions/message_action.py

+        button = {
+            "type": "button",
+            "text": self._get_button_text(),
+        }
+        if self.value:
+            button["action_id"] = self.value
+            button["value"] = self.value
+
+        if self.action_id:
+            button["action_id"] = self.action_id
+
+        if self.url:
+            button["url"] = self.url
+            button["value"] = "link_clicked"


we could add some constants here to keep it safer

ykamo001 · 2024-05-03T20:40:42Z

src/sentry/integrations/slack/message_builder/issues.py

+def _ignore_action(group: Group) -> SlackMessageAction | None:
+    if group.issue_category == GroupCategory.FEEDBACK:
+        return None
+
+    if group.get_status() == GroupStatus.IGNORED:
+        return SlackMessageAction(
+            name="status", label="Mark as Ongoing", value="unresolved:ongoing"
+        )
+
+    return SlackMessageAction(name="status", label="Archive", value="archive_dialog")
+
+
+def _resolve_action(group: Group, project: Project) -> SlackMessageAction:
+    if group.get_status() == GroupStatus.RESOLVED:
+        return SlackMessageAction(
+            name="unresolved:ongoing", label="Unresolve", value="unresolved:ongoing"
+        )
+    if not project.flags.has_releases:
+        return SlackMessageAction(name="status", label="Resolve", value="resolved")
+
+    return SlackMessageAction(
+        name="status",
+        label="Resolve",
+        value="resolve_dialog",
+    )
+
+
+def _assign_action(group: Group) -> SlackMessageAction:


moved these out from being anonymous closures in a function to named functions that we can now test and enforce contracts

ykamo001 · 2024-05-03T20:42:35Z

src/sentry/integrations/slack/message_builder/notifications/base.py

+
+        return {"type": "actions", "elements": actions_block}
+
+    def build(self) -> SlackBlock:


broke this function down and added helper methods to make it easier to read and understand the flow. We can now also add tests to enforce individual logic and force contracts

ykamo001 · 2024-05-03T20:43:38Z

src/sentry/integrations/slack/webhooks/action.py

            if "name" in action_data
        ]

+    @classmethod
+    def get_action_list(


refactored to use early returns for easier reading, and less indentations. Also pulled some common code out for more reusability

ykamo001 · 2024-05-03T20:44:19Z

src/sentry/notifications/utils/actions.py

-    # Optional label. This falls back to name.
-    label: str | None = None
+@dataclass(kw_only=True)
+class BaseMessageAction:


created a base class to help build off and capture commonalities

getsantry · 2024-05-25T07:01:52Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

ykamo001 requested a review from a team May 2, 2024 19:30

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 2, 2024

ykamo001 assigned ykamo001 and cathteng May 2, 2024

ykamo001 commented May 2, 2024

View reviewed changes

ykamo001 force-pushed the refactor-integrations branch from 9f9e34e to 1efe721 Compare May 2, 2024 21:50

ykamo001 commented May 2, 2024

View reviewed changes

vercel bot deployed to Preview May 2, 2024 21:52 View deployment

vercel bot deployed to Preview May 2, 2024 22:09 View deployment

vercel bot deployed to Preview May 2, 2024 22:17 View deployment

vercel bot deployed to Preview May 2, 2024 23:35 View deployment

vercel bot deployed to Preview May 2, 2024 23:53 View deployment

ykamo001 commented May 3, 2024

View reviewed changes

vercel bot deployed to Preview May 3, 2024 00:16 View deployment

ykamo001 and others added 8 commits May 3, 2024 09:46

refactor(slack): Code cleanup

ebfe7ac

remove unused code

fe9208e

move pieces together, wip

c28ac44

remove unused MessageBuilder

0c7da1a

small refactor

0072cf2

move code around

5d01565

put back dependent code

45c0293

put back used method in getsentry

5a17034

ykamo001 force-pushed the refactor-integrations branch from 94b7f17 to 5a17034 Compare May 3, 2024 16:46

vercel bot deployed to Preview May 3, 2024 16:49 View deployment

add more tests, move stuff around, add logging and helper funcs

51def70

vercel bot deployed to Preview May 3, 2024 20:40 View deployment

ykamo001 commented May 3, 2024

View reviewed changes

fix typing

2dd8752

vercel bot deployed to Preview May 3, 2024 20:51 View deployment

ykamo001 added the Do Not Merge Don't merge label May 3, 2024

ykamo001 changed the title ~~refactor(slack): Code cleanup~~ [WIP] Catch All - Code cleanup May 3, 2024

ykamo001 changed the title ~~[WIP] Catch All - Code cleanup~~ [WIP][Slack] Catch All - Code cleanup May 3, 2024

forgot typing

0253ce3

vercel bot deployed to Preview May 3, 2024 20:59 View deployment

fix typing inheritance

c4cc46e

vercel bot deployed to Preview May 3, 2024 22:57 View deployment

getsantry bot added the Stale label May 25, 2024

getsantry bot closed this Jun 2, 2024

github-actions bot locked and limited conversation to collaborators Jun 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[WIP][Slack] Catch All - Code cleanup #70169

[WIP][Slack] Catch All - Code cleanup #70169

Uh oh!

ykamo001 commented May 2, 2024

Uh oh!

ykamo001 May 2, 2024 •

edited

Loading

Uh oh!

ykamo001 May 2, 2024

Uh oh!

codecov bot commented May 2, 2024 •

edited

Loading

Uh oh!

ykamo001 May 3, 2024

Uh oh!

ykamo001 May 3, 2024

Uh oh!

ykamo001 May 3, 2024

Uh oh!

ykamo001 May 3, 2024

Uh oh!

ykamo001 May 3, 2024

Uh oh!

ykamo001 May 3, 2024

Uh oh!

ykamo001 May 3, 2024

Uh oh!

ykamo001 May 3, 2024

Uh oh!

getsantry bot commented May 25, 2024

Uh oh!

Uh oh!

		from sentry.notifications.utils.actions import MessageAction


		def get_slack_button(action: MessageAction) -> Mapping[str, Any]:


		return {"type": "actions", "elements": actions_block}

		def build(self) -> SlackBlock:

Uh oh!

[WIP][Slack] Catch All - Code cleanup #70169

[WIP][Slack] Catch All - Code cleanup #70169

Uh oh!

Conversation

ykamo001 commented May 2, 2024

Uh oh!

ykamo001 May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

getsantry bot commented May 25, 2024

Uh oh!

Uh oh!

ykamo001 May 2, 2024 •

edited

Loading

codecov bot commented May 2, 2024 •

edited

Loading