[ADD] hr_payroll_document_queue

[SirPyTech]

Forward porting of the module already proposed in #252:

Add a new module to process the payslips asynchronously.

I have also included a small improvement to allow multiple wizards to be processed at the same time without conflicts.
A more elaborate solution could be implemented (like using https://docs.python.org/3/library/tempfile.html) but that would require more refactoring.

Let me know what you think!

[OCA-git-bot]

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

[HekkiMelody]

Code review, LGTM

[HekkiMelody]

chore (non-blocking): my understanding is that this (and also is_send_visible below) don't need to be added manually anymore in v18, so they could be removed to simplify the view.

[nimarosa]

Thanks for forward porting this, the module itself is useful and the tests are a good shape (arrange/act/assert, trap_jobs, a real mail assertion). CI is green and it merges clean on 18.0, so this is close.

Two things I'd like sorted before it goes in.

The first is the /tmp handling in hr_payroll_document. The commit sells it as thread safe, and per-wizard folders do fix the case it was written for, but the path is still predictable and shared: payroll_management_wizard_5 is the same string in every database served by the same host, so two databases processing wizard id 5 at the same time still land in one folder. On top of that the payslips sit unencrypted in a world-writable directory (encryption is optional via no_payroll_encryption) and nothing ever removes them. tempfile.mkdtemp() plus cleanup gives you all three for less code, and you already point at tempfile in the description.

The second is smaller: this PR changes behaviour in hr_payroll_document (_rec_name, the temp path) without bumping its version. It's at 18.0.1.0.2 on 18.0 now, so 18.0.1.1.0 would be right. That module is maintained by @peluko00, who got pinged but hasn't looked yet, so I'd rather wait for that too before merging.

The rest below is small stuff.

[nimarosa]

This is the one I'd really like changed. self._table and self.id are not unique across databases, so two databases on the same host still collide here, which is the case the commit says it fixes. And /tmp is world-writable with a guessable name, so anyone on the box can pre-create the directory (or symlink it) and read payslips that were written without encryption. Nothing deletes any of it either, so the files pile up until the host reboots.

tempfile.mkdtemp() gets you uniqueness, 0700, and a handle you can clean up, all at once:

import tempfile

@contextmanager
def _temp_dir(self):
    self.ensure_one()
    path = tempfile.mkdtemp(prefix=f"{self._table}_{self.id}_")
    try:
        yield path
    finally:
        shutil.rmtree(path, ignore_errors=True)

and have send_payrolls hold the context for the whole run so merge_pdfs reuses the same directory.

[nimarosa]

You already have temp_path from line 109, use it here. Right now every loop iteration re-runs the mkdir for nothing, and if the path ever becomes per-call (which it would with mkdtemp) this line silently writes somewhere else than line 126 reads.

[nimarosa]

typo: aynchronously -> asynchronously

[nimarosa]

Worth adding mail here. You call message_notify on mail.thread and read notification_type, and today that only works because hr happens to pull mail in. Cheap to make explicit.

[nimarosa]

with_delay().send_payrolls() returns a Job object, and /web/dataset/call_button drops anything that isn't a dict (return False, dataset.py). So the user clicks Send Async, the dialog just closes, and nothing tells them the job was queued. Returning a display_notification here would match what the sync button already does:

def send_payrolls_async(self):
    self.with_delay().send_payrolls()
    return {
        "type": "ir.actions.client",
        "tag": "display_notification",
        "params": {
            "title": self.env._("Payrolls queued"),
            "message": self.env._("You will be notified when they have been sent."),
            "type": "info",
        },
    }

[nimarosa]

self.env._() rather than the module level _, the 18.0 way. The base wizard you're inheriting already uses it everywhere, so this is the only odd one out.

[nimarosa]

This search has no bound on it and runs on every form load of the wizard, so it walks every non-terminal job for this method and deserializes records for each one. Adding a company or create_uid leg would help, and matching on attachment ids instead of checksums would let you push the filter into the domain instead of doing it in Python.

The checksum match also reaches across companies (the field is compute_sudo), so uploading the same PDF in company A blocks company B. If that's deliberate a one line comment saying so would save the next reader the trip.

[nimarosa]

This field is just not is_payroll_being_processed, and the view can express that on its own with invisible="is_payroll_being_processed". One less field on a public model, and one less compute per form load. The guard in send_payrolls can read is_payroll_being_processed directly.