Skip to content

[18.0][ADD] sale_loyalty_margin_computation: New module#264

Open
Andrii9090-tecnativa wants to merge 2 commits intoOCA:18.0from
Tecnativa:18.0-add-sale_loyalty_margin_computation
Open

[18.0][ADD] sale_loyalty_margin_computation: New module#264
Andrii9090-tecnativa wants to merge 2 commits intoOCA:18.0from
Tecnativa:18.0-add-sale_loyalty_margin_computation

Conversation

@Andrii9090-tecnativa
Copy link
Copy Markdown

@Andrii9090-tecnativa Andrii9090-tecnativa commented May 4, 2026

@OCA-git-bot OCA-git-bot added series:18.0 mod:sale_loyalty_margin_computation Module sale_loyalty_margin_computation labels May 4, 2026
@pedrobaeza pedrobaeza added this to the 18.0 milestone May 4, 2026
Comment thread sale_loyalty_margin_computation/models/loyalty_reward.py
@Andrii9090-tecnativa Andrii9090-tecnativa force-pushed the 18.0-add-sale_loyalty_margin_computation branch 2 times, most recently from 41a466d to aacddac Compare May 4, 2026 08:40
origin_lines[0].margin_percent = (
origin_lines[0].margin / origin_lines[0].price_subtotal
)
break
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure if this case can happen in practice, but what would occur if two loyalty programs were active at the same time, each with its own sale_margin_formula? The break would exit the loop after the first reward, so the second program's formula would never run.

I understand the break prevents applying the same formula multiple times when a multi_gift reward generates several lines (all sharing the same reward_id). But wouldn't skipping by already-processed reward_id be safer?

Something like this:

processed_reward_ids = set()
for line in self.filtered(...):
    if line.reward_id.id in processed_reward_ids:
        continue
    processed_reward_ids.add(line.reward_id.id)
    ...

Copy link
Copy Markdown

@cristina-hidalgo-tecnativa cristina-hidalgo-tecnativa left a comment

Choose a reason for hiding this comment

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

Locally tested since runboat appears red.

I'm not sure if this is intentional, but I created a multi-gift promotion with 2 gifts and a formula result = origin_lines[0].margin + 10.

Image

After applying the reward, the margin on the origin line was 22.40

Image

Shouldn't it be 12.40? Since sale_margin_formula is defined at the reward level (not per gift line), I'd expect the formula to apply once per reward, regardless of how many gift lines it generates.

@Andrii9090-tecnativa
Copy link
Copy Markdown
Author

@cristina-hidalgo-tecnativa I tested this, and it seems to calculate correctly.
Unit price of the original product: 15.8
Total: 31.6
Cost: 14.00
Margin: 3.60
After applying the formula, the margin is set to 13.60.
Could you please provide more details about your test cases?

image

@Andrii9090-tecnativa Andrii9090-tecnativa force-pushed the 18.0-add-sale_loyalty_margin_computation branch 3 times, most recently from 133c571 to 1d078c7 Compare May 5, 2026 19:51
@cristina-hidalgo-tecnativa
Copy link
Copy Markdown

@cristina-hidalgo-tecnativa I tested this, and it seems to calculate correctly. Unit price of the original product: 15.8 Total: 31.6 Cost: 14.00 Margin: 3.60 After applying the formula, the margin is set to 13.60. Could you please provide more details about your test cases?
image

It seems our result of 22.40 was indeed transient — on a subsequent order with the same configuration we got the expected 12.40.

That said, I also tested with two multi-gift programs applied to the same quotation, each with their own sale_margin_formula (+10 and +5).

imagen imagen

And I obtained the following margin:

imagen

Shouldn't it be 17.40 (2.40 + 10 + 5) ?

Copy link
Copy Markdown
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Not sure if for consistency with the rewards not having a formula, we should as well transfer the margin of the reward line to the main one.

@Andrii9090-tecnativa
Copy link
Copy Markdown
Author

@cristina-hidalgo-tecnativa Have you tested it with the changes I pushed yesterday?

@Andrii9090-tecnativa
Copy link
Copy Markdown
Author

@pedrobaeza After yesterday's changes, when a reward has a formula configured, the margin of each reward line is now set to 0. Then, the result of the margin formula is applied to one of the origin lines.
And now each formula is applied only once per reward, regardless of whether it is a single gift or a multi-gift reward.

@pedrobaeza
Copy link
Copy Markdown
Member

Yes, thanks, Andrii, please check what do you think of #264 (review)

@Andrii9090-tecnativa Andrii9090-tecnativa force-pushed the 18.0-add-sale_loyalty_margin_computation branch from 1d078c7 to 967eb48 Compare May 6, 2026 11:07
@cristina-hidalgo-tecnativa
Copy link
Copy Markdown

@cristina-hidalgo-tecnativa Have you tested it with the changes I pushed yesterday?

Yes, tested this morning. Single multi-gift formula looks fixed. But the two-programs case still fails — with +10 and +5 formulas we get 22.40 instead of 17.40.

@Andrii9090-tecnativa Andrii9090-tecnativa force-pushed the 18.0-add-sale_loyalty_margin_computation branch from 967eb48 to 177a3c1 Compare May 6, 2026 12:11
@Andrii9090-tecnativa Andrii9090-tecnativa force-pushed the 18.0-add-sale_loyalty_margin_computation branch from 177a3c1 to 9accf1d Compare May 6, 2026 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mod:sale_loyalty_margin_computation Module sale_loyalty_margin_computation series:18.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants