[14.0] [IMP] sale_order_product_assortment: Use original product_id field keeping domain. Use cache to performance

[renda-dev]

Back port of #2538

Depends on:

• [14.0] [IMP] base_view_inheritance_extension: Add new options domain_add and text_add server-tools#2941
• [14.0] [FIX] base_view_inheritance_extension: parse old value server-tools#2942

Edit: I've also added a commit that changes the AND in the _compute_product_assortment_ids into an OR since, in my opinion, it's more likely what an user would expect by combining more assortments

[OCA-git-bot]

Hi @CarlosRoca13,
some modules you are maintaining are being modified, check this out!

[francesco-ooops]

Functional ok, good for merge once depending PR is merged

[francesco-ooops]

@CarlosRoca13 what do you think?

We had to apply the latest commit as the behavior was inconsistent, eg:

Create assortment "Chair", product allowed: "Office Chair", partner: Azure Interior
Create assortment "Screen", product allowed: "Acoustic bloc screens", partner: Azure Interior
SO for Azure Interior: 2 products can be selected

Create assortment "Chair", allowed product domain: "Name contains Office Chair", partner: Azure Interior
Create assortment "Screen", allowed product domain: "Name contains Acoustic bloc screens", partner: Azure Interior
SO for Azure Interior: no products can be selected

We'll fw-port this commit

[aleuffre]

Code review, LGTM.

I think it makes more sense and is more intuitive for the user to have different assortments linked with OR than with AND. Otherwise the effect is that, as you add more assortments to the customer, you actually shrink the selection they have available.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[CarlosRoca13]

Makes sense. Can you please add in the commit a brief explanation and the steps to reproduce the problem?

[CarlosRoca13]

On second thought... Let's imagine that we have 2 assortments for the same customer, in these two assortments, what is done, is to ban the purchase of chairs in the first one and tables in the second one. With this change, the customer will be able to see all the products, both chairs and tables.

[francesco-ooops]

@CarlosRoca13 yours is a valid point, but it seems like this is a short blanket situation, as some assortments will be used to exclude products and others to include them and some to do both - there is no right or wrong.

We will look into a solution for the matter, do you have any idea how this could be solved?

[CarlosRoca13]

Right now, I don't know how this could be faced... But I know that we have customers who are using the assortments excluding products and we cannot assume this change...

@pedrobaeza @carlosdauden @sergio-teruel can you give your point of view about this?

[pedrobaeza]

There shouldn't be any feature regression. It's OK to optimize, but not losing features.

[francesco-ooops]

There shouldn't be any feature regression. It's OK to optimize, but not losing features.

We're on the same page on that (we've always been), but we need to address the inconsistency in the example I posted above:

Create assortment "Chair", product allowed: "Office Chair", partner: Azure Interior
Create assortment "Screen", product allowed: "Acoustic bloc screens", partner: Azure Interior
SO for Azure Interior: 2 products can be selected

Create assortment "Chair", allowed product domain: "Name contains Office Chair", partner: Azure Interior
Create assortment "Screen", allowed product domain: "Name contains Acoustic bloc screens", partner: Azure Interior
SO for Azure Interior: no products can be selected

Also note that website assortment module works with OR as per OCA/e-commerce#838

[pedrobaeza]

Are you telling that previously it was not correct? I agree then to cover the case correctly with the optimization.

[CarlosRoca13]

The real problem of this is that we are taking all assortment domain at this point:
7e8b7a4#diff-d857107550de88517b0f4cb32ae7770a25a630dc254b97191e7f71f262183089R36
Since we don't know what type of assortment is involved

I think the right solution would be to take 2 product domains out of the assortment. One for excluded products and one for included products. In this way we could combine them in the way that suits us.

Note: The excluded products can't fit with the included with OR in the domain

[francesco-ooops]

Sounds like something we could think about!

Note: The excluded products can't fit with the included with OR in the domain

@CarlosRoca13 Can you expand on this?

[CarlosRoca13]

4 sure, what I mean is that the excluded domain needs to be set with AND because it is restrictive, an excluded product discards any posible included product

[pedrobaeza]

At the end, both should combine to get a unique domain for the product selection, but include part should be treated on one part, joining all the inclusions together, and exclusions should be also joined, and excluded from the remaining result.

[aleuffre]

Code review seems ok, I definitely prefer this solution (add all whitelisted products, remove all blacklisted products) but I think we should add a couple of tests here to make sure.

Also, outside of this PR, the logic should be unified and moved to the base module so that there are no differences with the e-commerce module.

[aleuffre]

Code review, LGTM

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[CarlosRoca13]

LGTM 👍

Please fw-port changes to 15.0 but note that a domain has been introduced for excluded products.

https://github.com/OCA/product-attribute/blob/50e3e48b2fcf5f504a857b367dd90e7ed7812fd0/product_assortment/models/ir_filters.py#L75

[pedrobaeza]

/ocabot merge major

[OCA-git-bot]

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

[OCA-git-bot]

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