[16.0][ADD] hr_personal_equipment_request_sign_oca: new module by quirino95 · Pull Request #127 · OCA/sign

quirino95 · 2025-09-09T12:24:21Z

New module to allow to generate manual and automatic signature requests from Personal Equipment Request.
Instead of a "static" template, every time a new Personal Equipment Request is created, PPE receipt document is generated and attached in the signature request.
Also, the PPE receipt report template in this module is overwritten, in order to keep the space for signature always in the same position.

HekkiMelody

Partial code review

HekkiMelody · 2025-09-09T15:14:02Z

@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<odoo>


suggestion (non-blocking):

I think it could be useful to have these as actual data, not just demo data, maybe with a noupdate, what do you think?

Why not, it helps save time in configuration. Applied changes.

Both files are still in the demo key in the manifest (and also still in the demo folder)

Moved files correctly, thank you.

HekkiMelody · 2025-09-09T15:18:12Z

+    def _compute_personal_equipment_request_id(self):
+        for item in self.filtered(
+            lambda x: x.record_ref
+            and x.record_ref._name == "hr.personal.equipment.request"
+        ):
+            item.personal_equipment_request_id = item.record_ref.id
+
+    @api.depends("record_ref")
+    def _compute_employee_id(self):
+        for item in self.filtered(
+            lambda x: x.record_ref
+            and x.record_ref._name == "hr.personal.equipment.request"
+        ):
+            equipment_request = self.env["hr.personal.equipment.request"].browse(
+                item.record_ref.id
+            )
+            item.employee_id = equipment_request.employee_id


suggestion: since these are so similar and so strictly linked, they could be merged into a single compute for both fields

HekkiMelody · 2025-09-09T15:19:05Z

+        compute_sudo=True,
+        readonly=True,


chore: fields that are computed are also readonly by default. If they are stored, they are also compute_sudo by default

HekkiMelody · 2025-09-09T15:24:56Z

+        old_employee_id = self.employee_id
+        new_employee_id = vals.get("employee_id")
+        res = super().write(vals)
+        if new_employee_id and new_employee_id != old_employee_id.id:
+            self._generate_sign_oca_request()
+        return res


issue: Careful! self can be a recordset with multiple records

suggestion:

Suggested change

old_employee_id = self.employee_id

new_employee_id = vals.get("employee_id")

res = super().write(vals)

if new_employee_id and new_employee_id != old_employee_id.id:

self._generate_sign_oca_request()

return res

if employee_id in vals:

old_employees = {rec.id: rec.employee_id for rec in self}

res = super().write(vals)

if employee_id in vals:

for rec in self:

if rec.employee_id and rec.employee_id != old_employees.get(rec.id):

rec._generate_sign_oca_request()

return res

HekkiMelody

Code review

HekkiMelody · 2025-09-23T08:36:50Z

+    personal_equipment_request_sign_oca_template_id = fields.Many2one(
+        comodel_name="sign.oca.template",
+        related="company_id.personal_equipment_request_sign_oca_template_id",
+        string="Personal Equipment Request Sign Oca Template",


chore: In a related field, there's no need to re-declare any fields that are the same as the original field (like this string here)

HekkiMelody · 2025-09-23T08:39:59Z

+    )
+
+    @api.depends("record_ref")
+    def _compute_fields(self):


issue: let's use a more descriptive name.

suggestion: Maybe employee_id could simply be a related field personal_equipment_request_id.employee_id, so this compute could be even simpler?

Also, maybe it could be called ppe_employee_id to make it clear it's linked to the ppe request? (I'm imagining a scenario with multiple sign related modules

