feat(integrations): Ignore duplicate group resolution #82001
Conversation
On integrations that sync status like jira, it can override the user's in app resolution. Workflow: - User marks an issue as resolved in release A - Group activity is recorded saying user resolved in release - We sync status to jira resolving the linked issue - Jira webhooks us back attempting to sync status (resolve in next release) - We update the group resolution to resolve in next release but we don't create an activity. When the issue regresses it is confusing since the GroupResolution and Group Activities don't make sense. This change would ignore the jira webhook if the group is already resolved and was resolved in the last 3 minutes. fixes #74448
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 #82001 +/- ##
=======================================
Coverage 80.35% 80.35%
=======================================
Files 7279 7279
Lines 321322 321378 +56
Branches 20957 20957
=======================================
+ Hits 258207 258254 +47
- Misses 62701 62710 +9
Partials 414 414 |
| # which would override the in-app resolution | ||
| for group in affected_groups: | ||
| if group.status == GroupStatus.RESOLVED and group_was_recently_resolved(group): | ||
| affected_groups.remove(group) |
There was a problem hiding this comment.
Is it safe to remove items from a list you're iterating through? I always thought of this as bad practice but I'm not sure how Python handles this.
There was a problem hiding this comment.
Just tested this, and removing items in place can result in skipping over list items unintentionally. Verified this behavior with the example from this SO post: https://stackoverflow.com/questions/4960968/how-to-safely-remove-elements-from-a-list-in-python
There was a problem hiding this comment.
whoops i remember seeing that before. python really has that nice .remove function that is so tempting to use
| if group.status == GroupStatus.RESOLVED and group_was_recently_resolved(group): | ||
| recently_resolved_groups.append(group) | ||
|
|
||
| affected_groups = [ | ||
| group for group in affected_groups if group not in recently_resolved_groups | ||
| ] |
There was a problem hiding this comment.
Super tiny nit, totally optional: this is a bit cleaner if you invert the check and append to a different list when a group has not been resolved recently. I'm not sure how many possible groups a single status inbound task can handle, but I imagine it won't make much of a practical difference.
related to #82001 logs when the group activities don't make sense
On integrations that sync status like jira, it can override the user's in-app resolution.
Problem Workflow:
This change would ignore the jira webhook if the group is already resolved and was resolved in the last 3 minutes.
fixes #74448
more internal notion docs
will follow up with a PR to test that GroupResolution and Group Activity always make sense