-
-
Notifications
You must be signed in to change notification settings - Fork 856
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
[17.0] [MIG] partner_deduplicate_acl #1805
[17.0] [MIG] partner_deduplicate_acl #1805
Conversation
Functional review LGTM, Thanks :) |
Functional review LGTM Thanks, Could you please squash administrative commits? https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate Thanks! |
Sure thing, Im on it! 😀 |
ea2c1e1
to
73de651
Compare
@carolinafernandez-tecnativa Done! Ty for review. |
Still administrative commits im seeing, please check this https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate You have to squash oca-git commits and others defined on link sent before |
The commit history is definitely wrong, with 486 commits and most of them are not from this module. |
Hi @pedrobaeza , I personally do believe so, as when I followed the guide it got all the commits from the branch, might you tell me how to fix it? or is this the normal behaviour? Thanks |
I think the module variable hasn't been correctly set, so no filter was done in the procedure. |
So what would be the process to follow in order to fix it? |
The specified in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-17.0, but taking into account if you don't have the standard bash, to adapt it to your CLI shell, or directly write the module name in the commands instead of the variable. |
Taking into account that the commit history was properly until I followed the guide, I dont think it was the variable, but might be a missunderstanding on my part when following the command as for in the guide it says "git rebase -i origin/x, where x is the Odoo version (14.0, 15.0, 16.0,...).", so I replace the "x" with 17.0, should I have used the branch where I was doing the migration instead of the 17.0 base one? |
This module allows any user to get special permissions to be able to deduplicate contacts, instead of just giving them to a sale manager with settings permissions. The new permissions allow the user to deduplicate: - *Manually*. - *Automatically* (dangerous). - *Without restrictions* (more dangerous). Beware what you do! 😯
…ers from the partner list views (action 'Merge automatically') Use <record id="crm.action_partner_merge"... to change only affected field.
* Don't overwrite code, but use inheritance system * Add tests
Now there's an argument for controlling extra checks
Need adapt menu to not depends on crm module as before
73de651
to
96fbb96
Compare
No, it should be |
[MIG] partner_deduplicate_acl: Migration to 15.0 [FIX] partner_deduplicate_acl: Put menu on lower sequence So that when clicking on Contacts main menu, it doesn't open the wizard by default. [FIX] partner_deduplicate_acl: Change menu group The menu for deduplicating was only accessible for the users having the group "Automatically", so the group "Manual" users weren't able to access it. This commits changes to this second group for both being able to access
96fbb96
to
6740da7
Compare
@pedrobaeza Hi, now it should be good,d can you check it out? Ty |
/ocabot migration partner_deduplicate_acl |
@@ -7,7 +7,7 @@ | |||
{ | |||
"name": "Deduplicate Contacts ACL", | |||
"summary": "Contact deduplication with fine-grained permission control", | |||
"version": "16.0.1.0.1", | |||
"version": "17.0.1.0.0", |
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.
You have changed the module version in the "pre-commit auto-fixes" commit instead of the migration one. I know that you are receiving a pre-commit error when trying to commit this one, but that's why in the migration guide, it puts that you must put --no-verify
in the command for avoiding it. At the end, it's just to follow strictly the migration guide. Please place this in the proper place. I'm being picky here because if not, reviewing by commit is not effective and error prone to not detect possible problems.
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.
Absolutely understandable, omw to change 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.
Change is done now, although now the contributors change is now on the pre-commit fixes, which wouldnt be properly, only way to actually fully fix this is to re-do it from zero, if you would say that was the thing to do let me know and Ill re-do it, Ty
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.
Don't worry, let's keep it this time this way, not being critical.
6740da7
to
62ef475
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 nobump
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at f3a0d6b. Thanks a lot for contributing to OCA. ❤️ |
Standart Migration