Renamed employee_id in ppe_employee_id (now it's a related field) and _compute_fields in _compute_personal_equipment_request_id.

HekkiMelody · 2025-09-23T08:43:15Z

+        for item in self.filtered(
+            lambda x: x.record_ref
+            and x.record_ref._name == "hr.personal.equipment.request"
+        ):
+            item.personal_equipment_request_id = item.record_ref.id
+            equipment_request = self.env["hr.personal.equipment.request"].browse(
+                item.record_ref.id
+            )
+            item.employee_id = equipment_request.employee_id


question: Should we explicitly set personal_requipment_request_id to False if record_ref is not a ppe request?

Suggested change

for item in self.filtered(

lambda x: x.record_ref

and x.record_ref._name == "hr.personal.equipment.request"

):

item.personal_equipment_request_id = item.record_ref.id

equipment_request = self.env["hr.personal.equipment.request"].browse(

item.record_ref.id

)

item.employee_id = equipment_request.employee_id

for item in self:

if item.record_ref and item._record_ref._name == "hr.personal.equipment.request":

item.personal_equipment_request_id = item.record_ref.id

equipment_request = self.env["hr.personal.equipment.request"].browse(

item.record_ref.id

)

item.employee_id = equipment_request.employee_id

else:

item.personal_equipment_request_id = False

I don't know but is okay to explicit (code is also more readable).

HekkiMelody · 2025-09-23T08:44:55Z

+        for item in res:
+            item._generate_sign_oca_request()


chore: _generate_sign_oca_request is safe for recordsets

Suggested change

for item in res:

item._generate_sign_oca_request()

res._generate_sign_oca_request()

HekkiMelody · 2025-09-23T08:46:01Z

+    sign_request_count = fields.Integer(
+        string="Sign request count",
+        compute="_compute_sign_request_count",
+        compute_sudo=True,


suggestion (non-blocking):

Suggested change

compute_sudo=True,

store=True,

HekkiMelody · 2025-09-23T08:58:44Z

+                        <table class="table table-condensed">
+                            <tbody>
+                                <div>
+                                    <t>


chore: empty <t> can be removed

HekkiMelody · 2025-09-23T08:59:38Z

+                                        <tr>
+                                            <th>
+                                           Product
+                                       </th>
+                                            <th>
+                                           Quantity
+                                       </th>
+                                            <th>
+                                           Indications
+                                       </th>
+                                        </tr>


chore: please fix the indentation here

HekkiMelody · 2025-09-23T09:03:53Z

+        requests = self.request_1 + self.request_2
+        wizard_form = Form(
+            self.env["sign.oca.template.generate.multi"].with_context(
+                default_model="hr.personal.equipment.request", active_ids=requests.ids
+            )
+        )
+        wizard_form.template_id = self.template
+        action = wizard_form.save().generate()
+        requests = self.env[action["res_model"]].search(action["domain"])


chore (non-blocking): it's somewhat confusing to reuse the variable requests for two different objects. Also below we have request_1 which is not quite the same thing as self.request_1.

I would be grateful for more descriptive and distinct variable names.

Just did some refactoring

HekkiMelody · 2025-09-23T09:04:43Z

+        self.assertIn(self.partner_1, request_1.mapped("signer_ids.partner_id"))
+        self.assertIn(self.partner_2, request_2.mapped("signer_ids.partner_id"))


question: shouldn't this be

Suggested change

self.assertIn(self.partner_1, request_1.mapped("signer_ids.partner_id"))

self.assertIn(self.partner_2, request_2.mapped("signer_ids.partner_id"))

self.assertEqual(self.partner_1, request_1.mapped("signer_ids.partner_id"))

self.assertEqual(self.partner_2, request_2.mapped("signer_ids.partner_id"))

HekkiMelody · 2025-09-23T09:06:05Z

+    def test_personal_equipment_request_write(self):
+        # flake8: noqa: B950
+        self.company_id.personal_equipment_request_sign_oca_template_id = self.template
+        self.request_1.write({"employee_id": False})


suggestion (non-blocking):

I personally prefer using object-oriented assignments whenever possible. Seems easier to read and to write

Suggested change

self.request_1.write({"employee_id": False})

self.request_1.employee_id = False

HekkiMelody

Code review, LGTM

github-actions · 2026-01-25T12:41:58Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

HekkiMelody · 2026-01-26T09:05:56Z

@OCA/document-maintainers Hello, we've been using this in production for a few months now, could you please take a look? Thanks!

SirPyTech

Thanks for the PR!
I reviewed the code and quickly tried locally and in runboat: when the PPE request has no items the PDF looks empty:
could you please check?

SirPyTech · 2026-05-11T08:56:09Z

+        # flake8: noqa: B950
+        self.company_id.personal_equipment_request_sign_oca_template_id = self.template


note: There is an identical line in the previous test that does not need this comment, maybe it can be removed?

SirPyTech · 2026-05-11T09:03:16Z

+            }
+        )
+
+        cls.user_1 = cls.env["res.users"].create(


suggestion: Executing the tests logs the following ERROR log:

2026-05-11 08:39:30,494 320 ERROR odoo odoo.addons.mail.models.mail_mail: failed sending mail (id: 44) due to You must either provide a sender address explicitly or configure using the combination of `mail.catchall.domain` and `mail.default.from` ICPs, in the server configuration file or with the --email-from startup parameter. Traceback (most recent call last): File "/opt/odoo/addons/mail/models/mail_mail.py", line 555, in _send msg = IrMailServer.build_email( File "/opt/odoo/odoo/addons/base/models/ir_mail_server.py", line 489, in build_email assert email_from, "You must either provide a sender address explicitly or configure "\ AssertionError: You must either provide a sender address explicitly or configure using the combination of `mail.catchall.domain` and `mail.default.from` ICPs, in the server configuration file or with the --email-from startup parameter.

(from https://github.com/OCA/sign/actions/runs/25659492416/job/75316125546?pr=127#step:8:99)
I think it is due to these users' creation, please have a look at mail_new_test_user (https://github.com/odoo/odoo/blob/fa0a4e7bc272105cad210867c4ddd46a5326cd22/addons/mail/tests/common.py#L28).

SirPyTech · 2026-05-11T09:13:34Z

note: I see that in other modules of this repo these files are in the demo folder, but I agree it makes sense to have these as data files instead.

Maybe one of @OCA/document-maintainers can clarify if we are missing something?

SirPyTech · 2026-05-11T09:17:01Z

+<odoo>
+    <template id="ppe_sign_report_template">
+        <t t-call="web.html_container">
+            <t t-foreach="docs" t-as="doc" t-if="doc.contains_ppe">
+                <t t-call="web.external_layout">
+                    <div class="page">
+                        <h3
+                            class="text-center"
+                        >Receipt of Personal Protection Equipment</h3>
+                        <br />
+                        <p>
+                            I, the undersigned accept the PPE that my employer is providing to me. I further acknowledge that I have been instructed
+                            in how to wear and maintain it. If it is lost or damaged, I will report the same to my employer promptly for replacement.
+                        </p>
+                        <p>
+                            I agree to wear the equipment when facing the exposure it is designed to protect against.
+                            I acknowledge that my failure to do so may subject me to disciplinary action.
+                        </p>


suggestion: This is the kind of record that the user might want to edit, please consider adding a noupdate.

SirPyTech · 2026-05-11T09:25:14Z

+                    "hr_personal_equipment_request_sign_oca.ppe_sign_report_template"
+                )
+                pdf = report._render_qweb_pdf(report.report_name, [self.id])[0]
+                request_dict.update({"data": base64.b64encode(pdf)})


issue: If the user assigns another PDF to the template, here we are overwriting that value: this is definitely unexpected.

Since we have implemented a nice template, I think we can just assign it to the data field of the record sign_oca_template_personal_equipment_request_demo.

On a second look, this is probably needed to obtain the dynamic behavior of the template, but it is still unexpected: please at least document it or show a warning in the sign template's form.

This dynamic behavior would be a nice addition to the sign_oca module itself 🚀
For instance: you might add the option to link a view instead of a static PDF in the template, and that view would be rendered using the current record.

SirPyTech · 2026-05-11T09:28:07Z

+1.  Go to Sign > Settings > Roles and create a role with the following data:
+
+- Partner type: Expression
+- Expression: {{object.employee_id.user_id.partner_id.id}}
+
+2.  Go to Sign > Templates and create a template with the following data:
+
+- Model: "This model allows to create a personal equipment request"
+- In one of the fields, you must set the previously created role.


note: In this module these records are already created, please mention the existing records here.

SirPyTech · 2026-05-11T09:32:45Z

polish: Right now this file is rendered as
Please change the indentation so that the sub-items are more indented than the main items.

SirPyTech · 2026-05-11T09:40:45Z

+    ppe_employee_id = fields.Many2one(
+        comodel_name="hr.employee",
+        related="personal_equipment_request_id.employee_id",
+        store=True,
+    )


suggestion: This would allow to change the PPE request's employee from the sign request and I think we don't want that, what do you think?
If this is not needed, please add a readonly attribute.

SirPyTech · 2026-05-11T09:44:35Z

+    "summary": """Hr Personal Equipment Request Sign Oca""",
+    "version": "16.0.1.0.0",
+    "license": "AGPL-3",
+    "author": "PyTech SRL, Odoo Community Association (OCA)",


suggestion: We should adopt this new module so we don't burden the PSC in the future.

SirPyTech · 2026-05-12T07:09:05Z

pre-commit will be fixed after #156.

quirino95 force-pushed the 16.0-hr_personal_equipment_request_sign_oca branch from 78020a4 to 0b98f6d Compare September 9, 2025 12:34

HekkiMelody reviewed Sep 9, 2025

View reviewed changes

quirino95 force-pushed the 16.0-hr_personal_equipment_request_sign_oca branch from 0b98f6d to 9f12bae Compare September 10, 2025 08:12

HekkiMelody suggested changes Sep 23, 2025

View reviewed changes

quirino95 force-pushed the 16.0-hr_personal_equipment_request_sign_oca branch 2 times, most recently from 06148e7 to 9fe93b3 Compare September 24, 2025 15:20

HekkiMelody approved these changes Sep 25, 2025

View reviewed changes

quirino95 mentioned this pull request Oct 16, 2025

[16.0][ADD] hr_personal_equipment_request_sign_oca: Show S/N in PPE Request Report #137

Open

github-actions Bot added the stale label Jan 25, 2026

github-actions Bot removed the stale label Feb 1, 2026

[ADD] hr_personal_equipment_request_sign_oca: new module

94346c7

SirPyTech force-pushed the 16.0-hr_personal_equipment_request_sign_oca branch from 9fe93b3 to 94346c7 Compare May 11, 2026 08:37

OCA-git-bot added mod:hr_personal_equipment_request_sign_oca Module hr_personal_equipment_request_sign_oca series:16.0 labels May 11, 2026

SirPyTech suggested changes May 11, 2026

View reviewed changes

		@@ -0,0 +1,34 @@
		<?xml version="1.0" encoding="UTF-8" ?>
		<odoo>

	for item in res:
	item._generate_sign_oca_request()
	res._generate_sign_oca_request()

		self.assertIn(self.partner_1, request_1.mapped("signer_ids.partner_id"))
		self.assertIn(self.partner_2, request_2.mapped("signer_ids.partner_id"))

	self.request_1.write({"employee_id": False})
	self.request_1.employee_id = False

		# flake8: noqa: B950
		self.company_id.personal_equipment_request_sign_oca_template_id = self.template

Uh oh!

Conversation

quirino95 commented Sep 9, 2025

Uh oh!

HekkiMelody left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HekkiMelody left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HekkiMelody left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jan 25, 2026

Uh oh!

HekkiMelody commented Jan 26, 2026

Uh oh!

SirPyTech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SirPyTech May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SirPyTech May 11, 2026 •

edited

Loading