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][MIG] dms #262

Merged
merged 133 commits into from
Nov 3, 2023
Merged

[16.0][MIG] dms #262

merged 133 commits into from
Nov 3, 2023

Conversation

vancouver29
Copy link

[16.0][MIG] dms

Mathias Markl and others added 30 commits September 18, 2023 17:01
versions of muk_dms than 12.0.2.0.0.

Tested from 1.2.4 version.
In v13, this test is programmed in such a way that the demo user is supposed to be able to copy that subdirectory: https://github.com/OCA/dms/blob/c3f802db43362127e70d8c7b4987fb71d4c1f01c/dms/tests/test_directory.py#L40

However, in OCA#7 that test was modified indicating that demo user didn't have permissions to do that: https://github.com/OCA/dms/blob/e3b6d8d24534f2a68bfb88e310cc70cefe46bb64/dms/tests/test_directory.py#L39

Rolling back that change to ensure premissions remain the same in both versions of the module.

Also changing the directory to test to ensure it contains no SVG files, whose detection seems to differ among environments, and which have some specific permission restrictions that can make the modified test fail or pass.

@Tecnativa TT25645
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Are video and audio correctly previewed without the extra dependencies?

dms/models/dms_file.py Outdated Show resolved Hide resolved
dms/views/dms_file.xml Show resolved Hide resolved
dms/models/dms_file.py Outdated Show resolved Hide resolved
@pedrobaeza
Copy link
Member

Please add #264

@vancouver29 vancouver29 force-pushed the 16.0-mig-dms-kbui branch 2 times, most recently from 70ee0d4 to 162ff0c Compare October 29, 2023 10:49
@vancouver29
Copy link
Author

vancouver29 commented Oct 29, 2023

Please add #264

Did you test it in dms v16?

I tried to reproduce the problem, which is described here #263 with the user Mark Demo. But No Error has occurred. So I think we don't need this.

dms/models/dms_file.py Outdated Show resolved Hide resolved
dms/models/dms_file.py Outdated Show resolved Hide resolved
@len-foss
Copy link
Contributor

With regards to the PR mentioned, the fix is in dms_field, you cannot apply half of it, and the half you can apply doesn't make sense on its own. Given that the compute field on model could be added in an override in dms_field, it's technically not really right to put that code in dms.

@pedrobaeza
Copy link
Member

Well, I disagree here, as any other chance to have a non sudo access will provoke the same problem, so it's better to fix it in the root, as it also doesn't cause any cons.

@len-foss
Copy link
Contributor

Well but the problem is that there is no path for reproduction, as @vancouver29 noted, and no test that can be written in dms.
As it stands it's the code from dms_field that emits the domain triggering the error.
So if you look at the code, it isn't used anywhere, and the commit message says "Improve search by model". Without going back to the Github PR, there is nothing that indicates why this code should exist.

There should be a comment explaining why it is useful, or at the very least this information should be in the commit message. Something like "while this field is not used, depending modules might need to search on model; however the document manager might not have read access to ir.model".

@pedrobaeza
Copy link
Member

OK, agreed of explaining everything better. The fix and its explanation can be added later in a subsequent PR.

Copy link

@agent-z28 agent-z28 left a comment

Choose a reason for hiding this comment

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

For me it looks very good now, i tested it and seems to be a nice module. Thanks for your effort :)

@marylla
Copy link
Contributor

marylla commented Nov 3, 2023

@pedrobaeza Can we merge this DMS PR?

@pedrobaeza
Copy link
Member

/ocabot merge nobump
/ocabot migration dms

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-262-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4101217 into OCA:16.0 Nov 3, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@vancouver29
Copy link
Author

On the file CREDITS.rst, put that the migration to 16.0 has been funded by agent-z18, and on contributors, put your names.

@pedrobaeza thank you very much for merging this work. But I don't see the changes in README.rst anymore (we are contributors and @agent-z28 is the sponsor for this migration from v15 to v16). Is something here missing? @marylla FYI

@pedrobaeza
Copy link
Member

You didn't modify readme/CREDITS.rst nor readme/CONTRIBUTORS.rst, but README.rst, so that's why your changes are lost. Read again my comment, in which I mention the file. Do another PR introducing that changes.

@len-foss
Copy link
Contributor

len-foss commented Nov 4, 2023

@vancouver29 You can check that your changes would be properly applied by running the maintainer tools manually. Just install it with pipx and run oca-gen-addon-readme --repo-name=dms --branch=16.0 --addon-dir=dms, this way you won't have any surprise once your PR is merged.

https://github.com/OCA/maintainer-tools#readme-generator

@pedrobaeza
Copy link
Member

Now only having pre-commit, the readme regeneration is done.

@vancouver29
Copy link
Author

You didn't modify readme/CREDITS.rst nor readme/CONTRIBUTORS.rst, but README.rst, so that's why your changes are lost. Read again my comment, in which I mention the file. Do another PR introducing that changes.

@pedrobaeza Sorry, my fault! Thanks for the hint again. I just created a new PR here #268

@vancouver29 You can check that your changes would be properly applied by running the maintainer tools manually. Just install it with pipx and run oca-gen-addon-readme --repo-name=dms --branch=16.0 --addon-dir=dms, this way you won't have any surprise once your PR is merged.

https://github.com/OCA/maintainer-tools#readme-generator

@len-foss, thank you very much for the hint. I will notice it next time :)

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.