Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[12.0][MIG] hr_calendar_rest_time #709

Merged
merged 2 commits into from
Feb 20, 2020

Conversation

jarroyomorales
Copy link

Migration to 12.0 of hr_calendar_rest_timeand it's hook module resource_hook

- hr_calendar_rest_time: Adds rest time to the calendar attendance records
- resource_hook: Extends the resource with hooks to standard methods
@pedrobaeza pedrobaeza added this to the 12.0 milestone Oct 22, 2019
@jarroyomorales
Copy link
Author

@etobella @alexey-pelykh @JordiBForgeFlow @Saran440 Could you review please? Thank you!

Copy link
Member

@Saran440 Saran440 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jarroyomorales Sorry for delay, In core odoo, you can create day period morning and afternoon in working times.
I think result is not difference.
Selection_001

but I tested this module
Selection_002

  • Average hour per day should be 8.00 hours. exclude Rest Time
  • Day Period should be add All Day

and code reviewed

~~~~~~~~~~~~~~~~~~~~~~~

* Define rest time in the resource calendar to adjust the number of worked
hours during the day taking this time into consideration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, add history version 12.0.1.0.0

* Introduce hook in method '_get_work_days_data' of ResourceMixing class in
order to allow changes in the logic to compute working hours within an
interval. A new method '_get_work_hours' is now provided for this purpose.
The method can be inherit to alter the standard behaviour.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, add history version 12.0.1.0.0

@jarroyomorales
Copy link
Author

@Saran440 Thanks for the review. Yes, the total hours per day will be the same
The difference is that in core Odoo as you say, you define 2 lines, one for the morning, form 8-12 for example and another one for the afternoon, from 13 to 17. In this example you are indirectly saying that the employee has dinner from 12 to 13 but in a company which dinner time is flexible, it is very hard to manage it like this.
The most realistic way is to say that the employee works from 8 to 17 but takes 1 hour to have dinner, which is what this module is for.

@Saran440
Copy link
Member

Okay, I see.
I will approve when you fixed.

@jarroyomorales
Copy link
Author

@Saran440 Well, it is the version and the date when it was done, why would I change it to 12.0 and today's date?

@Saran440
Copy link
Member

@jarroyomorales Sorry, I mean add history v.12 in newline but not sure that necessary.
https://github.com/OCA/maintainer-tools/blob/master/template/module/readme/HISTORY.rst

@jarroyomorales
Copy link
Author

@Saran440 Oh sorry, I misunderstood you. There haven't been major changes so I think we can keep it like this.

@Saran440
Copy link
Member

@jarroyomorales Okay. by the way, I'm not sure that you see this sentence.
Selection_001

@jarroyomorales
Copy link
Author

@Saran440 Comments attended! Added a new day_period and rewrote _onchange_hours_per_day to compute the average hours properly.

def _onchange_hours_per_day(self):
if self.env.context.get('use_old_onchange_hours_per_day'):
return super()._onchange_hours_per_day()
attendances = self.attendance_ids.filtered(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you filtered attendance_ids not date_from and date_to?
I tested create Starting Date and End Date.
It not used Rest Time to calculate Average hour per day

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know this is an issue but it is the same way odoo does it in the core resource module.
At first I implemented the right solution taking into account the start and end date but it is not that easy. You would need a cron to check every day which lines are active are need to be used for the calculation and then update the value, since it is computed in an onchange.
Maybe we can add this behavior but it should probably be in another module.

Copy link
Member

@Saran440 Saran440 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional Test. LGTM 👍

Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@etobella
Copy link
Member

@OCA/human-resources-maintainers Can this be merged?

@dreispt
Copy link
Sponsor Member

dreispt commented Feb 20, 2020

/ocabot merge

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-709-by-dreispt-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Feb 20, 2020
Signed-off-by dreispt
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 12.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 12.0-ocabot-merge-pr-709-by-dreispt-bump-no, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 3c036de into OCA:12.0 Feb 20, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 0b4e741. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants