Summary

Define the local site field for file attachments on creation.

Review Request #15164 — Created July 11, 2026 and updated July 11, 2026, 9:14 p.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-8.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Local sites for file attachments have a messy history. Currently, the
local_site foreign key is only present for user file attachments
(attachments that were uploaded into a comment box or from the user file
attachments API), and for ones linked to file diffs (although our docs
erroneously do not mention this). Attachments that are uploaded directly to
a review request do not have this relation populated. Instead we grab the
local site from their related review request. We have a get_local_site()
method, which was originally added to help grab the right local site
depending on the type of file attachment.

We recently ran into a problem where we needed to grab a file attachment's
local site before it has been linked to its review request. But we currently
have no way of doing so.

This change improves things by always setting the local_site field on
file attachment creation. For existing pre-RB8.1 file attachments, calling
get_local_site() will now set the local_site field for ones that
don't have it set. We have a _defined_local_site key in the file
attachment's extra data that lets us know that the local_site field value
is authoritative. We need that because a local_site=None value is ambiguous
on its own, it can mean that a file attachment doesn't have a local site, or
that it's a pre-RB8.1 file attachment where we haven't resolved and set the
local site from its review request yet.

Testing Done

Did the following tests on my local RB 8 dev server and my local
RBCommons dev server:
- Ran unit tests.
- Created a review request and uploaded a pdf file to it.
- Created a user file attachment by dragging an image into a comment.
- rbt posted a change containing binary files.
- Viewed existing file attachments.

Also used in upcoming changes that add local site support to
document review.

Commits

Summary	ID
Define the local site field for file attachments on creation. Local sites for file attachments have a messy history. Currently, the `local_site` foreign key is only present for user file attachments (attachments that were uploaded into a comment box or from the user file attachments API), and for ones linked to file diffs (although our docs erroneously do not mention this). Attachments that are uploaded directly to a review request do not have this relation populated. Instead we grab the local site from their related review request. We have a `get_local_site()` method, which was originally added to help grab the right local site depending on the type of file attachment. We recently ran into a problem where we needed to grab a file attachment's local site before it has been linked to its review request. But we currently have no way of doing so. This change improves things by always setting the `local_site` field on file attachment creation. For existing pre-RB8.1 file attachments, calling `get_local_site()` will now set the `local_site` field for ones that don't have it set. We have a `_defined_local_site` key in the file attachment's extra data that lets us know that the `local_site` field value is authoritative. We need that because a `local_site=None` value is ambiguous on its own, it can mean that a file attachment doesn't have a local site, or that it's a pre-RB8.1 file attachment where we haven't resolved and set the local site from its review request yet.	d440cdca5506530cc72876896da1e2812ea678f4

Summary

Define the local site field for file attachments on creation.

Local sites for file attachments have a messy history. Currently, the `local_site` foreign key is only present for user file attachments (attachments that were uploaded into a comment box or from the user file attachments API), and for ones linked to file diffs (although our docs erroneously do not mention this). Attachments that are uploaded directly to a review request do not have this relation populated. Instead we grab the local site from their related review request. We have a `get_local_site()` method, which was originally added to help grab the right local site depending on the type of file attachment. We recently ran into a problem where we needed to grab a file attachment's local site before it has been linked to its review request. But we currently have no way of doing so. This change improves things by always setting the `local_site` field on file attachment creation. For existing pre-RB8.1 file attachments, calling `get_local_site()` will now set the `local_site` field for ones that don't have it set. We have a `_defined_local_site` key in the file attachment's extra data that lets us know that the `local_site` field value is authoritative. We need that because a `local_site=None` value is ambiguous on its own, it can mean that a file attachment doesn't have a local site, or that it's a pre-RB8.1 file attachment where we haven't resolved and set the local site from its review request yet.

d440cdca5506530cc72876896da1e2812ea678f4

Issues

Description	From	Last Updated
'reviewboard.attachments.models.FileAttachment' imported but unused Column: 1 Error code: F401	reviewbot	July 11, 2026, 3:50 a.m.
We probably should copy the dict before mutating it.	david	July 11, 2026, 9:14 p.m.
There's an extra blank line here.	david	July 11, 2026, 9:14 p.m.
Can we add a "Version Changed" detailing the new behavior here?	david	July 11, 2026, 9:14 p.m.
I believe if you call this on a FileAttachment that hasn't yet been created, it will error out. We should …	david	July 11, 2026, 9:14 p.m.
We probably should copy the dict before mutating it.	david	July 11, 2026, 9:14 p.m.
We probably should copy the dict before mutating it.	david	July 11, 2026, 9:14 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/attachments/tests/test_user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'reviewboard.attachments.models.FileAttachment' imported but unused

Column: 1
Error code: F401
```

Commits:

	Summary	ID
	Define the local site field for file attachments on creation. Local sites for file attachments have a messy history. Currently, the `local_site` foreign key is only present for user file attachments (attachments that were uploaded into a comment box or from the user file attachments API), and for ones linked to file diffs (although our docs erroneously do not mention this). Attachments that are uploaded directly to a review request do not have this relation populated. Instead we grab the local site from their related review request. We have a `get_local_site()` method, which was originally added to help grab the right local site depending on the type of file attachment. We recently ran into a problem where we needed to grab a file attachment's local site before it has been linked to its review request. But we currently have no way of doing so. This change improves things by always setting the `local_site` field on file attachment creation. For existing pre-RB8.1 file attachments, calling `get_local_site()` will now set the `local_site` field for ones that don't have it set. We have a `_defined_local_site` key in the file attachment's extra data that lets us know that the `local_site` field value is authoritative. We need that because a `local_site=None` value is ambiguous on its own, it can mean that a file attachment doesn't have a local site, or that it's a pre-RB8.1 file attachment where we haven't resolved and set the local site from its review request yet.	9d77e1c604c67c7e528e66f227d7e682372aef6c
	Define the local site field for file attachments on creation. Local sites for file attachments have a messy history. Currently, the `local_site` foreign key is only present for user file attachments (attachments that were uploaded into a comment box or from the user file attachments API), and for ones linked to file diffs (although our docs erroneously do not mention this). Attachments that are uploaded directly to a review request do not have this relation populated. Instead we grab the local site from their related review request. We have a `get_local_site()` method, which was originally added to help grab the right local site depending on the type of file attachment. We recently ran into a problem where we needed to grab a file attachment's local site before it has been linked to its review request. But we currently have no way of doing so. This change improves things by always setting the `local_site` field on file attachment creation. For existing pre-RB8.1 file attachments, calling `get_local_site()` will now set the `local_site` field for ones that don't have it set. We have a `_defined_local_site` key in the file attachment's extra data that lets us know that the `local_site` field value is authoritative. We need that because a `local_site=None` value is ambiguous on its own, it can mean that a file attachment doesn't have a local site, or that it's a pre-RB8.1 file attachment where we haven't resolved and set the local site from its review request yet.	d440cdca5506530cc72876896da1e2812ea678f4

Diff:

Revision 2 (+954 -90)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests/test_file_attachment.py
	reviewboard/attachments/tests/test_file_attachment_manager.py
	reviewboard/attachments/tests/test_user_file_attachment.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/tests/test_file_attachment.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

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

An issue was opened. Show all issues

We probably should copy the dict before mutating it.

reviewboard/attachments/managers.py (Diff revision 2)
An issue was opened. Show all issues
```
There's an extra blank line here.
```
reviewboard/attachments/models.py (Diff revision 2)
An issue was opened. Show all issues
```
Can we add a "Version Changed" detailing the new behavior here?
```

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

An issue was opened. Show all issues

I believe if you call this on a FileAttachment that hasn't yet been created, it will error out. We should probably check if the model is new, and if so, just hold off on saving.

reviewboard/testing/testcase.py (Diff revision 2)
An issue was opened. Show all issues
```
We probably should copy the dict before mutating it.
```
reviewboard/testing/testcase.py (Diff revision 2)
An issue was opened. Show all issues
```
We probably should copy the dict before mutating it.
```