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.

Sign up for GitHub

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

Jump to bottom

[16.0][ADD] hr_timesheet_overtime_rate_begin_end #33

Open
wants to merge 9 commits into
base: 16.0
Choose a base branch
from
Open

[16.0][ADD] hr_timesheet_overtime_rate_begin_end #33

wants to merge 9 commits into from

Conversation

carmenbianca
Copy link
Member

@carmenbianca carmenbianca commented Jun 20, 2024 •
edited
Loading

Description

Compatibility module, depends on OCA/timesheet#692

Built on top of #41

Odoo task (if applicable)

T12629

Checklist before approval

  • Tests are present (or not needed).
  • Credits/copyright have been changed correctly.
  • (If a new module) Moving this to OCA has been considered.

@carmenbianca carmenbianca requested a review from huguesdk June 20, 2024 15:27
@carmenbianca
Copy link
Member Author

I think this correctly addresses the need in the internal task.

Backporting this to v12 may be a touch tricky because v12 doesn't use compute for unit_amount, but it's probably fine.

@carmenbianca carmenbianca mentioned this pull request Jun 21, 2024
Merged
huguesdk
huguesdk requested changes Jun 25, 2024
View reviewed changes
Copy link
Member

@huguesdk huguesdk left a comment

Choose a reason for hiding this comment

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

just some changes depending on my other reviews in OCA/timesheet, otherwise lgtm.

hr_timesheet_overtime_begin_end/models/account_analytic_line.py Outdated Show resolved Hide resolved
hr_timesheet_overtime_begin_end/models/account_analytic_line.py Outdated Show resolved Hide resolved
hr_timesheet_overtime_begin_end/tests/test_analytic_line.py Outdated Show resolved Hide resolved
@carmenbianca carmenbianca force-pushed the 16.0-overtime-improve-coefficient branch from 008bef9 to be05f31 Compare June 28, 2024 14:28
@carmenbianca carmenbianca mentioned this pull request Jul 4, 2024
Closed
@carmenbianca carmenbianca force-pushed the 16.0-overtime-improve-coefficient branch from be05f31 to 6f39f33 Compare July 4, 2024 08:37
@carmenbianca
Copy link
Member Author

Temporarily rebased on top of #37

@carmenbianca carmenbianca mentioned this pull request Jul 4, 2024
Open
4 tasks
@carmenbianca
Copy link
Member Author

Ugh this is terrible. I wrote a test that fails: test_rate_not_double_applied.

Problem:

The rate is applied twice. Once on the transient record that is shown to the user before it is actually created, and then again when the transient record is written to the database.

There are two solutions that I can see:

  1. Somehow add an exception in the create method such that the rate isn't doubly applied.
  2. Fix hr_timesheet_overtime so that it doesn't subtly override the values you give to write/create.

@carmenbianca carmenbianca mentioned this pull request Jul 4, 2024
Open
4 tasks
@carmenbianca
[ADD] hr_timesheet_overtime_rate: Factored out of hr_timesheet_overtime
9c6f4cb 
This functionality was completely unlinked from the rest of
hr_timesheet_overtime.

I would have done this in two commits, but then the files wouldn't move
over cleanly, losing history.

Signed-off-by: Carmen Bianca BAKKER <[email protected]>
@carmenbianca
[REM] hr_timesheet_overtime_rate: Make summary view read-only
907d466 
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
@carmenbianca
[FIX] hr_timesheet_overtime_rate: Populate hours_worked on installation
90d017e 
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
@carmenbianca carmenbianca force-pushed the 16.0-overtime-improve-coefficient branch from c09e734 to 7b15e21 Compare July 11, 2024 12:20
@carmenbianca carmenbianca changed the title [16.0][ADD] hr_timesheet_overtime_begin_end [16.0][ADD] hr_timesheet_overtime_rate_begin_end Jul 11, 2024
@carmenbianca
[REF] hr_timesheet_overtime_rate: Rate acquisition is no longer an @a…
22179d1 
…pi.model method

This makes it easier to extend with additional available information on
the record.

Signed-off-by: Carmen Bianca BAKKER <[email protected]>
@carmenbianca carmenbianca force-pushed the 16.0-overtime-improve-coefficient branch from 6d5fbbd to 8ead2d8 Compare August 29, 2024 08:16
huguesdk
huguesdk requested changes Sep 10, 2024
View reviewed changes
Copy link
Member

@huguesdk huguesdk left a comment

Choose a reason for hiding this comment

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

in the timesheet lines tree, the hours_worked field appears before time_start and time_stop, while it would be more logical if it would appear after them, between time_stop and unit_amount.

hr_timesheet_overtime_rate_begin_end/readme/DESCRIPTION.rst Outdated Show resolved Hide resolved
@carmenbianca
[IMP] hr_timesheet_overtime_rate: Improve code aesthetics
509de5b 
No functional differences; code is a little prettier.

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

carmenbianca commented Sep 16, 2024 •
edited
Loading

Fixed your field order remark in 2136f92, different PR #41

@carmenbianca
[IMP] hr_timesheet_overtime_rate: Decrease priority of views
dc3d3d1 
By decreasing the priority, we make sure that hours_worked is as close
to unit_amount as possible.

Signed-off-by: Carmen Bianca BAKKER <[email protected]>
@carmenbianca
[IMP] hr_timesheet_overtime_rate: Add hours_worked to task view
faf9def 
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
@carmenbianca carmenbianca force-pushed the 16.0-overtime-improve-coefficient branch 2 times, most recently from 234ea2e to b0b445e Compare September 16, 2024 14:09
huguesdk
huguesdk requested changes Oct 4, 2024
View reviewed changes
Copy link
Member

@huguesdk huguesdk left a comment

Choose a reason for hiding this comment

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

just 1 small thing to fix, otherwise lgtm.

hr_timesheet_overtime_rate_begin_end/models/account_analytic_line.py Outdated Show resolved Hide resolved
@carmenbianca
[ADD] hr_timesheet_overtime_begin_end
ace05fa 
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
@carmenbianca carmenbianca force-pushed the 16.0-overtime-improve-coefficient branch from b0b445e to ace05fa Compare October 11, 2024 08:46
@carmenbianca carmenbianca requested a review from huguesdk October 11, 2024 08:46
huguesdk
huguesdk reviewed Nov 18, 2024
View reviewed changes
Copy link
Member

@huguesdk huguesdk left a comment

Choose a reason for hiding this comment

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

oops, approved too fast? some tests are still failing. is this because this still depends on other changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huguesdk huguesdk huguesdk approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carmenbianca @huguesdk @github-grap-bot