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][IMP] allow different workday lengths #323

Merged

Conversation

huguesdk
Copy link
Member

description

  • allow contracts with different workday lengths for different employees of the same company.
  • use only the resource.calendar of the contracts to define the length of a workday, instead of using the resource.calendar of the company.
  • improve compatibility with hr_holidays.
  • test and fix access rights.

odoo task

t12627

* allow contracts with different workday lengths for different employees
  of the same company.
* use only the resource.calendar of the contracts to define the length
  of a workday, instead of using the resource.calendar of the company.
* improve compatibility with hr_holidays.
* test and fix access rights.
@huguesdk huguesdk force-pushed the 12.0-imp-allow_different_resource_calendar_day_lengths branch from 4ca25c1 to 07aa5f0 Compare August 27, 2024 08:02
Copy link
Member

@carmenbianca carmenbianca left a comment

Choose a reason for hiding this comment

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

Some comments and change requests. I'm not 100% sure I grok the implementation, but I understand the tests, so that's good.

Comment on lines +25 to +26
all_attendances = self.attendance_ids
if not all_attendances:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
all_attendances = self.attendance_ids
if not all_attendances:
self.ensure_one()
all_attendances = self.attendance_ids
if not all_attendances:

Copy link
Member Author

Choose a reason for hiding this comment

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

how do you decide whether to call self.ensure_one() or not? because this could be used everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

I have frankly no idea.

resource_work_time_from_contracts/models/resource_mixin.py Outdated Show resolved Hide resolved
resource_work_time_from_contracts/models/resource_mixin.py Outdated Show resolved Hide resolved
resource_work_time_from_contracts/models/resource_mixin.py Outdated Show resolved Hide resolved
By splitting this method up, it will be easier for me to do some
extension work elsewhere.

Signed-off-by: Carmen Bianca BAKKER <[email protected]>
Copy link
Member

@carmenbianca carmenbianca left a comment

Choose a reason for hiding this comment

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

Did a second pass. Some comments.

@huguesdk huguesdk force-pushed the 12.0-imp-allow_different_resource_calendar_day_lengths branch 2 times, most recently from 06ecfc4 to 010e92a Compare September 10, 2024 08:32
Copy link
Member

@carmenbianca carmenbianca left a comment

Choose a reason for hiding this comment

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

LGTM, amazing work :)

@huguesdk huguesdk force-pushed the 12.0-imp-allow_different_resource_calendar_day_lengths branch from a6516d0 to 7338dbd Compare October 9, 2024 13:00
@huguesdk
Copy link
Member Author

huguesdk commented Oct 9, 2024

/ocabot merge major

@github-grap-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 12.0-ocabot-merge-pr-323-by-huguesdk-bump-major, awaiting test results.

@github-grap-bot github-grap-bot merged commit 387a23d into 12.0 Oct 9, 2024
2 checks passed
@github-grap-bot github-grap-bot deleted the 12.0-imp-allow_different_resource_calendar_day_lengths branch October 9, 2024 13:16
@github-grap-bot
Copy link
Contributor

Congratulations, your PR was merged at cfddd49. Thanks a lot for contributing to coopiteasy. ❤️

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.

3 participants