Added Employment change model, refactoring working calculation #981
Conversation
| @@ -1,6 +1,7 @@ | |||
| """Models for the employment app.""" | |||
|
|
|||
| from datetime import date, timedelta | |||
| from turtle import mode | |||
| @@ -13,7 +14,7 @@ | |||
| from timed.models import WeekdaysField | |||
| from timed.projects.models import CustomerAssignee, ProjectAssignee, TaskAssignee | |||
| from timed.tracking.models import Absence | |||
|
|
|||
| from timed.employment.scheduls import get_schedule | |||
There was a problem hiding this comment.
There's a typo in scheduls, should be schedules
also in the filename of timed/employment/schedules.py
| def get_schedule(start,end,employment: Union[Employment, EmploymentChange]): | ||
|
|
||
| """ | ||
| Obtaining weekly working days, holidays, overtime credit, | ||
| reported working time and absences | ||
|
|
||
| :params start: working starting time on a given day | ||
| :params end : working end time of a given day | ||
| :employment : Employement or EmploymentChange object | ||
| """ |
There was a problem hiding this comment.
get_schedule makes me expect some planned or scheduled entity. however the actually returned object is more of a summary or report on actually accounted for time spent and expected workload taking into account spent hours, scheduled workdays and days off..
so the original calculate_worktime or estimate_worktime or even estimate_timeframed_workload that would make reference to the parameters were somehow more descriptive.
| timedelta(), | ||
| ) | ||
|
|
||
| return (workdays,holidays,overtime_credit,reported_worktime,absences) |
There was a problem hiding this comment.
While the return value is perfectly fine in terms of correctness it could be nice for a different developer (or even the same far enough in the future ;) to get some datastructure back that documents it's elements meaning like a namedtuple or something.
Workload is just an example name..
Workload = namedtuple('Workload', ["workdays", "holidays", "overtime_credit", "reported_worktime", "absenses"])This could also be used in the function's definition of the return value -> Workload
That would then be much more informative when dealing with the function's output.
output.workdays rather than output[0]
| expected_worktime = self.worktime_per_day * (schedule(0) - schedule(1)) | ||
| reported = schedule(3) + schedule(4) + schedule(2) |
There was a problem hiding this comment.
get_schedule does not return a function. So I assume these calls are actually meant to be lookups schedule[0] etc.
see my comment regarding the named tuples, though.
| # shorten time frame to employment | ||
| start = max(start, employment.start_date) | ||
| end = min(employment.end_date or date.today(), end) | ||
|
|
There was a problem hiding this comment.
why did you remove the comment?
# workdays is in isoweekday, byweekday expects Monday to be zero
| @@ -259,54 +260,11 @@ def calculate_worktime(self, start, end): | |||
| :returns: tuple of 3 values reported, expected and delta in given | |||
| time frame | |||
| """ | |||
There was a problem hiding this comment.
The docstring does not seem to fit the function anymore since you moved the code to another function that is called by it.
I wonder if it would be sufficient to create the part for the employment.
But I'm not familiar enough with the model situation here e. g. why the calculation is bound to the Location model in the first place..
No description provided.