[IMP] hr_holidays: Improve general UX (back2basics)

TaskID: 2048892

closes odoo/odoo#35541

Signed-off-by: Yannick Tivisse (yti) <yti@odoo.com>

5 files changed, 141 insertions, 27 deletions

diff --git a/addons/hr_holidays/models/hr_leave.py b/addons/hr_holidays/models/hr_leave.py
index 00488836eb5..a3bc928a46c 100644
--- a/addons/hr_holidays/models/hr_leave.py
+++ b/addons/hr_holidays/models/hr_leave.py
@@ -391,7 +391,8 @@ class HolidaysRequest(models.Model):
     @api.onchange('employee_id')
     def _onchange_employee_id(self):
         self._sync_employee_details()
-        self.holiday_status_id = False
+        if self.employee_id.user_id != self.env.user:
+            self.holiday_status_id = False
 
     @api.onchange('date_from', 'date_to', 'employee_id')
     def _onchange_leave_dates(self):
@@ -587,9 +588,9 @@ class HolidaysRequest(models.Model):
         if not self._context.get('leave_fast_create'):
             # FIXME remove these, as they should not be needed
             if employee_id:
-                holiday._sync_employee_details()
+                holiday.with_user(SUPERUSER_ID)._sync_employee_details()
             if 'number_of_days' not in values and ('date_from' in values or 'date_to' in values):
-                holiday._onchange_leave_dates()
+                holiday.with_user(SUPERUSER_ID)._onchange_leave_dates()
 
             # Everything that is done here must be done using sudo because we might
             # have different create and write rights
@@ -630,6 +631,11 @@ class HolidaysRequest(models.Model):
         # Allow an employee to always write his own out of office message
         if len(self) == 1 and values.keys() == {'out_of_office_message'} and self.employee_id.user_id == self.env.user:
             return super(HolidaysRequest, self.sudo()).write(values)
+        is_officer = self.env.user.has_group('hr_holidays.group_hr_holidays_user')
+
+        if not is_officer:
+            if any(hol.date_from.date() < fields.Date.today() for hol in self):
+                raise UserError(_('You cannot update a leave that already begun'))
 
         employee_id = values.get('employee_id', False)
         if not self.env.context.get('leave_fast_create') and values.get('state'):
@@ -647,8 +653,14 @@ class HolidaysRequest(models.Model):
         return result
 
     def unlink(self):
-        for holiday in self.filtered(lambda holiday: holiday.state not in ['draft', 'cancel', 'confirm']):
-            raise UserError(_('You cannot delete a time off which is in %s state.') % (holiday.state,))
+        error_message = _('You cannot delete a time off which is in %s state')
+
+        if not self.user_has_groups('hr_holidays.groups_hr_user'):
+            if any(hol.state != 'draft' for hol in self):
+                raise UserError(error_message % self[:1].state)
+        else:
+            for holiday in self.filtered(lambda holiday: holiday.state not in ['draft', 'cancel', 'confirm']):
+                raise UserError(_('You cannot delete a time off which is in %s state.') % (holiday.state,))
         return super(HolidaysRequest, self).unlink()
 
     def copy_data(self, default=None):
@@ -834,7 +846,7 @@ class HolidaysRequest(models.Model):
             # Post a second message, more verbose than the tracking message
             if holiday.employee_id.user_id:
                 holiday.message_post(
-                    body=_('Your %s planned on %s has been refused' % (holiday.holiday_status_id.display_name, holiday.date_from)),
+                    body=_('Your %s planned on %s has been refused') % (holiday.holiday_status_id.display_name, holiday.date_from),
                     partner_ids=holiday.employee_id.user_id.partner_id.ids)
 
         self._remove_resource_leave()
@@ -855,6 +867,10 @@ class HolidaysRequest(models.Model):
 
             if not is_manager and state != 'confirm':
                 if state == 'draft':
+                    if holiday.state == 'refuse':
+                        raise UserError(_('Only a Leave Manager can reset a refused leave.'))
+                    if holiday.date_from.date() <= fields.Date.today():
+                        raise UserError(_('Only a Leave Manager can reset a started leave.'))
                     if holiday.employee_id != current_employee:
                         raise UserError(_('Only a Leave Manager can reset other people leaves.'))
                 else:
diff --git a/addons/hr_holidays/tests/test_access_rights.py b/addons/hr_holidays/tests/test_access_rights.py
index 3986668f9f7..62197350437 100644
--- a/addons/hr_holidays/tests/test_access_rights.py
+++ b/addons/hr_holidays/tests/test_access_rights.py
@@ -175,6 +175,35 @@ class TestAcessRightsStates(TestLeavesRights):
             leave = self.request_leave(1, datetime.today() + relativedelta(days=5 + i), 1, values)
             leave.with_user(self.user_employee.id).action_draft()
 
+    def test_base_user_draft_refused_leave(self):
+        """
+            Should not be able to draft a refused leave
+        """
+        for i, status in enumerate(self.draft_status):
+            values = {
+                'name': 'Random Leave',
+                'employee_id': self.employee_emp.id,
+                'holiday_status_id': status.id,
+                'state': 'refuse',
+            }
+            leave = self.request_leave(1, datetime.today() + relativedelta(days=5 + i), 1, values)
+            with self.assertRaises(UserError):
+                leave.with_user(self.user_employee.id).action_draft()
+
+    def test_base_user_draft_current_leave(self):
+        """
+            Should not be able to draft a passed leave
+        """
+        for i, status in enumerate(self.draft_status):
+            values = {
+                'name': 'Random Leave',
+                'employee_id': self.employee_emp.id,
+                'holiday_status_id': status.id,
+            }
+            leave = self.request_leave(1, datetime.today() + relativedelta(days=-20 + i), 1, values)
+            with self.assertRaises(UserError):
+                leave.with_user(self.user_employee.id).action_draft()
+
     def test_holiday_user_draft_his_leave(self):
         """
             Should be able to draft his own leave
@@ -237,6 +266,35 @@ class TestAcessRightsStates(TestLeavesRights):
             leave = self.request_leave(1, datetime.today() + relativedelta(days=5 + i), 1, values)
             leave.with_user(self.user_hruser.id).action_draft()
 
+    def test_holiday_user_draft_refused_leave(self):
+        """
+            Should not be able to draft a refused leave
+        """
+        for i, status in enumerate(self.draft_status):
+            values = {
+                'name': 'Random Leave',
+                'employee_id': self.employee_hruser.id,
+                'holiday_status_id': status.id,
+                'state': 'refuse',
+            }
+            leave = self.request_leave(1, datetime.today() + relativedelta(days=5 + i), 1, values)
+            with self.assertRaises(UserError):
+                leave.with_user(self.user_hruser.id).action_draft()
+
+    def test_holiday_user_draft_current_leave(self):
+        """
+            Should not be able to draft a passed leave
+        """
+        for i, status in enumerate(self.draft_status):
+            values = {
+                'name': 'Random Leave',
+                'employee_id': self.employee_hruser.id,
+                'holiday_status_id': status.id,
+            }
+            leave = self.request_leave(1, datetime.today() + relativedelta(days=-20 + i), 1, values)
+            with self.assertRaises(UserError):
+                leave.with_user(self.user_hruser.id).action_draft()
+
     def test_holiday_manager_draft_his_leave(self):
         """
             The holiday manager should be able to do everything
@@ -291,6 +349,33 @@ class TestAcessRightsStates(TestLeavesRights):
             leave = self.request_leave(1, datetime.today() + relativedelta(days=5 + i), 1, values)
             leave.with_user(self.user_hrmanager.id).action_draft()
 
+    def test_holiday_manager_draft_refused_leave(self):
+        """
+            The holiday manager should be able to do everything
+        """
+        for i, status in enumerate(self.draft_status):
+            values = {
+                'name': 'Random Leave',
+                'employee_id': self.employee_hruser.id,
+                'holiday_status_id': status.id,
+                'state': 'refuse',
+            }
+            leave = self.request_leave(1, datetime.today() + relativedelta(days=5 + i), 1, values)
+            leave.with_user(self.user_hrmanager.id).action_draft()
+
+    def test_holiday_manager_draft_current_leave(self):
+        """
+            The holiday manager should be able to do everything
+        """
+        for i, status in enumerate(self.draft_status):
+            values = {
+                'name': 'Random Leave',
+                'employee_id': self.employee_hruser.id,
+                'holiday_status_id': status.id,
+            }
+            leave = self.request_leave(1, datetime.today() + relativedelta(days=-20 + i), 1, values)
+            leave.with_user(self.user_hrmanager.id).action_draft()
+
 @tests.tagged('access_rights', 'access_rights_create')
 class TestAccessRightsCreate(TestLeavesRights):
     @mute_logger('odoo.models.unlink', 'odoo.addons.mail.models.mail_mail')
diff --git a/addons/hr_holidays/views/hr_holidays_views.xml b/addons/hr_holidays/views/hr_holidays_views.xml
index d51a5404f25..45ea1cf7340 100644
--- a/addons/hr_holidays/views/hr_holidays_views.xml
+++ b/addons/hr_holidays/views/hr_holidays_views.xml
@@ -44,21 +44,18 @@
         id="menu_hr_holidays_approvals"
         name="Managers"
         parent="menu_hr_holidays_root"
-        sequence="3"
-        groups="hr_holidays.group_hr_holidays_manager,hr_holidays.group_hr_holidays_user,hr_holidays.group_hr_holidays_user"/>
+        sequence="3"/>
 
     <menuitem
         id="hr_holidays_menu_manager_approve"
         name="To Approve"
         parent="menu_hr_holidays_approvals"
