-
-
Notifications
You must be signed in to change notification settings - Fork 789
[18.0][MIG] product_attribute_custom_variant #1934
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
[18.0][MIG] product_attribute_custom_variant #1934
Conversation
|
/ocabot migration product_attribute_custom_variant |
|
@SirAionTech This can be your interest. I've added two improvements you can maybe backport to your PR. |
|
I think the sale part can be splitted. |
monen17
left a comment
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.
Thanks for the PR!
I have tried this module in a local instance and it works great!
I also have reviewed the code and noticed a few thinkgs, see the other comments.
Please edit the title of commit 38ddd11 because it's too long according to the guidelines:
please check if the commit message is cut with ellipsis
and gets truncated:

Since the module name consumes a lot of space already, you can either shorten it or the commit description with something like:
[IMP] product_attribute_custom_value_variant: Readability
Split code in functions
One more thought: it might also be a [REF] because the module's behavior is not changing.
About the backporting, I prefer to do such things when PRs are merged for two reasons:
- avoid the overhead of keeping multiple PRs synced
- one more reason to merge PRs 😏
I think the sale part can be splitted.
I agree that ideally this module would only depend on product and we'd have a bridge module to make it work with sale, but as far as I know custom attribute values have very little (maybe none?) use without sales so the bridge module would be installed very often making the decoupling useless.
This said, if someone wants to do it it's ok by me.
product_attribute_custom_value_variant/models/sale_order_line.py
Outdated
Show resolved
Hide resolved
product_attribute_custom_value_variant/models/sale_order_line.py
Outdated
Show resolved
Hide resolved
product_attribute_custom_value_variant/models/sale_order_line.py
Outdated
Show resolved
Hide resolved
product_attribute_custom_value_variant/models/sale_order_line.py
Outdated
Show resolved
Hide resolved
Yes, I approved your 16.0's PR. If you can attract people there, we can merge :-) |
I will publish the split here soon. The idea was also to simplify the code to be called on sale side in order to replace the variant by the new one. The idea was to be able to get new variant from custom attribute in any other flow. |
I agree with short commit message BUT I'm not with non meaning ones. And that's not because github interface truncates it that they should go to something like |
…plit code in functions
Move the logic out of sale order model in order to simplify the calls on that one. Split also the tests with setup in a common class.
e4bf9c0 to
597bd5b
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
Based on :
See related module : OCA/sale-workflow#3631