[10.0][MIG] product_multi_company #35

lasley · 2017-01-13T19:46:02Z

Migration of product_multi_company to v10

Rename manifest
Change openerp references to odoo
Bump version

This depends on

[10.0][ADD] base_multi_company: Create new module #39

pedrobaeza · 2017-01-13T20:25:49Z

It seems there are some problems with the tests.

lasley · 2017-01-13T20:28:45Z

Ahhhh yup I have these modules installed in my local. Fix incoming, simple simple!

Weird runbot didn't fail though.

lasley · 2017-01-13T20:47:28Z

Am I reading v10 product security right? There's no way for a non-admin user to edit a product without depending on sales_team?

Adding a dependency simply for a test seems like overkill IMO. I almost feel like it would be better to create a new group in the test with proper permissions. Or maybe just edit an existing group. Thoughts?

lasley · 2017-01-13T22:13:29Z

Hooray editing the security seems to have worked & I don't think will have any side effects due to transaction rollback. This is ready for review.

pedrobaeza · 2017-01-14T10:42:21Z

product_multi_company/hooks.py

+        template_model = env['product.template']
+        groups = template_model.read_group([], ['company_id'], ['company_id'])
+        for group in groups:
+            if not group['company_id']:


Put here # pragma: no cover as the next line is not important to test and you'll achieve full coverage.

pedrobaeza

Little nitpicking

lasley · 2017-04-20T16:23:21Z

This is now WIP, I'm about to strip some logic out and move it into base_multi_company #39

Cedric-Pigeon · 2017-05-03T11:41:44Z

@lasley @pedrobaeza Hello I totally disagree with this PR. From my point of view, it should not depends on #39. For example, in my project I use product-multi-company but I am not interested at all to have active per company.
The suggestion is really interesting but should not be imposed to every one.
I will suggest a new PR to simply migrate product-multi-company as it was, not less, not more.

pedrobaeza · 2017-05-03T12:02:55Z

@Cedric-Pigeon, if you don't include active mechanism too, you will have usability problems. I have already found that problems, but minimize them with the users, covering the underlying problem.

What are your concerns? Performance?

Cedric-Pigeon · 2017-05-03T12:08:59Z

@pedrobaeza My concern is that my customer don't want this active mechanism. He want a single flag common to all companies.
Yes I am afraid we will have a lot of problems. My customer has more than 80.000 products and performance were a real day-to-day fight until now and I don't want to take any additional risk. 2 separated PR let the choice to use it or not.

pedrobaeza · 2017-05-03T12:12:59Z

Maybe an opt-in/opt-out mechanism inside the same module? What I want to avoid is to have partner_multi_company, partner_multi_company_active, product_multi_company, product_multi_company_active and so on.

@lasley what do you think?

Cedric-Pigeon · 2017-05-03T12:22:57Z

@pedrobaeza I understand your position but I still think we are too far from a simple migration, adding features should be done separatly otherwise backports will be totally impossible.

pedrobaeza · 2017-05-03T12:27:45Z

Well, if someone do the migration for you, that's not a problem. I also understand your position about solving your specific problem and thus not complicating with other not needed features, but if the architecture is good (which is), migration for both modules (product_multi_company and base_multi_company) is not going to be a problem. Even more, you will reduce the needed code, as part of the logic is shared on base_multi_company.

Cedric-Pigeon · 2017-05-03T12:31:56Z

@pedrobaeza Sorry for me the architecure is not good. You will multiply queries by 3.
And, as I said backport will be totally impossible.

lmignon · 2017-05-03T12:32:10Z

@pedrobaeza The proposed implementation in #39 would have a serious problem at scale!!!!! (#39 (comment), #39 (comment))

lmignon · 2017-05-03T13:15:17Z

@pedrobaeza IMO 'visibility by company' and 'active by company' desserves different purposes and we must be able to choice one and/or other. As commented the proposed architecture will lead to serious performance issue. Since product_multi_company is a requirement for our client and is a central module of our solution we propose to migrate the current implementation from 9.0 to 10.0. The current PR could be rebased on the migrated module once a solution to avoid performance issues and to put in place your opt-in/opt-out mechanism will be found. This approach will let us move forward and benefits to others users of the product_multi_company module.

lasley · 2017-05-03T15:43:25Z

I recommend you take a look at how much code I have reduced in this module before stating that a 1:1 migration is the best option. This module and all like it duplicate a bunch of code, and 100% need a base module.

I see your point that active by company could not be desired in some instances. I can remove that feature from the #39 base, then just add it into another module. We could still avoid the cascading module issue mentioned by Pedro, because we'd be using a common base and would only need one module to change its operation.

TBD whether an enable/disable option is feasible once I come up with a new implementation, but I'll keep that in mind as well. If that works, it can just be an upgrade to the base instead of a new module.

pedrobaeza · 2017-05-03T17:20:51Z

OK, for me let's go for base_multi_company + product_multi_company depending on it with a lot of duplication removed, and then an extra module base_multi_company_active that adds automatically on all dependent modules (partner_multi_company, product_multi_company...) the active by company feature.

lasley · 2017-05-03T17:23:30Z

Just as a note, once the base merges in I will also upgrade partner_multi_company to v10 using it too.

Cedric-Pigeon · 2017-05-03T19:01:13Z

@lasley @pedrobaeza great for me if you separate active per company in another module. Thanks for your consideration.

Cedric-Pigeon · 2017-05-10T10:02:33Z

@lasley Could you please remove base_multi_company module form this PR ?

lasley · 2017-05-10T16:45:09Z

@Cedric-Pigeon - This PR is dependent on base_multi_company and will not work without it. I will rebase once #39 is merged, which will remove base_multi_company

Cedric-Pigeon · 2017-05-11T07:20:47Z

@lasley Yes I know I tested it on my own branch with the PR base_mutli_company and it works very well.

========================================== 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.

lasley · 2017-05-26T18:13:30Z

This has been rebased and squashed now that #39 has been merged, and is now ready for review!

lasley · 2017-05-26T18:26:37Z

product_multi_company/tests/test_product_multi_company.py

+        self.product_company_none.sudo(self.user_company_2.id).name = "Test"
+
+    def test_company_1(self):
+        self.assertEqual(self.product_company_1.company_id, self.company_1)


I think I missed some part of the functionality of this module. Looking here we are operating on the product.product, but we have implemented on product.template.

Is the intent of this module to distribute the companies to the variants too? In my case this is less than ideal, because the variants are owned by the companies themselves. Are there cases I am not considering, in which the variants would be owned by multiple companies?

@lasley yes we have cases where the variants are owner by multiple companies...

@lmignon The same companies in all instances?

@lasley I agree with @lmignon. I think that by default the sharing of variants must be identical than the sharing of their template.

Yeah I have to design this. My use case is not quite so simple, and requires isolated company sets. I've set some time next week for this.

@lasley I investigated deeper your problem. This line has no chance to succeed because the user on self.product_company_1 is admin and its company is not self.company_1 but main company. I think the test should be self.assertEqual(self.product_company_1.sudo(self.user_company_1).company_id, self.company_1) because in base_multi_company, the company_id field is computed based on the user of self.env.

Another way to solve it is to create product_company_1 with self.user_company_1 in the setUp method.

elicoidal

LGTM

elicoidal · 2017-05-27T01:21:32Z

product_multi_company/hooks.py

+_logger = logging.getLogger(__name__)
+
+try:
+    from odoo.addons.base_multi_company import hooks


Isnot this redundant with the dependency in manifest?

It is- but if for some reason the module isn't there, Odoo will fail to start.

Cedric-Pigeon · 2017-05-29T08:12:45Z

@lasley Could you check why tests are failing ?

lasley · 2017-05-31T17:43:55Z

@Cedric-Pigeon - tests are failing due to the product variant discussion here

Cedric-Pigeon · 2017-06-02T10:00:17Z

product_multi_company/tests/test_product_multi_company.py

+        self.product_company_none.sudo(self.user_company_2.id).name = "Test"
+
+    def test_company_1(self):
+        self.assertEqual(self.product_company_1.company_id, self.company_1)


@lasley I investigated deeper your problem. This line has no chance to succeed because the user on self.product_company_1 is admin and its company is not self.company_1 but main company. I think the test should be self.assertEqual(self.product_company_1.sudo(self.user_company_1).company_id, self.company_1) because in base_multi_company, the company_id field is computed based on the user of self.env.

Another way to solve it is to create product_company_1 with self.user_company_1 in the setUp method.

Cedric-Pigeon · 2017-06-02T10:00:26Z

product_multi_company/tests/test_product_multi_company.py

+            self.product_company_2.sudo(self.user_company_1).name = "Test"
+
+    def test_company_2(self):
+        self.assertEqual(self.product_company_2.company_id, self.company_2)


Same issue here, the test should be self.assertEqual(self.product_company_2.sudo(self.user_company_2).company_id, self.company_2)

