-
-
Notifications
You must be signed in to change notification settings - Fork 238
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] operating_unit: migration to 16.0 #576
Conversation
…per the OCA guidelines.
* [MIG] operating_unit to v10.0
/ocabot migration operating_unit |
Sorry @huguesdk you are not allowed to mark the addon tobe migrated. To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons. If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the |
5d4d1c4
to
a94a0aa
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.
👍 Technical review
/ocabot migration operating_unit |
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.
Functional review. I can remove operating unit for users except for the admin, If I remove the operating unit for the admin and I jump out and jump in again the user form view of the admin I see all OUs are assigned. Can't remove. Could you check that?
Hi @huguesdk, did you see the comment from Aaron ? Does it needs action from your side ? It's an improvement or this should work as the Aaron's expectations ? Thank you :) |
hi @houssine78. yes, i saw the comment, but thanks for the heads up! i did not intend to change the behavior, so if it does not work as in 15.0, then i need to fix it. @AaronHForgeFlow: thanks for the review! do you know if this is something that works as expected in 15.0? |
@@ -41,22 +42,6 @@ class OperatingUnit(models.Model): | |||
), | |||
] | |||
|
|||
@api.model | |||
def name_search(self, name="", args=None, operator="ilike", limit=100): |
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.
is there a reason for the removal of the name_search method?
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.
yes, it is because the new _rec_names_search
class variable now allows for the same behavior without the need to define .name_search()
. see the guide to migrate to 16.0.
This PR has the |
@AaronHForgeFlow: i tested in 15.0 and the behavior is the same. it comes from this code, which forces the users belonging to the |
Thanks for checking @huguesdk I think that is ok then. I just won't assign users to that group when they don't have access to all OU. |
/ocabot merge nobump |
On my way to merge this fine PR! |
Congratulations, your PR was merged at 60ca2d0. Thanks a lot for contributing to OCA. ❤️ |
No description provided.