[WIP] Add an is_draft property to file attachments.

Review Request #13138 — Created July 13, 2023 and discarded

Information

Review Board
release-6.x

Reviewers

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.

  • Ran unit tests.
  • Used the property in upcoming changes.
  • Tested uploading user file attachments to comments, saw
    nothing out of the ordinary.
Summary ID
Add an is_draft property to file attachments.
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 …

chipx86chipx86

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

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

My concern is that this can easily lead to ballooning queries in any typical usage. Every is_draft is going to …

chipx86chipx86

There's an extra : here.

chipx86chipx86

Can you do assertQueries for all these?

chipx86chipx86

line too long (85 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

at least two spaces before inline comment Column: 47 Error code: E261

reviewbotreviewbot
maubin
maubin
chipx86
  1. Nice change! Everything here is small.

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

    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.

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

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

  4. Missing a trailing comma.

  5. Missing a trailing comma.

  6. Missing a trailing comma.

  7. Missing a trailing comma.

  8. 
      
maubin
maubin
maubin
david
  1. Ship It!
  2. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. Most of this change looks perfect. The big part is ensuring our query counts don't balloon.

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

    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.

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

    There's an extra : here.

  4. reviewboard/attachments/tests.py (Diff revision 7)
     
     

    Can you do assertQueries for all these?

  5. 
      
maubin
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
Review request changed

Status: Discarded

Change Summary:

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

Loading...