[9.0][ADD] hr_holiday_notify_employee_manager #331

LoisRForgeFlow · 2017-04-13T13:42:30Z

HR Holiday Notify Employee Manager

This module extends the functionality of the Leaves App to make possible
to notify by mail the manager of the employee requesting the leave.

Configuration

To configure this module, you need to:

#. Go to Settings > Users > Companies.
#. In the company form go to the Configuration tab and in the Leaves
Management section.
#. Check Leave Requests notified to employee's manager box.

Usage

To use this module, you need to:

#. Go to Leaves > My Leaves > Leaves Request and create a leave request to be
approved.
#. Your manager will be notified by email.

…anager

JordiBForgeFlow

👍 Technical and functional review

jcdrubay

Nice 👍

jcdrubay · 2017-04-24T10:41:28Z

hr_holiday_notify_employee_manager/models/hr_holidays.py

+
+    def _get_approvers_to_notify(self):
+        """Defines who to notify."""
+        company = self.env['res.company']._company_default_get('hr.holidays')


Should we call directly self.env['res.users']._get_company()?

Or without 'hr.holidays' which is ignored anyway company = self.env['res.company']._company_default_get()

what about: self.env.user.company_id?

Or even better:
self.employee_id.company_id

Because what is needed is the configuration for the company of that employee.

Yes, that's the best

jcdrubay · 2017-04-24T10:43:30Z

hr_holiday_notify_employee_manager/models/hr_holidays.py

+    def _get_approvers_to_notify(self):
+        """Defines who to notify."""
+        company = self.env['res.company']._company_default_get('hr.holidays')
+        if company.leave_notify_manager and self.employee_id.parent_id:


Could we notify someone by default? in case we forget to set the manager?
Normally, there is only one case of person without manager and he might not input his leave request...

This could be another setting in the company form.

what do you mean? by default it notifies the followers of the department, which probably include the department's manager. I don't know if that covers your case... 😕

I did not know that. Yes it does cover my case. Let's just hope that each department as at least one follower.

Yes, also if you want you can check the first version of this module (79f8095) in which we had more options in who to notify.

jcdrubay · 2017-04-24T10:46:09Z

hr_holiday_notify_employee_manager/models/hr_holidays.py

+        approvers = self._get_approvers_to_notify()
+        if not approvers:
+            return True
+        for aprover in approvers:


aprover > approver
:)

moylop260 · 2017-04-28T14:54:05Z

hr_holiday_notify_employee_manager/models/hr_holidays.py

+    _inherit = 'hr.holidays'
+
+    def _get_approvers_to_notify(self):
+        """Defines who to notify."""


Adds ensure_one and @api.multi decorator

moylop260 · 2017-04-28T14:55:13Z

hr_holiday_notify_employee_manager/models/hr_holidays.py

+        company = self.employee_id.company_id
+        if company.leave_notify_manager and self.employee_id.parent_id:
+            return self.employee_id.parent_id
+        else:


no-else-return
Use directly return False without else

moylop260 · 2017-04-28T14:55:50Z

hr_holiday_notify_employee_manager/models/hr_holidays.py

+    @api.model
+    def create(self, vals):
+        res = super(HrHolidays, self).create(vals)
+        res._notify_approvers()


Use a context in order to avoid notify if you are importing information

can you provide a example on how to do it correctly?

moylop260

Review lints from travis.

E.g. api.one is seudo-deprecated: https://github.com/odoo/odoo/blob/a9da6e9f3f73674df3354e5e6d8f354e8b26eecb/openerp/api.py#L405
(Better use api.multi plus ensure_one)

LoisRForgeFlow · 2017-04-28T15:04:02Z

@moylop260 Thanks for your review, I'm attending it.

elicoidal · 2017-04-30T02:00:15Z

hr_holiday_notify_employee_manager/README.rst

+HR Holiday Notify Employee Manager
+==================================
+
+This module extends the functionality of the Leaves App to make possible


I am wondering the difference with the standard notification/follower system. I would swear that if you are a follower of an employee as manager you receive their leaves/expenses notifications

In any case, maybe you can explicit the difference here with the standard notification system

That is not true. You would become a follower of the employee's leave if you are following his/her department and you would receive a email notification depending on how you've configured yourself as follower of the department. If you are just follower of the employe and/or his/her manager you will neither follow the leave or be notified.

This said, a good use case for this module would be a company with the following requirements:

There are several followers by department.

Not all of the followers of the department can approve the leaves, but just the ones that are explicitly managers of the employee requesting the leave.

In this case you will have to configure the system in the following way:

Configure all the followers of the department to not receive email notifications on leaves requests.

Check the box Leave Requests notified to employee's manager added by this module.

LoisRForgeFlow · 2017-06-09T14:18:55Z

Hello @moylop260

Can you refresh your review 😬? Thanks!

moylop260 · 2017-06-09T14:36:17Z

Thank you!

