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

[15.0][FIX] hr_holidays_public: wrong computation in case resources in different countries #47

Closed

Conversation

gurneyalex
Copy link
Member

Fix an issue in hr_holidays_public where
resource.calendar::_attendance_intervals_batch_exclude_public_holidays
would return a wrong result when called with multiple resources working
in different countries.

The original implementation uses a context key employee_id to find the
employee, which only allows this method to be called with a single
employee. The fix searches for the employees related to the resources
passed in arguments, and default to the context key if there are no
provided resources.

Also rework the implementation to use a python set rather than a list
for faster in test.

@pedrobaeza pedrobaeza changed the title [FIX] hr_holidays_public: wrong computation in case resources in different countries [15.0][FIX] hr_holidays_public: wrong computation in case resources in different countries Jun 16, 2022
@pedrobaeza
Copy link
Member

Can you add a regression test that exercises the problem you have found?

@pedrobaeza pedrobaeza added this to the 15.0 milestone Jun 16, 2022
@gurneyalex gurneyalex force-pushed the 15.0-fix_hr_holidays_public_issue branch from 43e18f8 to 0912bef Compare June 16, 2022 07:15
@gurneyalex
Copy link
Member Author

@pedrobaeza currently the test is in a customer-specific module, at a higher level. I'll addapt it for this module soon but I need to hotfix this in prod first ;)

@pedrobaeza
Copy link
Member

@gurneyalex please amend the PR without merge commits and add the regression test.

…erent countries

Fix an issue in hr_holidays_public where
resource.calendar::_attendance_intervals_batch_exclude_public_holidays
would return a wrong result when called with multiple resources working
in different countries.

The original implementation uses a context key employee_id to find the
employee, which only allows this method to be called with a single
employee. The fix searches for the employees related to the resources
passed in arguments, and default to the context key if there are no
provided resources.

Also rework the implementation to use a python set rather than a list
for faster in test.

[FIX] hr_holidays_public: provide defaults to process holidays
@gurneyalex gurneyalex force-pushed the 15.0-fix_hr_holidays_public_issue branch from ca5a107 to acf0c22 Compare March 10, 2023 08:53
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@gurneyalex
Copy link
Member Author

@pedrobaeza done :-)

@pedrobaeza
Copy link
Member

Thanks. I still see two commits (one being fixup). Please squash them, as the bot can't do it.

@github-actions
Copy link

github-actions bot commented Jul 9, 2023

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.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 9, 2023
Copy link

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Code LG

@pedrobaeza
Copy link
Member

@gurneyalex can you please squash your 2 commits into one? ocabot doesn't do it.

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 30, 2023
@dreispt
Copy link
Member

dreispt commented Nov 18, 2023

Superseded by #98

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.

8 participants