[MGR] Migrated product_customer_code to version 10

[PCatinean]

No description provided.

[pedrobaeza]

Please check Travis status

[PCatinean]

Fixes to pass all tests added

[cvinh]

fix issue with product search (PCatinean#1)

[PCatinean]

@invitu thanks for the fix. I also thought about what was the intention, showing only the code, only the product, or a mix of both (I would have wanted to go this route).

But it's up for debate I guess

[PCatinean]

I think it needs product_code.mapped('product').name_get()?

[cvinh]

BTW, I think it would be useful to show both product and customer_code in display_name (in sale_order for example)
something like [default_code] Product (customer_code)

[PCatinean]

@invitu I thought of the exact same pattern. If others agree I will make it like that np

[lukebranch]

I believe something similar exists (unported from 7 or 8):

https://github.com/OCA/sale-workflow/blob/10.0/product_customer_code_sale/__manifest__.py

Not exactly the same as you described but quite close.

[cvinh]

I think that we don't need product_customer_code_sale anymore if we append customer_code by overwriting ProductProduct.name_get()
@PCatinean do you agree ?

[pedrobaeza]

Well, you still need to get somehow (with the context) the customer involved, so a minimum change in sale is needed.

[cvinh]

@pedrobaeza self._context.get('partner_id') in ProductProduct does the job doesn't it ?

[PCatinean]

@invitu it does not, that implies the partner_id has been sent via the context which does not happen by default.

One semi-solution would be to browse the active_id (whichever model it is) and scan for relational fields towards res.partner and use that (if there is just one or such). But this approach is messy and can have bad consequences

[cvinh]

I just saw that another PR is pending here #122

@pedrobaeza suggested to use product_supplierinfo_for_customer #122 (comment) in order not to create a new model and it never happened

Another PR for product_customer_code_sale is here OCA/sale-workflow#345

[cvinh]

@PCatinean I think that we only need customer code in a sale_order context so partner_id can be retrieved. In other case, we don't need customer_code, so it's not a problem if we don't have it
I can find the same code in your PR (line 30 in product.py):
@api.model
def name_search(self, name='', args=None, operator='ilike', limit=100):
res = super(ProductProduct, self).name_search(
name=name, args=args, operator=operator, limit=limit)
if not res:
partner_id = self._context.get('partner_id')

[cvinh]

@PCatinean could you check PCatinean#4 ? It seems to work for me...

[LoisRForgeFlow]

Runbot is not working even after rebuilt 😕

[eLBati]

@PCatinean I would rename the fields as defined in #122, for instance
product_customer_codes --> product_customer_code_ids
product --> product_id

We can make a PR

[PCatinean]

@eLBati I followed the contributing guideline from OCA: https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#variable-names

If this is required/needed by OCA maintainers then I will rename of course.

What else is needed to merge this PR?

[eLBati]

@PCatinean variable names are different from field names, see
https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#fields

[PCatinean]

@eLBati right you are, my bad I mixed thing up.

Updating now

[LoisRForgeFlow]

Check the runbot warning related to this:

WARNING openerp_test odoo.models.schema: Table 'product_customer_code': unable to add constraint 'unique(product_code,company,partner)'!
If you want to have it, you should update the records and execute manually:

[lasley]

I'm not familiar with product_customer_supplierinfo_code, but product_customer_code and product_customer_code_sale should remain independent modules IMO. Aren't the _x modules just binding on the appropriate context?

That said, it does need a rename - sale_product_customer_code

[gdgellatly]

@lasley no the context is already bound. In fact in my functional review I noted that the context is potentially problematic as it is set in views other than sale related ones. partner_id is a very common context key. All customer_code_sale does is add the column to sale order line, which I guess was kind of my point about the name_get return. I don't really care, but it seems pointless to have a module which stores customer partcodes, then another to display them. What other module would make use of such a base module? Why would you not just have as a security option like uom or discount? But in any case the main thrust was the fact that we have PR's for 2 modules doing the same thing more or less being the supplierinfo one and this.

[eLBati]

@gdgellatly @lasley I don't know about product_customer_supplierinfo_code, but product_customer_code_sale extends product_customer_code only adding and displaying product_customer_code on sale.order.line (we could also add that field to sale order report)

Recap on the main functionalities

• product_customer_code allows to specify product codes depending on customers: every customer can have their product code, if needed
• product_customer_code_sale extends product_customer_code to display product customer code on sale order line

About menu

<menuitem action="action_product_customer_code_tree" id="menu_product_customer_code" sequence="50" parent="product.menu_root" groups="group_product_customer_code_manager"/>

we would move it to product_customer_code_sale because product_customer_code depends on product only and product does not introduce menu items

That said, it does need a rename - sale_product_customer_code

Agree

[eLBati]

@gdgellatly I can't see the problem with partner_id in context: if the key is there, we assume that the current partner is partner_id, thus we search products by customer code also; if it's not, standard search is performed.
Note that partner_id is only involved by name_search method

[gdgellatly]

@eLBati @lasley I wasn't overly clear. The 2 modules do roughly the same as the supplierinfo_customer modules. As I say I don't care whether you keep these as one or two, but its hard to converse if no one even wants to look at the other module to see if they are functionally similar or the same. See https://github.com/OCA/product-attribute/tree/8.0/product_supplierinfo_for_customer and #271

The potential problem with partner_id in context is every other model that isn't customer related. For example, purchase orders. In the context of purchases, is the partner a partner, a customer or a supplier? Should customer codes be searched on purchases? What if supplierinfo is also set? If functionally the answer is yes, no problem, it was just to me when I saw customer_code I guess I expected it used only in customer related contexts. What about manufacturing, pickings etc?

[lasley]

@gdgellatly - I see your point. Maybe what we need to do instead is abstract the terminology into more of a partner code instead.

In the context of sales, it's a customer code. In the context of purchases, it's a vendor code. In the context of manufacturing, maybe a supplier code or something.

Basically we're trying to maintain a code for Product A against Partners A, B, and C regardless of context. Then whatever glue modules simply add the context.

Am I oversimplifying this?

[gdgellatly]

@lasley It wasn't what I was thinking but yes. Ultimately there is a need to maintain partners codes and they wouldn't differ depending on the users context. That said, I do wonder how big the case is that a vendor buys its own products from its customers, and those codes happen to overlap with internal codes, and those codes would be stored. Only time I really see it is when vendor runs out of stock and needs to supply so rounds it up from their customers. Maybe it doesn't matter, not every possible corner case or strange implementation is able to be mitigated. My point was more was that using a very common context key for a specific purpose in a very common ORM method may have unintended side effects. More a warning, than a recommendation.

However, I still urge you to look at the supplierinfo_for_customer module. Having looked at both in detail now, functionally I think they serve almost identical use cases (in fact their readme's are nearly the same), but supplierinfo is very DRY in its approach and would additionally support pricing, qty breaks, descriptions. I guess its full featured, enhancing the existing model, whereas this adds customer codes as a seperate model to emulate some of supplierinfo model but not all. Not saying either approach is better or worse, just we potentially have two functionally very similar modules with very different technical approaches in the same repo.

[eLBati]

@gdgellatly I looked at product_supplierinfo_for_customer. It provides the same basic functionality of product_customer_code (defining product codes depending on customers) plus it allows to define prices and pricelists based on them.
Also, it is already available for 8.0, while, for product_customer_code, we have an open PR.

I think we should drop product_customer_code in favour of product_supplierinfo_for_customer and add extensions of it where needed (sale/purchase orders, reports...)

[AaronHForgeFlow]

Hi @PCatinean! Thank you for this proposal. In my opinion it makes sense to keep both modules, this one and the proposed here: #122 That one can be an extension of the product_customer_code. What do you think @eLBati ?

[eLBati]

Hi @aheficent, the module proposed in this PR and the one proposed in PR #122 are the same module.

Instead, product_supplierinfo_for_customer is at https://github.com/OCA/product-attribute/tree/8.0/product_supplierinfo_for_customer

[AaronHForgeFlow]

@eLBati Yes, it's the same module name, but as you said, it allows to define prices and pricelists based on them. So it adds some extra features that wasn't there before. If you think that the approach in your PR is correct, then the PR in v10 should also incorporate those additional features.

BTW, I'm planning to add this to v9.

[eLBati]

Yes, it's the same module name, but as you said, it allows to define prices and pricelists based on them

No, product_customer_code does not.
product_supplierinfo_for_customer does

[AaronHForgeFlow]

@eLBati so I guess both modules cannot coexist, right?

[eLBati]

@aheficent they overlap providing the same functionality "defining product codes depending on customers". This is why I propose to drop product_customer_code

[AaronHForgeFlow]

@eLBati All right, as product_supplierinfo_for_customer is already in OCA/8.0 I agree to keep using that module

[AaronHForgeFlow]

We need to decide which module is going to be ported either this or product_supplierinfo_for_customer. @eLBati @PCatinean

[PCatinean]

@aheficent I was not sure what to say, there a quite a few differences of opinion. Can't say I have a definitive one myself. If further changes are needed to the module I'll be glad to carry them out.

Regarding organisation of modules in OCA I don't have such a broad perspective as others to decide

[ EDIT ]
Maybe @lasley can have say as he is PSC?

[lasley]

Take this for a grain of salt, because I don't use either of these. I looked at the two and agree that one should be deprecated.

I am thinking that product_supplierinfo_for_customer should be the preferred here due to more features + it already being in OCA/8. Looks like there are some other product_supplierinfo* modules too, so it wins out in the realm of more integrated as well.

[gdgellatly]

@lasley @PCatinean @aheficent

Sorry, I didn't really mean to kick all this off, thought it would take a couple of days to sort through, not months. Just saw 2 modules with largely the same functionality and thought 1 would be better that combines best of both. I'm not invested either way as like @lasley I don't use either, but if I did it would be the product_supplierinfo_for_customer module.

[PCatinean]

Well then it's settled, the other module will be used and this will be dropped.

[lasley]

@gdgellatly - no need to be sorry, these types of discussions 100% have to happen. Sometimes it takes a while because we're all volunteers here, but it's all for the greater goal of having the ultra-supreme modules as the ones we support. Nobody said ultra-supreme was easy 😉