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

[17.0][MIG] product_main_supplierinfo #1660

Merged
merged 11 commits into from
Dec 10, 2024

Conversation

adriresu
Copy link

@adriresu adriresu commented Jun 17, 2024

Standard migration, reused #1353 (Unfinished migration to v16.0)

sebalix and others added 10 commits June 14, 2024 13:30
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.
@adriresu adriresu force-pushed the 17.0-mig-product_main_supplierinfo branch from 962ec11 to 7c662c7 Compare June 17, 2024 11:28
@adriresu
Copy link
Author

Can you review, please?
@aliciagaarzo
@sebalix
@Kev-Roche

@dalonsod
Copy link

Tested on runboat 👍

@adriresu
Copy link
Author

Can you review please?
@bizzappdev

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

Choose a reason for hiding this comment

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

this line is weird, could you explain the use of the duplicate assignation on the same variable?

Choose a reason for hiding this comment

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

This line belongs to previous implementation of this addon, added for 14.0 in #1077 , we could ask to @TDu

Copy link
Member

Choose a reason for hiding this comment

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

This looks like a typo, should be IMO

Suggested change
sellers = sellers = all_sellers.filtered(lambda s: not s.product_id)
sellers = all_sellers.filtered(lambda s: not s.product_id)

@TDu
Copy link
Member

TDu commented Jul 5, 2024

Also the directory setup should be removed. Packaging of addons has changed (cf OCA/maintainer-tools#582) in v17

@Anxo82 Anxo82 force-pushed the 17.0-mig-product_main_supplierinfo branch from 7c662c7 to 08a9af4 Compare August 23, 2024 07:10
@Anxo82
Copy link

Anxo82 commented Aug 23, 2024

Done! Review please @TDu
Thanks

Copy link
Contributor

@FernandoRomera FernandoRomera left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@flotho flotho left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@rousseldenis
Copy link
Contributor

/ocabot migration product_main_supplierinfo

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Dec 10, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Dec 10, 2024
61 tasks
@sebalix
Copy link
Contributor

sebalix commented Dec 10, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-1660-by-sebalix-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit f94f5a4 into OCA:17.0 Dec 10, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

sellers = product._get_sellers()
product.main_seller_id = fields.first(sellers)

def _get_sellers(self):
Copy link
Contributor

@rousseldenis rousseldenis Dec 10, 2024

Choose a reason for hiding this comment

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

@adriresu @sebalix My question arrives late but why not using standard method _get_filtered_sellers() or _select_seller() to avoid code duplication ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question, had to re-check the original v14 PR to see if I mentioned something there, but nothing!
The only difference that remembers me a thing is that _select_seller() is sorting the sellers based on price before returning one (which is OK when encoding a PO). Here the price is not a criteria to become the "main vendor" of a product, but sequence and dates matter.

Copy link
Contributor

@sebalix sebalix Dec 10, 2024

Choose a reason for hiding this comment

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

Just saw that starting from Odoo v17, _select_seller method accepts a new ordered_by parameter, which could be used to force the use of sequence, if that works we could drop this method 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that neither, we are sorting on the price at the end of this method too... I don't remember then why I duplicated this code, for sure there is a reason.

Copy link
Contributor

@sebalix sebalix Dec 10, 2024

Choose a reason for hiding this comment

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

Find out, so we could not use _select_seller in v14 because of this filter:

But starting from v15, which introduced the submethod _get_filtered_sellers you were talking about, it should definitely work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Trying this here: #1812
One test is failing, to check.

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.