[19.0][MIG] product_multi_company - #1014
Conversation
chienandalu
left a comment
chienandalu
left a comment
There was a problem hiding this comment.
There's something wrong with the base
ddf481d to
aade9d5
Compare
OCA-git-bot
added
series:19.0
mod:product_multi_company
Fixed! Thanks for the warning. |
fcvalgar
left a comment
fcvalgar
left a comment
There was a problem hiding this comment.
Hi @EmilioPascual, thanks for the migration work on this module. I found an important functional case that I think should be reviewed before approval.
When a product is initially shared across all companies, with no company restriction, and it has already been used in sales orders from different companies, restricting that product afterwards to only one company can cause errors when accessing historical documents from the other companies.
From a functional point of view, I think the ideal behavior would be that this restriction can still be applied without blocking access to past documents. Historical sales orders, invoices, pickings or other existing records should remain readable and usable, even if the product is no longer available for new operations in that company.
As an alternative minimum safeguard, the system could show a clear warning and prevent the change, for example indicating that the product is already used in documents from other companies and therefore cannot be restricted in a way that would exclude them. However, I think this should be considered a fallback option, because ideally the company restriction should affect product availability for new usage without breaking existing historical records.
Test 1: OK - Shared product without company restriction is visible from multiple companies.
Test 2: OK - Product restricted to one company is only visible from that company.
Test 3: OK - Product restricted to two selected companies is visible only from those companies.
Test 4: OK - Product allowed for the active company can be used in a sales order.
Test 5: OK - Product restricted to another company is not available in a new sales order.
Test 6: OK - Updating the allowed companies changes the product visibility as expected.
Test 7: OK - Product template and variants follow the same multi-company restriction.
Test 8: OK - Existing demo product can be restricted to a specific company.
Test 9: OK - Removing all company restrictions makes the product shared again.
Test 10: OK - Product list respects the user company access rules.
Test 11: Not OK - A product initially shared and already used in sales orders from multiple companies causes an error after being restricted to only one company and then accessing historical documents from an excluded company.
Also, there seems to be a small typo in the PR title: product_multi_coimpany should probably be product_multi_company.
Could you please review this case? Once adjusted, I will be happy to validate the PR again.
EmilioPascual
changed the title
I think the current approach is appropriate, as if a product has been restricted to one company, it is normal for other companies not to be able to view the documents containing that product. That said, I do wonder whether it makes sense to revoke a company’s access to a product that it has already used. |
fcvalgar
left a comment
fcvalgar
left a comment
There was a problem hiding this comment.
Hi @EmilioPascual, thanks for the explanation.
I agree with your point. If the product is explicitly restricted to one company, it makes sense that other companies should no longer be able to access documents containing that product. The doubtful part is indeed more about whether it makes functional sense to revoke access to a product that has already been used by another company, but I agree this is aligned with the current approach of the module.
LGTM, thanks for the work.
|
Please @OCA/intercompany-maintainers could you /ocabot migration product_multi_company We are running in production this module and dependencies |
|
Sorry @rafaelbn you are not allowed to mark the addon to be migrated. To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons. If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the |
Gelojr
left a comment
Gelojr
left a comment
There was a problem hiding this comment.
Great work on this migration, everything looks good from a functional point of view.
The following tests have been performed:
- Shared product across all companies: A product without assigned companies was created and correctly remained visible from all companies. OK
- Product restricted to one company: A product assigned only to one company was visible from that company and not available from another one. OK
- Product shared between two companies: A product assigned to two companies was correctly visible and editable from both of them. OK
- Product creation with an unauthorized company: A user tried to create a product assigned to a company they do not have access to, and Odoo correctly raised the expected multi-company access error. OK
- Assigned companies update: A product initially assigned to one company was updated to include another company, and it became available for the newly added company. OK
- Product made global again: All assigned companies were removed from a product, and it correctly became visible from all companies. OK
- Product variants behavior: It is possible to assign companies from a product variant. However, adding or removing a company on a single variant automatically updates the company assignment for all variants of the same product. OK. Note: Please confirm whether this behavior is the expected functional design of the module.
- Product visible in two companies but not in a third one: A product assigned to two companies was visible from both of them and not visible from a third company not included in the selection. OK
BhaveshHeliconia
left a comment
BhaveshHeliconia
left a comment
There was a problem hiding this comment.
Functional review LGTM!
|
/ocabot migration product_multi_company |
| @@ -0,0 +1 @@ | |||
| odoo-addon-base_multi_company @ git+https://github.com/OCA/multi-company.git@refs/pull/889/head#subdirectory=base_multi_company | |||
There was a problem hiding this comment.
@EmilioPascual can you remove the "DON'T MERGE commit"?
There was a problem hiding this comment.
Done! Thank you for letting me know.
========================================== Product permissions for discrete companies ========================================== This modules allows to select in which of the companies you want to use each of the products. Installation ============ This module uses the post and uninstall hooks for updating default product template security rule. This only means that updating the module will not restore the security rule this module changes. Only a complete removal and reinstallation will serve. Usage ===== On the product form view, go to the "Information" tab, and put the companies in which you want to use that product. If none is selected, the product will be visible in all of them. The default value is the current one.
* Rename manifest * Change openerp references to odoo * Bump version * Add pragma no cover * Edit security of product employee to allow writes (in tests) * Fix permissions in tests * Fix domain & add test * Implement base_multi_company on product_multi_company * Add related cols for product variant
Currently translated at 100.0% (4 of 4 strings) Translation: multi-company-16.0/multi-company-16.0-product_multi_company Translate-URL: https://translation.odoo-community.org/projects/multi-company-16-0/multi-company-16-0-product_multi_company/it/
product.product has a delageted inheritance from product.template so we
are getting the fields logic but not the ORM logic. We need some of that
logic in the search method to be able to get the right results with
domains like [("company_id", "in", [1, False]) to include records which
are shared between companies.
TT51779
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: multi-company-17.0/multi-company-17.0-product_multi_company Translate-URL: https://translation.odoo-community.org/projects/multi-company-17-0/multi-company-17-0-product_multi_company/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: multi-company-17.0/multi-company-17.0-product_multi_company Translate-URL: https://translation.odoo-community.org/projects/multi-company-17-0/multi-company-17-0-product_multi_company/
Currently translated at 100.0% (3 of 3 strings) Translation: multi-company-18.0/multi-company-18.0-product_multi_company Translate-URL: https://translation.odoo-community.org/projects/multi-company-18-0/multi-company-18-0-product_multi_company/sl/
aade9d5 to
53f680c
Compare
LoisRForgeFlow
left a comment
LoisRForgeFlow
left a comment
There was a problem hiding this comment.
Thanks! let's move forward 🚀
/ocabot merge nobump
|
On my way to merge this fine PR! |
|
This PR has the |
|
Congratulations, your PR was merged at 751b152. Thanks a lot for contributing to OCA. ❤️ |
20 participants
Supersed #917 whilst preserving the original commits. There has been no response from the author following several requests.
Depends on:
@fcvalgar @Gelojr @chienandalu @bizzappdev @weinni2000 please review, thank you.
MT-14467 @moduon