Update the API to allow undoing the pending deletion of file attachments.

Review Request #13251 — Created Aug. 31, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

This updates the file attachments API update endpoints to allow undoing the
pending deletion of file attachments. We add a pending_deletion query
parameter that can only be set to False to do this.

This also reduces the number of queries performed when deleting multiple
revisions of file attachments. Instead of removing revisions one by one,
we do a batch remove which reduces this to just one query instead of
one query per revision.

  • Ran unit tests.
  • Used in an upcoming change.
Summary ID
Update the API to allow undoing the pending deletion of file attachments.
c6879af7e21ebb928e20e101fc5faf478daa99d6
Description From Last Updated

"false" for the API. (We normalize case, but document lowercase.)

chipx86chipx86

In the case of the API, "parameter" isn't the right word. Instead, we can say something like "Set pending_deletion=false in …

chipx86chipx86

I keep reading this final sentence incorrectly, and realized I was missing how this connected. Maybe we can reword it …

chipx86chipx86

This should have a trailing comma.

daviddavid

This should have a trailing comma.

daviddavid

When wrapping a multi-line string in parens, leave the ) aligned with the line containing the (, so that it's …

chipx86chipx86

This should have a trailing comma.

daviddavid

This should have a trailing comma.

daviddavid

Missing a type.

chipx86chipx86

Since we just need the IDs, you can do: all_revs = list( FileAttachment.objects .filter(...) .values_list('pk', flat=True) ) (The list() resolves …

chipx86chipx86

.save() isn't needed to update M2M relations, like file_attachments, so let's avoid this if we're not otherwise modifying state on …

chipx86chipx86

Missing a type.

chipx86chipx86

Same notes as above.

chipx86chipx86

Going forward (starting with recent API error work), I want to start comparing the entirety of rsp for errors, so …

chipx86chipx86
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. "false" for the API. (We normalize case, but document lowercase.)

  3. In the case of the API, "parameter" isn't the right word. Instead, we can say something like "Set pending_deletion=false in the request to ..."

  4. I keep reading this final sentence incorrectly, and realized I was missing how this connected. Maybe we can reword it to:

    Setting this to ``true`` is unsupported and will not delete the file attachment. To perform a deletion, perform a HTTP DELETE on the resource instead. 
    
  5. When wrapping a multi-line string in parens, leave the ) aligned with the line containing the (, so that it's easier to add additional text/lines (same sort of rule as ensuring list items have a , at the end).

  6. Since we just need the IDs, you can do:

    all_revs = list(
        FileAttachment.objects
        .filter(...)
        .values_list('pk', flat=True)
    )
    

    (The list() resolves this, converting from a ValuesQuerySet to a list of IDs.)

  7. .save() isn't needed to update M2M relations, like file_attachments, so let's avoid this if we're not otherwise modifying state on the object.

  8. Same notes as above.

  9. Going forward (starting with recent API error work), I want to start comparing the entirety of rsp for errors, so we can be thorough. It also eases readability a bit. Let's update that here.

  10. 
      
maubin
david
  1. Just a few trivial style nits:

  2. This should have a trailing comma.

  3. This should have a trailing comma.

  4. This should have a trailing comma.

  5. This should have a trailing comma.

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

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (8a11f1f)
Loading...