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

feat(issue-states): Add presave checks for GroupSubStatus #47420

Merged
merged 15 commits into from
Apr 18, 2023

Conversation

snigdhas
Copy link
Member

  • Add presave checks for GroupSubStatus to ensure we don't end up with inconsistent status/substatus pairings
  • Add substatuses in the code when we change the status values
  • Update tests to include substatuses in test data
  • Add tests to check the substatus presave validation

WOR-2973

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 14, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #47420 (05b49da) into master (d3aab77) will decrease coverage by 0.49%.
The diff coverage is 88.88%.

❗ Current head 05b49da differs from pull request most recent head 28fa05a. Consider uploading reports for the commit 28fa05a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #47420      +/-   ##
==========================================
- Coverage   80.73%   80.25%   -0.49%     
==========================================
  Files        4760     4759       -1     
  Lines      201264   201328      +64     
  Branches    11626    11626              
==========================================
- Hits       162481   161566     -915     
- Misses      38525    39504     +979     
  Partials      258      258              
Impacted Files Coverage Δ
src/sentry/api/helpers/group_index/delete.py 96.15% <ø> (ø)
src/sentry/deletions/defaults/group.py 100.00% <ø> (ø)
src/sentry/receivers/releases.py 93.63% <ø> (ø)
src/sentry/tasks/auto_resolve_issues.py 93.33% <ø> (ø)
src/sentry/models/group.py 93.22% <81.81%> (-0.39%) ⬇️
src/sentry/api/helpers/group_index/update.py 94.21% <100.00%> (+0.24%) ⬆️
src/sentry/issues/status_change.py 100.00% <100.00%> (ø)
src/sentry/models/release.py 96.98% <100.00%> (ø)
src/sentry/tasks/clear_expired_snoozes.py 100.00% <100.00%> (ø)
...c/sentry/tasks/integrations/sync_status_inbound.py 96.29% <100.00%> (+0.14%) ⬆️

... and 64 files with indirect coverage changes

@snigdhas snigdhas marked this pull request as ready for review April 17, 2023 17:25
@snigdhas snigdhas requested review from a team April 17, 2023 17:25
@snigdhas snigdhas requested a review from a team as a code owner April 17, 2023 17:25
@snigdhas snigdhas requested review from a team April 17, 2023 17:25
Copy link
Member

@armenzg armenzg left a comment

Choose a reason for hiding this comment

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

Thanks for all the effort on this PR! It must have not been the most fun to go through all those files.

instance.status not in [GroupStatus.UNRESOLVED, GroupStatus.IGNORED]
and instance.substatus is not None
):
raise ValueError(
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should use logger.exception first and then on a follow up PR make all the ValueError changes.

What do you think?

Maybe I'm paranoid.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a great idea - we can let it run for a week and switch to ValueError if things look safe

src/sentry/models/group.py Outdated Show resolved Hide resolved
src/sentry/tasks/integrations/sync_status_inbound.py Outdated Show resolved Hide resolved
Copy link
Member

@armenzg armenzg left a comment

Choose a reason for hiding this comment

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

🎉

@snigdhas snigdhas merged commit 24daa10 into master Apr 18, 2023
@snigdhas snigdhas deleted the snigdha/substatus-presave branch April 18, 2023 16:35
@snigdhas snigdhas added this to the Issue States and Filters milestone Apr 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants