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

[12.0][ADD] hr_timesheet_task_domain: task domain depends on project #180

Merged
merged 1 commit into from Jun 5, 2019
Merged

[12.0][ADD] hr_timesheet_task_domain: task domain depends on project #180

merged 1 commit into from Jun 5, 2019

Conversation

alexey-pelykh
Copy link
Contributor

A complimentary module to #139 in order to provide a seamless substitute for OCA/project#447

This was referenced Feb 12, 2019
Closed
Closed
lmignon
lmignon suggested changes Apr 26, 2019
View reviewed changes
Copy link
Sponsor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

@alexey-pelykh I'm not a fan of this kind of code where we ckeck if an addon is installed to constrain in a way or an other the logic. I personally prefer glue addons. Nevertheless I'll not block this PR.

What do you think about the proposed optimization?

@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-hr_timesheet_task_domain branch from abb1260 to 10444d0 Compare April 26, 2019 07:31
@alexey-pelykh
Copy link
Contributor Author

@lmignon thanks for your feedback! Likewise, I don't like duck-typing or duck-programming, yet this particular case this is a trade-off, since a separate module to add 2 lines of check would be quite significant overkill, IMO

lmignon
lmignon approved these changes Apr 26, 2019
View reviewed changes
Copy link
Sponsor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

Thank you @alexey-pelykh LGTM (code review only)

@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-hr_timesheet_task_domain branch 2 times, most recently from 5f79abf to b45541a Compare April 26, 2019 07:50
pedrobaeza
pedrobaeza reviewed Jun 4, 2019
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

The flow we have in project_timesheet_time_control is that all the tasks are shown if no project selected, and only opened ones if a project is selected.

hr_timesheet_task_domain/README.rst Outdated Show resolved Hide resolved
@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-hr_timesheet_task_domain branch from b45541a to ae03d9a Compare June 5, 2019 04:14
@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza right, this module has the same approach and is part of #139 #179 and OCA/project#474

@pedrobaeza pedrobaeza added this to the 12.0 milestone Jun 5, 2019
@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). 🤖

@pedrobaeza pedrobaeza merged commit b9d71c6 into OCA:12.0 Jun 5, 2019
@alexey-pelykh alexey-pelykh deleted the 12.0-add-hr_timesheet_task_domain branch June 5, 2019 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmignon lmignon lmignon approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
approved ready to merge
Projects
None yet
Milestone
12.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alexey-pelykh @OCA-git-bot @lmignon @pedrobaeza