-
-
Notifications
You must be signed in to change notification settings - Fork 785
[13.0][FIX] product: merge product variants with same attributes #2513
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
[13.0][FIX] product: merge product variants with same attributes #2513
Conversation
|
I can retarget to v11 branch if you prefer. |
|
We have not the constrain for combination_indices in pre-migration so maybe better to calculate combination_indices and then group product by product template and combination_indices? |
cfd69a8 to
e297acb
Compare
e297acb to
29d8bb4
Compare
29d8bb4 to
a1f3395
Compare
|
Hi, @MiquelRForgeFlow Could you rebase it? |
a1f3395 to
c272fd6
Compare
|
Maybe need to be added in a commit? odoo/odoo#32086 |
kos94ok-3D
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.
Tested on a real DB
pedrobaeza
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.
Shouldn't you update combination_indices after the merge?
No, why? The combination_indices are the same, they don't change. |
| for template_id in templates: | ||
| openupgrade_merge_records.merge_records( | ||
| env, 'product.product', templates[template_id][1:], | ||
| templates[template_id][0], field_spec=None, method='sql', |
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.
The question now is: should I put a field_spec?
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.
It depends if the standard fields require it or default merge values are enough
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.
Yeah, my best option is {'default_code': 'first_not_null', 'active': 'or', 'barcode': 'first_not_null', 'volume': 'max', 'weight': 'max'}. My concern is for custom modules that add float/char/boolean fields, which would need to add more things in this dict. Maybe we could add in openupgradelib a functionality like if the dict contains 'other_fields': 'preserve', then every other field is excluded and preserved.
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.
|
My take on this is that merging complete products is something that shouldn't be corrected at OpenUpgrade level. If they have done such ugly thing, then it must be fixed before doing the migration. |
|
Ok (don't delete branch) |
|
FYI this PR fixed our issue where we have a product with same attributes |
Make use of merge_records tool to merge records.
--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr