Summary

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

Review Request #13251 — Created Aug. 31, 2023 and submitted Sept. 28, 2023, 11:42 a.m.

Information

Owner

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

People

Description

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.

Testing Done


Ran unit tests.
Used in an upcoming change.

Commits

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

Issues

Description	From	Last Updated
"false" for the API. (We normalize case, but document lowercase.)	chipx86	Sept. 20, 2023, 10:17 a.m.
In the case of the API, "parameter" isn't the right word. Instead, we can say something like "Set pending_deletion=false in …	chipx86	Sept. 20, 2023, 10:19 a.m.
I keep reading this final sentence incorrectly, and realized I was missing how this connected. Maybe we can reword it …	chipx86	Sept. 20, 2023, 10:19 a.m.
This should have a trailing comma.	david	Sept. 26, 2023, 10:56 a.m.
This should have a trailing comma.	david	Sept. 26, 2023, 10:56 a.m.
When wrapping a multi-line string in parens, leave the ) aligned with the line containing the (, so that it's …	chipx86	Sept. 20, 2023, 10:20 a.m.
This should have a trailing comma.	david	Sept. 26, 2023, 10:57 a.m.
This should have a trailing comma.	david	Sept. 26, 2023, 10:57 a.m.
Missing a type.	chipx86	Sept. 20, 2023, 10:21 a.m.
Since we just need the IDs, you can do: all_revs = list( FileAttachment.objects .filter(...) .values_list('pk', flat=True) ) (The list() resolves …	chipx86	Sept. 20, 2023, 10:25 a.m.
.save() isn't needed to update M2M relations, like file_attachments, so let's avoid this if we're not otherwise modifying state on …	chipx86	Sept. 20, 2023, 10:26 a.m.
Missing a type.	chipx86	Sept. 20, 2023, 10:33 a.m.
Same notes as above.	chipx86	Sept. 20, 2023, 10:33 a.m.
Going forward (starting with recent API error work), I want to start comparing the entirety of rsp for errors, so …	chipx86	Sept. 20, 2023, 11:04 a.m.

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"false" for the API. (We normalize case, but document lowercase.)
```

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

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

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

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.

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

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

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing a type.
```

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

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

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

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

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing a type.
```

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

Same notes as above.

reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 1)

The issue has been resolved. Show all issues

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.

Commits:

	Summary	ID
	Update the API to allow undoing the pending deletion of file attachments.	65d6a7d1f3c832ae446d19855a4d171e90e6c59c
	Update the API to allow undoing the pending deletion of file attachments.	178112957297f2a521382a36b90476515436effb

Diff:

Revision 2 (+2900 -58)

	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Just a few trivial style nits:
```
reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
This should have a trailing comma.
```
reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
This should have a trailing comma.
```
reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
This should have a trailing comma.
```
reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
This should have a trailing comma.
```

Change Summary:

Added some missing commas.

Commits:

	Summary	ID
	Update the API to allow undoing the pending deletion of file attachments.	178112957297f2a521382a36b90476515436effb
	Update the API to allow undoing the pending deletion of file attachments.	c6879af7e21ebb928e20e101fc5faf478daa99d6

Diff:

Revision 3 (+2900 -58)

	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (8a11f1f)

Loading...