Fix some bugs when creating attachments for FileDiffs.

Review Request #13546 — Created Feb. 16, 2024 and submitted

Information

Review Board
master

Reviewers

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.

  • 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.
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
Description From Last Updated

I take it this is the last paragraph in your description. RelationCounterField changes are atomic, made on the table using …

chipx86chipx86

Same as above.

chipx86chipx86
chipx86
  1. 
      
  2. reviewboard/attachments/forms.py (Diff revision 1)
     
     
     
     

    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.

    1. 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.

  3. reviewboard/attachments/managers.py (Diff revision 1)
     
     
     

    Same as above.

  4. 
      
david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (7835894)
Loading...