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][MIG] base_multi_company #270

Merged
merged 66 commits into from
Mar 24, 2022

Conversation

rousseldenis
Copy link
Sponsor Contributor

@rousseldenis rousseldenis commented Mar 9, 2021

lasley and others added 30 commits March 9, 2021 18:24
* Create new module to provide base multi company logic and mixin
* Add deactivation by company mixin
* Add company_id/ids handling
* Add break after company is found
Squashed commits:
[854cc36] Increase test coverage
[770bd71] Revert hook view create back to model init
[40e803e] Fix apples and oranges
[7a4dfb4] Use registry correctly
[6e9f170] Switch company_id to computed & move company aliased view creation to post init hook
[faa4fc9] Remove active functionality
[fecfb59] Add explicit tests for active and inactive searches
* Revert "Revert hook view create back to model init"

This reverts commit 770bd71.

* [FIX] base_multi_company: Always create the view into a pre_init_hook to avoid error in log
Squashed commits:
[4c17d04] auto_join company_ids
…ch domain on company_id/company_ids (+1 squashed commit)

Squashed commits:
[fe161fe] [ADD] setup.py
- Test if a company is set in inverse method (+1 squashed commit)
Squashed commits:
[d670f30] [FIX] fix init hooks as company_id is not stored anymore
* Add implementation instructions to ReadMe
[FIX] Fix issue based on the computation of company_id for multi_company_abstract models

[FIX] Fix issue based on the computation of company_id for multi_company_abstract models

[FIX] Fix issue based on the computation of company_id for multi_company_abstract models

[FIX] Bad branch version pushed before

[FIX] Bad branch version pushed before

[FIX] manifest

Fix flake

Flake8 on OCA tests
Currently translated at 88.9% (8 of 9 strings)

Translation: multi-company-10.0/multi-company-10.0-base_multi_company
Translate-URL: https://translation.odoo-community.org/projects/multi-company-10-0/multi-company-10-0-base_multi_company/pt/
Odoo now checks if the user who creates the transient model is the same that is accessing
its data, so we need to create it with the same user.
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
* Abstract model shouldn't have default for `company_ids`. It will be got by
  default `company_id` value.
* Record rule based on `company_id` makes the evaluation slower. Switch to `company_ids` field.
* Add `@api.depends` to `company_id` computation for proper refresh.
* Fix tests that were not correct but luckily previously suceeded. Now that other code has
  been removed, they have been uncovered.
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: multi-company-12.0/multi-company-12.0-base_multi_company
Translate-URL: https://translation.odoo-community.org/projects/multi-company-12-0/multi-company-12-0-base_multi_company/
Currently translated at 70.0% (7 of 10 strings)

Translation: multi-company-12.0/multi-company-12.0-base_multi_company
Translate-URL: https://translation.odoo-community.org/projects/multi-company-12-0/multi-company-12-0-base_multi_company/zh_CN/
@rousseldenis
Copy link
Sponsor Contributor Author

hi @rousseldenis , any plans to finish this one?

I'll try but I need to dig some time

@LoisRForgeFlow
Copy link
Contributor

@rousseldenis A friendly reminder here, have you got the time to finish this?

@GSLabIt
Copy link

GSLabIt commented Mar 11, 2022

@rousseldenis did u have the chance to dive into this?

@rousseldenis
Copy link
Sponsor Contributor Author

rousseldenis commented Mar 14, 2022

@GSLabIt @LoisRForgeFlow @pedrobaeza This is done


for record in self:
company = record.company_id
if company and company not in record.company_ids:
Copy link
Member

Choose a reason for hiding this comment

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

Wait, this is not like my PR. We can't do make conditional that way, or we are modifying what is intended to do assigning a company, that is to change all the allowed companies to this one.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

That was done with that purpose. See comment before.

If people has no access to other companies, he cannot remove them.

Copy link
Member

Choose a reason for hiding this comment

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

But this is not OK. The intention to add the writeable company_id is to do exactly what is expected to do: limit the record to such company. This is working perfectly in v13. If not, please specify steps to reproduce a problem with this behavior.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

The behaviour changed in 13 from which it was at origin:

f5f3270

You reintroduced the inverse BUT changing the behaviour.

See the original commit at creation in 10:

7051f69#diff-768c66d1106f5fdcb827fc104212e9b76a2739fae84f770ffe2687ec3bea1c4bR45

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

My change was, at purpose, restablishing the original behaviour.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

@JoanSForgeFlow @LoisRForgeFlow @JordiBForgeFlow @JasminSForgeFlow

As you use/migrated this one in several versions, could you help here ? What is the expected behaviour with the write on company_id field ?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I'll fix this next week. Need to sleep on it :-)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for considering it.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

@pedrobaeza I've taken your changes entirely.

I need now to improve README

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks. Let me know when this can be merged.

pedrobaeza and others added 2 commits March 21, 2022 16:40
Not being writeable, we have warnings on constraints over this field
(it's used for example in `sale` module for `product.template` model).

Giving an inverse to the field, we avoid this problem and give
compatibility to any other module that writes this field.
@rousseldenis rousseldenis force-pushed the 14.0-mig-base-multicompany-dro branch from d099deb to cbbdb0e Compare March 21, 2022 16:30
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

👍 Thank you!

@rousseldenis
Copy link
Sponsor Contributor Author

@pedrobaeza @Cedric-Pigeon @LoisRForgeFlow This one is ready

base_multi_company/README.rst Outdated Show resolved Hide resolved
base_multi_company/README.rst Outdated Show resolved Hide resolved
base_multi_company/README.rst Outdated Show resolved Hide resolved
@rousseldenis rousseldenis force-pushed the 14.0-mig-base-multicompany-dro branch from 81af8f9 to 3a8ab65 Compare March 24, 2022 15:25
@pedrobaeza
Copy link
Member

Thanks, and I'm going to launch the bot with patch option for satisfying you although I don't like that glitch :)

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-270-by-pedrobaeza-bump-patch, awaiting test results.

@rousseldenis
Copy link
Sponsor Contributor Author

Thanks, and I'm going to launch the bot with patch option for satisfying you although I don't like that glitch :)

/ocabot merge patch

contribution is a location of compromises 😄 🚀

@OCA-git-bot OCA-git-bot merged commit 72ea6ec into OCA:14.0 Mar 24, 2022
@OCA-git-bot
Copy link
Contributor

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

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.

None yet