Conversation
OCA-git-bot
added
series:18.0
mod:payroll_rule_history
|
tests fail in payroll_document |
|
I merged #311 fixing this because CI it's failing in all 18.0 PRs. |
nimarosa
left a comment
nimarosa
left a comment
There was a problem hiding this comment.
Nice to see this one land as its own module, it doesn't duplicate anything we already have upstream and the idea of versioning a rule instead of editing it in place is the right one.
I'd like it in once the date filter and the batch compute are sorted out, both of them raise today rather than misbehave quietly. The rest of my notes are small.
CI is red on hr_payroll_document, that's not yours, #311 fixes it, so please rebase.
| for rule in self: | ||
| rules |= ( | ||
| rule.history_rule_ids.filtered( | ||
| lambda x: (not date and not x.date_start or x.date_start <= date) |
There was a problem hiding this comment.
The filter reads (not date and not x.date_start) or (x.date_start <= date) because and binds tighter than or. A version with no start date hits False <= date and raises TypeError, and so does a call with no active_date in context, where it becomes x.date_start <= None. I think you want (not x.date_start or x.date_start <= date) with an explicit guard for date is None.
| if rule.has_history and rule.history_rule_ids | ||
| else rule | ||
| ) | ||
| return rules |
There was a problem hiding this comment.
This can return an empty recordset when no version covers the payslip date, and _compute_rule calls ensure_one(), so the user gets "Expected singleton" instead of something actionable. Worth either falling back to the base rule or raising a UserError naming the rule. The overlap TODO above is the other half of the same problem, two versions covering the same day get you the same error.
| # Set active_date in the context | ||
| # To be used to filter active Master Data Values | ||
| if not self.env.context.get("active_date"): | ||
| self = self.with_context(active_date=self.date_to) |
There was a problem hiding this comment.
compute_sheet is called on multi-record sets, see hr_payroll_payslips_by_employees.py, so self.date_to raises here and generating a batch breaks. Setting the context per payslip inside the loop, or overriding get_lines_dict instead, would keep that working.
| "type": "ir.actions.act_window", | ||
| "view_mode": "list, form", | ||
| "target": "current", | ||
| "domain": [("base_rule_id", "in", self.id)], |
There was a problem hiding this comment.
[("base_rule_id", "in", self.id)] passes an int to in, which the domain parser rejects, = works. Also default_rule_id does not exist on the model, I think you meant default_base_rule_id, and view_mode just below has a stray space in "list, form".
| <field name="date_end" invisible="not base_rule_id" /> | ||
| </field> | ||
|
|
||
| <xpath expr="//notebook/page[1]" position="attributes"> |
There was a problem hiding this comment.
The page[1] xpath and the override of the payroll.action_salary_rule_form context further down both worry me a little. #309 and #310 are editing that same form, and the action context gets reset on any update of payroll. A named page and a separate action would be more stable. The page name="rules" block right after this also sets the same attribute twice.
| { | ||
| "name": "Payroll Rule History", | ||
| "summary": "Set validity dates on Payroll rules", | ||
| "version": "18.0.1.0.0", |
There was a problem hiding this comment.
Could you add a tests folder before we merge? Two versions of a rule and two payslips on either side of the boundary would cover most of what this module does, and it would have caught the date comparison in _get_active_rule.
4 participants
Thanks @dreispt