[16.0][ADD] hr_personal_equipment_request_sign_oca: new module #127

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.

aleuffre

Partial code review

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

hr_personal_equipment_request_sign_oca/demo/sign_oca_template.xml

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

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

hr_personal_equipment_request_sign_oca/models/sign_oca_request.py

+    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

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

hr_personal_equipment_request_sign_oca/models/sign_oca_request.py

+        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

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

hr_personal_equipment_request_sign_oca/models/hr_personal_equipment_request.py

+        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

aleuffre

Code review

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

hr_personal_equipment_request_sign_oca/models/res_config_settings.py

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

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

hr_personal_equipment_request_sign_oca/models/sign_oca_request.py

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

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

hr_personal_equipment_request_sign_oca/models/sign_oca_request.py

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

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

hr_personal_equipment_request_sign_oca/models/hr_personal_equipment_request.py

+        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()

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

hr_personal_equipment_request_sign_oca/models/hr_personal_equipment_request.py

+    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,

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

hr_personal_equipment_request_sign_oca/reports/ppe_sign_report_template.xml

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


chore: empty <t> can be removed

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

hr_personal_equipment_request_sign_oca/reports/ppe_sign_report_template.xml

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


chore: please fix the indentation here

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

hr_personal_equipment_request_sign_oca/tests/test_hr_personal_equipment_request_sign_oca.py

+        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

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

hr_personal_equipment_request_sign_oca/tests/test_hr_personal_equipment_request_sign_oca.py

+        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"))

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

hr_personal_equipment_request_sign_oca/tests/test_hr_personal_equipment_request_sign_oca.py

+    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

aleuffre

Code review, LGTM

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

aleuffre 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

aleuffre suggested changes Sep 23, 2025

View reviewed changes

quirino95 force-pushed the 16.0-hr_personal_equipment_request_sign_oca branch from 9f12bae to 06148e7 Compare September 24, 2025 14:57

[ADD] hr_personal_equipment_request_sign_oca: new module

9fe93b3

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

aleuffre 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

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

Uh oh!

[16.0][ADD] hr_personal_equipment_request_sign_oca: new module #127

Are you sure you want to change the base?

[16.0][ADD] hr_personal_equipment_request_sign_oca: new module #127

Uh oh!

Conversation

quirino95 commented Sep 9, 2025

Uh oh!

aleuffre 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!

aleuffre 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!

aleuffre left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants