✨ feat(aci): use Action & Group for Slack Issue Alert Threads #84821
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #84821 +/- ##
========================================
Coverage 87.55% 87.56%
========================================
Files 9661 9666 +5
Lines 546963 547244 +281
Branches 21566 21566
========================================
+ Hits 478920 479178 +258
- Misses 67705 67728 +23
Partials 338 338 |
|
|
||
| self.record_notification_sent(event, channel, rule, notification_uuid) | ||
|
|
||
| def send_notification_noa(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: |
There was a problem hiding this comment.
this function is extremely long. is there any way to break this up a bit, like the shared logic between this and send_notification? there are also nested functions in here i'm not sure are totally necessary but i get that it was the previous implementation
There was a problem hiding this comment.
i can pull this out, but i didn't want to reuse code b/c it makes feature flagging annoying since we have nested if else indentation deep into the function which makes readability poor.
There was a problem hiding this comment.
actually i am not sure if i can pull it out either (need to test), since we are returning this function itself and the signature can't change. it would have to be a pure function outside of the class in that case but it also needs access to variables that exist in the outer function 🤔
There was a problem hiding this comment.
another day, another punt to notification platform :D
this pr uses the new threads repo i have created and saves/fetches messages based action and group id. it is FF'ed behind the NOA flag.
Note: Links will still be broken in this implementation. this is considered in my spec and i have a later milestone that will fix that once the new UI is in place.