-
Notifications
You must be signed in to change notification settings - Fork 10
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
[MIG] hr_infraction: migrate to 14.0 #37
base: 14.0
Are you sure you want to change the base?
Conversation
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'm impressed, Abel. Good job!
"author": "TREVI Software, Michael Telahun Makonnen", | ||
"license": "AGPL-3", | ||
"website": "https://github.com/trevi-software/trevi-hr", | ||
"depends": ["hr", "hr_employee_status", "hr_job_transfer", "group_payroll_manager"], |
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.
why do you need group_payroll_manager? All it does is create Payroll Manager group.
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.
the group "group_payroll_manager" is associated with "ACL" defined in "hr_infraction" module.
access_hr_infraction_category_employee,access_hr_infraction_category,hr_infraction.model_hr_infraction_category,base.group_user,1,0,0,0 | ||
access_hr_infraction_category_user,access_hr_infraction_category,hr_infraction.model_hr_infraction_category,hr.group_hr_user,1,0,0,0 | ||
access_hr_infraction_category_manager,access_hr_infraction_category,hr_infraction.model_hr_infraction_category,hr.group_hr_manager,1,1,1,1 | ||
access_hr_infraction_pm,access_hr_infraction,hr_infraction.model_hr_infraction,group_payroll_manager.group_payroll_manager,1,0,0,0 |
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.
If there is any code that depends on the payroll module it (and related group permissions) should be moved to a separate module: hr_infraction_payroll
} | ||
).create_action() | ||
transfer = self.Transfer.search([], limit=1) | ||
self.assertFalse(len(transfer) == 0) |
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 is not a good assert. This assert would pass even if the transfer failed but the module's demo data contained a demo transfer. Instead include terms in the search domain that would uniquely identify the transfer you created.
inf_action = self.InfractionAction.search( | ||
[("infraction_id", "=", infraction.id)] | ||
) | ||
inf_waring = self.InfractionWarning.search( | ||
[("action_id", "=", inf_action.id)] | ||
) |
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.
Move this outside (above) the 'with' block.
inf_action = self.InfractionAction.search( | ||
[("infraction_id", "=", infraction.id)] | ||
) |
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.
Same here: move above block
|
||
|
||
class InfractionBatch(models.TransientModel): | ||
_name = "hr.infraction.batch" |
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.
Just one test validating this model works as expected would be nice.
inf_action = self.InfractionAction.search( | ||
[("infraction_id", "=", infraction.id)] | ||
) | ||
inf_waring = self.InfractionWarning.search( | ||
[("action_id", "=", inf_action.id)] | ||
) |
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.
Move this outside (above) the 'with' block.
See my comments further down: in that case it should be in a separate
module: hr_infraction_payroll
But also you shouldn't depend on group_payroll_manager module because
the Payroll Manager group is already provided by the OCA module
"payroll". You should depend on that instead. In fact I will be removing
the module "group_payroll_manager" in v. 15.
…On 20/05/2022 15:36, Abel Abera wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In hr_infraction/__manifest__.py
<#37 (comment)>:
> @@ -0,0 +1,23 @@
+# Copyright (C) 2021 Trevi Software (https://trevi.et)
+# Copyright (C) 2013 Michael Telahun ***@***.***>.
+# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).
+
+{
+ "name": "Employee Infraction Management",
+ "version": "14.0.1.0.0",
+ "category": "Human Resource",
+ "author": "TREVI Software, Michael Telahun Makonnen",
+ "license": "AGPL-3",
+ "website":"https://github.com/trevi-software/trevi-hr",
+ "depends": ["hr", "hr_employee_status", "hr_job_transfer", "group_payroll_manager"],
the group "group_payroll_manager" is associated with "ACL" defined in
"hr_infraction" module.
—
Reply to this email directly, view it on GitHub
<#37 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4YOHHB7BFWAPIUAHV2BZDVK6BN5ANCNFSM5WMT4TQQ>.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
/trevibot rebase |
Congratulations, PR rebased to 14.0. |
c497a1b
to
26214b4
Compare
No description provided.