[17.0][MIG] product_variant_attribute_name_manager: Migration to 17.0

[david-s73]

The module already had a migration PR #1611, but as I needed to add new changes I made this PR adding the option to show the values of the variant attributes when the variant has only one value, adding the option as another check in the attributes. f43c1bb

cc @johnny-longneck

[rousseldenis]

/ocabot migration product_variant_attribute_name_manager

[rousseldenis]

Note that this can conflict between two modules not aware of each other.

[david-s73]

I don't understand exactly what you are referring to.

[percevaq]

I also do not understand what you mean by conflict between modules that do not know each other.
Can you explain it to us?

Thanks for your help

[johnny-longneck]

value_count is in base module "product" which is the only dependency of this module.
single_variant_attribute is in this module

So I do not see a conflict here.

The addition basically is nice and I appreciate it.
But I know, the single variant part is also playing a role in sales description and so on.

The fact that there are no tests added for this change, I would also be a little bit unsure about it and would not approve it.

[rousseldenis]

I wanted to say, domain is a static attribute, so, if you change it here, you cannot ensure another module will override domain value too.

[david-s73]

@rousseldenis I have already changed the domain so that it functions in this way; it is no longer a static domain. Please review it, and if everything is correct, you can give me the go-ahead.

[david-s73]

@pedrobaeza You can check out this pr

[pedrobaeza]

Sorry, not using it. Other PSC or maintainer should review it.

[Alexgars73]

@JordiMForgeFlow could you review please?

[JordiMForgeFlow]

@david-s73 I don't understand the new feature you are adding, seems to be the same as the already existing display_single_variant_attribute

[david-s73]

@JordiMForgeFlow This new field has been made to show the attribute values of the variant when the variant has only one value.

[JordiMForgeFlow]

@david-s73 I don't see how it is different than display_single_variant_attribute, which is exactly meant to display the value when it has only one.

[david-s73]

@JordiMForgeFlow I use the “single_variant_attribute” field, for the ‘product_template_variant_value_ids’ field of “product.product” so that even if it has only one value, it is displayed.

[JordiMForgeFlow]

@david-s73 honestly I don't really get what you are wanting to do, maybe you could make an example to illustrate?

What I see is that you are adding a variable that says "Display the attribute value of the variant when the variant has only one value". However, this variable has no effect on the display name, you are using it only to filter the variant values of the product. Am I missing something?

[david-s73]

@JordiMForgeFlow Here are some pictures of what happens if the check is checked or not, the first one is without the check and the second one with the check so you can see the difference between having it or not.

[JordiMForgeFlow]

@david-s73 I see, thank you for the example.

As I thought, the change is not strictly related to the display of the product variant name (which is the main goal of the module stated in the README). That is why it is a bit confusing to have it here.

I would personally prefer to have this as a separate module. The feature you are proposing is something that other people may also want to have without needing to have all the configurations of the product variant display name.

Nonetheless, this is just my opinion, not sure what others may think about it CC @rousseldenis

[david-s73]

@JordiMForgeFlow The change was added in this module because it made sense that if it has to do with the attribute name of the variants it should be in this module as an enhancement type.

[david-s73]

@dreispt You can check out this pr

[Reyes4711-S73]

LGTM

[Reyes4711-S73]

@OCA/product-maintainers Please, can you merge this PR?

[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). 🤖

[trisdoan]

Hello @david-s73 @HviorForgeFlow

I am very confused about the option

    display_no_variant_attribute = fields.Boolean(
        "Display No Variant Attributes on Product Variant",
        help="If checked, it will display the no variant attribute.",
    )

It does not work as expected. Could you take a look please? Thank you so much.

[david-s73]

As you can see in the image, there are three variants, but only two of them show the “Variant values.”

This is because the product templates have the values ‘2022’ and “2023” in the attribute.

While the other only has a simple attribute value, which is “2022.”

But if you check “single_variant_attribute” in the “Añada” attribute, even if the variant template only has one attribute value, it will be displayed.

@trisdoan I hope that with this explanation you can see and understand why the improvement was made. Also, the option you are showing me is only used when the “Variant creation mode” is set to “Never.”

[trisdoan]

@david-s73 Thanks for the explanation, your improvement makes sense to me, and works perfectly.

but what I am not sure is the display_no_variant_attribute, which was introduced in 16.0, does not work at all (because Never-mode attribute value is not written to db, in here).

Maybe @HviorForgeFlow has any hint?

That's why I asked, either my understanding of the option is wrong (if so, please help instruct me), or we remove/fix the option ?

[david-s73]

@trisdoan I have run tests and the problem arises when the “display_no_variant_attribute” checkbox has to be displayed, as it only appears when variants will never be created, so the checkbox is not necessary. However, if the checkbox is always displayed, it works correctly as it does not create a variant with that attribute. Therefore, it is possible that this checkbox is not necessary or that it should only be hidden if variants will never be created.

[HviorForgeFlow]

You are right @trisdoan about that with odoo core you can't reach to that specific configuration. Nevertheless combined with https://github.com/OCA/product-variant/tree/17.0/product_variant_configurator or some other custom modules that allow the system to reach that configuration was needed to display the attribute names of that attributes that don't produce variants in the product name itself.

[david-s73]

@trisdoan @HviorForgeFlow So if my changes are okay, you can approve it for merging.

[AlienAtSystem]

I found an incompatibility with the OCA module product_configurator: The view modifications for product_attribute_view_form can't be properly loaded since product_configurator sets the field create_variant to have a group, which conflicts with its use in the invisible attribute for the field display_no_variant_attribute in this module.

[david-s73]

@AlienAtSystem I'm sorry, but I'm not quite sure what you're referring to. If you could be a little more specific and provide an example screenshot or the part of the code that is incompatible, that would be very helpful.

[AlienAtSystem]

@david-s73 The conflicting Lines are:
create_variant used in invisible condition in your migration
create_variant set to have a group in product_configurator
I discovered it when attempting to install your migrated module on a system that also has product_configurator installed

[david-s73]

@AlienAtSystem I have already made a change so that an error message does not appear when installing this module if the product_configurator module is already installed. Please check it and if everything is correct, approve it.

[weinni2000]

LGTM, is there anything needed to do?

[Reyes4711-S73]

@OCA/product-maintainers Please, can you review/merge this PR?