Skip to content

Conversation

@quirino95
Copy link

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.

@quirino95 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
Copy link

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review, just a minor comment, otherwise LGTM

Comment on lines 24 to 30
@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)
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, updated settings.

Comment on lines 8 to 11
# flake8: noqa: B950
cls.template = cls.env.ref(
"hr_personal_equipment_request_sign_oca.sign_oca_template_personal_equipment_request_demo"
)
Copy link

@aleuffre aleuffre Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very useful, thank you!

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

show_sn_report = fields.Boolean(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review, LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants