Summary

[WIP] Add an is_draft property to file attachments.

Review Request #13138 — Created July 13, 2023 and discarded Sept. 1, 2023, 10:11 a.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	ID
Add an is_draft property to file attachments.	ec7e207f9fdb2fd005f17aaf3aef2693325b9d47

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, 1:20 p.m.
self.user is the cheapest thing to check, so let's check that first.	chipx86	July 18, 2023, 12:14 p.m.
Missing a trailing comma.	chipx86	July 18, 2023, 12:15 p.m.
Missing a trailing comma.	chipx86	July 18, 2023, 12:15 p.m.
Missing a trailing comma.	chipx86	July 18, 2023, 12:15 p.m.
Missing a trailing comma.	chipx86	July 18, 2023, 12: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, 10:10 a.m.
There's an extra : here.	chipx86	Aug. 10, 2023, 9:27 a.m.
Can you do assertQueries for all these?	chipx86	Aug. 10, 2023, 11:14 a.m.
line too long (85 > 79 characters) Column: 80 Error code: E501	reviewbot	Aug. 11, 2023, 3:13 p.m.
at least two spaces before inline comment Column: 47 Error code: E261	reviewbot	Aug. 11, 2023, 3: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	ID
	Add an is_draft property to file attachments.	620f38594d9a73caed6a8a5c26a3e7bdabe24fc7
	Add an is_draft property to file attachments.	867734ed832959a4d13425352fc5dda29dfca12a

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	ID
	Add an is_draft property to file attachments.	867734ed832959a4d13425352fc5dda29dfca12a
	Add an is_draft property to file attachments.	8bf7ce8d7fd6b9ad63838f29e3368f309460b7e8

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)

The issue has been resolved. 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)
The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
reviewboard/webapi/tests/test_file_attachment.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```

Commits:

	Summary	ID
	Add an is_draft property to file attachments.	8bf7ce8d7fd6b9ad63838f29e3368f309460b7e8
	Add an is_draft property to file attachments.	73b3d50977a7066d84c01bb25f6b2d23f8ab809d

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	ID
	Add an is_draft property to file attachments.	73b3d50977a7066d84c01bb25f6b2d23f8ab809d
	Add an is_draft property to file attachments.	abbe4bf1fc1aa6e3e9f64e56ab0aa45e022feb17

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	ID
	Add an is_draft property to file attachments.	abbe4bf1fc1aa6e3e9f64e56ab0aa45e022feb17
	Add an is_draft property to file attachments.	5b5a86f9d27463fa19edb89d9e1188b801991025

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	ID
	Add an is_draft property to file attachments.	5b5a86f9d27463fa19edb89d9e1188b801991025
	Add an is_draft property to file attachments.	b260322ebc3a72fe4dcaf4f88d0f51da6ce4400c

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)

The issue has been dropped. 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)
The issue has been resolved. Show all issues
```
There's an extra : here.
```
reviewboard/attachments/tests.py (Diff revision 7)
The issue has been resolved. 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	ID
	Add an is_draft property to file attachments.	b260322ebc3a72fe4dcaf4f88d0f51da6ce4400c
	Add an is_draft property to file attachments.	ec7e207f9fdb2fd005f17aaf3aef2693325b9d47

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)
The issue has been dropped. 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)
The issue has been dropped. 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/