Skip to content

[16.0][MIG] dms - #262

Merged
OCA-git-bot merged 133 commits into
OCA:16.0from
elego:16.0-mig-dms-kbui
Nov 3, 2023
Merged

[16.0][MIG] dms#262
OCA-git-bot merged 133 commits into
OCA:16.0from
elego:16.0-mig-dms-kbui

Conversation

@vancouver29

Copy link
Copy Markdown

[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
Copy Markdown
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). 🤖

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

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?

Comment thread dms/models/dms_file.py Outdated
Comment thread dms/views/dms_file.xml
Comment thread dms/models/dms_file.py Outdated
@pedrobaeza

Copy link
Copy Markdown
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

vancouver29 commented Oct 29, 2023

Copy link
Copy Markdown
Author

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.

Comment thread dms/models/dms_file.py Outdated
Comment thread dms/models/dms_file.py Outdated
@len-foss

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member

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

@agent-z28 agent-z28 left a comment

Copy link
Copy Markdown

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

marylla commented Nov 3, 2023

Copy link
Copy Markdown
Contributor

@pedrobaeza Can we merge this DMS PR?

@pedrobaeza

Copy link
Copy Markdown
Member

/ocabot merge nobump
/ocabot migration dms

@OCA-git-bot

Copy link
Copy Markdown
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
@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

@vancouver29

Copy link
Copy Markdown
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
Copy Markdown
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

len-foss commented Nov 4, 2023

Copy link
Copy Markdown

@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
Copy Markdown
Member

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

@vancouver29

Copy link
Copy Markdown
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.