[16.0][IMP] hr_employee_ppe: Using related fields#1536
[16.0][IMP] hr_employee_ppe: Using related fields#1536
Conversation
|
Hi @eduaparicio, @marcelsavegnago, |
anusriNPS
force-pushed
the
16.0-update-fields
branch
2 times, most recently
from
7b98412 to
eeacc49
Compare
|
This PR has the |
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for the improvement! Using related fields instead of @api.onchange is definitely the right direction -- related fields work consistently regardless of whether the change comes from the UI, ORM, or import, while onchange only fires from the UI.
A couple of things I'd like to discuss:
1. store attribute on related fields
The new related fields default to store=False. The old fields (is_ppe, expire_ppe, indications) were regular stored fields with database columns. With this change:
- Existing stored data in those columns becomes orphaned (not a functional problem, Odoo handles this, but worth being aware of).
- If any external module or report filters/groups by
is_ppeorexpire_ppein a domain, it will become significantly slower or fail since unstored fields can't be efficiently searched. The tree view already displaysexpire_ppe, which means a list view with a filter on that field would hit this.
Have you considered adding store=True to the related fields? That would keep the DB column, allow efficient search/filter, and Odoo would auto-maintain the values through the related mechanism. The trade-off is that related stored fields add DB triggers, but for a Boolean and a Text field this is negligible.
2. Behavioral change on expire_ppe editability
In the current code, expire_ppe is a regular field with the view allowing edits when state in ['draft', 'accepted']. The form even has readonly="state not in ['draft','accepted']" on expire_ppe, implying it was intended to be editable per-record. With a related field (which is readonly=True by default at ORM level), users can no longer override expire_ppe on a specific equipment record independently of the product.
If overriding per-record was not actually needed (which seems to be the case from the original onchange logic), then this is fine -- just wanted to confirm the intent.
3. Minor: Test assertions inside assertRaises block
In test_check_dates, the new assertions (assertTrue, assertEqual) are placed inside the with self.assertRaises(ValidationError) block before the call to validate_allocation(). If any of those assertions fail, they'll raise AssertionError (not ValidationError), which would cause the test to fail with a confusing error message. Consider moving those assertions before the assertRaises context manager, or into a separate test.
| _inherit = ["hr.personal.equipment"] | ||
|
|
||
| is_ppe = fields.Boolean() | ||
| is_ppe = fields.Boolean(related="product_id.is_ppe") |
There was a problem hiding this comment.
Consider adding store=True here. The old field was stored, and the tree view displays this field, so filtering/grouping on is_ppe in list views would require a stored field to work efficiently.
There was a problem hiding this comment.
Restored store=True for fields which are modified as related
| self.hr_employee_ppe_expirable.start_date = "2020-01-01" | ||
| self.hr_employee_ppe_expirable.expiry_date = "2019-12-31" | ||
| self.hr_employee_ppe_expirable._compute_fields() | ||
| self.assertTrue(self.hr_employee_ppe_expirable.is_ppe) |
There was a problem hiding this comment.
These assertions are inside the assertRaises(ValidationError) block. If any assertion fails, it raises AssertionError (not ValidationError), which would cause assertRaises to re-raise it with a confusing traceback. Consider moving these assertions before the with block:
self.hr_employee_ppe_expirable.start_date = "2020-01-01"
self.hr_employee_ppe_expirable.expiry_date = "2019-12-31"
self.assertTrue(self.hr_employee_ppe_expirable.is_ppe)
self.assertTrue(self.hr_employee_ppe_expirable.expire_ppe)
...
with self.assertRaises(ValidationError):
self.hr_employee_ppe_expirable.validate_allocation()There was a problem hiding this comment.
Moved assert checks outside assertRaises block as suggested
anusriNPS
force-pushed
the
16.0-update-fields
branch
3 times, most recently
from
586d731 to
7fb000b
Compare
Using related fields for "is_ppe", "expire_ppe" and "indications" instead of computing these fields based on product change from UI. Related fields are made as 'store=True' to restore earlier behaviour.
anusriNPS
force-pushed
the
16.0-update-fields
branch
from
7fb000b to
5f624d7
Compare
|
Depends on #1550 which resolves error seen in pre-commit job |
|
#1550 has been merged, so we should be able to merge this as well. The PR has enough approvals. Thank you! |
6 participants
Using related fields for "is_ppe", "expire_ppe"
and "indications" instead of computing these fields based on product change from UI.