Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[16.0] fs_attachment: fix meaningful name rewriting #312

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

len-foss
Copy link
Contributor

This fix the name rewriting in case the storage is "directory name optimized".

It adds a test to ensure that the name is consistently used between write and read.

Closes #285

@OCA-git-bot
Copy link
Contributor

Hi @lmignon,
some modules you are maintaining are being modified, check this out!

Copy link
Sponsor Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @len-foss LGTM (Code review)
I wonder if this behaviour also works correctly when using the 'file like' api to modify the contents of the attachment....

with attachment.open("wb") as f:
    f.write(...)

Nevertheless, can you add a file named 312.bugfix into the fs_attachment/readme/newsfragment directory to describe the change in an 'end user 'oriented way. This description will be add to the history section of the module description at merge

@len-foss len-foss marked this pull request as draft December 13, 2023 16:11
@len-foss
Copy link
Contributor Author

I'm very confused, as the newly added test breaks on the write, and only so if other tests from the same file (test_fs_attachment) are run. The way it breaks is also very strange:

FileNotFoundError: [Errno 2] No such file or directory: '/github/home/.local/share/Odoo/filestore/odoo/storage//tmp/tmpq6abj5qo/github/home/.local/share/Odoo/filestore/odoo/storage//te/st/test.txt'

Let's take a bit more time to understand where the issue is coming from, unless you already have some pointer?

@lmignon
Copy link
Sponsor Contributor

lmignon commented Dec 13, 2023

I'm very confused, as the newly added test breaks on the write, and only so if other tests from the same file (test_fs_attachment) are run. The way it breaks is also very strange:

FileNotFoundError: [Errno 2] No such file or directory: '/github/home/.local/share/Odoo/filestore/odoo/storage//tmp/tmpq6abj5qo/github/home/.local/share/Odoo/filestore/odoo/storage//te/st/test.txt'

Let's take a bit more time to understand where the issue is coming from, unless you already have some pointer?

The tests are failing due to the env cleanup at transaction rollback after each test. Indeed, the fs.storage backend inherit from the server.env.mixin. Therefore some fields are pseudo computed fields and need to be reset before running each test. To fix your issue, you need to update your created fs.storage record into the setup method....

def setUp(self)::
    super().setUp()
    self.backend_optimized = write(
            {
                "protocol": "file",
                "code": "tmp_opt",
                "directory_path": temp_dir,
                "optimizes_directory_path": True,
            }
        )

@len-foss len-foss marked this pull request as ready for review December 13, 2023 17:25
@len-foss
Copy link
Contributor Author

Oh, thank you very much!
I see, that also explains why I had to create the other backend, reusing the other made the tests break unexplainably in a similar fashion.

We should be good now then :-)

Copy link
Sponsor Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little comment otherwise LGTM (Code review).

Thank you for the contrib. Don't forget to add you name into the contributor list 😏

Let "image.png" be an image.
Thumbnails might be created with a path such as "38/5b/image-128.png".
Before, the path was forgotten when updating the attachment,
so accessing the file would crash, making the record inaccessible.
@len-foss
Copy link
Contributor Author

All right. I have added this to the test and a bit more explanations to make it easier to check the assumptions.

@lmignon
Copy link
Sponsor Contributor

lmignon commented Dec 20, 2023

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-312-by-lmignon-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 3d4300d into OCA:16.0 Dec 20, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at ad8a4d3. Thanks a lot for contributing to OCA. ❤️

@len-foss len-foss deleted the 16.0-fixfsattachment-len branch December 20, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants