- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 348
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
[16.0][FIX] hr_timesheet_sheet: deal with time off line #712
[16.0][FIX] hr_timesheet_sheet: deal with time off line #712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested 👍
Also, technically correct.
A test would be great |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Functional review 👍
What kind of test are you thinking of? @alexey-pelykh |
@maisim a test that ensures the fix fixes the issue. And also, that way when I comment out the fix, it breaks - so that we don't rely on manual testing of this issue in future. |
We cannot install modules from tests: Our choices:
Can you tell me how you would proceed? In any case, if it looks like it will take too long to set up, I would like us to merge this and see about testing in another PR |
My vote is on merge it too. |
This PR has the |
Can we merge this? :) |
Not with that commit message: |
Fixes inability to add lines to timesheets that containstime off lines, managed from project_timesheet_holidays. Fixes OCA#711
b060cab
to
659eab8
Compare
@pedrobaeza @MiquelRForgeFlow |
/ocabot merge patch |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at fff331b. Thanks a lot for contributing to OCA. ❤️ |
Fix #711