…of the employee requesting the leave. (OCA#331)

Fix _onchange_employee() arguments OCA Transbot updated translations from Transifex OCA Transbot updated translations from Transifex [fix] `hr_employee_id` use deprecated `sequence.get_id()` (OCA#358) [add] hr_employee_address_improved (OCA#357) [ADD] hr_employee_category_parent (OCA#366) * [ADD] hr_employee_category_parent * [IMP] human readable name [UPD] addons table in README.md [ADD] setup.py [ADD] hr_holiday_notify_employee_manager: Notify by mail the manager of the employee requesting the leave. (OCA#331) OCA Transbot updated translations from Transifex [MIG] hr_holidays_notify_employee_manager: Migration to 10.0 [10.0][ADD] hr_holidays_settings [FIX] add company_id in view hr_worked_days_from_timesheet OCA Transbot updated translations from Transifex [ADD] hr_worked_days_from_timesheet v10 [IMP] Improved code [IMP] Improved code [MIG] hr_employee_legacy_id [FIX] update header [UPD] addons table in README.md [ADD] setup.py [MIG] 10.0 porting hr_skill [UPD] Move Skills menu into HR configuration and add widget m2m_tags in employee form view and add to contributors list [UPD] Update copyright [IMP] Add unit test to test nam_get function on hr_skill Refactor code for pep-8 Add group to Skill menu Fix imports & lost things in pull/300 [UPD] addons table in README.md [ADD] setup.py [MIG][9.0] hr_expense_analytic_plans module OCA Transbot updated translations from Transifex [MIG] hr_expense_analytic_distribution: Migration to 10.0 [REM] hr_expense_analytic_plans: Replaced by hr_expense_analytic_distribution [UPD] addons table in README.md [ADD] setup.py [FIX] Fix issue with 2 invoices on the same partner and the same total amount (OCA#237) On the same expense, when we have 2 or more lines with different invoices, and each invoices have the same total amount, reconcile is not possible. The fix is to exclude reconcile account.move.line, and the first time if we have more than one line to reconcile on the same amount, we keep the first. OCA Transbot updated translations from Transifex [MIG] hr_expense_invoice: Migration to 10.0 [UPD] addons table in README.md [ADD] setup.py Update view in holidays status, to remove duplicated field on company_id. (OCA#405) [FIX][hr_holidays_settings] Make 'Leave Types' view accessible (OCA#408) [MIG] hr_holidays_meeting_name (OCA#334) * Add hr_holidays_meeting_name module * Typo * [MIG] hr_holidays_meeting_name * [CHG] According upstream changes. * [CHG] Rewrite hr_holidays_meeting_name witout Odoo patch. Add tests. * [CHG] Ignore .eggs directory. [UPD] addons table in README.md OCA Transbot updated translations from Transifex [FIX] hr_employee_seniority permission problem normally hr.contract are only readable by Employee / Officer and Employee / Managers, not basic employees => we need compute_sudo to compute the employee's seniority otherwise the employee cannot display his own record. [FIX] employee seniority tests [UPD] addons table in README.md [FIX] holiday meeting name tests Change the date to avoid conflicts with the demo data of hr_holidays. [IMP] hr_public_holidays: Improve README [FIX] employee seniority search crash [imp] hr_employee_id: ease override w/ custom ID [UPD] addons table in README.md

[BSSFL-452] Import and process RMA sent

…of the employee requesting the leave. (OCA#331)

forecast updates

LoisRForgeFlow added 2 commits April 13, 2017 15:41

[9.0][ADD] hr_holiday_notify_approver

79f8095

[9.0][REW] hr_holiday_notify_approver: simplify, just manager option.

7d86e66

LoisRForgeFlow force-pushed the 9.0-add-hr_holidays_notify_approvers branch 2 times, most recently from e2cc220 to f43bf00 Compare April 24, 2017 09:40

LoisRForgeFlow changed the title ~~[9.0][ADD] hr_holiday_notify_approver~~ [9.0][ADD] hr_holiday_notify_employee_manager Apr 24, 2017

[9.0][REN] hr_holiday_notify_approver -> hr_holiday_notify_employee_m…

8fe8840

…anager

LoisRForgeFlow force-pushed the 9.0-add-hr_holidays_notify_approvers branch from f43bf00 to 8fe8840 Compare April 24, 2017 09:54

JordiBForgeFlow approved these changes Apr 24, 2017

View reviewed changes

jcdrubay suggested changes Apr 24, 2017

View reviewed changes

LoisRForgeFlow added 2 commits April 28, 2017 13:58

[9.0][FIX] several fixes

cfb89bd

[9.0][IMP] Add tests

20dd871

LoisRForgeFlow force-pushed the 9.0-add-hr_holidays_notify_approvers branch from f79866f to 20dd871 Compare April 28, 2017 14:47

jcdrubay approved these changes Apr 28, 2017

View reviewed changes

moylop260 reviewed Apr 28, 2017

View reviewed changes

moylop260 requested changes Apr 28, 2017

View reviewed changes

ensure_one + no-else-return

e04bb2d

LoisRForgeFlow force-pushed the 9.0-add-hr_holidays_notify_approvers branch from cc5bccc to e04bb2d Compare April 28, 2017 15:01

elicoidal approved these changes Apr 30, 2017

View reviewed changes

JordiBForgeFlow added a commit to ForgeFlow/ao-odoo-buildout that referenced this pull request Jun 8, 2017

[imp] add OCA/purchase-workflow#381, OCA/hr#331

5c66121

moylop260 approved these changes Jun 9, 2017

View reviewed changes

moylop260 merged commit bb7a31d into OCA:9.0 Jun 9, 2017

LoisRForgeFlow deleted the 9.0-add-hr_holidays_notify_approvers branch June 12, 2017 08:57

AaronHForgeFlow pushed a commit to ForgeFlow/hr that referenced this pull request Aug 1, 2017

[ADD] hr_holiday_notify_employee_manager: Notify by mail the manager …

7182591

…of the employee requesting the leave. (OCA#331)

MiquelRForgeFlow pushed a commit to ForgeFlow/hr that referenced this pull request Aug 22, 2017

[ADD] hr_holiday_notify_employee_manager: Notify by mail the manager …

85421b8

…of the employee requesting the leave. (OCA#331)

MiquelRForgeFlow pushed a commit to ForgeFlow/hr that referenced this pull request Sep 20, 2017

[ADD] hr_holiday_notify_employee_manager: Notify by mail the manager …

5b672a9

…of the employee requesting the leave. (OCA#331)

feketemihai pushed a commit to feketemihai/hr that referenced this pull request Nov 14, 2017

[ADD] hr_holiday_notify_employee_manager: Notify by mail the manager …

9a91ebe

…of the employee requesting the leave. (OCA#331)

nikul-serpentcs pushed a commit to nikul-serpentcs/hr that referenced this pull request Jan 20, 2018

[ADD] hr_holiday_notify_employee_manager: Notify by mail the manager …

5361937

…of the employee requesting the leave. (OCA#331)

sambarros pushed a commit to sambarros/hr that referenced this pull request Jul 26, 2018

Merge pull request OCA#331 from TelmoSenseFly/BSSFL-452

02b4959

[BSSFL-452] Import and process RMA sent

i-vyshnevska pushed a commit to i-vyshnevska/hr that referenced this pull request Mar 5, 2019

[ADD] hr_holiday_notify_employee_manager: Notify by mail the manager …

ff1500a

…of the employee requesting the leave. (OCA#331)

i-vyshnevska pushed a commit to i-vyshnevska/hr that referenced this pull request Mar 12, 2019

[ADD] hr_holiday_notify_employee_manager: Notify by mail the manager …

b2d52bb

…of the employee requesting the leave. (OCA#331)

quentingigon pushed a commit to quentingigon/hr that referenced this pull request Oct 11, 2019

[ADD] hr_holiday_notify_employee_manager: Notify by mail the manager …

e14cfe8

…of the employee requesting the leave. (OCA#331)

Mraimou pushed a commit to camptocamp/hr that referenced this pull request Nov 25, 2019

Merge pull request OCA#331 from Mraimou/bso_sales_forecast_fix

26d11b6

forecast updates

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[9.0][ADD] hr_holiday_notify_employee_manager #331

[9.0][ADD] hr_holiday_notify_employee_manager #331

LoisRForgeFlow commented Apr 13, 2017 •

edited

Loading

JordiBForgeFlow left a comment

jcdrubay left a comment

jcdrubay Apr 24, 2017

LoisRForgeFlow Apr 28, 2017

jcdrubay Apr 28, 2017

LoisRForgeFlow Apr 28, 2017

jcdrubay Apr 24, 2017

LoisRForgeFlow Apr 28, 2017

jcdrubay Apr 28, 2017

LoisRForgeFlow Apr 28, 2017

jcdrubay Apr 24, 2017

moylop260 Apr 28, 2017

moylop260 Apr 28, 2017

moylop260 Apr 28, 2017

LoisRForgeFlow Apr 28, 2017

moylop260 left a comment

LoisRForgeFlow commented Apr 28, 2017

elicoidal Apr 30, 2017

elicoidal Apr 30, 2017

LoisRForgeFlow May 2, 2017

LoisRForgeFlow May 2, 2017

LoisRForgeFlow commented Jun 9, 2017

moylop260 commented Jun 9, 2017

[9.0][ADD] hr_holiday_notify_employee_manager #331

[9.0][ADD] hr_holiday_notify_employee_manager #331

Conversation

LoisRForgeFlow commented Apr 13, 2017 • edited Loading

HR Holiday Notify Employee Manager

Configuration

Usage

JordiBForgeFlow left a comment

Choose a reason for hiding this comment

jcdrubay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moylop260 left a comment

Choose a reason for hiding this comment

LoisRForgeFlow commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LoisRForgeFlow commented Jun 9, 2017

moylop260 commented Jun 9, 2017

LoisRForgeFlow commented Apr 13, 2017 •

edited

Loading