Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
Define the local site field for file attachments on creation.
Review Request #15164 — Created July 11, 2026 and submitted
Local sites for file attachments have a messy history. Currently, the
local_siteforeign 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 aget_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_sitefield on
file attachment creation. For existing pre-RB8.1 file attachments, calling
get_local_site()will now set thelocal_sitefield for ones that
don't have it set. We have a__defined_local_sitekey in the file
attachment's extra data that lets us know that thelocal_sitefield value
is authoritative. We need that because alocal_site=Nonevalue 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.
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.
| Summary | ID |
|---|---|
| 2a39ed82d63a61463c047e98b950f1c01bd7ab1a |
| Description | From | Last Updated |
|---|---|---|
|
'reviewboard.attachments.models.FileAttachment' imported but unused Column: 1 Error code: F401 |
reviewbot
|
|
|
We probably should copy the dict before mutating it. |
david
|
|
|
There's an extra blank line here. |
david
|
|
|
Can we add a "Version Changed" detailing the new behavior here? |
david
|
|
|
I believe if you call this on a FileAttachment that hasn't yet been created, it will error out. We should … |
david
|
|
|
We probably should copy the dict before mutating it. |
david
|
|
|
We probably should copy the dict before mutating it. |
david
|
|
|
We use this literal across four different files. Can we define a constant somewhere? |
david
|
|
|
If we use a double __ prefix, that will filter it out of API responses and prevent API clients from … |
chipx86
|
|
|
If we check self.user_id instead, we'll save a query in the event that it's not already in the queryset cache. |
chipx86
|
|
|
We're a bit inconsistent in some places on how we capitalize, but I think we should be using "Local Site" … |
chipx86
|
|
|
Same note about double underscore. |
chipx86
|
|
|
There are a few additional uses of self.review_request in this method that can use this new variable instead. |
david
|
Review request changed
- Commits:
-
Summary ID 9d77e1c604c67c7e528e66f227d7e682372aef6c d440cdca5506530cc72876896da1e2812ea678f4 - Diff:
-
Revision 2 (+954 -90)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Review request changed
- Commits:
-
Summary ID d440cdca5506530cc72876896da1e2812ea678f4 9c851a76aac0bc3f677c5e847d8306c7e5057876 - Diff:
-
Revision 3 (+982 -90)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
If we use a double
__prefix, that will filter it out of API responses and prevent API clients from overriding it. -
If we check
self.user_idinstead, we'll save a query in the event that it's not already in the queryset cache. -
We're a bit inconsistent in some places on how we capitalize, but I think we should be using "Local Site" for anything public in order to make it clear this is a Term and not, like, "your local website" or something.
-
Review request changed
- Commits:
-
Summary ID 9c851a76aac0bc3f677c5e847d8306c7e5057876 e1e60005f50648a8aded278f713ca2d9f1f329a0 - Diff:
-
Revision 4 (+1004 -98)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Review request changed
- Description:
-
Local sites for file attachments have a messy history. Currently, the
local_siteforeign 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_sitefield onfile attachment creation. For existing pre-RB8.1 file attachments, calling get_local_site()will now set thelocal_sitefield for ones that~ don't have it set. We have a _defined_local_sitekey in the file~ don't have it set. We have a __defined_local_sitekey in the fileattachment's extra data that lets us know that the local_sitefield valueis authoritative. We need that because a local_site=Nonevalue is ambiguouson 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. - Commits:
-
Summary ID e1e60005f50648a8aded278f713ca2d9f1f329a0 2a39ed82d63a61463c047e98b950f1c01bd7ab1a - Diff:
-
Revision 5 (+1048 -106)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.