-        groups="hr_holidays.group_hr_holidays_manager,hr_holidays.group_hr_holidays_user,hr_holidays.group_hr_holidays_user"
         sequence="1"/>
 
     <menuitem
         id="menu_open_department_leave_approve"
         name="Time Off"
         parent="hr_holidays_menu_manager_approve"
-        groups="hr_holidays.group_hr_holidays_manager,hr_holidays.group_hr_holidays_user,hr_holidays.group_hr_holidays_user"
         action="hr_leave_action_action_approve_department"
         sequence="1"/>
 
@@ -66,7 +63,6 @@
         id="hr_holidays_menu_manager_approve_allocations"
         name="Allocations"
         parent="hr_holidays_menu_manager_approve"
-        groups="hr_holidays.group_hr_holidays_manager,hr_holidays.group_hr_holidays_user"
         action="hr_leave_allocation_action_approve_department"
         sequence="2"/>
 
@@ -74,14 +70,14 @@
         id="hr_holidays_menu_manager_all"
         name="All"
         parent="menu_hr_holidays_approvals"
-        groups="hr_holidays.group_hr_holidays_manager,hr_holidays.group_hr_holidays_user,hr_holidays.group_hr_holidays_user"
+        groups="hr_holidays.group_hr_holidays_manager,hr_holidays.group_hr_holidays_user"
         sequence="2"/>
 
     <menuitem
         id="menu_open_employee_leave"
         name="Time Off"
         parent="hr_holidays_menu_manager_all"
-        groups="hr_holidays.group_hr_holidays_manager,hr_holidays.group_hr_holidays_user,hr_holidays.group_hr_holidays_user"
+        groups="hr_holidays.group_hr_holidays_manager,hr_holidays.group_hr_holidays_user"
         action="hr_leave_action_all"
         sequence="1"/>
 
@@ -130,6 +126,13 @@
         sequence="2"/>
 
     <menuitem
+        id="menu_hr_holidays_summary_all"
+        name="Time Off Summary"
+        parent="menu_hr_holidays_report"
+        action="act_hr_employee_holiday_request"
+        sequence="3"/>
+
+    <menuitem
         id="menu_hr_holidays_configuration"
         name="Configuration"
         parent="menu_hr_holidays_root"
diff --git a/addons/hr_holidays/views/hr_leave_views.xml b/addons/hr_holidays/views/hr_leave_views.xml
index 71d7ee4f090..aaa881c4f95 100644
--- a/addons/hr_holidays/views/hr_leave_views.xml
+++ b/addons/hr_holidays/views/hr_leave_views.xml
@@ -63,8 +63,7 @@
                 <filter string="Unread Messages" name="message_needaction" domain="[('message_needaction','=',True)]"/>
                 <separator/>
                 <filter string="My Department Time Off" name="department" domain="['|', ('department_id.member_ids.user_id', '=', uid), ('employee_id.user_id', '=', uid)]" help="My Department Time Off"/>
-                <filter string="People I Manage" name="managed_people" domain="[('employee_id.parent_id.user_id', '=', uid)]" help="Time off of people you are manager of"/>
-                <filter string="My Team Time Off" name="my_team_leaves" domain="[('employee_id.leave_manager_id', '=', uid)]" groups="hr_holidays.group_hr_holidays_user" help="Time off of Your Team Members"/>
+                <filter string="People I Manage" name="managed_people" domain="[('employee_id.leave_manager_id', '=', uid)]" help="Time off of people you are manager of"/>
                 <filter string="My Time Off" name="my_leaves" domain="[('employee_id.user_id', '=', uid)]"/>
                 <separator/>
                 <filter string="To Report in Payslip" name="gray" domain="[('payslip_status', '=', False)]" groups="hr_holidays.group_hr_holidays_user"/>
@@ -241,7 +240,7 @@
                                         'readonly': [('state', 'not in', ('draft', 'confirm'))],
                                         'invisible': ['|', ('request_unit_half', '=', True), ('request_unit_hours', '=', True)]
                                     }"/>
-                                <field name="request_date_from_period" class="oe_inline"
+                                <field name="request_date_from_period" class="oe_inline oe_edit_only"
                                     string="In"
                                     options="{'horizontal': True}"
                                     attrs="{
@@ -249,7 +248,7 @@
                                         'required': [('request_unit_half', '=', True)],
                                         'invisible': [('request_unit_half', '=', False)]}"/>
                             </div>
-                            <div class="o_row o_row_readonly">
+                            <div class="o_row o_row_readonly oe_edit_only">
                                 <label for="request_unit_half" attrs="{
                                     'readonly': [('state', 'not in', ('draft', 'confirm'))],
                                     'invisible': [('leave_type_request_unit', '=', 'day')]
@@ -328,11 +327,24 @@
         </field>
     </record>
 
+    <record id="hr_leave_view_form_dashboard" model="ir.ui.view">
+        <field name="name">hr.leave.view.form.dashboard</field>
+        <field name="model">hr.leave</field>
+        <field name="inherit_id" ref="hr_holidays.hr_leave_view_form"/>
+        <field name="mode">primary</field>
+        <field name="priority">100</field>
+        <field name="arch" type="xml">
+            <xpath expr="//header" position="attributes">
+                <attribute name="invisible">1</attribute>
+            </xpath>
+        </field>
+    </record>
+
     <record id="hr_leave_view_dashboard" model="ir.ui.view">
         <field name="name">hr.leave.view.dashboard</field>
         <field name="model">hr.leave</field>
         <field name="arch" type="xml">
-            <calendar js_class="time_off_calendar" string="Time Off Request" event_open_popup="true" date_start="date_from" date_stop="date_to" mode="month" quick_add="False" color="employee_id">
+            <calendar js_class="time_off_calendar" string="Time Off Request" form_view_id="%(hr_holidays.hr_leave_view_form_dashboard)d" event_open_popup="true" date_start="date_from" date_stop="date_to" mode="month" quick_add="False" color="employee_id">
                 <field name="display_name"/>
             </calendar>
         </field>
@@ -475,8 +487,6 @@
         <field name="res_model">hr.leave</field>
         <field name="view_mode">calendar,tree,form</field>
         <field name="context">{
-            'search_default_year': 1,
-            'search_default_group_employee': 1,
             'form_view_ref': 'hr_holidays.hr_leave_view_form_manager',
         }</field>
         <field name="domain">[('holiday_type','=','employee'), ('state', '!=', 'refuse')]</field>
@@ -528,7 +538,7 @@
     <record id="hr_leave_action_my_request_view_form" model="ir.actions.act_window.view">
         <field name="view_mode">form</field>
         <field name="act_window_id" ref="hr_leave_action_my_request"/>
-        <field name="view_id" ref="hr_leave_view_form"/>
+        <field name="view_id" ref="hr_leave_view_form_dashboard"/>
     </record>
 
     <record id="hr_leave_action_my" model="ir.actions.act_window">
@@ -576,7 +586,7 @@
     </record>
 
     <record id="hr_leave_action_action_approve_department" model="ir.actions.act_window">
-        <field name="name">Department Time Off</field>
+        <field name="name">Time Off</field>
         <field name="res_model">hr.leave</field>
         <field name="view_mode">tree,kanban,form,calendar,activity</field>
         <field name="context">{
diff --git a/addons/project_timesheet_holidays/tests/test_timesheet_holidays.py b/addons/project_timesheet_holidays/tests/test_timesheet_holidays.py
index e7977266eb5..6aad4f1e51a 100644
--- a/addons/project_timesheet_holidays/tests/test_timesheet_holidays.py
+++ b/addons/project_timesheet_holidays/tests/test_timesheet_holidays.py
@@ -1,10 +1,10 @@
 # -*- coding: utf-8 -*-
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
-from datetime import datetime, time
+from datetime import datetime
 from dateutil.relativedelta import relativedelta
 
-from odoo import fields
+from odoo import fields, SUPERUSER_ID
 
 from odoo.tests import common, new_test_user
 from odoo.addons.hr_timesheet.tests.test_timesheet import TestCommonTimesheet
@@ -97,11 +97,11 @@ class TestTimesheetHolidays(TestCommonTimesheet):
             'date_to': self.leave_end_datetime,
             'number_of_days': number_of_days,
         })
-        holiday.sudo().action_validate()
+        holiday.with_user(SUPERUSER_ID).action_validate()
         self.assertEquals(len(holiday.timesheet_ids), number_of_days, 'Number of generated timesheets should be the same as the leave duration (1 per day between %s and %s)' % (fields.Datetime.to_string(self.leave_start_datetime), fields.Datetime.to_string(self.leave_end_datetime)))
 
         # manager refuse the leave
-        holiday.sudo().action_refuse()
+        holiday.with_user(SUPERUSER_ID).action_refuse()
         self.assertEquals(len(holiday.timesheet_ids), 0, 'Number of linked timesheets should be zero, since the leave is refused.')
 
     def test_validate_without_timesheet(self):
@@ -115,5 +115,5 @@ class TestTimesheetHolidays(TestCommonTimesheet):
             'date_to': self.leave_end_datetime,
             'number_of_days': number_of_days,
         })
-        holiday.sudo().action_validate()
+        holiday.with_user(SUPERUSER_ID).action_validate()
         self.assertEquals(len(holiday.timesheet_ids), 0, 'Number of generated timesheets should be zero since the leave type does not generate timesheet')