Summary

Fix some bugs when creating attachments for FileDiffs.

Review Request #13546 — Created Feb. 16, 2024 and submitted Feb. 27, 2024, 8:24 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

There are two more bugs I ran into when creating attachments for a
FileDiff:

When creating a FileAttachment for a given FileDiff, we were creating
a draft for the review request. Diff-specific attachments aren't part
of the usual draft flow for the review request, so this isn't
necessary.
Review requests and review request drafts both have relations to file
attachments. When we create a new attachment for a FileDiff, we were
correctly associating it with the review request, but we were not
associating it with the draft. This meant that if there was a draft
(as there often is when posting a new diff), publishing that draft
would erase the relation to the ReviewRequest.

These objects also have a RelationCounterField for the FileAttachment
relation, which is used when computing things like the ETag to
shortcut if there are no attachments. This change also makes sure
we're saving those fields when adding new attachments.

Testing Done


Ran unit tests.
Posted changes including binary files and no longer saw extra drafts.
Verified that the file_attachment_counts field was set correctly.
Published a diff and saw that my file attachments didn't disappear and

  file_attachment_counts didn't zero out.

Commits

Summary	ID
Fix some bugs when creating attachments for FileDiffs. There are two more bugs I ran into when creating attachments for a FileDiff: 1. When creating a FileAttachment for a given FileDiff, we were creating a draft for the review request. Diff-specific attachments aren't part of the usual draft flow for the review request, so this isn't necessary. 2. Review requests and review request drafts both have relations to file attachments. When we create a new attachment for a FileDiff, we were correctly associating it with the review request, but we were not associating it with the draft. This meant that if there was a draft (as there often is when posting a new diff), publishing that draft would erase the relation to the ReviewRequest. These objects also have a RelationCounterField for the FileAttachment relation, which is used when computing things like the ETag to shortcut if there are no attachments. This change also makes sure we're saving those fields when adding new attachments. Testing Done: - Ran unit tests. - Posted changes including binary files and no longer saw extra drafts. - Verified that the file_attachment_counts field was set correctly. - Published a diff and saw that my file attachments didn't disappear and file_attachment_counts didn't zero out.	ce6ae9ef0fc0c77d83279c1afccdd9d7018e88e2

Summary

Fix some bugs when creating attachments for FileDiffs.

There are two more bugs I ran into when creating attachments for a FileDiff: 1. When creating a FileAttachment for a given FileDiff, we were creating a draft for the review request. Diff-specific attachments aren't part of the usual draft flow for the review request, so this isn't necessary. 2. Review requests and review request drafts both have relations to file attachments. When we create a new attachment for a FileDiff, we were correctly associating it with the review request, but we were not associating it with the draft. This meant that if there was a draft (as there often is when posting a new diff), publishing that draft would erase the relation to the ReviewRequest. These objects also have a RelationCounterField for the FileAttachment relation, which is used when computing things like the ETag to shortcut if there are no attachments. This change also makes sure we're saving those fields when adding new attachments. Testing Done: - Ran unit tests. - Posted changes including binary files and no longer saw extra drafts. - Verified that the file_attachment_counts field was set correctly. - Published a diff and saw that my file attachments didn't disappear and file_attachment_counts didn't zero out.

ce6ae9ef0fc0c77d83279c1afccdd9d7018e88e2

Issues

Description	From	Last Updated
I take it this is the last paragraph in your description. RelationCounterField changes are atomic, made on the table using …	chipx86	Feb. 19, 2024, 9:13 p.m.
Same as above.	chipx86	Feb. 19, 2024, 9:12 p.m.

flake8 passed.

JSHint passed.

reviewboard/attachments/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

I take it this is the last paragraph in your description. RelationCounterField changes are atomic, made on the table using an UPDATE .. WHERE .., and then stored back in the local model. They don't need to be saved.

If saving fixes this, we have a bug somewhere where we're overwriting state (which this ends up doing as well), and we'll need to get to the root of that bug.

david Feb. 19, 2024, 9:15 p.m.

Looks like you maybe misclicked on the comment, but I had just assumed I needed to save the relation counter. Removing that part and tests still pass fine.

reviewboard/attachments/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same as above.
```

Commits:

	Summary	ID
	Fix some bugs when creating attachments for FileDiffs. There are two more bugs I ran into when creating attachments for a FileDiff: 1. When creating a FileAttachment for a given FileDiff, we were creating a draft for the review request. Diff-specific attachments aren't part of the usual draft flow for the review request, so this isn't necessary. 2. Review requests and review request drafts both have relations to file attachments. When we create a new attachment for a FileDiff, we were correctly associating it with the review request, but we were not associating it with the draft. This meant that if there was a draft (as there often is when posting a new diff), publishing that draft would erase the relation to the ReviewRequest. These objects also have a RelationCounterField for the FileAttachment relation, which is used when computing things like the ETag to shortcut if there are no attachments. This change also makes sure we're saving those fields when adding new attachments. Testing Done: - Ran unit tests. - Posted changes including binary files and no longer saw extra drafts. - Verified that the file_attachment_counts field was set correctly. - Published a diff and saw that my file attachments didn't disappear and file_attachment_counts didn't zero out.	8b02de6d19721be6e4963562eb8602cd54dfa7d2
	Fix some bugs when creating attachments for FileDiffs. There are two more bugs I ran into when creating attachments for a FileDiff: 1. When creating a FileAttachment for a given FileDiff, we were creating a draft for the review request. Diff-specific attachments aren't part of the usual draft flow for the review request, so this isn't necessary. 2. Review requests and review request drafts both have relations to file attachments. When we create a new attachment for a FileDiff, we were correctly associating it with the review request, but we were not associating it with the draft. This meant that if there was a draft (as there often is when posting a new diff), publishing that draft would erase the relation to the ReviewRequest. These objects also have a RelationCounterField for the FileAttachment relation, which is used when computing things like the ETag to shortcut if there are no attachments. This change also makes sure we're saving those fields when adding new attachments. Testing Done: - Ran unit tests. - Posted changes including binary files and no longer saw extra drafts. - Verified that the file_attachment_counts field was set correctly. - Published a diff and saw that my file attachments didn't disappear and file_attachment_counts didn't zero out.	ce6ae9ef0fc0c77d83279c1afccdd9d7018e88e2

Diff:

Revision 2 (+90 -16)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/attachments/tests.py
	reviewboard/webapi/resources/diff_file_attachment.py
	reviewboard/webapi/tests/test_diff_file_attachment.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 (7835894)