Skip to content
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][IMP/FIX] base_user_role_company - re-add logic around multiple active companies #242

Merged
merged 1 commit into from
Dec 26, 2023

Conversation

jdidderen-noviat
Copy link
Contributor

@jdidderen-noviat jdidderen-noviat commented Oct 5, 2023

Since this PR #147, the logic when you have several companies active at the same time is broken because only the main/active company is considered (And then inconsistent with the README of the module)

Imagine the following scenario where all companies below are active :

  • COMPANY A (Active/Main company): Accounting Manager
  • COMPANY B: Purchase Manager
  • COMPANY C: Inventory Manager
  • ALL COMPANY : Seller

Without the patch: The user will be Seller and Account Manager on all companies
With the patch: The user will be only Seller on all companies

Another situation is also managed. Having the same role for several companies.

@jdidderen-noviat jdidderen-noviat changed the title [IMP/FIX] base_user_role_company - re-add logic around multi active cids [IMP/FIX] base_user_role_company - re-add logic around multiple active companies Oct 5, 2023
@jdidderen-noviat jdidderen-noviat force-pushed the 14.0-user_role_company_fix branch 4 times, most recently from 30566d3 to efc8a8e Compare October 5, 2023 10:04
@jdidderen-noviat jdidderen-noviat marked this pull request as ready for review October 5, 2023 10:09
@jdidderen-noviat jdidderen-noviat changed the title [IMP/FIX] base_user_role_company - re-add logic around multiple active companies [14.0][IMP/FIX] base_user_role_company - re-add logic around multiple active companies Oct 5, 2023
@jdidderen-noviat
Copy link
Contributor Author

@OCA/tools-maintainers @dreispt Can you have a look at this PR ?

@dreispt
Copy link
Member

dreispt commented Dec 21, 2023

I did not test it, but I agree with the implementation goals and code review looks good.

@dreispt
Copy link
Member

dreispt commented Dec 26, 2023

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-242-by-dreispt-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 5cb8df1 into OCA:14.0 Dec 26, 2023
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

SiesslPhillip pushed a commit to grueneerde/OCA-server-backend that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-backend (15.0)
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.

3 participants