Update FileAttachment.objects.create_from_filediff to set needed fields.

Review Request #13532 — Created Feb. 12, 2024 and submitted

Information

Review Board
master

Reviewers

It turns out that having a FileAttachment for a diff that does not have
the review request set will cause the review UI instantiation to fail.
This change fixes the create_from_filediff method to set the review
request and local site fields correctly based on the given FileDiff's
review request.

  • Ran unit tests.
  • Used this when automatically creating a diff file attachment in a
    later change.
Summary ID
Update FileAttachment.objects.create_from_filediff to set needed fields.
It turns out that having a FileAttachment for a diff that does not have the review request set will cause the review UI instantiation to fail. This change fixes the `create_from_filediff` method to set the review request and local site fields correctly based on the given FileDiff's review request. Testing Done: - Ran unit tests. - Used this when automatically creating a diff file attachment in a later change. Reviewed at https://reviews.reviewboard.org/r/13532/
03af6a5de80fa01c8dce9b878cb73e6cb6b5b899
Description From Last Updated

'django.core.exceptions.ObjectDoesNotExist' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

By default, .set() will query for existing objects, compare to what's in there, remove any old ones, and then set …

chipx86chipx86

This is missing a Version Added.

chipx86chipx86

Do we want to cache the None locally, or check again the next time this is called? I feel like …

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/attachments/managers.py (Diff revision 2)
     
     
    Show all issues

    By default, .set() will query for existing objects, compare to what's in there, remove any old ones, and then set new ones. That's minimum 2, max 3 queries.

    That can be reduced to a guaranteed 2 if we pass clear=True, which will just delete what's in there and replace it.

    However, we know this model is new and that we're adding to it for the first time, so we can avoid all that and just use .add() to get 1 query.

  3. reviewboard/diffviewer/models/filediff.py (Diff revision 2)
     
     
     
    Show all issues

    This is missing a Version Added.

  4. reviewboard/diffviewer/models/filediff.py (Diff revision 2)
     
     
     
    Show all issues

    Do we want to cache the None locally, or check again the next time this is called? I feel like the former avoids unexpected behavior, since this is only during the lifetime of a request.

    In either case, we should document this.

    1. I don't think we should cache it if it's None. Having a FileDiff that's not attached to a review request in some form is a corner case that should only happen in cases where we haven't attached it yet. In that case, we may theoretically want to use this later in the process, even in the same request. I'll add a comment to that effect.

  5. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (aa9529f)