-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
[14.0][FIX] partner_multi_company: default value for res_partner.company_ids #575
[14.0][FIX] partner_multi_company: default value for res_partner.company_ids #575
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.
Functional ok
@pedrobaeza asking for your opinion on this:
- for sure it's not correct that users with only one allowed company can create partners for all companies, so that should be fixed somehow
- I also don't want to change existing behaviors without notice
How do you suggest we approach this? Should we implement this behavior optionally through a setting?
b242947
to
aff412e
Compare
aff412e
to
6557670
Compare
cf87043
to
40b4c6f
Compare
Updated the docs with the new option |
40b4c6f
to
cc5c420
Compare
I also added a test to address these changes |
339bb23
to
014fc22
Compare
Ok, we should be good by now. Let me know if you see any issue |
db0bac4
to
cf8cc37
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.
Code and functional review, LGTM
cf8cc37
to
1bf88ee
Compare
@pedrobaeza in the end we went for the setting, in order to minimize disruption. Let us know if it's good for merge, thanks! |
This PR has the |
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 review, LGTM
In the tests, there are probably simpler ways to to fix the issue with the missing accounts, but that's still fine IMO.
@pedrobaeza shall we merge? |
/ocabot merge minor |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 732e0cf. Thanks a lot for contributing to OCA. ❤️ |
When
company_ids
is not set that means the contact will be available to all companies. This might create some noise and unwanted behavior.A user assigned to only one company that creates a new contact without changing the
company_ids
field is going to make the contact available for anyone.We can set the field to the current company in order to prevent this. If the user really wants to make the contact available to everyone they have to manually change the field.
Moreover the current module behavior goes against what's written in the USAGE file