Skip to content

[18.0][FIX] payroll_account: company-aware accounts, real reversal on cancel, computed journal - #308

Open
nimarosa wants to merge 4 commits into
OCA:18.0from
nimarosa:18.0-fix-payroll_account
Open

nimarosa wants to merge 4 commits into
OCA:18.0from
nimarosa:18.0-fix-payroll_account

Conversation

@nimarosa

Copy link
Copy Markdown
Contributor
  • Debit/credit accounts, analytic account and taxes are now resolved in the
    payslip's own company instead of the confirming user's active company.
  • Cancelling a payslip reverses its posted entry instead of deleting it, and
    confirming twice no longer duplicates the accounting entry.
  • journal_id is a real computed/editable field (batch > contract) instead
    of a readonly mirror of the contract's journal.
  • Adds a README, a smart button to the journal entry, clearer error
    messages, and fixes a broken test fixture.

@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

… company

`hr.salary.rule.account_debit` and `account_credit` are company-dependent
fields, but `action_payslip_done` read them off the bare recordset, i.e. in
`self.env.company`.  Confirming a payslip while working in another company
therefore silently produced an entry with the wrong accounts -- or, when the
rule has no value configured for that company, no accounting entry at all and
no error.

Every read of the rule's accounting configuration now goes through
`with_company(<the payslip's company>)`, and the generated `account.move`
carries that company explicitly instead of inheriting it from the environment.

Also tightened around the same problem:

* `hr.contract.journal_id` and `hr.payslip.run.journal_id` get `check_company`
  and a company domain, and the batch's default journal is looked up in the
  active company instead of taking the first `general` journal of any company.
* `hr.salary.rule.account_debit` / `account_credit` get `check_company`.
* `_get_tax_details` filters the `account.tax.repartition.line` lookups by
  company, and limits the one whose `.id` is read to a single record -- a tax
  spread over several repartition lines on the same account made that read
  raise `Expected singleton`.  Tax and tag values are built with `Command`.
…g it

Cancelling a payslip deleted its accounting entry outright -- posted or not --
with `force_delete=True`, unless the journal happened to have the hash table
restriction enabled. That destroys the audit trail: a posted entry disappears
from the books with no trace, and leaves a hole in the journal's sequence.
The deletion also ran BEFORE `super()`, which can still refuse the
cancellation, so a refused cancel could leave a confirmed payslip whose entry
had already been destroyed.

The entry is now always reversed, after `super()` has accepted the
cancellation, with an explicit reversal date (the entry's own date, or today
when the entry is backdated) so that nothing is posted before the entry it
reverses. An entry that was never posted is still simply deleted -- nothing
ever reached the books.

Confirming a payslip twice created a second entry and doubled every amount in
the books, because nothing checked for an existing `move_id`. It is now a
no-op, logged. So that cancelling and confirming again still generates a
fresh entry, the reversed pair is moved to a new `cancelled_move_ids` field,
which is also where the audit trail now lives.

Finally, a payslip that still carries a posted entry can no longer be
deleted: it has to be cancelled first, so that its entry is reversed.
…mirror of the contract

`hr.payslip.journal_id` was a stored *related* field on
`contract_id.journal_id`. Being related it is readonly, so the form -- which
marks it `required="1"` -- offered a field nobody could fill in: a payslip
whose contract has no journal could not be confirmed and had no way out.
Being related it was also recomputed from the contract, with three
consequences:

* the batch's journal was ignored. `hr.payslip.employees.compute_sheet`
  injects `default_journal_id` from the batch precisely so the payslips of a
  batch land in the batch's journal; the related field overwrote it with the
  contract's on the spot;
* the same happened to any journal set by hand or by another module;
* editing a contract's journal rewrote the journal of payslips that were
  already confirmed and posted, after the fact.

It becomes an ordinary stored computed field with `readonly=False`, depending
on `contract_id` and `payslip_run_id` themselves rather than on their journal:
it proposes a journal when the contract or the batch changes and never
overwrites the payslip afterwards. The batch's journal wins over the
contract's. `check_company` and a company domain were added while the field
was being redefined.

The `onchange_contract` override goes away: it existed to work around the
related field, and the compute now covers it.
…s and fixtures

* `readme/CONFIGURE.md` was mangled -- its list markers were escaped, so the
  whole thing rendered as one paragraph starting with a stray `#`. Rewritten,
  and it now says what the module actually does: debit/credit per rule and
  that both are company-dependent, the partner rule per account type, the
  analytic precedence (contract over rule), and the journal's default account
  as the adjustment plug.
* Added `readme/USAGE.md`: one entry per payslip, confirming twice is a no-op,
  cancel reverses rather than deletes, refunds invert the sides.
* The payslip form showed the accounting entry as a bare readonly Many2one.
  It is a smart button now, next to the payslip lines one.
* The two "no default account" errors named an *Expense Journal* and a
  *Credit/Debit Account* that exist nowhere in the module. They now name the
  salary journal and its default account, and say what the account is used
  for.
* `logger.info` was called with an f-string; it takes lazy arguments now.
* Dropped the `_description` on the `hr.contract` extension: `_inherit`
  already carries the base model's.
* Test fixtures: `hr.employee.bank_account_id` was given the id of a
  `res.bank`, not of the `res.partner.bank` created right above it; the
  employee's bank account was therefore an unrelated record, and the
  bank-account branch of `_get_partner_id` was never really exercised. Fixed,
  and covered by a test. The fixture also moved to `setUpClass`.
@nimarosa
nimarosa force-pushed the 18.0-fix-payroll_account branch from 59d71a6 to 6af5ec9 Compare September 22, 2026 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants