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

[12.0][ADD] hr_timesheet_holiday_analysis: timesheet entries by Is-Leave attribute #168

Conversation

alexey-pelykh
Copy link
Contributor

@alexey-pelykh alexey-pelykh commented Jan 3, 2019

This module allows to filter and group timesheet entries by when-on-leave attribute.

@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-hr_timesheet_holiday_analysis branch from 10b4ad2 to 3b7267e Compare January 3, 2019 03:10
@alexey-pelykh
Copy link
Contributor Author

@nikul-serpentcs @sswapnesh @aheficent I think this module would be of interest for you

@pedrobaeza
Copy link
Member

I don't like the approach of creating fake timesheet entries on leaves, and that's why I developed the module for real theoretical time https://github.com/OCA/hr/tree/10.0/hr_attendance_report_theoretical_time, and compare with the proper data, which is attendances, not timesheet entries.

@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza got your point, this module is an extension to https://github.com/odoo/odoo/tree/12.0/addons/project_timesheet_holidays for those who use it.

Copy link
Contributor

@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.

👍 Functional review. It works fine. Just one thing. As this is not a report maybe it is better to use a different word than "analysis" but it is not blocking.

@alexey-pelykh
Copy link
Contributor Author

Thanks! I've used "analysis" instead of "report" word in module name for the same reason, in a similar way as hr_timesheet_analysis module, where it adds pivot/graph views, and latter one was named like that as per @pedrobaeza, thus I think his opinion would be great on this question would be relevant.


@api.multi
@api.depends('holiday_id')
def _compute_is_leave(self): # pragma: no cover
Copy link
Member

Choose a reason for hiding this comment

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

This is cheating!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no, yet it's better to cover this with test

@pedrobaeza
Copy link
Member

This module has nothing to do with a BI view, but it's a bunch of extra filters + a field. Sincerely, I don't think this deserves a module.

Even more: the computed field is totally unneeded, as you can query the same (or at least create a filter with "holiday_id" > "is set" - ('holiday_id', '!=', False)).

👎 to the whole module.

@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza apart of being UX-targeted, this module serves as extension to #165 for the Split-By feature. Additionally, I'm open to hear your thoughts how to add more value to this module

@pedrobaeza
Copy link
Member

As said, you don't need this compute field. You can put directly the domain in the other module. Include there a filter in the search view for this condition instead.

@alexey-pelykh
Copy link
Contributor Author

I'm almost sure that we're not on the same page here, since report generator mentioned does not have search view, yet it supports splitting report (A/B reporting) by a boolean fields...

@pedrobaeza
Copy link
Member

Well, not really. I'm talking about adding that extra feature in that module, but the report generation can be led as said with the domain instead of an extra compute field. You don't need the field for doing the split.

@alexey-pelykh
Copy link
Contributor Author

Improved #165

@alexey-pelykh alexey-pelykh deleted the 12.0-add-hr_timesheet_holiday_analysis branch February 3, 2019 08:06
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.

None yet

3 participants