Summary

Fix more deletion bugs with review request drafts.

Review Request #13863 — Created May 16, 2024 and submitted May 21, 2024, 8:43 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Bullet-proofing the API serialization will happen separately in a later
version, since that's likely to be invasive and risky, and we're close
to release for RB7. This change handles the deletion issues.

The one thing which is potentially still undeleted is the screenshots
relation, but we no longer provide any way for users to create
Screenshot objects, so that should be virtually impossible to hit.

Testing Done


Ran unit tests.
Created drafts with various types of data attached and verified that

  after I discarded those drafts, all related data was properly deleted.

Commits

Summary	ID
Fix more deletion bugs with review request drafts. We had two additional deletion bugs lurking with review request drafts. Deleting ("discarding") the draft was failing to delete the associated ChangeDescription and DiffSet objects, causing these to be left orphaned. This was once again exposing the API bug where attempting to serialize a link to an orphaned object was crashing when it tried to get the parent object (in this case the draft that was the parent of a DiffSet that had no associated history object). Bullet-proofing the API serialization will happen separately in a later version, since that's likely to be invasive and risky, and we're close to release for RB7. This change handles the deletion issues. The one thing which is potentially still undeleted is the screenshots relation, but we no longer provide any way for users to create Screenshot objects, so that should be virtually impossible to hit. Testing Done: - Ran unit tests. - Created drafts with various types of data attached and verified that after I discarded those drafts, all related data was properly deleted.	905be5f9139b985726871c9f5664bedea4959c7d

Summary

Fix more deletion bugs with review request drafts.

We had two additional deletion bugs lurking with review request drafts. Deleting ("discarding") the draft was failing to delete the associated ChangeDescription and DiffSet objects, causing these to be left orphaned. This was once again exposing the API bug where attempting to serialize a link to an orphaned object was crashing when it tried to get the parent object (in this case the draft that was the parent of a DiffSet that had no associated history object). Bullet-proofing the API serialization will happen separately in a later version, since that's likely to be invasive and risky, and we're close to release for RB7. This change handles the deletion issues. The one thing which is potentially still undeleted is the screenshots relation, but we no longer provide any way for users to create Screenshot objects, so that should be virtually impossible to hit. Testing Done: - Ran unit tests. - Created drafts with various types of data attached and verified that after I discarded those drafts, all related data was properly deleted.

905be5f9139b985726871c9f5664bedea4959c7d

Issues

Description	From	Last Updated
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	May 16, 2024, 10:28 a.m.
This should be in alphabetical order.	chipx86	May 18, 2024, 3:25 p.m.
What's in the 3 queries for the deletion? Can we flesh this bit out, since we're not using assertQueries()? (Same …	chipx86	May 19, 2024, 11:50 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_signal_handlers.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Fix more deletion bugs with review request drafts. We had two additional deletion bugs lurking with review request drafts. Deleting ("discarding") the draft was failing to delete the associated ChangeDescription and DiffSet objects, causing these to be left orphaned. This was once again exposing the API bug where attempting to serialize a link to an orphaned object was crashing when it tried to get the parent object (in this case the draft that was the parent of a DiffSet that had no associated history object). Bullet-proofing the API serialization will happen separately in a later version, since that's likely to be invasive and risky, and we're close to release for RB7. This change handles the deletion issues. The one thing which is potentially still undeleted is the screenshots relation, but we no longer provide any way for users to create Screenshot objects, so that should be virtually impossible to hit. Testing Done: - Ran unit tests. - Created drafts with various types of data attached and verified that after I discarded those drafts, all related data was properly deleted.	b90424e202660956e416df0f04410cbf32a62ded
	Fix more deletion bugs with review request drafts. We had two additional deletion bugs lurking with review request drafts. Deleting ("discarding") the draft was failing to delete the associated ChangeDescription and DiffSet objects, causing these to be left orphaned. This was once again exposing the API bug where attempting to serialize a link to an orphaned object was crashing when it tried to get the parent object (in this case the draft that was the parent of a DiffSet that had no associated history object). Bullet-proofing the API serialization will happen separately in a later version, since that's likely to be invasive and risky, and we're close to release for RB7. This change handles the deletion issues. The one thing which is potentially still undeleted is the screenshots relation, but we no longer provide any way for users to create Screenshot objects, so that should be virtually impossible to hit. Testing Done: - Ran unit tests. - Created drafts with various types of data attached and verified that after I discarded those drafts, all related data was properly deleted.	905be5f9139b985726871c9f5664bedea4959c7d

Diff:

Revision 2 (+114 -32)

Show changes

	reviewboard/reviews/signal_handlers.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_signal_handlers.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/reviews/tests/test_signal_handlers.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should be in alphabetical order.
```

reviewboard/reviews/tests/test_signal_handlers.py (Diff revision 2)

The issue has been dropped. Show all issues

What's in the 3 queries for the deletion? Can we flesh this bit out, since we're not using assertQueries()? (Same elsewhere.)

david May 19, 2024, 9:25 a.m.

It's basically:
1. Find the ChangeDescription
2. Query to make sure there are no StatusUpdates that need to be updated
3. Set the draft's foreign key to NULL
4. Delete the ChangeDescription.

I feel like we don't gain a ton of utility from making these lists a lot more complex to explain every single query, because if something changes that doesn't affect the number of queries, we still wouldn't know. Switching to assertQueries will be a much clearer win.

chipx86 May 20, 2024, 11:11 p.m.
```
Yeah that's fine. I dropped the issue.
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (eb0aaa5)