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

[REF] x_product_pack: pricing refactor #173

Merged
merged 6 commits into from
Nov 4, 2024

Conversation

bruno-zanotti
Copy link
Contributor

@bruno-zanotti bruno-zanotti commented Jun 27, 2024

Pricing refactor

This PR aims to perform a thorough refactor of the product_pack, sale_product_pack and website_sale_product_pack modules. The primary goal is to enhance the pricing logic for product packs, ensuring consistency and improved functionality.

Key improvements / fixes:

  1. Refactor Pack Price Computation: Removed the inheritance from price_compute in favor of a clearer and
    more comprehensible _get_product_price method within the Pricelist class

  2. 'get_price' -> '_get_pack_line_price': The get_price method on pack lines has been improved and renamed to _get_pack_line_price. It is now capable of returning the price of the pack line for a specified pricelist.

  3. List Price Calculation: Enhanced the _compute_product_lst_price method to return the list price of a pack. It now leverages a new method analogous to price_compute, named _pack_line_price_compute, which can determine the prices of pack components, including nested packs.

  4. Resolves an issue related to the action "Update Prices" on Sale Orders. Now when update the pricelist just take into account the components of packs "Detailed - Detailed per component" and ignore the rest. More info about how to replicate the bug in the commit message.

  5. website Pack prices: show the correct price on the website.


@OCA-git-bot
Copy link
Contributor

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

@bruno-zanotti bruno-zanotti marked this pull request as draft June 27, 2024 14:46
@bruno-zanotti bruno-zanotti force-pushed the 17.0-t-33685-bz branch 4 times, most recently from de9be6c to fe6885a Compare July 4, 2024 14:46
@bruno-zanotti
Copy link
Contributor Author

WIP: This PR is based on #174 so we need to merge it first

@bruno-zanotti bruno-zanotti force-pushed the 17.0-t-33685-bz branch 2 times, most recently from 124d006 to b84ab77 Compare July 4, 2024 15:05
@bruno-zanotti bruno-zanotti force-pushed the 17.0-t-33685-bz branch 3 times, most recently from 9d6cd98 to 6bd0e02 Compare July 16, 2024 15:03
@bruno-zanotti bruno-zanotti force-pushed the 17.0-t-33685-bz branch 2 times, most recently from 7d92402 to c8019da Compare October 4, 2024 12:45
@bruno-zanotti bruno-zanotti marked this pull request as ready for review October 7, 2024 15:50
@bruno-zanotti
Copy link
Contributor Author

bruno-zanotti commented Oct 7, 2024

Hello @pedrobaeza @FrancoMaxime @augusto-weiss @rousseldenis @ernestotejeda

This PR is mainly a migration to v17 of #149. In Adhoc, we are already using it in our v17 clientes and everything seems to work fine. It is ready for review, but not for merge since depends on the migration of website_sale_product_pack #174 (I included in the firs commit so you can test it on runboat). It is an important fix / change so let's try to merge it ASAP, I tried to explain the change in the description and commits messages but if you have any doubt please let me know.

Copy link

@nicolascol nicolascol left a comment

Choose a reason for hiding this comment

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

Functional review OK

Copy link
Member

@FrancoMaxime FrancoMaxime left a 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, Great work!

product_pack/models/product_pricelist.py Show resolved Hide resolved
website_sale_product_pack/controllers/variant.py Outdated Show resolved Hide resolved
sale_product_pack/models/product_pack_line.py Outdated Show resolved Hide resolved
product_pack/models/product_product.py Outdated Show resolved Hide resolved
This commit improves the way the price of the packs is computed to
ensure it aligns with the behavior of normal products in Odoo.

Key improvements include:

1. Refactor Pack Price Computation:

Removed the inheritance from price_compute in favor of a clearer and
more comprehensible _get_product_price method within the Pricelist class

2. 'get_price' -> '_get_pack_line_price':

The get_price method on pack lines has been improved and renamed to
_get_pack_line_price. It is now capable of returning the price of the
 pack line for a specified pricelist.

3. List Price Calculation:

Enhanced the _compute_product_lst_price method to return the list price
of a pack. It now leverages a new method analogous to price_compute,
named _pack_line_price_compute, which can determine the prices of pack
components, including nested packs.
Packs totalized and non-detailed ignores the product pack list price
when computing the price.

This commit is a hack to keep the behavior of the modules before
this refactor.

More information in the issue OCA#169.
Adapt the module sale_product_pack to the price refactor
Resolves an issue in the "Update Prices" action for packs.
When update the pricelist, ignore all the "cero" lines, this
is the components of packs "Detailed: Totalized in main product" or
"Detailed - Ignored".

We only want to update component lines if the parent pack is
"Detailed - Detailed per component".

Also keep in the so line the component discount settled in the parent
pack settings.

Steps to replicate the bug:
1. In a Sale Order add a product "Detailed - Detailed per component"
2. Save to see all the pack components
3. Change the pricelist to one with discount
4. Call the button action "Update Prices".

Before this commit The price of the parent product gets updated,
but the component prices remain unchanged. Even if the same product is
added again (while retaining the first instance), the component prices
show discrepancies.

Now, when a pricelist with discounts is applied, all line items in the
Sale Order including both parent and component products are updated
accordingly.
@bruno-zanotti
Copy link
Contributor Author

Thanks for the PR, Great work!

@FrancoMaxime thanks for the review, I already made the changes!

@augusto-weiss
Copy link

LGTM!! Waiting on this to migrate product pack to 18!! @bruno-zanotti

Copy link

@nicolascol nicolascol left a comment

Choose a reason for hiding this comment

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

Functional review OK

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

@pedrobaeza pedrobaeza added this to the 17.0 milestone Nov 4, 2024
@pedrobaeza
Copy link
Member

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-173-by-pedrobaeza-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 3887561 into OCA:17.0 Nov 4, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@FrancoMaxime
Copy link
Member

Thanks for the PR, Great work!

@FrancoMaxime thanks for the review, I already made the changes!

With a little delay, thanks for the changes!

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.

7 participants