[ENH] New copy functionality.

[richard-willdooit]

Copy configurable function added to product.product when a new copy is needed
with the same attributes - ensures that validation is reperformed, as well as
ensuring new custom values are created and not just referencing the old ones.

Copy on SO line ensures that a configured product is copied.

[richard-willdooit]

@PCatinean

@richard-willdooit very nice contribution, also passing all tests and coverage also increased. Super!

My only question for this is regarding the copy function itself.

In theory there should never be two variants with the exact same set of attribute ids (and custom values).

This is to prevent duplicates and have only one result when doing the search for an existing configuration just before creation in both configuration interfaces.

Since this doesn't generate an error I think I made the mistake of not adding a constraint.

Does your scenario fit in duplicate products with the same configuration without a problem?

Yes, interesting you say that, because there is no such search in the backend wizard. It always creates a new variant, and I thought is was a design feature. Certainly, for our implementation, this is not undesired behaviour, as once the variant is created, we can do things with that particular instance.

The scary thing, though, is that if Order 1 and Order 2 are configured identically, and therefore point to the same product id as you said the code should do, there is a glaring issue. If you go back to the first order, and change the configuration, your code currently modifies the product id directly, rather than unlinking and pointing to a new one. This would change the definition of the product for Order 1 AND Order 2. This is why I thought the behaviour was deliberate. If you do want the code to only allow a varaint to exist once in the product.product file, then you would need to re-factor that code.

I had a quick look through the Purchase Order proposed pull request (#42 by @elemire ), and notice that he has introduced an option to not create a new variant if it already existed, ("reuse_variant" on a template by template basis) and maybe this is a good solution. (However, it still has the problem that modifying a configuration on a line if that functionality were introduced on PO, would redefine the product rather than unlinking and linking to the new one).

[codecov-io]

Codecov Report

Merging #65 into 10.0 will increase coverage by 0.1%.
The diff coverage is 70.58%.

@@            Coverage Diff            @@
##             10.0      #65     +/-   ##
=========================================
+ Coverage   59.04%   59.14%   +0.1%     
=========================================
  Files          14       14             
  Lines        1355     1356      +1     
=========================================
+ Hits          800      802      +2     
+ Misses        555      554      -1

Impacted Files	Coverage Δ
product_configurator/models/product.py	75.86% <100%> (+1.51%)	⬆️
product_configurator/models/sale.py	58.33% <37.5%> (-41.67%)	⬇️
...configurator_wizard/wizard/product_configurator.py	26.27% <0%> (-0.14%)	⬇️
website_product_configurator/controllers/main.py	73.82% <0%> (+0.17%)	⬆️
product_configurator/models/product_config.py	77.17% <0%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0190c19...5c6b2d6. Read the comment docs.

[PCatinean]

@richard-willdooit thanks for the detailed clarifications.

It was not a design feature I assume it's a part I just missed when porting the customer-specific version of the backend configurator to the generic method.

A PR has already been opened to fix this. While the conversation stalled I will take over the PR and merge it (I just did not have the time).

See PR here: #30

This is a pretty good approach and if it creates/retrieves a new variant on each change instead of editing the original (which is a bad idea) nothing bad should happen.

Also an idea I had for a long time was to keep the custom values that are not searchable on the sale.order.line level. Since it's specific to the customer order while the variant itself is a single product just like any other that can be references by anyone.

I will do my best to integrate that PR and rename create_variant to something like create_get_variant or just get_variant which will do the search beforehand and prevent duplicates.

But long story short I think there should never be any duplicates in the database with the same attributes and a reconfiguration that changes the attributes should just retrieve/create a new variant

[richard-willdooit]

@PCatinean I don't think you should keep it on the SO. That would lock the configurator in to Sales. When the configurattion hits Manufacturing, as it will in our solution, the manufacturing area do not care where it was sourced from. Also, the possibility will exist in our solution long term, to create a configuration and have it manufactured, without having an order.

Also, I like the idea of an option to create a new product each time, or to re-use existing an one. Creating a new one each time, as I was suggesting in our implementation here, allows us to then modify details on the product.product which will impact on the individual build. We have an option where it basically is too hard to configure it, so they choose "custom" and then fill in manual details. Staff at the manufacturing department then access the created variant, and change a couple of key details there, so when it goes in to manufacturing, the MO for the two variants with identical attributes may be quite different.

I think this PR has things that would be useful in any case. It just may need to check the "Create new Variant Each Time" flag.

I will have a look at #30, too.