Ahhh yes how could I forget the fancy new computation?! Good call 😄

lasley · 2017-06-07T19:39:17Z

This PR should be good to go now.

Note - I am now proposing a module rename to product_template_multi_company in order to more closely align with this module's functionality.

I will be submitting a new module product_product_multi_company, which matches the functionality I need and thought was provided here (companies at the variant level).

Cedric-Pigeon

Good for me

pedrobaeza · 2017-06-07T20:00:18Z

Why don't you put both functionalities in this module?

lasley · 2017-06-07T20:12:05Z

@pedrobaeza - In this module, the template and variant operate in identical fashion in respect to the companies. It was stated that the product variants and templates need to be aligned in this convo. While it was mentioned that this need only be the default operation, I need something that is fairly at odds with this module.

In my case, I require the template companies to be completely separate from the variant companies. A company with access to the variant will only receive read access to the template, unless that company is also in the template's companies.

This will effectively allow for management of the template by Company A, while the variants are managed by their individual companies (Variant 1 by Company B, Variant 2 by Companies C and D). Think of something like amazon.com's marketplace, where the UPCs are primarily controlled by Amazon, but some aspects of the posted product are controlled by the vendor.

In order to make this operate, my solution is to simply implement base_multi_company at the variant level + add some security rules. If I were to do that in this module, a significant amount of logic would be required in order to provide a flip switch - similar to what you did in product_multi_image. While this is definitely feasible, it adds a layer of complexity that I don't feel is required here.

Cedric-Pigeon · 2017-06-07T20:13:37Z

@pedrobaeza I completely agree with @lasley I think it should be split to a more specific module

lasley · 2017-06-07T20:31:52Z

@Cedric-Pigeon @lmignon - How are you guys currently handling pricelists? Looking at this code, I feel like there should be massive security issues:

    <record model="ir.rule" id="product_pricelist_comp_rule">
        <field name="name">product pricelist company rule</field>
        <field name="model_id" ref="model_product_pricelist"/>
        <field name="global" eval="True"/>
        <field name="domain_force"> ['|',('company_id','=',user.company_id.id),('company_id','=',False)]</field>
    </record>
    <record model="ir.rule" id="product_pricelist_item_comp_rule">
        <field name="name">product pricelist item company rule</field>
        <field name="model_id" ref="model_product_pricelist_item"/>
        <field name="global" eval="True"/>
        <field name="domain_force"> ['|',('company_id','=',user.company_id.id),('company_id','=',False)]</field>
    </record>

Edit: added the code, whoops

lasley · 2017-06-07T20:49:02Z

Oh snap nevermind, seems they work just fine. Makes sense in hindsight, they just restrict by company & there would be no reason for multiple companies to sell them!

So yeah, I think this is good for merge - unless there's disagreement on the name change?

I'll squash after confirmation & before merge - there's a few logical commits in here that we don't want to lose.

pedrobaeza · 2017-06-07T22:46:55Z

I prefer then to keep this one as product_multi_company, which is the usual way of working, and call the other module product_variant_multi_company, following same convention as the one used in https://github.com/OCA/product-variant

lasley · 2017-06-14T19:49:27Z

@pedrobaeza - renamed back to product_multi_company

pedrobaeza

Please squash at your taste and I'll merge.

Cedric-Pigeon · 2017-06-15T14:02:54Z

@lasley I think it should be a good idea to include record rules on product_template and product_product don't you ?

* 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

lasley · 2017-06-15T15:45:29Z

@Cedric-Pigeon - record rules are applied and removed by the base hooks

@pedrobaeza - squashed!

pedrobaeza · 2017-06-15T16:25:04Z

@lasley the product.product rule is missing as @Cedric-Pigeon says.

lasley · 2017-06-15T16:30:04Z

@pedrobaeza - I don't see a product.product rule to update. product_comp_rule is the only security rule for products in core. Am I misunderstanding something?

lasley · 2017-06-15T16:33:13Z

Wait thinking about it further - the company_id and company_ids is being implemented on the template. Wouldn't the security rules implicitly pass through due to the inheritance? I will need to create a new rule in my product.product module, but I think we're fine here. You guys have been using this previously with no security issues, yes?

pedrobaeza · 2017-06-15T16:38:52Z

Yeah, you're right, I didn't remember that part. Indeed the security rule is inherited. Merging...

lasley force-pushed the release/10.0/product_multi_company branch from a1f1894 to 15d7865 Compare January 13, 2017 19:46

pedrobaeza mentioned this pull request Jan 13, 2017

Migration to version 10.0 #29

Closed

8 tasks

lasley force-pushed the release/10.0/product_multi_company branch from 3f8c107 to d36886c Compare January 13, 2017 22:09

pedrobaeza reviewed Jan 14, 2017

View reviewed changes

pedrobaeza approved these changes Jan 14, 2017

View reviewed changes

lasley force-pushed the release/10.0/product_multi_company branch from 556a713 to 196b444 Compare April 22, 2017 00:11

lasley mentioned this pull request Apr 22, 2017

[10.0][ADD] base_multi_company: Create new module #39

Merged

lasley force-pushed the release/10.0/product_multi_company branch from 196b444 to 65bb528 Compare May 26, 2017 18:09

lasley commented May 26, 2017

View reviewed changes

elicoidal approved these changes May 27, 2017

View reviewed changes

Cedric-Pigeon reviewed Jun 7, 2017

View reviewed changes

lasley force-pushed the release/10.0/product_multi_company branch from 2a661bf to bad0856 Compare June 7, 2017 19:22

lasley changed the title ~~[10.0][MIG] product_multi_company~~ [10.0][MIG] product_multi_company (renamed product_template_multi_company) Jun 7, 2017

Cedric-Pigeon approved these changes Jun 7, 2017

View reviewed changes

lasley changed the title ~~[10.0][MIG] product_multi_company (renamed product_template_multi_company)~~ [10.0][MIG] product_multi_company Jun 14, 2017

lasley added this to the 10.0 milestone Jun 14, 2017

lasley added the needs review label Jun 14, 2017

pedrobaeza approved these changes Jun 15, 2017

View reviewed changes

lasley force-pushed the release/10.0/product_multi_company branch from b74e821 to ab61142 Compare June 15, 2017 15:45

pedrobaeza merged commit acd0b78 into OCA:10.0 Jun 15, 2017

[10.0][MIG] product_multi_company #35

[10.0][MIG] product_multi_company #35

Conversation

lasley commented Jan 13, 2017 • edited Loading

pedrobaeza commented Jan 13, 2017

lasley commented Jan 13, 2017 • edited Loading

lasley commented Jan 13, 2017 • edited Loading

lasley commented Jan 13, 2017

Choose a reason for hiding this comment

pedrobaeza left a comment

Choose a reason for hiding this comment

lasley commented Apr 20, 2017

Cedric-Pigeon commented May 3, 2017

pedrobaeza commented May 3, 2017

Cedric-Pigeon commented May 3, 2017

pedrobaeza commented May 3, 2017

Cedric-Pigeon commented May 3, 2017

pedrobaeza commented May 3, 2017

Cedric-Pigeon commented May 3, 2017

lmignon commented May 3, 2017

lmignon commented May 3, 2017

lasley commented May 3, 2017 • edited Loading

pedrobaeza commented May 3, 2017

lasley commented May 3, 2017 • edited Loading

Cedric-Pigeon commented May 3, 2017

Cedric-Pigeon commented May 10, 2017

lasley commented May 10, 2017

Cedric-Pigeon commented May 11, 2017

lasley commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elicoidal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cedric-Pigeon commented May 29, 2017

lasley commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley commented Jun 7, 2017

Cedric-Pigeon left a comment

Choose a reason for hiding this comment

pedrobaeza commented Jun 7, 2017

lasley commented Jun 7, 2017 • edited Loading

Cedric-Pigeon commented Jun 7, 2017

lasley commented Jun 7, 2017 • edited Loading

lasley commented Jun 7, 2017

pedrobaeza commented Jun 7, 2017

lasley commented Jun 14, 2017

pedrobaeza left a comment

Choose a reason for hiding this comment

Cedric-Pigeon commented Jun 15, 2017

lasley commented Jun 15, 2017

pedrobaeza commented Jun 15, 2017

lasley commented Jun 15, 2017

lasley commented Jun 15, 2017

pedrobaeza commented Jun 15, 2017

lasley commented Jan 13, 2017 •

edited

Loading

lasley commented Jan 13, 2017 •

edited

Loading

lasley commented Jan 13, 2017 •

edited

Loading

lasley commented May 3, 2017 •

edited

Loading

lasley commented May 3, 2017 •

edited

Loading

lasley May 26, 2017 •

edited

Loading

lasley commented Jun 7, 2017 •

edited

Loading

lasley commented Jun 7, 2017 •

edited

Loading