[ADD] hr_attendance_missing_days #140
Conversation
fkantelberg
force-pushed
the
15.0-hr_attendance_missing_days
branch
from
a95d841
to
ff9677d
Compare
|
Hello @fkantelberg However if i check the attendance entries of my employee i only can see the entries created by hand. Normally i would expect also entries with 0.00 at least for the following days: Do you have any advise ? |
|
@fkantelberg Ok, i have forgotten to set the autoclose reason in the settings.
|
| <field name="numbercall">-1</field> | ||
| <field | ||
| name="nextcall" | ||
| eval="(DateTime.now() + timedelta(minutes=60)).strftime('%Y-%m-%d 04:00:00')" |
There was a problem hiding this comment.
that's going to give a weird time for timezones very different from UTC. Better use admin's tz and calculate 04:00 from this TZ's point of view?
There was a problem hiding this comment.
It's only executed the on creation of the record and not on each call. I don't think that it matter when the job runs it will catch the other dates tomorrow.
| if not date_to: | ||
| date_to = datetime.now() | ||
|
|
||
| date_from, date_to = map(ensure_tz, (date_from, date_to)) |
There was a problem hiding this comment.
I expect the way you localize date_from to cause trouble for people working at fringe times: If overtime start date is 2023-08-14 and the employee's timezone Europe/Amsterdam, we want date_from to be 2023-08-13 22:00:00 UTC, but here we get 2023-08-14 00:00:00 UTC.
Currently you don't use the employee's timezone at all here, but when you do, you'll have to localize the other way around:
pytz.timezone(employee.tz).localize(date_from)There was a problem hiding this comment.
I added a unit tests to cover some edge cases like 23:30 in various tz. It works fine. Be aware that the actual times are coming from the resource.calendar _work_intervals_batch and are already localized. date_from and date_to are just the time frame to calculate (if you only want to partially invalidate those).
|
|
||
| domain = [ | ||
| ("check_in", ">=", date_from.replace(tzinfo=None)), | ||
| ("check_in", "<=", date_to.replace(tzinfo=None)), |
There was a problem hiding this comment.
this gives you wrong dates before 01:00/02:00 in timezone Europe/Amsterdam, in the US or more so NZ the error will be bigger.
| ("check_in", "<=", date_to.replace(tzinfo=None)), | |
| ("check_in", "<=", date_to.astimezone(pytz.utc).replace(tzinfo=None)), |
same for date_from
There was a problem hiding this comment.
I found the problem. The problem was when the cron runs in the mid of a working day and had nothing to do with timezones. E.g. working day goes from 8:00 - 17:00 UTC. The cron runs 11:30 but the employee logs in at 12:00. The employee is present (only late) but the function created a 0 attendance. Fixed because the job filters out working days where the corresponding days aren't over yet.
There was a problem hiding this comment.
should the function maybe better work on dates (instead of datetime) anyways? Then it's clear which intervals are meant, and if you then create datetime objects as
datetime_from = timezone(employee.tz).localize(
datetime.combine(date_from, time.min)
).astimezone(pytz.utc).replace(tzinfo=None)
datetime_to = timezone(employee.tz).localize(
datetime.combine(date_to, time.max)
).astimezone(pytz.utc).replace(tzinfo=None)it's guaranteed you never miss an attendance from that date from the point of view of the employee's timezone.
This will also make it easier for fellow programmers to use the function, because they don't have to consider any timezone stuff.
With this all my other comments would be covered too
There was a problem hiding this comment.
@fkantelberg if you don't have time to implement this but generally agree with the proposal, I'll be happy to PR this to your PR
There was a problem hiding this comment.
I reworked it to use dates as argument and
| "name": "Test Employee", | ||
| "user_id": cls.env.user.id, | ||
| "company_id": cls.env.company.id, | ||
| "tz": "UTC", |
There was a problem hiding this comment.
this masks the timezone issues I think you'll have with the code above, I'd strongly suggest to test with timezones Europe/Amsterdam for yourself, but also America/New_York and Pacific/Auckland for timezones with a big offset to UTC.
Then the tests should also create a few attendances at local times 00:30 or 23:00 to be sure there aren't any off-by-one errors
|
oh, and I think we also should delete 0 attendances when an employee records some time after the fact, right? |
I don't know if we should do it automatically. With the latest patch Odoo only generates those if the working day is over. This means the employee creates the attendance on a different date. The missing attendances aren't hitting the computation of the worked hours or overtime but can also signal that it missed (e.g. for the HR officer). It would make the code more and more complex and would require an additional flag because the reason might not be unique because it's configurable. I would favor to keep those for now but if you need it I can make the attendance creation hookable if you like. If the employee is late on the working day it can get strange. I tried to handle it with the latest commit but it really gets weird around the edges independent of the timezone. E.g. if an employee has a working shift around midnight but comes late (=post midnight). Technically speaking the attendance is on the next day BUT the original shift starts before and a 0 attendance would be created because we are only checking against the starting times. |
|
The cron fails in my case, if an employee has a valid attendance record
If I change the relevant record |
|
@fkantelberg @hbrunn Hello, i can confirm the overtime caIculation is correct now. I have modified this chart a little bit: The overtime calculation is correct for several cases:
I assume this PR is ready to be merged. |
|
This PR has the |
|
FYI We discovered a problem in combination with contracts. Currently the cron will also generate attendances for employees who have no contract causing the high negative overtimes. I'll update the PR in the next days with a solution. |
fkantelberg
force-pushed
the
15.0-hr_attendance_missing_days
branch
4 times, most recently
from
aec3865
to
d19ebbe
Compare
|
I pushed a 2nd module to the PR to handle contracts. If you have contract installed the cron will only generate for employee if there is a open/closed contract during the time frame. If you have an employee (like Administrator) without a contract the cron won't create any missing attendances as a side effect. |
There was a problem hiding this comment.
For our use-case it seems to still work after your last changes.
We haven't installed hr_contract but use hr_employee_calendar_planning from OCA. With this module we can define (chained) working times with start and end date per employee. And even remove all working times is possible. Your module hr_attendance_missing_days does not add dummy attendance records for these employees.
So for us, it seems pretty good.
|
I wanted to do a functional review, but the attendance app does not load (white screen) on runboat without displaying any error message. Other than that, it would be great if we could merge it soon, so we can port it to 16 and 17. |
|
@Highcooley there seems to be an issue with That said, it's not a prerequisite for this PR to be merged to migrate it to other versions. Actually, having this migrated to other versions will show @OCA/human-resources-maintainers that there's interest for this functionality from several parties. |
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
bot
added
the
stale
|
Is there a way to restart the CI? It seems to be stuck. Apart from that, anything missing that needs to be done for this PR to be merge-ready? |
|
@pedrobaeza Do you know how to reset the CI? |
|
A forced push by @fkantelberg should work |
…g days without one
…tendance mid working day
…of the missing days
…dances (#1) * [IMP] hr_attendance_missing_days: don't break on multi work day attendances * [ADD] hr_attendance_contract_missing_days: tests * fixup! [ADD] hr_attendance_contract_missing_days: tests
ortlam
force-pushed
the
15.0-hr_attendance_missing_days
branch
from
532f10f
to
77038dd
Compare
Thank you, I updated the branch and fixed the unit test issue that came up. I think this PR should be ready to merge. |
github-actions
bot
removed
the
stale
|
/ocabot merge nobump |
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at 93bc519. Thanks a lot for contributing to OCA. ❤️ |
The overtime calculation of Odoo 15.0 ignores working days without attendances. To compensate this behaviour this modules implements a cron job which generates 0min attendances for working days without an attendance.