Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔧 chore(aci): add action group to action handler abc #87259

Merged
merged 4 commits into from
Mar 19, 2025

Conversation

iamrajjoshi
Copy link
Member

so that the ui can group different action types together, we introduce a action_group class variable on the NotificationActionHandler which will let us group different types of notification actions togather.

defined the groups based on
image

@iamrajjoshi iamrajjoshi requested a review from ameliahsu March 18, 2025 01:12
@iamrajjoshi iamrajjoshi self-assigned this Mar 18, 2025
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 18, 2025
@iamrajjoshi iamrajjoshi marked this pull request as ready for review March 18, 2025 15:38
@iamrajjoshi iamrajjoshi requested a review from a team as a code owner March 18, 2025 15:38
@iamrajjoshi iamrajjoshi requested a review from saponifi3d March 18, 2025 15:38
@iamrajjoshi iamrajjoshi changed the title 🔧 chore(aci): add action group to action handlers 🔧 chore(aci): add action group to action handler abc Mar 18, 2025
Copy link

codecov bot commented Mar 18, 2025

Codecov Report

Attention: Patch coverage is 98.24561% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...low_engine/handlers/action/notification/handler.py 98.07% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #87259      +/-   ##
==========================================
- Coverage   87.74%   87.73%   -0.01%     
==========================================
  Files        9863     9863              
  Lines      558344   558658     +314     
  Branches    22024    22024              
==========================================
+ Hits       489892   490164     +272     
- Misses      68069    68111      +42     
  Partials      383      383              

Copy link

codecov bot commented Mar 18, 2025

Codecov Report

Attention: Patch coverage is 98.24561% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...low_engine/handlers/action/notification/handler.py 98.07% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           master   #87259     +/-   ##
=========================================
  Coverage   87.74%   87.74%             
=========================================
  Files        9863     9864      +1     
  Lines      558344   559356   +1012     
  Branches    22024    22024             
=========================================
+ Hits       489892   490830    +938     
- Misses      68069    68143     +74     
  Partials      383      383             

@iamrajjoshi iamrajjoshi requested a review from Copilot March 19, 2025 05:25

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces an action group feature for notification action handlers so that actions can be categorized into groups (e.g. NOTIFICATION, TICKET_CREATION, OTHER). The changes add a new ActionGroup enumeration in the types module and update the handler registrations to assign each action handler a group.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/sentry/workflow_engine/handlers/action/notification/handler.py Added new functions to execute notifications via a group type registry and updated action handler classes to assign groups.
src/sentry/workflow_engine/types.py Introduced a new ActionGroup enum and a group class variable on ActionHandler.
Comments suppressed due to low confidence (2)

src/sentry/workflow_engine/handlers/action/notification/handler.py:133

  • Rename 'GithubActionHandler' to 'GitHubActionHandler' for consistency with the proper branding.
class GithubActionHandler(NotificationActionHandler):

src/sentry/workflow_engine/handlers/action/notification/handler.py:138

  • Rename 'GithubEnterpriseActionHandler' to 'GitHubEnterpriseActionHandler' to maintain consistent naming conventions.
class GithubEnterpriseActionHandler(NotificationActionHandler):


@action_handler_registry.register(Action.Type.DISCORD)
class DiscordActionHandler(NotificationActionHandler):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 such SOLID design - also like that now we have a place to easily override the data_schema if any of these require different data attributes (same thing for the config 🎉) -- we could now have strict enforcement per type. For example, that would mean if Discord requires the target_identifier and none of the other properties, we could setup the config_schema on this class to be different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep i saw how useful it would be here when i made this pr to override the config schema:
#87372

detector.type,
extra={"detector_id": detector.id, "action_id": action.id},
)
execute_via_group_type_registry(job, action, detector)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to make this even moar SOILD, you could define the execute interface on the ABC then invoke the execute_via_group_type_registry in each class. it's a bit of replication, but it would be how you uphold the I in solid (inversion principle).

that said, i don't think we'd gain much value from doing that work unless we plan to start overriding this method in the individual action handlers (we could do this refactor when that use case arises), just wanted to call out the example. :)


@action_handler_registry.register(Action.Type.MSTEAMS)
class MsteamsActionHandler(NotificationActionHandler):
group = NotificationActionHandler.ActionGroup.NOTIFICATION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
group = NotificationActionHandler.ActionGroup.NOTIFICATION
group = ActionHandler.Group.NOTIFICATION

^ we should probably access this from the top level ActionHandler -- that way it's a little clearer that this is defined a little higher up the OOP stack.

@@ -49,6 +49,13 @@ class ActionHandler:
config_schema: ClassVar[dict[str, Any]]
data_schema: ClassVar[dict[str, Any]]

class ActionGroup(StrEnum):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class ActionGroup(StrEnum):
class Group(StrEnum):

nit: ActionHandler.ActionGroup makes Action in ActionGroup feel a little redundant. Then in each handler it could be: ActionHandler.Group.TICKET_CREATION which reads pretty cleanly to me.

@iamrajjoshi iamrajjoshi merged commit c8f1a90 into master Mar 19, 2025
48 checks passed
@iamrajjoshi iamrajjoshi deleted the raj/aci/action-group branch March 19, 2025 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants