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

[16.0][MIG] product_multi_company #459

Closed
wants to merge 38 commits into from

Conversation

atchuthan
Copy link
Member

@SodexisTeam SodexisTeam force-pushed the 16.0-mig-product_multi_company branch from 2a652e9 to adfd3b0 Compare April 20, 2023 08:02
@amkarthik amkarthik mentioned this pull request Apr 21, 2023
20 tasks
@SodexisTeam SodexisTeam force-pushed the 16.0-mig-product_multi_company branch 3 times, most recently from f8d9673 to 7094739 Compare April 24, 2023 13:49
@SodexisTeam SodexisTeam force-pushed the 16.0-mig-product_multi_company branch 3 times, most recently from cd71b94 to 5be65d8 Compare July 26, 2023 10:25
Comment on lines 12 to 49
company_ids = fields.Many2many(
string="Companies",
comodel_name="res.company",
default=lambda self: self._default_company_ids(),
)

@api.model_create_multi
def create(self, vals_list):
"""Ensure the template initially has the same companies as the variant."""
for values in vals_list:
if not values.get("company_ids") and values.get("product_tmpl_id"):
values["company_ids"] = [
(
6,
0,
self.env["product.template"]
.browse(values.get("product_tmpl_id"))
.company_ids.ids,
)
]
res = super(ProductProduct, self).create(vals_list)
if "from_create_variants" not in self._context:
res.product_tmpl_id.write({"company_ids": [(6, 0, res.company_ids.ids)]})
return res

def write(self, values):
"""Ensure the template's companies are a superset of the variant's."""
if self.env.context.get("company_inverse"):
return super(ProductProduct, self).write(values)
if values.get("company_ids") and values["company_ids"][0][0] == 6:
# Add companies to the template, but don't remove them.
companies_to_add = list(
set(values.get("company_ids")[0][2]).difference(self.company_ids.ids)
)
companies_to_add.extend(self.product_tmpl_id.company_ids.ids)
if companies_to_add:
self.product_tmpl_id.write({"company_ids": [(6, 0, companies_to_add)]})
return super(ProductProduct, self).write(values)
Copy link
Member

Choose a reason for hiding this comment

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

All this code doesn't exist in the previous branch. Actually, there was no product_product.py file. Only a small product_template.py:

https://github.com/OCA/multi-company/tree/ae4570d4f327ce5e0ad3779bbcde3369b82c3acb/product_multi_company/models

I observe also that in the commit 9633be1, labeled [MIG] product_multi_company: black, isort, prettier, you're not actually just passing the automated pre-commit reformats. You're refactoring the module completely.

Most of the logic you add there should be unnecessary at first sight, as it should exist already in the inherited multi.company.abstract model.

I'm confused. Why did you have to refactor the module? Didn't the abstract model work? Why did you hide these big changes in the reformat commit?

Copy link
Member

Choose a reason for hiding this comment

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

@yajo

In v13, they added those extra changes to avoid the below error.
#252
And it is still happening in the latest v15 code.

Detailed info regarding the issues is in the below link
#264

We are having the same issue in migrated code too.

I think we need to add those back.

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

@yajo
Creating a related stored company_ids field on the product.product solves this issue.
Shall we go with this instead of having those all changes from v13?
I can't think of any disadvantages with this method.

pedrobaeza and others added 20 commits September 26, 2023 13:43
==========================================
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
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: multi-company-12.0/multi-company-12.0-product_multi_company
Translate-URL: https://translation.odoo-community.org/projects/multi-company-12-0/multi-company-12-0-product_multi_company/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: multi-company-12.0/multi-company-12.0-product_multi_company
Translate-URL: https://translation.odoo-community.org/projects/multi-company-12-0/multi-company-12-0-product_multi_company/
@amkarthik
Copy link
Member

@yajo
We used 13.0 branch during our migration as it contain more commits than previous.
And we tested by migrating the previous branch and it looks those changes in 13.0 is not needed any more.
We will contribute it ASAP

Thanks!

@yajo
Copy link
Member

yajo commented Sep 28, 2023

At this point, it will probably be easier to just restart migrating from 15.0.

@SodexisTeam SodexisTeam force-pushed the 16.0-mig-product_multi_company branch 2 times, most recently from 2ad1ce2 to 7d4068d Compare October 3, 2023 07:25
@amkarthik
Copy link
Member

@yajo

We are finished with the work. Please check again.

Thanks!

company = self.env.company
self.assertTrue(company.id in product.company_ids.ids)
self.assertFalse(product.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.

issue: wrong test fix.

This test was checking that the current company was added to product.company_ids. That's exactly the purpose of this module.

If you commit this change and the tests go ✔️, it means that something's not working properly in the module. Instead, probably you should undo this change and actually fix the module.

Copy link
Member

Choose a reason for hiding this comment

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

@yajo
We will check on it.
Have you had a chance to look into this comments?
#459 (comment)
#459 (comment)

Please review and let us know your feedback, that will be very helpful.

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

issue: wrong test fix.

This test was checking that the current company was added to product.company_ids. That's exactly the purpose of this module.

If you commit this change and the tests go ✔️, it means that something's not working properly in the module. Instead, probably you should undo this change and actually fix the module.

We have removed the default value from the company_ids fields.
Please review this link
https://github.com/OCA/multi-company/pull/458/files#diff-768c66d1106f5fdcb827fc104212e9b76a2739fae84f770ffe2687ec3bea1c4bR19

And this is to support the statement that if a product doesn't have any companies set then it is shared by all the company.

I think we just need to rename the test method name. Correct me If I misunderstood something.

Thanks

@Shide
Copy link
Contributor

Shide commented Dec 15, 2023

Tests still failing

@legalsylvain
Copy link
Contributor

/ocabot migration product_multi_company

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Dec 17, 2023
@amkarthik amkarthik force-pushed the 16.0-mig-product_multi_company branch from 5fb53a3 to 24e9503 Compare December 18, 2023 07:32
@amkarthik
Copy link
Member

@Shide
The reason for the error is #459 (comment)
The suggested fix in v16 is #459 (comment)
I would like to have your thoughts on the fix.
cc: @yajo

@amkarthik amkarthik force-pushed the 16.0-mig-product_multi_company branch from 24e9503 to 0ede9b3 Compare December 27, 2023 11:19
@amkarthik amkarthik force-pushed the 16.0-mig-product_multi_company branch from 0ede9b3 to d8e1d26 Compare January 4, 2024 08:17
@pilarvargas-tecnativa
Copy link

Superseed by #628

@pedrobaeza pedrobaeza closed this May 23, 2024
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.