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.

Sign up for GitHub

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

Jump to bottom

[15.0][IMP] partner_multi_company: prevent inconsistencies between user and partner #667

Merged
merged 2 commits into from
Jul 23, 2024
Merged

[15.0][IMP] partner_multi_company: prevent inconsistencies between user and partner #667

merged 2 commits into from
Jul 23, 2024

Conversation

ArnauCForgeFlow
Copy link

@ArnauCForgeFlow ArnauCForgeFlow commented Jul 16, 2024 •
edited
Loading

This improvement prevent some inconsistencies between user and partner company_ids

  • If you try to modify the partner companies and the user companies are not included in the partner companies, an error will be raised.
  • If the user's companies are modified, they will be added to the partner while retaining the companies the partner already has.
  • Don't delete all partner company_ids when the user's related company_id is modified.

The change on the res_company_code was needed in order to pass odoo tests

@ArnauCForgeFlow ArnauCForgeFlow force-pushed the 15.0-imp-partner_multi_company branch 7 times, most recently from d0139c7 to b219e53 Compare July 17, 2024 12:39
LoisRForgeFlow
LoisRForgeFlow reviewed Jul 18, 2024
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArnauCForgeFlow Functional review

There is an issue when creating a new company, the constraint raises and it is blocked:

image

LoisRForgeFlow
LoisRForgeFlow approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion, the problem is only due to demo data coming from different modules. It is a status impossible to reach in a blank database.

Therefore, LGTM 👍

@JordiBForgeFlow
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-667-by-JordiBForgeFlow-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 19, 2024
@OCA-git-bot
Merge PR #667 into 15.0
88fd6c4 
Signed-off-by JordiBForgeFlow
@pedrobaeza pedrobaeza added this to the 15.0 milestone Jul 19, 2024
@OCA-git-bot
Copy link
Contributor

@JordiBForgeFlow your merge command was aborted due to failed check(s), which you can inspect on this commit of 15.0-ocabot-merge-pr-667-by-JordiBForgeFlow-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

pedrobaeza
pedrobaeza requested changes Jul 19, 2024
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Standard doesn't put any company to the partners of the users. Why changing the strategy?

partner_multi_company/migrations/15.0.1.1.0/post-migration.py Outdated

def migrate(cr, version):
env = api.Environment(cr, SUPERUSER_ID, {})
for user in env["res.users"].search([]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't duplicate code. Just import with from ...hooks import post_init_hook and call it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good remark

cc @ArnauCForgeFlow

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

partner_multi_company/models/res_users.py Show resolved Hide resolved
partner_multi_company/models/res_users.py Show resolved Hide resolved
@ArnauCForgeFlow ArnauCForgeFlow force-pushed the 15.0-imp-partner_multi_company branch 2 times, most recently from 11625f4 to 59d8794 Compare July 19, 2024 09:14
@LoisRForgeFlow
Copy link
Contributor

@pedrobaeza let us know if it looks good to you now

@LoisRForgeFlow
Copy link
Contributor

Hi @pedrobaeza

Does it looks better now? Could you update your review?

Thanks!

pedrobaeza
pedrobaeza approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ocabot merge nobump

Please fw-port it to upper versions.

partner_multi_company/migrations/15.0.1.1.0/post-migration.py
@@ -0,0 +1,10 @@
# Copyright 2024 ForgeFlow S.L. (http://www.forgeflow.com)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double copyright

partner_multi_company/migrations/15.0.1.1.0/post-migration.py
Comment on lines +5 to +6
# pylint: disable=W7950
from odoo.addons.partner_multi_company import hooks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# pylint: disable=W7950
from odoo.addons.partner_multi_company import hooks
from ...hooks import fix_user_partner_companies

and change the call

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not critical, so I launched the merge command.

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 15.0-ocabot-merge-pr-667-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 23, 2024
@OCA-git-bot
Merge PR #667 into 15.0
c5afa07 
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 15.0-ocabot-merge-pr-667-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 93dcce3 into OCA:15.0 Jul 23, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@LoisRForgeFlow LoisRForgeFlow deleted the 15.0-imp-partner_multi_company branch July 23, 2024 07:52
@ArnauCForgeFlow ArnauCForgeFlow mentioned this pull request Jul 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza approved these changes

@LoisRForgeFlow LoisRForgeFlow LoisRForgeFlow approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
15.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ArnauCForgeFlow @JordiBForgeFlow @OCA-git-bot @LoisRForgeFlow @pedrobaeza