[14.0][IMP] shopinvader_variant: main product: add hooks #1529
Conversation
mmequignon
force-pushed
the
shopinvader_variant-add_hooks_for_sort
branch
5 times, most recently
from
bb48f34 to
725a06f
Compare
| def _get_main_product_sorted_variants(cls, variants): | ||
| # NOTE: if the order is changed by adding `asc/desc` this can be broken | ||
| # but it's very unlikely that the default order for product.product | ||
| # will be changed. |
There was a problem hiding this comment.
🤔 that's not a nice assumption
Perhaps it's out of the scope of this PR, but I wonder why this method is even necessary.
When variants are searched (Line 330-336), the order is explicitly set to order="shopinvader_product_id". which should be enough for the ORM to return the records in the right variant order (according to the product.product model order)
Am I missing something?
There was a problem hiding this comment.
No, this is what I copied from the original method.
I don't like this implementation, but we don't have the budget to improve it for now.
Using sql syntax order and converting it to a python sort lambda function looks very risky to me…
There was a problem hiding this comment.
It's stable guys, please don't touch it! Merci! 😄
There was a problem hiding this comment.
The reason for the 1st sorting in the search is to be able to use groupby, the 2nd sorting is the real sorting that normally happens when you pick up variants. @mmequignon you can add this my old comment 😉
There was a problem hiding this comment.
I understand this is a feature distinct from this PR, but if someone wants to improve it, it has been done in v16 by @qgroulard (because the default Odoo ordering on product.product changed and a priority desc was added) and it suffices to backport it.
Feel free to take the inspiration from
| return ["shopinvader_product_id", "backend_id", "lang_id"] + order_by | ||
|
|
||
| @api.model | ||
| def _get_main_product_sorted_variants(cls, variants): |
There was a problem hiding this comment.
I'm not sure I see the value for this hook.
Wouldn't _pick_main_variant suffice?
| def _get_main_product_sorted_variants(cls, variants): | ||
| # NOTE: if the order is changed by adding `asc/desc` this can be broken | ||
| # but it's very unlikely that the default order for product.product | ||
| # will be changed. |
There was a problem hiding this comment.
The reason for the 1st sorting in the search is to be able to use groupby, the 2nd sorting is the real sorting that normally happens when you pick up variants. @mmequignon you can add this my old comment 😉
|
FTR tests are red on whole 14.0. See #1532 |
|
Forward-port in version 16 here: #1524 |
|
@mmequignon Can you rebase to trigger tests plz? |
mmequignon
force-pushed
the
shopinvader_variant-add_hooks_for_sort
branch
from
725a06f to
bbe8d6b
Compare
mmequignon
force-pushed
the
shopinvader_variant-add_hooks_for_sort
branch
from
bbe8d6b to
2fb2c24
Compare
|
/ocabot merge minor |
|
This PR has the |
|
On my way to merge this fine PR! |
|
@simahawk your merge command was aborted due to failed check(s), which you can inspect on this commit of 14.0-ocabot-merge-pr-1529-by-simahawk-bump-minor. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
This pr adds the ability to modify the way variants are sorted to compute main variants.
For instance now, you can add new fields in
_get_sort_read_fieldsin order to be able to filter out variants that cannot be the main variant for a given product.