Summary:

[WIP] Add an is_draft property to file attachments.

Review Request #13138 — Created July 13, 2023 and discarded Sept. 1, 2023, 5:11 p.m.

Information
Owner:	maubin
Repository:	Review Board
Branch:	release-6.x
Bugs:
Depends On:
Blocks:	13142, 13135
Reviewers
Groups:	reviewboard
People:

Description

This adds an is_draft property to file attachments, to easily check
when a file attachment is a draft or if it's published. This also
updates areas in the code where we check if a file attachment is a
draft. And unit tests for the file attachment resources have been
updated to check all of the fields of the file attachment.

Testing Done


Ran unit tests.
Used the property in upcoming changes.
Tested uploading user file attachments to comments, saw

  nothing out of the ordinary.

Commits

Summary
Add an is_draft property to file attachments.

Screenshots

Issues

Description	From	Last Updated
For a property, we just use Type, like it's an attribute. Same with the summary: It should be in more …	chipx86	July 17, 2023, 8:20 p.m.
self.user is the cheapest thing to check, so let's check that first.	chipx86	July 18, 2023, 7:14 p.m.
Missing a trailing comma.	chipx86	July 18, 2023, 7:15 p.m.
Missing a trailing comma.	chipx86	July 18, 2023, 7:15 p.m.
Missing a trailing comma.	chipx86	July 18, 2023, 7:15 p.m.
Missing a trailing comma.	chipx86	July 18, 2023, 7:15 p.m.
My concern is that this can easily lead to ballooning queries in any typical usage. Every is_draft is going to …	chipx86	Sept. 1, 2023, 5:10 p.m.
There's an extra : here.	chipx86	Aug. 10, 2023, 4:27 p.m.
Can you do assertQueries for all these?	chipx86	Aug. 10, 2023, 6:14 p.m.
line too long (85 > 79 characters) Column: 80 Error code: E501	reviewbot	Aug. 11, 2023, 10:13 p.m.
at least two spaces before inline comment Column: 47 Error code: E261	reviewbot	Aug. 11, 2023, 10:13 p.m.

flake8 passed.

JSHint passed.

Change Summary:


Added the is_draft property to the file attachment JS model.
Updated unit tests for file attachment resources.

Description:

		This adds an `is_draft` property to file attachments, to easily check
		when a file attachment is a draft or if it's published. This also
		updates areas in the code where we check if a file attachment is a
~		draft.
	~	draft. And unit tests for the file attachment resources have been
	+	updated to check all of the fields of the file attachment.

Commits:

	Summary
-	Add an is_draft property to file attachments.
+	Add an is_draft property to file attachments.

Diff:

Revision 2 (+190 -18)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.es6.js
	reviewboard/webapi/resources/base_file_attachment.py
	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.

Change Summary:

Posted my last version too early by accident.

Commits:

	Summary
-	Add an is_draft property to file attachments.
+	Add an is_draft property to file attachments.

Diff:

Revision 3 (+476 -46)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.es6.js
	reviewboard/static/rb/js/resources/models/tests/fileAttachmentModelTests.es6.js
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Nice change! Everything here is small.
```

reviewboard/attachments/models.py (Diff revision 3)

Show all issues

For a property, we just use Type, like it's an attribute.

Same with the summary: It should be in more of an attribute-style summary, rather than saying it returns something.

reviewboard/attachments/models.py (Diff revision 3)

Show all issues

self.user is the cheapest thing to check, so let's check that first.

reviewboard/webapi/tests/test_file_attachment.py (Diff revision 3)
Show all issues
```
Missing a trailing comma.
```
reviewboard/webapi/tests/test_file_attachment.py (Diff revision 3)
Show all issues
```
Missing a trailing comma.
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 3)
Show all issues
```
Missing a trailing comma.
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 3)
Show all issues
```
Missing a trailing comma.
```

Commits:

	Summary
-	Add an is_draft property to file attachments.
+	Add an is_draft property to file attachments.

Diff:

Revision 4 (+474 -46)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.es6.js
	reviewboard/static/rb/js/resources/models/tests/fileAttachmentModelTests.es6.js
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:


Defaults isDraft to true on the JS model since newly created models will always be drafts.

Testing Done:

		Ran unit tests.
		Used the property in upcoming changes.
	+	Tested uploading user file attachments to comments, saw nothing out of the ordinary.

Commits:

	Summary
-	Add an is_draft property to file attachments.
+	Add an is_draft property to file attachments.

Diff:

Revision 5 (+474 -46)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.es6.js
	reviewboard/static/rb/js/resources/models/tests/fileAttachmentModelTests.es6.js
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:


Moved to RB6.

Commits:

	Summary
-	Add an is_draft property to file attachments.
+	Add an is_draft property to file attachments.

Branch:

-	release-5.0.x
+	release-6.x

Diff:

Revision 6 (+488 -46)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.es6.js
	reviewboard/static/rb/js/resources/models/tests/fileAttachmentModelTests.es6.js
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Change Summary:


Updated to work with the TS/spina version of the file attachment model.

Commits:

	Summary
-	Add an is_draft property to file attachments.
+	Add an is_draft property to file attachments.

Diff:

Revision 7 (+478 -40)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/common/resources/models/fileAttachmentModel.ts
	reviewboard/static/rb/js/common/resources/models/tests/fileAttachmentModelTests.ts
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Most of this change looks perfect. The big part is ensuring our query counts don't balloon.

reviewboard/attachments/models.py (Diff revision 7)

Show all issues

My concern is that this can easily lead to ballooning queries in any typical usage. Every is_draft is going to check upwards of 3 tables, so if we have 10 attachments, that's 30 queries.
We have this same problem in other models, but also a solution. We can set precomputed state to check instead of assuming a query must be performed. Maybe this is a _is_draft flag, or maybe it's a flag for the review request or inactive review request.
In any case, code fetching file attachments should know which they're fetching from (and review_request / inactive_review_request are mutually-exclusive), so this state can be set.
We actually have a _review_request, used by get_review_request(), so there's precedence here (but we'd need different state, since I think _review_request will be set for either case).
The goal would be to avoid any queries from this function when loading the review request page or via the API.
This should probably also be a @cached_property.

reviewboard/attachments/models.py (Diff revision 7)
Show all issues
```
There's an extra : here.
```
reviewboard/attachments/tests.py (Diff revision 7)
Show all issues
```
Can you do assertQueries for all these?
```

Change Summary:

Posting my WIP code for trying to reduce queries when calling is_draft

Summary:

-	Add an is_draft property to file attachments.
+	[WIP] Add an is_draft property to file attachments.

Commits:

	Summary
-	Add an is_draft property to file attachments.
+	Add an is_draft property to file attachments.

Diff:

Revision 8 (+982 -58)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/common/resources/models/fileAttachmentModel.ts
	reviewboard/static/rb/js/common/resources/models/tests/fileAttachmentModelTests.ts
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment.py
	1 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/attachments/models.py (Diff revision 8)

Show all issues

line too long (85 > 79 characters)

Column: 80
Error code: E501

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 8)
Show all issues
```
at least two spaces before inline comment

Column: 47
Error code: E261
```

Status: Discarded

Change Summary:

Discarded in favour of a more general change for file attachment states /r/13238/