Summary

Fix over-eager deletion with drafts.

Review Request #13905 — Created May 28, 2024 and submitted May 28, 2024, 11:13 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change fixes that up to properly check that those objects are no
longer alive.

Unit tests have been added for this case with both of those as well as
file attachments, which didn't have this same problem because of the way
we calculate the file attachment state.

Testing Done


Ran unit tests.
Published a new review request that included a diff and saw that my

  diff wasn't immediately deleted.

Commits

Summary	ID
Fix over-eager deletion with drafts. When I added change descriptions and diffsets to the draft deletion handler, I wasn't thinking about how we delete the draft after publishing it. In that case, we've assigned the change description to the review request, and assigned the diffset to the history, but we haven't cleared them from the draft object. We were therefore deleting the data right after we published it. This change fixes that up to properly check that those objects are no longer alive. Unit tests have been added for this case with both of those as well as file attachments, which didn't have this same problem because of the way we calculate the file attachment state. Testing Done: - Ran unit tests. - Published a new review request that included a diff and saw that my diff wasn't immediately deleted.	59a8cb95c50660ade1ba03302d3c9c0e5b8bd46d

Summary

Fix over-eager deletion with drafts.

When I added change descriptions and diffsets to the draft deletion handler, I wasn't thinking about how we delete the draft after publishing it. In that case, we've assigned the change description to the review request, and assigned the diffset to the history, but we haven't cleared them from the draft object. We were therefore deleting the data right after we published it. This change fixes that up to properly check that those objects are no longer alive. Unit tests have been added for this case with both of those as well as file attachments, which didn't have this same problem because of the way we calculate the file attachment state. Testing Done: - Ran unit tests. - Published a new review request that included a diff and saw that my diff wasn't immediately deleted.

59a8cb95c50660ade1ba03302d3c9c0e5b8bd46d

Issues

Description	From	Last Updated
Might as well pull out changedesc here too.	maubin	May 28, 2024, 11:10 p.m.
Can you pull the diffset out into a local variable? It'll help this read better, and avoids the expense of …	chipx86	May 28, 2024, 11:09 p.m.
line break before binary operator Column: 9 Error code: W503	reviewbot	May 28, 2024, 11:09 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/signal_handlers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
line break before binary operator

Column: 9
Error code: W503
```

reviewboard/reviews/signal_handlers.py (Diff revision 1)

The issue has been resolved. Show all issues

Can you pull the diffset out into a local variable? It'll help this read better, and avoids the expense of attribute lookups on models.

Plus that operator Review Bot complained about.

reviewboard/reviews/signal_handlers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Might as well pull out changedesc here too.
```

Commits:

	Summary	ID
	Fix over-eager deletion with drafts. When I added change descriptions and diffsets to the draft deletion handler, I wasn't thinking about how we delete the draft after publishing it. In that case, we've assigned the change description to the review request, and assigned the diffset to the history, but we haven't cleared them from the draft object. We were therefore deleting the data right after we published it. This change fixes that up to properly check that those objects are no longer alive. Unit tests have been added for this case with both of those as well as file attachments, which didn't have this same problem because of the way we calculate the file attachment state. Testing Done: - Ran unit tests. - Published a new review request that included a diff and saw that my diff wasn't immediately deleted.	4c8f69eb77058beb49bb4f3c34b282aa53c7b0f1
	Fix over-eager deletion with drafts. When I added change descriptions and diffsets to the draft deletion handler, I wasn't thinking about how we delete the draft after publishing it. In that case, we've assigned the change description to the review request, and assigned the diffset to the history, but we haven't cleared them from the draft object. We were therefore deleting the data right after we published it. This change fixes that up to properly check that those objects are no longer alive. Unit tests have been added for this case with both of those as well as file attachments, which didn't have this same problem because of the way we calculate the file attachment state. Testing Done: - Ran unit tests. - Published a new review request that included a diff and saw that my diff wasn't immediately deleted.	59a8cb95c50660ade1ba03302d3c9c0e5b8bd46d

Diff:

Revision 2 (+162 -48)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

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