[Andrii9090-tecnativa]

Depends:

• [18.0][ADD] sale_margin_pricelist_computation: New module #262
• [18.0][ADD] loyalty_margin_computation: New module #263

@pedrobaeza ping

TT62183
@Tecnativa

[cristina-hidalgo-tecnativa]

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

[cristina-hidalgo-tecnativa]

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.

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

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]

@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?

[cristina-hidalgo-tecnativa]

@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?

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

And I obtained the following margin:

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

[pedrobaeza]

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]

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

[Andrii9090-tecnativa]

@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]

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

[cristina-hidalgo-tecnativa]

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

[victoralmau]

Could you rebase and remove test-requirements.txt commit?

[cristina-hidalgo-tecnativa]

Suggested change

	def _compute_margin(self):
	res = super()._compute_margin()
	for origin_line in self.filtered(lambda x: x.reward_generated_line_ids):
	processed_reward = set()
	calculated_margin = 0
	purchase_price_total = origin_line.purchase_price
	for reward_line in origin_line.reward_generated_line_ids:
	purchase_price_total += reward_line.purchase_price
	if (
	not reward_line.reward_id.sale_margin_formula
	or reward_line.reward_id.id in processed_reward
	):
	continue
	processed_reward.add(reward_line.reward_id.id)
	eval_context = origin_line._get_reward_eval_context(reward_line)
	safe_eval(
	str(reward_line.reward_id.sale_margin_formula).strip(),
	eval_context,
	mode="exec",
	nocopy=True,
	)
	calculated_margin += eval_context.get("result", 0)
	if calculated_margin:
	origin_line.margin = float_round(
	calculated_margin
	* origin_line.purchase_price
	/ purchase_price_total,
	precision_digits=2,
	)
	margin_percent_total = calculated_margin / origin_line.price_subtotal
	origin_line.margin_percent = float_round(
	origin_line.purchase_price
	/ purchase_price_total
	* margin_percent_total,
	precision_digits=2,
	)
	for reward_line in origin_line.reward_generated_line_ids.filtered(
	lambda line: line.reward_id.sale_margin_formula
	):
	reward_line.margin = float_round(
	calculated_margin
	* reward_line.purchase_price
	/ purchase_price_total,
	precision_digits=2,
	)
	reward_line.margin_percent = float_round(
	reward_line.purchase_price
	/ purchase_price_total
	* margin_percent_total,
	precision_digits=2,
	)
	return res
	def _compute_margin(self):
	super()._compute_margin()
	for origin_line in self.filtered("reward_generated_line_ids"):
	all_reward_lines = origin_line.reward_generated_line_ids
	for reward in all_reward_lines.reward_id:
	reward_lines = all_reward_lines.filtered(
	lambda x, reward=reward: x.reward_id == reward
	)
	# A reward line can be linked to several origin lines: transfer its
	# margin only once, to the first of them
	if origin_line != reward_lines.reward_origin_generated_line_ids[:1]:
	continue
	if reward.sale_margin_formula:
	eval_context = origin_line._get_reward_eval_context(
	reward, reward_lines
	)
	safe_eval(
	reward.sale_margin_formula.strip(),
	eval_context,
	mode="exec",
	nocopy=True,
	)
	if "result" not in eval_context:
	continue
	# Setting it here chains the formulas of several rewards
	origin_line.margin = eval_context["result"]
	else:
	# The standard margin of the reward lines is recomputed, as the
	# stored one is already transferred to the origin line
	origin_line.margin += sum(
	line.price_subtotal - line.purchase_price * line.product_uom_qty
	for line in reward_lines
	)
	reward_lines.margin = reward_lines.margin_percent = 0
	origin_line.margin_percent = (
	origin_line.price_subtotal
	and origin_line.margin / origin_line.price_subtotal
	)

I tested this locally and found four problems in _compute_margin:

1. Two programs add their results up. With formulas origin_line.margin + 10 and + 5 on Main × 2, the order margin is 108.91 instead of 61.96.

When It should be:

2. result = 0 is ignored. if calculated_margin: treats 0 as "no formula", so the standard margin (35.96) is kept instead of 0.

With the fix:

3. ZeroDivisionError with zero price/cost products. Free × 2 + Reward raises it in _compute_margin: neither purchase_price_total nor price_subtotal division is guarded.

When you try to add the reward:

With the fix:

4. A reward linked to several origin lines runs the formula once per origin line. With result = 20 on Main × 2 and Main2 × 2, the order margin is 30.23 and depends on the line order.

Adding the fix:

All four should be fixed by the suggestion above, which moves the margin of each reward to the origin line instead of distributing it among the lines.

[cristina-hidalgo-tecnativa]

Suggested change

	def _get_reward_eval_context(self, reward_line):
	return {
	"env": self.env,
	"user": self.env.user,
	"origin_line": self,
	"reward": reward_line.reward_id,
	"reward_line": reward_line,
	"reward_lines": self.reward_generated_line_ids,
	"float_compare": float_compare,
	}
	def _get_reward_eval_context(self, reward, reward_lines):
	return {
	"env": self.env,
	"user": self.env.user,
	"origin_line": self,
	"reward": reward,
	"reward_line": reward_lines[:1],
	"reward_lines": reward_lines,
	"float_compare": float_compare,
	}

_get_reward_eval_context now receives the reward and only its own lines. Before, reward_lines contained the lines of all the rewards linked to the origin line, so with two programs a formula could see the gift lines of the other one. The view already documents it as "The Reward lines".

It is also needed by the new _compute_margin, which evaluates each formula once per reward, with the lines of that reward.

[cristina-hidalgo-tecnativa]

Suggested change

	@api.depends(
	"reward_generated_line_ids",
	"reward_origin_generated_line_ids",
	"reward_generated_line_ids.purchase_price",
	)
	@api.depends(
	"reward_generated_line_ids",
	"reward_origin_generated_line_ids",
	"reward_generated_line_ids.purchase_price",
	"reward_generated_line_ids.price_subtotal",
	)

The transfer of the margin of rewards without a formula reads price_subtotal from the reward lines, so without this dependency the origin line would not be recomputed if that subtotal changes.

[cristina-hidalgo-tecnativa]

Suggested change

	from odoo.tools.float_utils import float_compare, float_round
	from odoo.tools.float_utils import float_compare

float_round is no longer used once the proportional distribution is gone.