Skip to content

[16.0][IMP] sale_pricelist_from_commitment_date: enable feature on pricelists#3205

Open
sebalix wants to merge 7 commits intoOCA:16.0from
camptocamp:16-sale_pricelist_from_commitment_date-add-parameter
Open

[16.0][IMP] sale_pricelist_from_commitment_date: enable feature on pricelists#3205
sebalix wants to merge 7 commits intoOCA:16.0from
camptocamp:16-sale_pricelist_from_commitment_date-add-parameter

Conversation

@sebalix
Copy link
Copy Markdown
Contributor

@sebalix sebalix commented Jun 25, 2024

Enable it by default (retro-compatibility).

@sebalix sebalix force-pushed the 16-sale_pricelist_from_commitment_date-add-parameter branch 2 times, most recently from ee11fd6 to 45e8635 Compare July 1, 2024 07:20
@rousseldenis rousseldenis added this to the 16.0 milestone Jul 1, 2024
@rousseldenis
Copy link
Copy Markdown
Contributor

@sebalix Could you check tests?

@sebalix sebalix force-pushed the 16-sale_pricelist_from_commitment_date-add-parameter branch 3 times, most recently from 299cedb to d08a08a Compare July 1, 2024 13:17
@sebalix
Copy link
Copy Markdown
Contributor Author

sebalix commented Jul 1, 2024

We have some tests related to sale_global_discount failing, like:

TestSaleGlobalDiscount.test_02_global_sale_discounts_from_partner
Traceback (most recent call last):
  File "/__w/sale-workflow/sale-workflow/sale_global_discount/tests/test_sale_global_discount.py", line 161, in test_02_global_sale_discounts_from_partner
    self.assertAlmostEqual(self.sale.amount_global_discount, 162.49)
AssertionError: 3.25 != 162.49 within 7 places (159.24 difference)

I cannot reproduce the issue locally with sale_global_discount installed alongside sale_pricelist_from_commitment_date 🤔 I still cannot confirm its related.

EDIT: it's definitely linked, setting the module as installable: False make the CI 🟢

@sebalix sebalix force-pushed the 16-sale_pricelist_from_commitment_date-add-parameter branch from ac23083 to d08a08a Compare July 1, 2024 14:53
Copy link
Copy Markdown

@cyrilmanuel cyrilmanuel left a comment

Choose a reason for hiding this comment

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

not an expert but LGTM

@sebalix sebalix force-pushed the 16-sale_pricelist_from_commitment_date-add-parameter branch from da3be3e to 7a4308a Compare February 20, 2025 13:51
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 1, 2026

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Feb 1, 2026
@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Feb 8, 2026
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.

Thanks for this improvement, @sebalix. Making the commitment date pricing opt-in per pricelist is a solid design choice -- it gives users proper control rather than silently overriding the standard date logic for all pricelists.

The implementation looks good overall. The refactoring of _compute_price_unit and _compute_pricelist_item_id to use groupby and batch lines per order is a welcome improvement over the previous per-line loop, and extracting _get_pricelist_date() on sale.order provides a clean extension point. The migration correctly enables the flag on all existing pricelists to preserve current behavior on upgrade.

One thing that still seems unresolved though: @sebalix confirmed back in July 2024 that this PR causes sale_global_discount tests to fail (the test_02_global_sale_discounts_from_partner assertion). Setting this module as non-installable makes CI green again, so the interaction is confirmed. Has there been any progress on diagnosing or fixing that? The PR won't be mergeable until cross-module compatibility is sorted out.

Minor observation on the view: the price_based_on_delivery_date field could benefit from a string and help attribute to make it clear to end users what this toggle does (e.g. "Use the commitment/expected delivery date instead of the order date for pricelist rule matching").

@cyrilmanuel cyrilmanuel force-pushed the 16-sale_pricelist_from_commitment_date-add-parameter branch 3 times, most recently from ff7f0e4 to 277526f Compare March 4, 2026 08:02
@cyrilmanuel cyrilmanuel force-pushed the 16-sale_pricelist_from_commitment_date-add-parameter branch from 42a8925 to 277526f Compare March 4, 2026 08:37
@cyrilmanuel
Copy link
Copy Markdown

for order, lines in groupby(self, key=lambda line: line.order_id):

odoo.tools.groupby requires the recordset to be sorted by the same key. If not sorted, grouping becomes inconsistent and some lines are skipped or mis-grouped. That silently corrupts the price computation. In standard

perhaps this came from that point. to be check if someone have another idea. i've try to change the group by and sort but nothing change.

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.

6 participants