-
-
Notifications
You must be signed in to change notification settings - Fork 118
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][MIG] hr_attendance_report_theoretical_time: Migration to version 17.0 #173
[17.0][MIG] hr_attendance_report_theoretical_time: Migration to version 17.0 #173
Conversation
32b0863
to
6f9fa21
Compare
ping @pedrobaeza |
/ocabot migration hr_attendance_report_theoretical_time |
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.
Functional review, LGTM.
Please squash administrative commits.
Check https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate for details.
from odoo.tests import common | ||
|
||
|
||
class TestHrAttendanceReportTheoreticalTimeBase(common.TransactionCase): |
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.
Please use BaseCommon
6f9fa21
to
a63a45f
Compare
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.
Squashing administrative is still pending, please check my comments before thanks :)
@classmethod | ||
def setUpClass(cls): | ||
super().setUpClass() | ||
cls.env = cls.env( |
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.
When using BaseCommon it is not necessary to add this.
<field name="res_model">recompute.theoretical.attendance</field> | ||
<field name="binding_model_id" ref="model_hr_employee" /> | ||
<field name="view_mode">form</field> | ||
<field name="context">{'default_employee_ids': id, 'employee_ids': id, }</field> |
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.
@pedrobaeza i am not sure if this is correct.
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.
Ok, I will wait for Pedro's response to this change before pushing again with the squashing of the administrative commits.
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.
No, it isn't. It's a x2m field, so it requires another nomenclature. Maybe [id]
, or [(4, id)]
, but it should be tried.
a63a45f
to
ecbfd8b
Compare
cls.env( | ||
context=dict( | ||
cls.env.context, | ||
mail_create_nolog=True, | ||
mail_create_nosubscribe=True, | ||
mail_notrack=True, | ||
no_reset_password=True, | ||
tracking_disable=True, | ||
) |
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.
This should be removed when using BaseCommon
ecbfd8b
to
94adca0
Compare
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 Thanks :)
Please remember to squash administrative commits
Theoretical vs Attended Time Analysis ===================================== This module adds a new report called "Theoretical vs Attended Time Analysis" that compares worked time, measured through attendances records, with the theoretical time, computed from employee's working calendar, public holidays and employee specific leaves. There is the possibility of counting as theoretical time some leave types if specified in them. Installation ============ On installation time, this module computes the theoretical hours for the day of the attendance check-in, so if you have a lot of records, this would be a bit slow. Configuration ============= You need to be at least "Attendance / Manual Attendance" for being able to see the attendances report. For including some leave types in the theoretical time, you have to: * Go to Leaves > Configuration. * Select leave type you want to include. * Check the mark "Include in theoretical hours". Usage ===== * Go to *Attendances > Reports > Theoretical vs Attended Time Analysis*. * Check pivot table or look at the graph view. Known issues / Roadmap ====================== * Employees with less than 1 week in the company will show full week theoretical hours. * Activate ORM cache for improving performance on computing theoretical hours, but assuring that the cache is cleared when the conditions of the computation changes. * If you change employee's working time, theoretical hours for non attended days will be computed according this new calendar. You have to define start and end dates inside the calendar for avoiding this side effect. * Recompute theoretical hours of affected days when changing the leave type to be included or not in theoretical time.
* Standard procedure * Adapt data model * Adapt views * Adapt code for date handling * Adapt tests [UPD] Update hr_attendance_report_theoretical_time.pot
…fault filter Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: hr-12.0/hr-12.0-hr_attendance_report_theoretical_time Translate-URL: https://translation.odoo-community.org/projects/hr-12-0/hr-12-0-hr_attendance_report_theoretical_time/
[UPD] Update hr_attendance_report_theoretical_time.pot hr_attendance_report_theoretical_time 12.0.1.1.0 Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: hr-12.0/hr-12.0-hr_attendance_report_theoretical_time Translate-URL: https://translation.odoo-community.org/projects/hr-12-0/hr-12-0-hr_attendance_report_theoretical_time/
Currently translated at 97.6% (41 of 42 strings) Translation: hr-12.0/hr-12.0-hr_attendance_report_theoretical_time Translate-URL: https://translation.odoo-community.org/projects/hr-12-0/hr-12-0-hr_attendance_report_theoretical_time/de/
- We add a wizard so the already existing attendances can be recomputed according to an employee calendar change, like a different for a set of dates that weren't took into consideration in advance, so the wrong theoretical hours are computed. [UPD] Update hr_attendance_report_theoretical_time.pot [UPD] README.rst hr_attendance_report_theoretical_time 12.0.1.2.0 Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: hr-12.0/hr-12.0-hr_attendance_report_theoretical_time Translate-URL: https://translation.odoo-community.org/projects/hr-12-0/hr-12-0-hr_attendance_report_theoretical_time/
- Although the minor permission to access the theoretical time report is "Manual Attendance", the menu wasn't accessible due to permissions. - Record rules also prevent such users from seeing other employees attendances. hr_attendance_report_theoretical_time 12.0.1.3.0
…rm related to employee public (user without employee groups)
94adca0
to
aa87e38
Compare
/> | ||
<!-- We need to leave the action empty for consistency because we are going | ||
to set different submenus, otherwise the Reports menu would not be | ||
displayed to a basic user. !--> | ||
<field name="action" eval="False" /> | ||
</record> | ||
<menuitem | ||
id="menu_hr_attendance_report" | ||
id="menu_hr_attendance_reporting" |
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.
I think this would not need to be changed.
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.
In the hr_attendance module, they have changed "report" to "reporting."
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.
That has nothing to do with this one, as this is a new XML-ID not linked with the main one. Please respect existing XML-IDs.
@@ -10,7 +10,7 @@ | |||
<field name="view_mode">form</field> | |||
<field | |||
name="context" | |||
>{'default_employee_ids': active_ids, 'employee_ids': active_ids, }</field> | |||
>{'default_employee_ids': [id], 'employee_ids': [id] }</field> |
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.
Please check this. If I try to use this action from an employee there is an error.
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.
I can't find any errors.
If you test it again, please include a screenshot and I'll look into whether I can fix it. Thanks Victor
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.
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.
Thank you, I was only able to get it to work by reverting to using active_ids, even though it will be deprecated in Odoo 18. Right now, [id] or [(4, id)] don't work.
Tests rely on removing tz from the current user and it isn't possible anymore after odoo/odoo@d6f2437#diff-38469def2f870bb866f971f57797dd7c21b6a95d52a8eae72f832f0eea2434f9R105
Currently translated at 100.0% (53 of 53 strings) Translation: hr-attendance-15.0/hr-attendance-15.0-hr_attendance_report_theoretical_time Translate-URL: https://translation.odoo-community.org/projects/hr-attendance-15-0/hr-attendance-15-0-hr_attendance_report_theoretical_time/it/
…cover different use cases.
Currently translated at 100.0% (53 of 53 strings) Translation: hr-attendance-16.0/hr-attendance-16.0-hr_attendance_report_theoretical_time Translate-URL: https://translation.odoo-community.org/projects/hr-attendance-16-0/hr-attendance-16-0-hr_attendance_report_theoretical_time/es/
… a basic user We need to leave the action empty for consistency because we are going to set different submenus, otherwise the Reports menu would not be displayed to a basic user. TT44849
Currently translated at 47.1% (25 of 53 strings) Translation: hr-attendance-16.0/hr-attendance-16.0-hr_attendance_report_theoretical_time Translate-URL: https://translation.odoo-community.org/projects/hr-attendance-16-0/hr-attendance-16-0-hr_attendance_report_theoretical_time/fr/
4b37c66
to
288660f
Compare
All changes have been made and the administrative commits are squashed |
288660f
to
864202d
Compare
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.
Functional review OK
This PR has the |
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.
/ocabot merge nobump
On my way to merge this fine PR! |
Congratulations, your PR was merged at a45170b. Thanks a lot for contributing to OCA. ❤️ |
@Tecnativa TT50116