-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(integration): Expand Issue Alert notification #66625
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #66625 +/- ##
==========================================
+ Coverage 84.29% 84.32% +0.02%
==========================================
Files 5306 5315 +9
Lines 237188 237647 +459
Branches 41023 41117 +94
==========================================
+ Hits 199948 200405 +457
- Misses 37021 37023 +2
Partials 219 219
|
|
|
||
| @dataclass(frozen=True) | ||
| class IssueAlertNotificationMessage(BaseNotificationMessage): | ||
| # TODO(Yash): do we really need this entire model, or can we whittle it down to what we need? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I saw a bunch of these so far. Can you create a ticket so if in future someone else owns this know what the plan is to figure these out and refactor? Bonus point for adding ticket number to comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let me add to the backlog!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! #66751
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the comments too :D
| "rule_action_uuid": rule_action_uuid, | ||
| }, | ||
| ) | ||
| raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this being handled in the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to the caller to decide how they want to proceed, and if this is a blocking data piece for them or not. For our use case in threads notification, we'll probably block, or fallback to just posting the message as a normal message in the channel (as apposed to a thread)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something we can get clarity on from product
For Issue Alert notification messages, we need to be able to retrieve the parent message such that we can create a thread. Added logic to help achieve that query and flow, along with some tests.
Requires: #66623