[FIX] hr_timesheet: avoid setting an archived employee to a timesheet in the form view

[mafo-odoo]

Steps to reproduce:

• Install timesheet
• Archive an employee
• Create a timesheet

Current behavior:
You can select the archived employee

Expected behavior:
You can not select the archived employee

Explanation:
In commit 17b2b07 the employee_id
field of the timesheet was set to have context active_test to false.
To fix this issue we reset the context for this field in every
timesheet form.

A task exist to fix the issue on master id 2884736

opw-2887727
opw-2870739

[robodoo]

Pull request status dashboard.

[mafo-odoo]

linked on enterprise to https://github.com/odoo/enterprise/pull/28749

[xavierbol]

Thanks for the fix, it seems good to me. However, I don't understand why we will block only in master, the creation of Timesheet with an archived employee on it. 🤔
Do you know why we don't do this in stable too? 🤔

[mafo-odoo]

However, I don't understand why we will block only in master, the creation of Timesheet with an archived employee on it. Do you know why we don't do this in stable too?

That's a good question I don't have the answer for, maybe @auon-odoo or @aju-odoo will be able to answer you as they created the other task.
Maybe it was to avoid breaking stuff in stable with a risky modification.

[mafo-odoo]

@xavierbol

[xavierbol]

? 🤔

Also it seems you have some changes unrelated to the issue described on this PR.

[mafo-odoo]

Also it seems you have some changes unrelated to the issue described on this PR.

Which changes do you speak of?

? 🤔

Does the current PR need some other changes?

[xavierbol]

Strange I saw other changes unrelated, but it is no longer the case. 🤔

Ah, you didn't ask to @aju-odoo to know that? 🤯

[mafo-odoo]

Ah, you didn't ask to @aju-odoo to know that? exploding_head

Seeing the changes of @auon-odoo in https://github.com/odoo/odoo/pull/93823/files I just guessed they were to big for stable.

[xavierbol]

I didn't ask to copy/paste the work of @auon-odoo 😅
Moreover, his work is not finished. 🤔

You could just check if the employee who will be assigned to the timesheet is archived or not, for instance...

[xavierbol]

So, you asked to @aju-odoo if you have to manage it and after that you decide to not do it because the code of @auon-odoo is too complex to do it in stable? 🤔

[mafo-odoo]

I didn't ask to copy/paste the work of @auon-odoo 😅

And yet I feel like it is what you want when you say

You could just check if the employee who will be assigned to the timesheet is archived or not, for instance...

As it is what he is doing but anyway you were right, after discussion with @aju-odoo, she will just ask @auon-odoo to retarget stable with his modifications.

[mafo-odoo]

Will be fixed by @auon-odoo with task 2884736

[xavierbol]

I think @auon-odoo wants to refactor it and also manages others cases since the HR Team added the possibility to the user to easily create an employee and link it to a user. Indeed, his task has the same purpose than for fix but in master, he could refactor the code to try to improve the maintainability of our code. So, I agree it is not a good idea to use his code in stable for the moment but it is not a reason to do nothing to fix it, that's why I asked you if you had discussed with @aju-odoo about that since you pinged without any context.

It is a bit pity that you decide to do nothing or cancel your fix without discussing with us (in private or here) to know what you have to do if really you don't understand what you have to do. Actually, you just had to add a check to prevent the user setting an archived employees instead of copy/paste the work from @auon-odoo.

So, I guess this additional diff was sufficient for your bug fix:

diff --git a/addons/hr_timesheet/models/hr_timesheet.py b/addons/hr_timesheet/models/hr_timesheet.py
index ceb9d35dc74..d77a6e86fad 100644
--- a/addons/hr_timesheet/models/hr_timesheet.py
+++ b/addons/hr_timesheet/models/hr_timesheet.py
@@ -129,6 +129,7 @@ class AccountAnalyticLine(models.Model):
         for employee in employees:
             employee_for_user_company[employee.user_id.id][employee.company_id.id] = employee.id
 
+        employee_ids = set()
         for vals in vals_list:
             # compute employee only for timesheet lines, makes no sense for other lines
             if not vals.get('employee_id') and vals.get('project_id'):
@@ -137,6 +138,11 @@ class AccountAnalyticLine(models.Model):
                     continue
                 company_id = list(employee_for_company)[0] if len(employee_for_company) == 1 else vals.get('company_id', self.env.company.id)
                 vals['employee_id'] = employee_for_company.get(company_id, False)
+            elif vals.get('employee_id'):
+                employee_ids.add(vals['employee_id'])
+
+        if any(not emp.active for emp in self.env['hr.employee'].browse(list(employee_ids))):
+            raise UserError(_('Timesheets must be created with an active employee.'))
 
         lines = super(AccountAnalyticLine, self).create(vals_list)
         for line, values in zip(lines, vals_list):
@@ -150,6 +156,10 @@ class AccountAnalyticLine(models.Model):
             raise AccessError(_("You cannot access timesheets that are not yours."))
 
         values = self._timesheet_preprocess(values)
+        if values.get('employee_id'):
+            employee = self.env['hr.employee'].browse(values['employee_id'])
+            if not employee.active:
+                raise UserError(_('You cannot set an archived employee to the existing timesheets.'))
         if 'name' in values and not values.get('name'):
             values['name'] = '/'
         result = super(AccountAnalyticLine, self).write(values)

(I didn't test this code, it is just a suggestion and this diff is done in master)

We could indeed waiting the @auon-odoo if you prefer but he is off this week so your customer will not have a fix before the next week or next 2 weeks.

cc @aju-odoo @IT-Ideas

[mafo-odoo]

Hi @xavierbol! I think it should be ok now :)

[xavierbol]

Thanks for the fix, I have just one comment. After that, it will look good to me. I let @IT-Ideas doing the final review. 🙂

[xavierbol]

Suggested change

	''' the timesheet can be created or edited only with an active employee
	'''
	""" test cannot create/update timesheet with archived employee """

[IT-Ideas]

Thanks for the fix 🙂
@robodoo r+

[fw-bot]

This pull request has forward-port PRs awaiting action (not merged or closed): #95011, #95016

[fw-bot]

This pull request has forward-port PRs awaiting action (not merged or closed): #95016

[fw-bot]

This pull request has forward-port PRs awaiting action (not merged or closed): #95016

[fw-bot]

This pull request has forward-port PRs awaiting action (not merged or closed): #95442, #95453

[fw-bot]

This pull request has forward-port PRs awaiting action (not merged or closed): #95453