Conversation
Currently translated at 100.0% (25 of 25 strings) Translation: hr-10.0/hr-10.0-hr_holidays_hour Translate-URL: https://translation.odoo-community.org/projects/hr-10-0/hr-10-0-hr_holidays_hour/de/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: hr-10.0/hr-10.0-hr_holidays_hour Translate-URL: https://translation.odoo-community.org/projects/hr-10-0/hr-10-0-hr_holidays_hour/
robinkeunen
left a comment
robinkeunen
left a comment
There was a problem hiding this comment.
It's heavy stuiff, good work on this one. I mainly asked questions or commented on style (discard if you don't agree).
A lundi!
| mapping = dict( | ||
| [ | ||
| (leave["employee_id"][0], leave["number_of_hours"]) | ||
| for leave in leaves | ||
| ] |
There was a problem hiding this comment.
You can write this as a dict comprehension:
mapping = {
leave["employee_id"][0]: leave["number_of_hours"]
for leave in leaves
}|
|
||
| @api.onchange("employee_id") | ||
| def onchange_employee(self): | ||
| # Get result of original onchange_employee from parent class: |
There was a problem hiding this comment.
When code is readable, no need for comments :-)
| # Workaround for api incompatibility: | ||
| if type(res) is dict and res.has_key("value"): | ||
| for field, value in res.get("value").items(): | ||
| if hasattr(self, field): | ||
| setattr(self, field, value) |
|
|
||
| self._check_dates() | ||
| self._check_employee() | ||
| self._set_number_of_hours_temp() |
There was a problem hiding this comment.
I don't think it's ok to use a multi method from an onchange method. The database cursor will be rolled back (I think)
There was a problem hiding this comment.
indeed onchange don't persist data. not sure it still apply on 12 though.
| working_hours = [] | ||
| if employee.calendar_id: | ||
| working_hours.append(employee.calendar_id) | ||
| else: | ||
| contracts = employee.sudo().contract_ids | ||
| for contract in contracts: | ||
| if contract.working_hours: | ||
| working_hours.append(contract.working_hours) | ||
| return working_hours |
There was a problem hiding this comment.
A good pythonista is a loop killer. Declarative style is more readable and less bug prone. Also, you don't need to declare the variable upfront here. I'd write:
if employee.calendar_id:
working_hours = [employee.calendar_id]
else:
working_hours = [contract.working_hours for contract in contracts if contract.working_hours]|
|
||
| @api.multi | ||
| def name_get(self): | ||
| res = [] |
| result = { | ||
| "max_hours": 0, | ||
| "remaining_hours": 0, | ||
| "hours_taken": 0, | ||
| "virtual_remaining_hours": 0, | ||
| } |
There was a problem hiding this comment.
I'd use variables in the code and build the dictionary at the end of the method.
| status.hours_taken = 0 | ||
| status.remaining_hours = 0 | ||
| status.max_hours = 0 | ||
| status.virtual_remaining_hours = 0 | ||
| if employee: | ||
| res = status.get_hours(employee) | ||
| status.hours_taken = res["hours_taken"] | ||
| status.remaining_hours = res["remaining_hours"] | ||
| status.max_hours = res["max_hours"] | ||
| status.virtual_remaining_hours = res["virtual_remaining_hours"] |
There was a problem hiding this comment.
I'd write
if employee:
hours = status.get_hours(employee)
status.hours_taken = hours["hours_taken"]
status.remaining_hours = hours["remaining_hours"]
status.max_hours = hours["max_hours"]
status.virtual_remaining_hours = hours["virtual_remaining_hours"]
else:
status.hours_taken = 0
status.remaining_hours = 0
status.max_hours = 0
status.virtual_remaining_hours = 0| # Computes start_dt, end_dt (with default values if not set) | ||
| # + off-interval work limits | ||
|
|
||
| def set_work_limits_start(end_dt, start_dt): |
There was a problem hiding this comment.
I don't understand this function :/
|
@vvrossem Also, try keeping the ci green :-) |
8 participants
From 10.0
update 09/12/19
I could not override the constraints defined in the old API (hr_holidays : _constraints list). The workaround I am using is to redefine the methods first in old API, then in new API.