-
-
Notifications
You must be signed in to change notification settings - Fork 799
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] project_administrator_restricted_visibility: Migration to 16.0 #1222
[16.0][MIG] project_administrator_restricted_visibility: Migration to 16.0 #1222
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good!
One minor guideline missing to follow.
Also, rebase fixing your committer email as we discussed privately.
To let the bot auto-add this as the migration PR for the module, we issue this command:
/ocabot migration project_administrator_restricted_visibility
Currently translated at 100.0% (1 of 1 strings) Translation: project-13.0/project-13.0-project_administrator_restricted_visibility Translate-URL: https://translation.odoo-community.org/projects/project-13-0/project-13-0-project_administrator_restricted_visibility/it/
Currently translated at 100.0% (1 of 1 strings) Translation: project-15.0/project-15.0-project_administrator_restricted_visibility Translate-URL: https://translation.odoo-community.org/projects/project-15-0/project-15-0-project_administrator_restricted_visibility/it/
d5c92a8
to
ec215a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ec215a3
to
3c87a06
Compare
Code looks good and the module works perfectly fine. I did find a small issue. When trying to uninstall the module, it throws this error: It seems to be related to the uninstall hook. In older versions of Odoo (13 and lower), the field |
3c87a06
to
44a3d32
Compare
Thanks @NachoAlesLopez . It seems that this bug has existed since version 14. I just fixed the uninstall hook so that it first removes the group from the rule and then deletes it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and great job, @edlopen! Just tested it and the module uninstalled successfully.
44a3d32
to
f39afe5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job with that test! 🎉
One minor fix to be ready
project_administrator_restricted_visibility/tests/test_uninstall_hook.py
Outdated
Show resolved
Hide resolved
f39afe5
to
8cc2be4
Compare
8cc2be4
to
5f67a66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ocabot merge patch
On my way to merge this fine PR! |
Congratulations, your PR was merged at c500365. Thanks a lot for contributing to OCA. ❤️ |
Standard migration of the project_administrator_restricted_visibility.
MT-4844
@moduon