Skip to content

[16.0][IMP] sale_pricelist_from_commitment_date force date on compute price rule#3744

Merged
OCA-git-bot merged 1 commit intoOCA:16.0from
camptocamp:16.0-imp-sale_pricelist_from_commitment_date-compute_price_rule-force_pricelist_date
Mar 2, 2026
Merged

[16.0][IMP] sale_pricelist_from_commitment_date force date on compute price rule#3744
OCA-git-bot merged 1 commit intoOCA:16.0from
camptocamp:16.0-imp-sale_pricelist_from_commitment_date-compute_price_rule-force_pricelist_date

Conversation

@santostelmo
Copy link
Copy Markdown
Contributor


This improvement allows _compute_price_rule to obtain the price and rule on a specific date passed by context

@santostelmo santostelmo changed the title [IMP] sale_pricelist_from_commitment_date force date on compute price rule [16.0][IMP] sale_pricelist_from_commitment_date force date on compute price rule Jun 5, 2025
@OCA-git-bot
Copy link
Copy Markdown
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). 🤖

@cyrilmanuel
Copy link
Copy Markdown

@OCA/sale-blanket-maintainers someone can merge this PR please ?

@cyrilmanuel
Copy link
Copy Markdown

hi @sbidoul , could you merge this pr please ? or let me know who can help me :)

Copy link
Copy Markdown

@alexey-pelykh alexey-pelykh left a comment

Choose a reason for hiding this comment

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

Review: sale_pricelist_from_commitment_date - force date on _compute_price_rule

Target: 16.0 (supported)
Scope: +16 lines, 2 files, 1 commit

Summary

This PR adds an override of _compute_price_rule on product.pricelist so that the force_pricelist_date context key (already honored by _get_product_rule) is also respected when computing price rules. This is a logical completeness fix -- without it, code paths going through _compute_price_rule would ignore the commitment date.

Findings

Area Status Notes
Correctness OK Override follows the exact same pattern as the existing _get_product_rule override. Method signature matches Odoo 16 base (positional args align).
Odoo conventions OK Properly uses super() delegation, context-based date override is the established pattern in this module.
Security OK No new input vectors. Context key usage is standard Odoo practice.
Performance OK Single context dict lookup, no additional queries.
Tests OK New test assertion directly exercises _compute_price_rule with force_pricelist_date context and verifies correct price is returned for the commitment date range.
Manifest OK No version bump needed -- this is an improvement to existing functionality, and the manifest was not changed. A version bump to 16.0.1.1.0 would be nice-to-have but is not blocking.
i18n OK No user-facing strings added.
License headers OK Existing files, no new files added.

Minor observation (non-blocking)

The parameter name quantity in the override differs from the base Odoo 16 signature which uses qty:

# Base Odoo 16:
def _compute_price_rule(self, products, qty, uom=None, date=False, **kwargs):

# This PR:
def _compute_price_rule(self, products, quantity, uom=None, date=False, **kwargs):

This works correctly since the argument is passed positionally, but matching the base parameter name (qty) would be more consistent and avoid potential confusion if someone tries to call with qty= as a keyword argument. Not blocking.

APPROVED -- clean, minimal, well-tested improvement.

@cyrilmanuel
Copy link
Copy Markdown

hi @pedrobaeza could you merge this PR please ?

@pedrobaeza pedrobaeza added this to the 16.0 milestone Mar 2, 2026
… rule

This improvement allows _compute_price_rule to obtain the price and rule on a specific date
@santostelmo santostelmo force-pushed the 16.0-imp-sale_pricelist_from_commitment_date-compute_price_rule-force_pricelist_date branch from c8af03e to 4b6a5e0 Compare March 2, 2026 08:08
@pedrobaeza
Copy link
Copy Markdown
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Copy Markdown
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-3744-by-pedrobaeza-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Mar 2, 2026
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Copy Markdown
Contributor

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-3744-by-pedrobaeza-bump-patch.

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.

@pedrobaeza
Copy link
Copy Markdown
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Copy Markdown
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 3289957 into OCA:16.0 Mar 2, 2026
15 checks passed
@OCA-git-bot
Copy link
Copy Markdown
Contributor

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

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