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] hr_leave_dashboard_extra_time #177

Open
wants to merge 5 commits into
base: 17.0
Choose a base branch
from

Conversation

xaviedoanhduy
Copy link

@xaviedoanhduy xaviedoanhduy commented Aug 12, 2024

The original development of this module has been done by Mint System.
It can be found in: https://github.com/Mint-System/Odoo-Apps-HR/blob/15.0/hr_attendance_overtime_negative_hours
This module has been ported to the OCA with their agreement, cc @janikvonrotz

  1. this PR add a new module to branch 17.0: hr_leave_dashboard_extra_time
  2. it is based upon the migration of a 15.0 module hr_attendance_overtime_negative_hours .
  3. in v15, the module made sense because if the employee had some Negative Hours, the Extra Hours card would not appear in the Time Off dashboard
  4. in v17, a module is still needed to ensure the Extra Hours card would appear, but for slightly different reasons: this Extra Hours card will never appear anyway because the Extra Hours leave type has requires_allocation == no more detail in here
  5. we renamed the module from hr_attendance_overtime_negative_hours to hr_leave_dashboard_extra_time to reflect that change

Results

  • before install module
    image

  • after
    image

@xaviedoanhduy xaviedoanhduy marked this pull request as ready for review August 12, 2024 09:57
@xaviedoanhduy xaviedoanhduy marked this pull request as draft August 12, 2024 09:59
@xaviedoanhduy xaviedoanhduy changed the title [17.0][ADD] hr_attendance_overtime_negative_hours [17.0][ADD] hr_leave_dashboard_extra_hours Aug 12, 2024
Copy link

@leemannd leemannd left a comment

Choose a reason for hiding this comment

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

Hello, could you add unit tests?

Copy link

@cyrilmanuel cyrilmanuel left a comment

Choose a reason for hiding this comment

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

Not an expert but lgtm

@xaviedoanhduy xaviedoanhduy marked this pull request as ready for review August 13, 2024 08:50
@pedrobaeza
Copy link
Member

@xaviedoanhduy xaviedoanhduy force-pushed the 17.0-mig-hr_attendance_overtime_negative_hours branch 2 times, most recently from 750b551 to eb12777 Compare August 13, 2024 11:59
@xaviedoanhduy
Copy link
Author

Please avoid plurals in module names: https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#11modules

Hello @pedrobaeza, the purpose of this module is to display an additional tab at the top of the Time Off Dashboard: Extra Hours, informing employees about overtime hours. For now we decided to keep this name consistent with the purpose of the module, if you have any ideas or suggestions please let us know.

@pedrobaeza
Copy link
Member

hr_leave_dashboard_overtime or hr_leave_dashboard_extra_time. You can always look for a mass noun or singular form noun.

@pedrobaeza pedrobaeza added this to the 17.0 milestone Aug 14, 2024
@xaviedoanhduy xaviedoanhduy force-pushed the 17.0-mig-hr_attendance_overtime_negative_hours branch from eb12777 to 3925389 Compare August 22, 2024 03:36
@xaviedoanhduy xaviedoanhduy changed the title [17.0][ADD] hr_leave_dashboard_extra_hours [17.0][ADD] hr_leave_dashboard_extra_time Aug 22, 2024
@xaviedoanhduy
Copy link
Author

hello @pedrobaeza , I have renamed the module name to hr_leave_dashboard_extra_time. Thanks for your suggestions

@xaviedoanhduy xaviedoanhduy force-pushed the 17.0-mig-hr_attendance_overtime_negative_hours branch from 3925389 to 96754c9 Compare August 22, 2024 03:50
@xaviedoanhduy xaviedoanhduy force-pushed the 17.0-mig-hr_attendance_overtime_negative_hours branch from 96754c9 to 3257f11 Compare August 22, 2024 03:53
@pedrobaeza
Copy link
Member

OK, but you should do it in the same commit. And another question is that you are not the original author of this, so Mint must authorize this move to OCA and their contributor should sign the OCA CLA.

@leemannd
Copy link

Hello @pedrobaeza Thank you for taking care of these formal points.
This is done after we received a written agreement from the Mint company. From my knowledge the author @janikvonrotz has already been contributing on the OCA, so he should have signed the CLA.

@janikvonrotz Could you give us a review on this PR. Could you confirm that you have signed the OCA CLA?

@janikvonrotz
Copy link

@pedrobaeza I have signed the CLA and authorized this move with @leemannd

Copy link

@janikvonrotz janikvonrotz left a comment

Choose a reason for hiding this comment

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

The module seems to do much more than the original module.

I was able to install the module and run the tests.

Then I tried to verify its functionality, but failed.

I got 2 extra hours:

image

But they were not shown on the dashboard:

image

A lot has changed in attendance and holidays, I am not up-to-date and so have missed to important changes.

Update 1

When I create and validate a leave of type extra hours, this is what's shown:

image

I have to admit, I don't really understand why it says 0.13 hours.

@leemannd Do you have test instructions to verify the functionality of the modul?

At Mint System we have this kind of standard: https://wiki.mint-system.ch/odoo-module-test-instructions.html

Choose a reason for hiding this comment

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

You can remove this image. Its our default background image for the Odoo app store.

@leemannd
Copy link

Hello @janikvonrotz , Thank you for your quick feedback.
I haven't been working actively on it. I'm checking internally and we will come back to you in this PR.

@pedrobaeza
Copy link
Member

Thanks for confirming about the grant.

@nilshamerlinck
Copy link
Contributor

nilshamerlinck commented Aug 22, 2024

Hi @janikvonrotz probably 1 hour taken = 0.125 day, rounded up to 0.13; should be expressed in hours, looking into it

@xaviedoanhduy
Copy link
Author

Hello @janikvonrotz, the reason why the Extra Hours amount is incorrectly expressed in days instead of hours is an issue in odoo itself, I just pushed this PR to fix it: github.com/odoo/odoo/pull/17756

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