[18.0][IMP] payroll_contract_advantages: add complex mechanism for advantag…

[cvinh]

…e calculations - helped by Claude Opus-4.7

[OCA-git-bot]

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

[CristianoMafraJunior]

English

General feedback

Thanks for the contribution — the intention here is solid. Adding computation modes and a quantity multiplier to payroll_contract_advantages is a meaningful improvement, and the migration script is a nice touch for backward compatibility.

However, the PR bundles too many concerns into a single force-pushed commit, which makes it harder to review and increases the risk surface. There are also a few correctness issues that should be addressed before merge.

---

🔴 Blocking issues

1. fixed_value = 0.0 silently falls back to stale amount

In _compute_unit_value, the fallback logic uses not self.fixed_value, which evaluates to True when fixed_value == 0.0. This means a deliberate zero-value advantage will silently use the previously stored amount — returning a stale value from the last payslip run instead of the intended zero.

# hr_contract_advantage.py
if not self.fixed_value and self.amount:  # breaks when fixed_value = 0.0 intentionally
    value = self.amount
else:
    value = self.fixed_value

Suggestion: remove this fallback entirely. The migration already populates fixed_value from amount for all existing records, so there is no case where fixed_value would be unset on a pre-existing advantage. For new records, fixed_value will be set explicitly by the user.

---

2. @api.constrains("amount") fires during payslip compute write

In hr_payslip.get_current_contract_dict, the line advantage.amount = amount triggers _check_bound_limits via the ORM constraint. The bounds were already validated inside _compute_advantage_amount → _check_bounds, so the constraint runs twice on every payslip generation.

More critically, if the ORM flushes at an intermediate state during a batch payslip computation, the constraint may fire on a partially updated record, leading to unpredictable ValidationError exceptions in production.

Suggestion: either remove @api.constrains("amount") since _check_bounds already handles validation at compute time, or write the amount using a context flag and guard inside the constraint:

# Option A: remove the constraint (preferred)
# _check_bounds in _compute_advantage_amount already covers this
 
# Option B: guard with context
@api.constrains("amount")
def _check_bound_limits(self):
    if self.env.context.get("skip_advantage_constraint"):
        return
    for record in self:
        record._check_bounds(record.amount)

---

🟠 Non-blocking issues

3. quantity_fixed_value is not False never triggers the fallback

Float fields in Odoo always return 0.0, never False. The else 1.0 branch in _compute_quantity is unreachable dead code. The intended default of 1.0 is already guaranteed by default=1.0 on the field definition and by the migration script.

# Dead code — Float fields never return False
value = (
    self.quantity_fixed_value
    if self.quantity_fixed_value is not False  # always True
    else 1.0
)

Suggestion: simplify to value = self.quantity_fixed_value.

---

4. Write to amount inside get_current_contract_dict is a performance risk

Writing advantage.amount = amount per advantage per payslip generates one SQL UPDATE per record per payslip generation. On payrolls with many employees and complex advantage setups this can be a significant bottleneck.

The OCA standard pattern would be to compute this value on-the-fly and persist it only on payslip confirmation, not during salary rule evaluation. If auditing is needed, consider a separate last_computed_amount field that is written only on slip confirmation.

---

5. _eval_code silently swallows result = None

return localdict.get("result", 0.0) or 0.0

This coerces None and False to 0.0 without any warning. A user whose Python formula forgets to set result will receive a silent zero — and _coerce_float will accept 0.0 without raising, making the bug completely invisible.

Suggestion:

result = localdict.get("result")
if result is None:
    raise UserError(
        _("Python formula for advantage '%s' did not set 'result'.")
        % (self.advantage_template_id.name or self.id)
    )
return result or 0.0

---

🔵 Process feedback

6. Consider splitting into smaller PRs

This PR mixes refactoring (_check_bounds extraction), a schema change with migration (new fields), new business logic (computation/quantity modes), payslip integration, and documentation updates — all in a single force-pushed commit. Each of these deserves its own PR and its own migration version bump.

Suggested split:

PR	Scope	Migration
PR 1	Refactor: extract _check_bounds, simplify _check_bound_limits	none
PR 2	Add computation_mode fields on template and advantage	18.0.2.0.0
PR 3	Add quantity_mode fields on template and advantage	18.0.3.0.0
PR 4	Integrate dynamic compute in hr_payslip	none
PR 5	Docs update (README, DESCRIPTION.md, USAGE.md)	none

---