[18.0][IMP] storage_file: improve public/private behavior#623
Open
simahawk wants to merge 9 commits into
Open
Conversation
simahawk
force-pushed
the
18-public-private-imp
branch
from
eaccbba to
bae2003
Compare
OCA-git-bot
added
series:18.0
mod:storage_media
For the case of private files stored externally it might be necessary to view the file via Odoo directly because you can avoid to setup a private CDN with complex authentication (eg: token, vpn, etc). In this way, the files can be proxied by the controller.
So far the backend was added only auto-magically on media or image creation. However, this is a partial behavior and since the backend is not editable you cannot decide where to put your file if not via global conf. This chance allows to edit the backend while still pre-loading the default one.
Provide data so that the URL is generated properly.
simahawk
force-pushed
the
18-public-private-imp
branch
from
bae2003 to
40b685a
Compare
henrybackman
approved these changes
May 28, 2026
SilvioC2C
approved these changes
May 28, 2026
Contributor
SilvioC2C
left a comment
SilvioC2C
left a comment
There was a problem hiding this comment.
Overall LGTM, a few things to check imho
| def test_reflects_backend_flag(self): | ||
| self.assertFalse(self.storage_file.is_public) | ||
| self.backend.is_public = True | ||
| self.storage_file.invalidate_recordset(["is_public"]) |
Contributor
There was a problem hiding this comment.
This cache invalidation should not be necessary. Does this test still succeed if this line is removed?
| result = self.env["storage.file"].search( | ||
| [("is_public", "=", True), ("id", "=", self.storage_file.id)] | ||
| ) | ||
| self.assertIn(self.storage_file, result) |
Contributor
There was a problem hiding this comment.
Nitpicking:
Suggested change
| self.assertIn(self.storage_file, result) | |
| self.assertEqual(self.storage_file, result) |
| result = self.env["storage.file"].search( | ||
| [("is_public", "=", False), ("id", "=", self.storage_file.id)] | ||
| ) | ||
| self.assertIn(self.storage_file, result) |
Contributor
There was a problem hiding this comment.
Nitpicking:
Suggested change
| self.assertIn(self.storage_file, result) | |
| self.assertEqual(self.storage_file, result) |
| def test_is_public_reflects_backend(self): | ||
| self.assertFalse(self.storage_image.is_public) | ||
| self.backend.sudo().is_public = True | ||
| self.storage_image.invalidate_recordset(["is_public"]) |
Comment on lines
+14
to
+18
| def setUp(self): | ||
| super().setUp() | ||
| self.media = self.env["storage.media"].create( | ||
| {"name": "test-media.txt", "backend_id": self.backend.id} | ||
| ) |
Contributor
There was a problem hiding this comment.
Can't this be done in setUpClass()?
|
|
||
| def test_is_public_reflects_backend(self): | ||
| self.backend.sudo().is_public = False | ||
| self.media.invalidate_recordset(["is_public"]) |
| self.media.invalidate_recordset(["is_public"]) | ||
| self.assertFalse(self.media.is_public) | ||
| self.backend.sudo().is_public = True | ||
| self.media.invalidate_recordset(["is_public"]) |
hparfr
reviewed
May 29, 2026
Contributor
hparfr
left a comment
hparfr
left a comment
There was a problem hiding this comment.
Interesting, two minor remarks
| try: | ||
| storage_file.check_access("read") | ||
| except AccessError as err: | ||
| raise NotFound() from err |
| def _get_url_for_file(self, storage_file, exclude_base_url=False): | ||
| """Return final full URL for given file.""" | ||
| backend = self.sudo() | ||
| if backend.served_by == "odoo": |
Contributor
There was a problem hiding this comment.
backend.servedy_by is external but the file is served by odoo
It's confusing
May be add a new served_by value like "force_odoo_for_external_backend" or whatever :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improve handling of storage backend visibility and allow serving private files directly through Odoo.
Changes summary:
storage_file
/storage.file/<slug>route.backend_ideditable on forms with a pre-loaded default (configurable via ir.config_parameter).storage_image / storage_media
storage_media_product:
NOTE: as the backend is now editable, the best is to use #622 to take care of swapping backends on change.