flake8
passed.
JSHint
passed.
[WIP] Add an is_draft property to file attachments.
Review Request #13138 — Created July 13, 2023 and discarded
This adds an
is_draftproperty 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.
- Ran unit tests.
- Used the property in upcoming changes.
- Tested uploading user file attachments to comments, saw
nothing out of the ordinary.
| Summary | ID |
|---|---|
| ec7e207f9fdb2fd005f17aaf3aef2693325b9d47 |
| 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
|
|
|
self.user is the cheapest thing to check, so let's check that first. |
chipx86
|
|
|
Missing a trailing comma. |
chipx86
|
|
|
Missing a trailing comma. |
chipx86
|
|
|
Missing a trailing comma. |
chipx86
|
|
|
Missing a trailing comma. |
chipx86
|
|
|
My concern is that this can easily lead to ballooning queries in any typical usage. Every is_draft is going to … |
chipx86
|
|
|
There's an extra : here. |
chipx86
|
|
|
Can you do assertQueries for all these? |
chipx86
|
|
|
line too long (85 > 79 characters) Column: 80 Error code: E501 |
 reviewbot
|
|
|
at least two spaces before inline comment Column: 47 Error code: E261 |
reviewbot
|
- Change Summary:
-
- Added the
is_draftproperty to the file attachment JS model. - Updated unit tests for file attachment resources.
- Added the
- Description:
-
This adds an
is_draftproperty to file attachments, to easily checkwhen 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 620f38594d9a73caed6a8a5c26a3e7bdabe24fc7 867734ed832959a4d13425352fc5dda29dfca12a - Diff:
-
Revision 2 (+190 -18)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Posted my last version too early by accident.
- Commits:
-
Summary ID 867734ed832959a4d13425352fc5dda29dfca12a 8bf7ce8d7fd6b9ad63838f29e3368f309460b7e8 - Diff:
-
Revision 3 (+476 -46)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Commits:
-
Summary ID 8bf7ce8d7fd6b9ad63838f29e3368f309460b7e8 73b3d50977a7066d84c01bb25f6b2d23f8ab809d - Diff:
-
Revision 4 (+474 -46)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
- Defaults
isDrafttotrueon the JS model since newly created models will always be drafts.
- Defaults
- 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 73b3d50977a7066d84c01bb25f6b2d23f8ab809d abbe4bf1fc1aa6e3e9f64e56ab0aa45e022feb17 - Diff:
-
Revision 5 (+474 -46)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
- Moved to RB6.
- Commits:
-
Summary ID abbe4bf1fc1aa6e3e9f64e56ab0aa45e022feb17 5b5a86f9d27463fa19edb89d9e1188b801991025 - Branch:
-
release-5.0.xrelease-6.x
- Diff:
-
Revision 6 (+488 -46)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
- Updated to work with the TS/spina version of the file attachment model.
- Commits:
-
Summary ID 5b5a86f9d27463fa19edb89d9e1188b801991025 b260322ebc3a72fe4dcaf4f88d0f51da6ce4400c - Diff:
-
Revision 7 (+478 -40)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
Most of this change looks perfect. The big part is ensuring our query counts don't balloon.
-
My concern is that this can easily lead to ballooning queries in any typical usage. Every
is_draftis 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_draftflag, 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_requestare mutually-exclusive), so this state can be set.We actually have a
_review_request, used byget_review_request(), so there's precedence here (but we'd need different state, since I think_review_requestwill 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. -
-
- 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 b260322ebc3a72fe4dcaf4f88d0f51da6ce4400c ec7e207f9fdb2fd005f17aaf3aef2693325b9d47 - Diff:
-
Revision 8 (+982 -58)