chore(deletions): Remove old group deletion task #96457
Conversation
github-actions
bot
added
the
Scope: Backend
armenzg
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #96457 +/- ##
==========================================
- Coverage 83.83% 83.82% -0.01%
==========================================
Files 10642 10643 +1
Lines 614424 614375 -49
Branches 24127 24127
==========================================
- Hits 515087 515022 -65
- Misses 98956 98972 +16
Partials 381 381 |
| @@ -142,9 +73,6 @@ def delete_groups_for_project( | |||
| f"is greater than GROUP_CHUNK_SIZE" | |||
| ) | |||
|
|
|||
| # This is a no-op on the Snuba side, however, one day it may not be. | |||
| eventstream_state = eventstream.backend.start_delete_groups(project_id, object_ids) | |||
There was a problem hiding this comment.
If we have Snuba events to delete we will call it here:
sentry/src/sentry/deletions/defaults/group.py
Lines 179 to 180 in f10b7e2
We have always been calling it more than once, this can be seen in the tests:
| @@ -4311,78 +4311,55 @@ def assert_deleted_groups(self, groups: Sequence[Group]) -> None: | |||
| assert not Group.objects.filter(id=group.id).exists() | |||
| assert not GroupHash.objects.filter(group_id=group.id).exists() | |||
|
|
|||
| @patch("sentry.eventstream.backend") | |||
| def test_delete_by_id(self, mock_eventstream: MagicMock) -> None: | |||
There was a problem hiding this comment.
This test does a few more things than testing that using id= works. I have simplified the test.
|
|
||
| self.login_as(user=self.user) | ||
| with self.feature("organizations:global-views"): | ||
| response = self.get_response( | ||
| qs_params={"id": [group1.id, group2.id], "group4": group4.id} |
There was a problem hiding this comment.
I think the original test introduced this non-useful parameter by mistake: "group4": group4.id
| extra_data=( | ||
| { | ||
| "transaction_id": transaction_id, | ||
| "project_id": self.project.id, |
There was a problem hiding this comment.
As I mentioned in one of my other PRs, project_id is a required parameter for Snuba (besides the group ids).
| assert mock_send.call_args_list == [ | ||
| call( | ||
| self.project.id, | ||
| "start_delete_groups", |
| ), | ||
| call( | ||
| self.project.id, | ||
| "end_delete_groups", |
|
|
||
| self.assert_deleted_groups(groups_1 + groups_2) | ||
|
|
||
| def test_bulk_delete_for_many_projects_with_option(self) -> None: |
There was a problem hiding this comment.
This test is now the default one.
| @patch("sentry.utils.audit.log_service.record_audit_log") | ||
| def test_delete_by_id(self, mock_record_audit_log, mock_eventstream): |
There was a problem hiding this comment.
Similar changes as what you read on the previous test, however, this test does not do any of the eventstream tests.
|
|
||
| self.login_as(user=self.user) | ||
| # Group 4 will not be deleted because it belongs to a different project |
There was a problem hiding this comment.
We already have other tests handling cross project deleting.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This deletes the old task plus updates some tests to work properly with the new task. Follow-up to #96310.
This deletes the old task plus updates some tests to work properly with the new task.
Follow-up to #96310.