Skip to content
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

product_main_supplierinfo: fix default sellers for variant #1077

Merged
merged 1 commit into from
May 27, 2022

Conversation

TDu
Copy link
Member

@TDu TDu commented May 18, 2022

If no specific record are found by the first filter. The Odoo default
method will return all active supplier info.
Which will include also supplier info record related to other specific
variant.

With this change if some supplier info related to the product
variant exist, they will be returned instead.

@OCA-git-bot
Copy link
Contributor

Hi @sebalix,
some modules you are maintaining are being modified, check this out!

@TDu TDu force-pushed the main-supplierinfo-filter-variant branch 3 times, most recently from 5f49df3 to 1188715 Compare May 24, 2022 08:52
@@ -49,5 +49,7 @@ def _get_sellers(self):
)
)
if not sellers:
sellers = all_sellers
sellers = all_sellers.filtered(lambda s: (s.product_id == self))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest then:

Suggested change
sellers = all_sellers.filtered(lambda s: (s.product_id == self))
sellers = all_sellers.filtered(lambda s: (s.product_id == self))
if not sellers:
# Fallback on template sellers if there is none for the current variant
sellers = all_sellers.filtered(lambda s: not s.product_id)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same logic, added.

sellers = all_sellers
sellers = all_sellers.filtered(lambda s: (s.product_id == self))
if not sellers:
sellers = all_sellers
return sellers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also add this to be in-line with what Odoo does:

Suggested change
return sellers
return sellers.sorted("price")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, tested on a purchase order and if multiple supplier info record are matching Odoo will select the cheaper alternative even if it has a higher sequence.

@TDu TDu force-pushed the main-supplierinfo-filter-variant branch from 1188715 to 519ff7b Compare May 24, 2022 13:48
@sebalix
Copy link
Contributor

sebalix commented May 24, 2022

@TDu your tests are now failing :)

@TDu TDu force-pushed the main-supplierinfo-filter-variant branch from 519ff7b to 4ba4191 Compare May 24, 2022 14:49
@TDu
Copy link
Member Author

TDu commented May 25, 2022

@sebalix Tests are good now...

@simahawk simahawk changed the title Imp p_main_supplierinfo default sellers for variant product_main_supplierinfo: fix default sellers for variant May 27, 2022
@simahawk
Copy link
Contributor

simahawk commented May 27, 2022

@TDu can you squash and rewrite the commit msg (eg: = PR title)?

@TDu TDu force-pushed the main-supplierinfo-filter-variant branch from 4ba4191 to d3a92f9 Compare May 27, 2022 12:04
@TDu
Copy link
Member Author

TDu commented May 27, 2022

Squashed and renamed. But Travis failed and it does not make sense to me https://app.travis-ci.com/github/OCA/product-attribute/jobs/571634140#L2242
Trying a fix with a fixup

If no specific record are found by the first filter. The Odoo default
method will return all active supplier info.
Which will include also supplier info record related to other specific
variant.

With this change if some supplier info related to the product
variant exist, they will be returned instead.

Also add a sort on the price to have the same behaviour than on a
purchase order line for the price selected.
@TDu TDu force-pushed the main-supplierinfo-filter-variant branch from d777d24 to ea0f7f6 Compare May 27, 2022 13:09
@TDu
Copy link
Member Author

TDu commented May 27, 2022

That fix worked so squashed again...

@simahawk
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-1077-by-simahawk-bump-patch, awaiting test results.

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 018ede5 into OCA:14.0 May 27, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at f2ae482. Thanks a lot for contributing to OCA. ❤️

DavidJForgeFlow pushed a commit to ForgeFlow/product-attribute that referenced this pull request Jul 26, 2022
Signed-off-by pedrobaeza
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants