Summary

Fix download-file-attachment URL for attachments on a local site.

Review Request #14226 — Created Nov. 4, 2024 and submitted Nov. 6, 2024, 8:56 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

FileAttachment has a confusing relation to LocalSite. For historical
reasons, attachments which are related to a review request have a blank
local_site field, using the review request's local site instead. The
local_site field is therefore only "usable" for user file attachments
(such as files which are dropped into a comment box or review request
field).

This was causing a crash for review requests with attachments when
attempting to create the new download URL.

This change adds a fix for the URL crash, a big comment on the
local_site field explaining what's up, and some unit tests to ensure
that get_absolute_url() does the right thing.

Testing Done


Was able to view a review request with attachments on the new RBC

  server.
Ran unit tests.

Commits

Summary	ID
Fix download-file-attachment URL for attachments on a local site. FileAttachment has a confusing relation to LocalSite. For historical reasons, attachments which are related to a review request have a blank `local_site` field, using the review request's local site instead. The `local_site` field is therefore only "usable" for user file attachments (such as files which are dropped into a comment box or review request field). This was causing a crash for review requests with attachments when attempting to create the new download URL. This change adds a fix for the URL crash, a big comment on the `local_site` field explaining what's up, and some unit tests to ensure that `get_absolute_url()` does the right thing. Testing Done: - Was able to view a review request with attachments on the new RBC server. - Ran unit tests.	5a873f9bda3b39edde9e8177e154a34f8cea0f91

Summary

Fix download-file-attachment URL for attachments on a local site.

FileAttachment has a confusing relation to LocalSite. For historical reasons, attachments which are related to a review request have a blank `local_site` field, using the review request's local site instead. The `local_site` field is therefore only "usable" for user file attachments (such as files which are dropped into a comment box or review request field). This was causing a crash for review requests with attachments when attempting to create the new download URL. This change adds a fix for the URL crash, a big comment on the `local_site` field explaining what's up, and some unit tests to ensure that `get_absolute_url()` does the right thing. Testing Done: - Was able to view a review request with attachments on the new RBC server. - Ran unit tests.

5a873f9bda3b39edde9e8177e154a34f8cea0f91

Issues

Description	From	Last Updated
It would be nice if we could just update the "getter" for the local_site property to return self.local_site if its …	maubin	Nov. 5, 2024, 7:46 a.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

It would be nice if we could just update the "getter" for the local_site property to return self.local_site if its a user file attachment, and review_request.local_site otherwise. That way callers don't have to remember which one to use and can just call self.local_site directly. But I don't think there's a way to safely do that.
Would it be a good idea to make a get_local_site() method and put that logic in there? Similar to how we have a get_review_request() one.

chipx86 Nov. 4, 2024, 4:31 p.m.

I think this would be ideal, given the situation around this. Then, going forward, we can ensure we don't access .local_site and just use this.

david Nov. 5, 2024, 8:02 a.m.

I briefly flirted with a change to rename the actual field to _local_site and add a new property, but that ends up being too invasive. I think a get_* method is going to have to suffice.

Commits:

	Summary	ID
	Fix download-file-attachment URL for attachments on a local site. FileAttachment has a confusing relation to LocalSite. For historical reasons, attachments which are related to a review request have a blank `local_site` field, using the review request's local site instead. The `local_site` field is therefore only "usable" for user file attachments (such as files which are dropped into a comment box or review request field). This was causing a crash for review requests with attachments when attempting to create the new download URL. This change adds a fix for the URL crash, a big comment on the `local_site` field explaining what's up, and some unit tests to ensure that `get_absolute_url()` does the right thing. Testing Done: - Was able to view a review request with attachments on the new RBC server. - Ran unit tests.	c2668ac3f0224fe8f853b7cde4b5763ff2a5d358
	Fix download-file-attachment URL for attachments on a local site. FileAttachment has a confusing relation to LocalSite. For historical reasons, attachments which are related to a review request have a blank `local_site` field, using the review request's local site instead. The `local_site` field is therefore only "usable" for user file attachments (such as files which are dropped into a comment box or review request field). This was causing a crash for review requests with attachments when attempting to create the new download URL. This change adds a fix for the URL crash, a big comment on the `local_site` field explaining what's up, and some unit tests to ensure that `get_absolute_url()` does the right thing. Testing Done: - Was able to view a review request with attachments on the new RBC server. - Ran unit tests.	5a873f9bda3b39edde9e8177e154a34f8cea0f91