-
Notifications
You must be signed in to change notification settings - Fork 117
[IMP] hr_skills: unifying certification reports #4902
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
base: master-hr-onboarding-haabo
Are you sure you want to change the base?
[IMP] hr_skills: unifying certification reports #4902
Conversation
|
This PR targets the un-managed branch odoo-dev/odoo:master-hr-onboarding-haabo, it needs to be retargeted before it can be merged. |
YassinWalid
left a comment
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.
Thanks for your work 🤩
I have one small suggestion though
|
|
||
| <record id="hr_employee_certification_report_action" model="ir.actions.act_window"> | ||
| <field name="name">Certification</field> | ||
| <field name="res_model">hr.employee.certification.report</field> | ||
| <field name="search_view_id" ref="hr_employee_certification_report_view_search"/> | ||
| <field name="view_mode">list,pivot</field> | ||
| <field name="context">{ | ||
| 'search_default_employee': 1, | ||
| }</field> | ||
| <field name="help" type="html"> | ||
| <p class="o_view_nocontent_empty_folder"> | ||
| </p><p> | ||
| This report will give you an overview of the certification per Employee. | ||
| Create them in configuration and add them on the Employee. | ||
| </p> | ||
| </field> | ||
| </record> |
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.
Since you removed the menu item that calls the action hr_employee_certification_report_action, and nobody else uses this action, the action should be deleted as well.
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.
Thanks! yes that makes sense. Updated.
c477e10 to
f03af86
Compare
|
Helloo 👋 |
YassinWalid
left a comment
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.
Thanks for the changes. I have one other comment though.
|
|
||
| <record id="hr_employee_certification_report_action" model="ir.actions.act_window"> | ||
| <field name="name">Certification</field> | ||
| <field name="res_model">hr.employee.certification.report</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.
Since this action was the only one calling the views of the model hr.employee.certification.report, I think we should also delete the model and its views.
(Also remember to extend the upgrade script to include those as well)
- Removed default group by from Learning > Certifications to be more generice - Removed certifications from Reporting > Skills to reduce redundancy task-5385053
f03af86 to
6c35e12
Compare
task-5385053