[16.0][ADD] hr_personal_equipment_request_sign_oca: Show S/N in PPE Request Report by quirino95 · Pull Request #137 · OCA/sign

quirino95 · 2025-10-16T13:37:26Z

Depends on #127

Added possibility to show Serial Numbers in PPEs request report file. A related setting is present (default is False).
This also means that Sign request cannot be created on Personal Equipment Request Acceptance, but now it's done only when related pickings are completed.
Furthermore, now in report only "Valid" lines are visible, since showing cancelled lines doesn't make sense.

HekkiMelody

Code review, just a minor comment, otherwise LGTM

HekkiMelody · 2025-10-21T10:07:45Z

+    @api.model
+    def _default_show_sn_report(self):
+        return bool(
+            self.env["ir.config_parameter"]
+            .sudo()
+            .get_param("hr_personal_equipment_request_sign_oca.show_sn_report", False)
+        )


suggestion: instead of doing this configuration for the entire installation, we could do this per company. What do you think?

I agree, updated settings.

HekkiMelody · 2025-10-21T10:11:04Z

+        # flake8: noqa: B950
        cls.template = cls.env.ref(
            "hr_personal_equipment_request_sign_oca.sign_oca_template_personal_equipment_request_demo"
        )


suggestion (non-blocking): you can avoid disabling flake with the comment by splitting the string on two lines

cls.template = cls.env.ref( "hr_personal_equipment_request_sign_oca." "sign_oca_template_personal_equipment_request_demo" )

Very useful, thank you!

HekkiMelody · 2025-10-21T12:25:37Z

+        readonly=False,
+    )
+
+    show_sn_report = fields.Boolean(


chore: sorry to nitpick, but settings and company have A LOT of fields from every possible module, so it's good practice to "namespace" the fields by using appropriately specific names. The likelihood of another entirely unrelated module adding a field with this name in the setting is relatively high.

Maybe ppe_report_show_sn?

Don't worry, you're absolutely right. Yes, ppe_report_show_sn is good for me too, so I've updated code.

HekkiMelody

Code review, LGTM

github-actions · 2026-02-22T12:50:08Z

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-02-23T13:11:30Z

@OCA/human-resources-maintainers Hello, we've been using this in prod for months. Could you take a look? We can forward port after the merge. Thanks!

… report

anusriNPS · 2026-05-11T13:49:02Z

Depends on #156 which resolves precommit issue seen

SirPyTech

Thanks for the PR!
I only reviewed the code and looks like adding the serials' management is editing the original code/flow of the module, please see the comments below.

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

-    "depends": ["sign_oca", "hr_personal_equipment_request", "hr_employee_ppe"],
+    "depends": [
+        "sign_oca",
+        "hr_personal_equipment_request",
+        "hr_employee_ppe",
+        "hr_personal_equipment_stock",
+    ],


suggestion: If/when #127 is merged, I would avoid adding a new dependency on a merged module because it might break existing installations that do not have the new dependency available.
Please consider adding a bridge module or including this dependency from the beginning, since #127 isn't merged yet.

SirPyTech · 2026-05-12T07:22:14Z

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


issue: The PDF report should be generated from the single PPE request in the loop.

Suggested change

request_dict.update({"data": self._get_pdf_report()})

request_dict.update({"data": item._get_pdf_report()})

SirPyTech · 2026-05-12T07:25:53Z

-    @api.model_create_multi
-    def create(self, vals_list):
-        res = super().create(vals_list)
-        res._generate_sign_oca_request()
-        return res
+    def check_sign_oca_request(self):
+        if all(line.state in ["valid", "cancelled"] for line in self.line_ids) and any(
+            line.state == "valid" for line in self.line_ids
+        ):
+            self._generate_sign_oca_request()


issue: If we aren't managing serial numbers, this sign request should still be generated upon record creation: instead of removing the create override, please add a check on ppe_report_show_sn if the flow should be different.

If we really want to modify the original flow, this change should go in the commit where such flow is added.

SirPyTech · 2026-05-12T07:30:16Z

+    _inherit = "stock.picking"
+
+    def _action_done(self):
+        is_return = any(x.origin_returned_move_id for x in self.move_ids)


issue: _action_done can be called on multiple pickings: some of them can be returns and others maybe are not.
The problem is that is_return is True if at least one of them is a return: please identify which picking is a return and which isn't instead.

SirPyTech · 2026-05-12T07:31:08Z

+        return res
+
+    def action_cancel(self):
+        is_return = any(x.origin_returned_move_id for x in self.move_ids)


issue: action_cancel can be called on multiple pickings: some of them can be return and others maybe are not.
The problem is that is_return is True if at least one of them is a return: please identify which picking is a return and which isn't instead.

SirPyTech · 2026-05-12T07:32:27Z

+    def action_cancel(self):
+        is_return = any(x.origin_returned_move_id for x in self.move_ids)
+        res = super().action_cancel()
+        if self.equipment_request_id and not is_return:
+            self.equipment_request_id.check_sign_oca_request()


question: Why should we generate a sign request when a picking is cancelled?

SirPyTech · 2026-05-12T07:36:51Z

-                                        t-if="line.is_ppe"
+                                        t-if="line.is_ppe and line.state == 'valid'"


question: Why is this needed only when serial numbers are added? Maybe this goes in the 1st commit?

SirPyTech · 2026-05-12T07:41:32Z

            {"name": "Employee Test 2", "user_id": cls.user_2.id}
        )

+        cls.location_employee = cls.env["stock.location"].create(


suggestion: This setup is the same as https://github.com/OCA/hr/blob/a2251a13108632bdd9c0c5ad3cb9e1ca60f3116f/hr_personal_equipment_stock/tests/test_hr_personal_equipment_stock.py#L15, please consider adding a common tests setup over there and reusing it here.

SirPyTech · 2026-05-12T07:48:50Z

-        cls.company_id = cls.env.company
        cls.template = cls.env.ref(
-            "hr_personal_equipment_request_sign_oca.sign_oca_template_personal_equipment_request_demo"
+            "hr_personal_equipment_request_sign_oca."
+            "sign_oca_template_personal_equipment_request_demo"
        )
+        cls.company = cls.env.company
+        cls.company.personal_equipment_request_sign_oca_template_id = cls.template
+        cls.warehouse = cls.env.ref("stock.warehouse0")


chore: Most of these changes do not belong here, please move them to the previous commit where this piece of code has been added.

SirPyTech · 2026-05-12T07:53:50Z

+        readonly=False,
+    )
+
+    ppe_report_show_sn = fields.Boolean(


suggestion: Please add some documentation for the new behavior, for example: this new flag might be added in the CONFIGURATION fragment.

quirino95 changed the title ~~16.0 sn in personal equipment request report~~ [16.0][ADD] hr_personal_equipment_request_sign_oca: Show S/N in PPE Request Report Oct 16, 2025

HekkiMelody reviewed Oct 21, 2025

View reviewed changes

quirino95 force-pushed the 16.0-sn_in_personal_equipment_request_report branch from c869182 to fc41970 Compare October 21, 2025 12:05

HekkiMelody reviewed Oct 21, 2025

View reviewed changes

quirino95 force-pushed the 16.0-sn_in_personal_equipment_request_report branch from fc41970 to 846b073 Compare October 21, 2025 13:01

HekkiMelody approved these changes Oct 21, 2025

View reviewed changes

github-actions Bot added the stale label Feb 22, 2026

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

[ADD] hr_personal_equipment_request_sign_oca: new module

94346c7

anusriNPS force-pushed the 16.0-sn_in_personal_equipment_request_report branch from 846b073 to 3fb2f82 Compare May 11, 2026 13:43

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

[IMP] hr_personal_equipment_request_sign_oca: show s/n in ppe request…

40ded60

… report

anusriNPS force-pushed the 16.0-sn_in_personal_equipment_request_report branch from 3fb2f82 to 40ded60 Compare May 11, 2026 13:46

SirPyTech suggested changes May 12, 2026

View reviewed changes

	request_dict.update({"data": self._get_pdf_report()})
	request_dict.update({"data": item._get_pdf_report()})

		t-if="line.is_ppe"
		t-if="line.is_ppe and line.state == 'valid'"

Uh oh!

Conversation

quirino95 commented Oct 16, 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!

HekkiMelody Oct 21, 2025 • 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!

HekkiMelody left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 22, 2026

Uh oh!

HekkiMelody commented Feb 23, 2026

Uh oh!

anusriNPS commented May 11, 2026

Uh oh!

SirPyTech left a comment • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HekkiMelody Oct 21, 2025 •

edited

Loading

SirPyTech left a comment •

edited

Loading