-
-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[16.0][ADD] product_variant_specific_description #306
[16.0][ADD] product_variant_specific_description #306
Conversation
4947830
to
79c3afd
Compare
Shouldn't |
79c3afd
to
a047f41
Compare
@pedrobaeza existing modules? I saw a couple of ones doing this and the name were sale_order_line_variant_description and product_variant_name. |
I mean https://github.com/OCA/product-variant/tree/11.0/product_variant_specific_tax, that is being migrated to 16 in #302 |
@pedrobaeza hmmm I guess there is not a clear pattern used till now, I can rename this one. Which one do you prefer? |
This is like asking which son is your favorite, hehe. No strong best option. You decide. |
a047f41
to
c9dec76
Compare
Let's go for "specific" one, it has one ocurrence and this will be the second, better than start with zero 😆 |
@LoisRForgeFlow maybe we could add the field in the |
c9dec76
to
4ba739a
Compare
@JordiMForgeFlow Thanks for the heads-up, it is done now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code + functional review 👍🏼
4ba739a
to
ff50236
Compare
Some improvements done in order to have a better UX:
|
ff50236
to
9383ec3
Compare
LGTM @LoisRForgeFlow , should we also consider this new approach in the init_hook, removing the description from the template when there are multiple variants? |
The template description is only cleared when you modify the description of any of its variants, while all the variants have the same description, the template will do too. So I think the init hook is aligned with this behavior, you start with all variants with the same description as their template. |
@pedrobaeza Do you have some advice/hint on how to better handle a translatable+computed+stored field? Translations are not being aligned in this case. |
9383ec3
to
17c4835
Compare
After some back and forth, there is no easy way to make it work correctly with translations, therefore a more pracmatic solutions has been followed: This information message redirecting to the variant and the description as readonly will only be shown as soon as there is more than one language active. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again 👍🏼
Now that translations are handled with JSON fields, isn't there a way to "dump" the full content of the field? If not, this one is as you say a pragmatic one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional tests. Works as expected.
/ocabot merge nobump |
On my way to merge this fine PR! |
No, that would be ideal, but I guess is not supported yet. There are some helper methods to update translations (e.g. |
Congratulations, your PR was merged at 2ef8efe. Thanks a lot for contributing to OCA. ❤️ |
This module allows having different product variant internal notes than
the one of their template.
@ForgeFlow