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

[17.0][ADD] project_timesheet_holidays_contract #1290

Open
wants to merge 1 commit into
base: 17.0
Choose a base branch
from

Conversation

yankinmax
Copy link

Module allows to take into account employee contract along with provided end date when timesheet on time off.

Copy link

@vvrossem vvrossem left a comment

Choose a reason for hiding this comment

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

pre-approving, some minor suggestions

@yankinmax yankinmax force-pushed the add-project_timesheet_holidays_contract branch from e6804d5 to fe6f507 Compare August 14, 2024 16:10
@yankinmax yankinmax force-pushed the add-project_timesheet_holidays_contract branch from fe6f507 to 4214bf0 Compare September 9, 2024 10:49
@yankinmax yankinmax force-pushed the add-project_timesheet_holidays_contract branch from 4214bf0 to 4c78005 Compare September 9, 2024 11:16
@yankinmax
Copy link
Author

@SilvioC2C PR is updated, thanks for the perfect suggestions.

Copy link

@SilvioC2C SilvioC2C left a comment

Choose a reason for hiding this comment

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

Could use some more tests, but overall LGTM

Thanks for your work!

@yankinmax yankinmax force-pushed the add-project_timesheet_holidays_contract branch from f2bfc8d to 81c88c6 Compare September 9, 2024 12:04
Copy link

@grindtildeath grindtildeath left a comment

Choose a reason for hiding this comment

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

Code looks ok but after reviewing it I'm still not sure what this module brings.

Comment on lines +31 to +32
Module allows to take into account employee contract along with provided
end date when timesheet on time off.

Choose a reason for hiding this comment

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

IMO you could develop a bit more how this works, because after reading it twice, I'm still not sure what the module does.

Comment on lines +39 to +44
# If only one contract, return it regardless of its state
if len(contracts) == 1:
return contracts[0].resource_calendar_id

# Filter out cancelled contracts if there are multiple contracts
valid_contracts = contracts.filtered(lambda c: c.state != "cancel")

Choose a reason for hiding this comment

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

So in case there's only one contract that is canceled, it still must be used? 🤨

self = self.with_context(date_to=to_datetime)

result = {}
for employee in self:

Choose a reason for hiding this comment

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

For all these functions, wouldn't grouping employees by calendar improve perf instead of iterating through all the employees?

@@ -0,0 +1,3 @@
from . import common
Copy link

Choose a reason for hiding this comment

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

Suggested change
from . import common

from odoo.tests import common


class ProjectTimesheetHolidaysContractCommon(common.TransactionCase):
Copy link

Choose a reason for hiding this comment

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

Suggested change
class ProjectTimesheetHolidaysContractCommon(common.TransactionCase):
class ProjectTimesheetHolidaysContractCommon(common.BaseCase):

To have all tracking disabled

@@ -0,0 +1 @@
Module allows to take into account employee contract along with provided end date when timesheet on time off.
Copy link

Choose a reason for hiding this comment

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

Can you elaborate here a bit?

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

Successfully merging this pull request may close these issues.

7 participants