fix(aci): ignore groups that shouldn't be manually resolved #102729
Conversation
github-actions
bot
added
the
Scope: Backend
| continue | ||
|
|
||
| # Users should only be able to manually resolve error issues | ||
| if group.type != ErrorGroupType.type_id: |
There was a problem hiding this comment.
This seems like it'd be a GroupType attribute check, not a type comparison. eg "enable_user_priority_change", "enable_auto_resolve"..
There was a problem hiding this comment.
Sure, that works too.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| # Users should only be able to manually resolve error issues | ||
| if not get_group_type_by_type_id(group.type).enable_user_priority_changes: | ||
| continue |
There was a problem hiding this comment.
Bug: Misused Flag Breaks Manual Resolution Flow Integrity
The new manual resolution check incorrectly uses enable_user_priority_changes (an attribute for priority changes) to control resolution, which can lead to unintended behavior. The accompanying comment is also misleading. Additionally, this check is only applied in handle_resolve_in_release(), allowing the restriction to be bypassed via other resolution paths.
There was a problem hiding this comment.
Groups permit manual priority updates iff they permit manual resolution updates. As for the second point, this was the only resolution path that I found in the API path, which is what we care about.
| continue | ||
|
|
||
| # Users should only be able to manually resolve error issues | ||
| if not get_group_type_by_type_id(group.type).enable_user_priority_changes: |
There was a problem hiding this comment.
you can use group.issue_type here.
| continue | ||
|
|
||
| # Users should only be able to manually resolve error issues | ||
| if not get_group_type_by_type_id(group.type).enable_user_priority_changes: |
There was a problem hiding this comment.
I'm not sure if 'priority changes' covers this case. My suggestion was to add a new field, but if an existing one covers it that's even better. I think enable_user_priority_changes defaults to True for all but metric issues, so this may be behaviorally right, it just doesn't fall under what I'd consider a priority change.
There was a problem hiding this comment.
Yeah, I left a comment explaining it. enable_user_priority_changes should probably default to False for all except Error issues. Since manual priority change is permitted if and only if manual resolution is permitted, I could rename the flag?
There was a problem hiding this comment.
Rename to broaden/clarify sounds good to me.
|
|
||
| # Users should only be able to manually resolve error issues | ||
| if not get_group_type_by_type_id(group.type).enable_user_status_and_priority_changes: | ||
| continue |
There was a problem hiding this comment.
Bug: Inconsistent Status Handling and Feedback for Changes
The enable_user_status_and_priority_changes flag is inconsistently applied. It's checked for priority and resolution, but not other status updates (e.g., ignored, unresolved), allowing issues like metric issues to be ignored but not resolved. Also, priority changes return a 400 for unsupported issues, while resolution silently skips them, causing confusing user feedback.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #102729 +/- ##
========================================
Coverage 80.91% 80.92%
========================================
Files 8929 8932 +3
Lines 391242 391238 -4
Branches 24866 24866
========================================
+ Hits 316574 316597 +23
+ Misses 74300 74273 -27
Partials 368 368 |
| priority=result["priority"], | ||
| group_list=groups, | ||
| acting_user=acting_user, | ||
| project_lookup=project_lookup, |
There was a problem hiding this comment.
I think the bot has a point. It seems more consistent to have a check on line 225 or so that does the if any( ..): return BAD_REQUEST like above.
handle_resolve_in_release should _maybe _ also check and raise a basic exception just because it's hard to claim the contract of handle_resolve_in_release ensures that the groups are all valid and it's be easy to accidentally reuse the code and allow misbehavior.
Co-authored-by: Kyle Consalus <kyle.consalus@sentry.io>
Metric issues should not be manually resolved. We prevent this on the frontend, but not in API calls. Add logic to ignore these groups on the backend. [Fixes SENTRY-5BXQ](https://sentry.sentry.io/issues/6995480460/) --------- Co-authored-by: Kyle Consalus <kyle.consalus@sentry.io>
Metric issues should not be manually resolved. We prevent this on the frontend, but not in API calls. Add logic to ignore these groups on the backend. [Fixes SENTRY-5BXQ](https://sentry.sentry.io/issues/6995480460/) --------- Co-authored-by: Kyle Consalus <kyle.consalus@sentry.io>
Metric issues should not be manually resolved. We prevent this on the frontend, but not in API calls. Add logic to ignore these groups on the backend.
Fixes SENTRY-5BXQ