Summary

Update FileAttachment.objects.create_from_filediff to set needed fields.

Review Request #13532 — Created Feb. 12, 2024 and submitted Feb. 20, 2024, 8:42 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Commits

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

Summary

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

Issues

Description	From	Last Updated
'django.core.exceptions.ObjectDoesNotExist' imported but unused Column: 1 Error code: F401	reviewbot	Feb. 12, 2024, 9:57 a.m.
By default, .set() will query for existing objects, compare to what's in there, remove any old ones, and then set …	chipx86	Feb. 15, 2024, 10:31 p.m.
This is missing a Version Added.	chipx86	Feb. 15, 2024, 10:31 p.m.
Do we want to cache the None locally, or check again the next time this is called? I feel like …	chipx86	Feb. 15, 2024, 10:37 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/attachments/managers.py (Diff revision 1)

The issue has been dropped. Show all issues

'django.core.exceptions.ObjectDoesNotExist' imported but unused

Column: 1
Error code: F401

Commits:

	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.	8906fa362559a4e2659dabb912af4a1df87d2361
	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.	bf4481c4c5909a1308757c840cc173ead17c30ee

Diff:

Revision 2 (+244 -50)

Show changes

	reviewboard/attachments/managers.py
	reviewboard/attachments/tests.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/webapi/tests/test_diff_file_attachment.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/attachments/managers.py (Diff revision 2)

The issue has been resolved. 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.

reviewboard/diffviewer/models/filediff.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This is missing a Version Added.
```

reviewboard/diffviewer/models/filediff.py (Diff revision 2)

The issue has been resolved. 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.

david Feb. 15, 2024, 10:42 p.m.

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.

Commits:

	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.	bf4481c4c5909a1308757c840cc173ead17c30ee
	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

Diff:

Revision 3 (+260 -50)

Show changes

	reviewboard/attachments/managers.py
	reviewboard/attachments/tests.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/webapi/tests/test_diff_file_attachment.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (aa9529f)