From 29bdb044218cfb4833bc2cb51d33c286221a4002 Mon Sep 17 00:00:00 2001 From: Yannick Tivisse Date: Mon, 18 May 2020 17:04:40 +0200 Subject: [PATCH 1/3] [IMP] project: Improve task's partner_id management Purpose ======= During the development of the task #2169100, it appeared that the synchronisation of a task's partner_id according to its project's partner_id and its parent's partner_id was not very clear. A first idea was to simplify the partner_id management: as it could be annoying for the customer to see all the task's partner_id overriden after project's partner_id modification, the idea was to just use the parent's parnter_id or the project's partner_id as default value of the task's partner_id. Then, if the partner_id of the project or of the parent is changed, there is no more impact on the task partner_id itself. The main problem of this solution is that the management of the task's partner_id is also extended in several others modules like sale_project and sale_timesheet. You can find below what seems to happen with all these task partners. Start following rules according to which module is installed. (for the record: sale_timesheet depends on sale_project which depends on project). Rules are applied in the order they are written. Rules labelled "O" only apply when changing the project through the task form view. Rules labelled "C" apply everywhere. technical: C=compute, O=onchange project C.1 If parent's partner changes and the partner is not set, then set the parent's partner C.2 Else if project's partner changes ant the partner is not set, then set the project's partner sale_project [+ project] C.1 If the project's sale order line's partner changes and the partner is not set, then set the project's sale order line's partner C.2 Else apply (project.C) rules sale_timesheet + [sale_project + project] C.1 Apply (sale_project.C) rules O.1 If the project is billed "At Project rate" or "At Employee Rate" and the partner is not set, then set the project's sale order's partner the sale order line's partner Specification ============= - If the partner changes, we ignore all current tasks and let them be. We set the new partner as default value on all new tasks. In detail, we have done this: - When the user creates a task give the customer of the default project or the task parent as default customer for the task. - When the user changes the project and no partner in the task, we take the partner of the new project (if this project has a customer). - When the user changes the customer of the project, the linked tasks are not impacted by this changes, even if some tasks have not a customer yet. - When the user changes the customerf of the parent task, the child tasks are not impacted by this changes, event if these tasks have not a customer yet. - remove the onchange rule to remain consistent. TaskID: 2232042 closes #51471 Co-authored-by: Xavier BOL (xbo) --- addons/project/models/project.py | 40 +++++++++++++------ addons/project/tests/test_project_flow.py | 23 +++++------ addons/project/views/project_views.xml | 2 +- addons/sale_project/models/project.py | 10 +---- addons/sale_project/tests/test_child_tasks.py | 11 ++++- addons/sale_timesheet/models/project.py | 15 ++++--- .../tests/test_project_billing.py | 34 ++++++++-------- .../sale_timesheet/tests/test_sale_service.py | 2 +- 8 files changed, 76 insertions(+), 61 deletions(-) diff --git a/addons/project/models/project.py b/addons/project/models/project.py index c8bbc2ca809..7f097d973d4 100644 --- a/addons/project/models/project.py +++ b/addons/project/models/project.py @@ -531,6 +531,14 @@ class Task(models.Model): _order = "priority desc, sequence, id desc" _check_company_auto = True + @api.model + def _get_default_partner_id(self, project=None, parent=None): + if parent and parent.partner_id: + return parent.partner_id.id + if project and project.partner_id: + return project.partner_id.id + return False + def _get_default_stage_id(self): """ Gives default stage_id """ project_id = self.env.context.get('default_project_id') @@ -1018,6 +1026,18 @@ class Task(models.Model): if 'repeat_weekday' in default_fields: vals['repeat_weekday'] = self._fields.get('repeat_weekday').selection[week_start][0] + if 'partner_id' in vals and not vals['partner_id']: + # if the default_partner_id=False or no default_partner_id then we search the partner based on the project and parent + project_id = vals.get('project_id') + parent_id = vals.get('parent_id', self.env.context.get('default_parent_id')) + if project_id or parent_id: + partner_id = self._get_default_partner_id( + project_id and self.env['project.project'].browse(project_id), + parent_id and self.env['project.task'].browse(parent_id) + ) + if partner_id: + vals['partner_id'] = partner_id + return vals @api.model_create_multi @@ -1136,20 +1156,16 @@ class Task(models.Model): for task in self: task.user_id = task.user_id or task.parent_id.user_id or self.env.uid - @api.depends('parent_id.partner_id', 'project_id.partner_id') + @api.depends('parent_id', 'project_id', 'display_project_id') def _compute_partner_id(self): + """ Compute the partner_id when the tasks have no partner_id. + + Use the project partner_id if any, or else the parent task partner_id. """ - If a task has no partner_id, use the project partner_id if any, or else the parent task partner_id. - Once the task partner_id has been set: - 1) if the project partner_id changes, the task partner_id is automatically changed also. - 2) if the parent task partner_id changes, the task partner_id remains the same. - """ - for task in self: - if task.partner_id: - if task.project_id.partner_id: - task.partner_id = task.project_id.partner_id - else: - task.partner_id = task.project_id.partner_id or task.parent_id.partner_id + for task in self.filtered(lambda task: not task.partner_id): + # When the task has a parent task, the display_project_id can be False or the project choose by the user for this task. + project = task.display_project_id if task.parent_id and task.display_project_id else task.project_id + task.partner_id = self._get_default_partner_id(project, task.parent_id) @api.depends('partner_id.email', 'parent_id.email_from') def _compute_email_from(self): diff --git a/addons/project/tests/test_project_flow.py b/addons/project/tests/test_project_flow.py index 73f7b7956c5..4fa67397c0e 100644 --- a/addons/project/tests/test_project_flow.py +++ b/addons/project/tests/test_project_flow.py @@ -134,11 +134,8 @@ class TestProjectFlow(TestProjectCommon): }) # the child task 1 is linked to a project without partner_id (goats project) - child_task_1 = Task.create({ + child_task_1 = Task.with_context(default_project_id=self.project_goats.id, default_parent_id=parent_task.id).create({ 'name': 'Task Child with project', - 'parent_id': parent_task.id, - 'project_id': self.project_goats.id, - 'display_project_id': self.project_goats.id, 'planned_hours': 3, }) @@ -156,8 +153,8 @@ class TestProjectFlow(TestProjectCommon): "When no project partner_id has been set, a subtask should have the same partner as its parent") self.assertEqual( - child_task_2.partner_id, child_task_2.project_id.partner_id, - "When a project partner_id has been set, a subtask should have the same partner as its project") + child_task_2.partner_id, child_task_2.parent_id.partner_id, + "When a project partner_id has been set, a subtask should have the same partner as its parent") self.assertEqual( parent_task.subtask_count, 2, @@ -182,17 +179,17 @@ class TestProjectFlow(TestProjectCommon): }) self.assertEqual( - child_task_2.partner_id, child_task_2.project_id.partner_id, + child_task_2.partner_id, parent_task.partner_id, "When changing the parent task of a subtask with a project, the partner_id should remain the same.") # set a project with partner_id to a subtask without project partner_id child_task_1.write({ - 'project_id': self.project_pigs.id + 'display_project_id': self.project_pigs.id }) - self.assertEqual( + self.assertNotEqual( child_task_1.partner_id, self.project_pigs.partner_id, - "When the project changes, the subtask should have the same partner id as the new project.") + "When the project changes, the subtask should keep its partner id as its partner id is set.") # restore the partner_id of the 'goats' project self.project_goats.write({ @@ -201,12 +198,12 @@ class TestProjectFlow(TestProjectCommon): # set a project with partner_id to a subtask with a project partner_id child_task_2.write({ - 'project_id': self.project_goats.id + 'display_project_id': self.project_goats.id }) self.assertEqual( - child_task_2.partner_id, self.project_goats.partner_id, - "When the project changes, the subtask should have the same partner id as the new project.") + child_task_2.partner_id, parent_task.partner_id, + "When the project changes, the subtask should keep the same partner id even it has a new project.") def test_rating(self): """Check if rating works correctly even when task is changed from project A to project B""" diff --git a/addons/project/views/project_views.xml b/addons/project/views/project_views.xml index 2503ec5b189..d07d2f43965 100644 --- a/addons/project/views/project_views.xml +++ b/addons/project/views/project_views.xml @@ -720,7 +720,7 @@ - + diff --git a/addons/sale_project/models/project.py b/addons/sale_project/models/project.py index e0cea2e0b96..9b5564a69a2 100644 --- a/addons/sale_project/models/project.py +++ b/addons/sale_project/models/project.py @@ -54,18 +54,12 @@ class ProjectTask(models.Model): invoice_count = fields.Integer("Number of invoices", related='sale_order_id.invoice_count') task_to_invoice = fields.Boolean("To invoice", compute='_compute_task_to_invoice', search='_search_task_to_invoice', groups='sales_team.group_sale_salesman_all_leads') - @api.depends('project_id.sale_line_id.order_partner_id') - def _compute_partner_id(self): - for task in self: - if not task.partner_id: - task.partner_id = task.project_id.sale_line_id.order_partner_id - super()._compute_partner_id() - @api.depends('commercial_partner_id', 'sale_line_id.order_partner_id.commercial_partner_id', 'parent_id.sale_line_id', 'project_id.sale_line_id') def _compute_sale_line(self): for task in self: if not task.sale_line_id: - task.sale_line_id = task.parent_id.sale_line_id or task.project_id.sale_line_id + # if the display_project_id is set then it means the task is classic task or a subtask with another project than its parent. + task.sale_line_id = task.display_project_id.sale_line_id or task.parent_id.sale_line_id or task.project_id.sale_line_id # check sale_line_id and customer are coherent if task.sale_line_id.order_partner_id.commercial_partner_id != task.partner_id.commercial_partner_id: task.sale_line_id = False diff --git a/addons/sale_project/tests/test_child_tasks.py b/addons/sale_project/tests/test_child_tasks.py index e7f8afb4c63..03d9e1dcde6 100644 --- a/addons/sale_project/tests/test_child_tasks.py +++ b/addons/sale_project/tests/test_child_tasks.py @@ -41,6 +41,11 @@ class TestNestedTaskUpdate(TransactionCase): def test_creating_subtask_partner_id_on_parent_goes_on_child(self): parent = self.env['project.task'].create({'name': 'parent', 'partner_id': self.user.partner_id.id}) child = self.env['project.task'].create({'name': 'child', 'parent_id': parent.id}) + child._compute_partner_id() # the compute will be triggered since the user set the parent_id. + self.assertEqual(child.partner_id, self.user.partner_id) + + # Another case, it is the parent as a default value + child = self.env['project.task'].with_context(default_parent_id=parent.id).create({'name': 'child'}) self.assertEqual(child.partner_id, self.user.partner_id) def test_creating_subtask_email_from_on_parent_goes_on_child(self): @@ -96,9 +101,9 @@ class TestNestedTaskUpdate(TransactionCase): child = self.env['project.task'].create({'name': 'child', 'partner_id': False, 'parent_id': parent.id}) self.assertFalse(child.partner_id) parent.write({'partner_id': self.user.partner_id.id}) - self.assertEqual(child.partner_id, parent.partner_id) + self.assertNotEqual(child.partner_id, parent.partner_id) parent.write({'partner_id': False}) - self.assertEqual(child.partner_id, self.user.partner_id) + self.assertNotEqual(child.partner_id, self.user.partner_id) def test_write_email_from_on_parent_write_on_child(self): parent = self.env['project.task'].create({'name': 'parent'}) @@ -121,6 +126,7 @@ class TestNestedTaskUpdate(TransactionCase): def test_write_sale_line_id_on_parent_write_on_child_with_partner_if_not_set(self): parent = self.env['project.task'].create({'name': 'parent', 'partner_id': self.partner.id}) child = self.env['project.task'].create({'name': 'child', 'parent_id': parent.id}) + child._compute_partner_id() self.assertFalse(child.sale_line_id) parent.write({'sale_line_id': self.order_line.id}) self.assertEqual(child.sale_line_id, parent.sale_line_id) @@ -192,6 +198,7 @@ class TestNestedTaskUpdate(TransactionCase): parent = self.env['project.task'].create({'name': 'parent', 'user_id': False, 'partner_id': self.partner.id}) children_values = [{'name': 'child%s' % i, 'user_id': False, 'parent_id': parent.id} for i in range(5)] children = self.env['project.task'].create(children_values) + children._compute_partner_id() # test writing sale_line_id for child in children: self.assertFalse(child.sale_line_id) diff --git a/addons/sale_timesheet/models/project.py b/addons/sale_timesheet/models/project.py index dee2619d4d1..ad0cf1a333b 100644 --- a/addons/sale_timesheet/models/project.py +++ b/addons/sale_timesheet/models/project.py @@ -255,6 +255,13 @@ class Project(models.Model): class ProjectTask(models.Model): _inherit = "project.task" + def _get_default_partner_id(self, project, parent): + res = super()._get_default_partner_id(project, parent) + if not res and project: + if project.pricing_type == 'employee_rate': + return project.sale_line_employee_ids.sale_line_id.order_partner_id[:1] + return res + # override sale_order_id and make it computed stored field instead of regular field. sale_order_id = fields.Many2one(compute='_compute_sale_order_id', store=True, readonly=False, domain="['|', '|', ('partner_id', '=', partner_id), ('partner_id', 'child_of', commercial_partner_id), ('partner_id', 'parent_of', partner_id)]") @@ -326,14 +333,6 @@ class ProjectTask(models.Model): for task in self: task.has_multi_sol = task.timesheet_ids and task.timesheet_ids.so_line != task.sale_line_id - @api.onchange('project_id') - def _onchange_project(self): - if self.project_id and self.project_id.pricing_type != 'task_rate': - if not self.partner_id: - self.partner_id = self.project_id.partner_id - if not self.sale_line_id: - self.sale_line_id = self.project_id.sale_line_id - def _get_last_sol_of_customer(self): # Get the last SOL made for the customer in the current task where we need to compute self.ensure_one() diff --git a/addons/sale_timesheet/tests/test_project_billing.py b/addons/sale_timesheet/tests/test_project_billing.py index f9c7002ee7e..3cd6bcc949c 100644 --- a/addons/sale_timesheet/tests/test_project_billing.py +++ b/addons/sale_timesheet/tests/test_project_billing.py @@ -78,8 +78,14 @@ class TestProjectBilling(TestCommonSaleTimesheet): cls.project_project_rate = cls.project_task_rate.copy({ 'name': 'Project with pricing_type="project_rate"', + 'sale_order_id': cls.sale_order_1.id, 'sale_line_id': cls.so1_line_order_no_task.id, }) + + # FIXME: [XBO] since the both projects have a SOL than the pricing_type should not be task_rate ! + cls.project_task_rate = cls.env['project.project'].search([('sale_line_id', '=', cls.so2_line_deliver_project_task.id)], limit=1) + cls.project_task_rate2 = cls.env['project.project'].search([('sale_line_id', '=', cls.so2_line_deliver_project_template.id)], limit=1) + cls.project_employee_rate = cls.project_task_rate.copy({ 'name': 'Project with pricing_type="employee_rate"', 'partner_id': cls.sale_order_1.partner_id.id, @@ -297,15 +303,14 @@ class TestProjectBilling(TestCommonSaleTimesheet): task.write({ 'project_id': self.project_task_rate.id, }) - task._onchange_project() self.assertTrue(task.allow_billable, "Task in project 'task rate' should be billed at task rate") - self.assertEqual(task.sale_line_id, self.so2_line_deliver_project_task, "The SOL in the task should be the one that we selected") - self.assertEqual(task.partner_id, task.project_id.partner_id, "Task created in a project billed on 'employee rate' should have the same customer as the one from the project") + self.assertEqual(task.sale_line_id, self.so1_line_deliver_no_task, "The task should keep the same SOL since the partner_id has not changed when the project of the task has changed.") + self.assertEqual(task.partner_id, self.partner_a, "Task created in a project billed on 'employee rate' should have the same customer when it has been created.") # move subtask into task rate project subtask.write({ - 'project_id': self.project_task_rate2.id, + 'display_project_id': self.project_task_rate2.id, }) self.assertTrue(subtask.allow_billable, "Subtask should keep the billable type from its parent, even when they are moved into another project") @@ -352,7 +357,6 @@ class TestProjectBilling(TestCommonSaleTimesheet): task = Task.with_context(default_project_id=self.project_task_rate.id).create({ 'name': 'first task', }) - task._onchange_project() self.assertEqual(task.sale_line_id, self.so2_line_deliver_project_task, "Task created in a project billed on 'task rate' should be linked to a SOL containing a prepaid service product and the remaining hours of this SOL should be greater than 0.") self.assertEqual(task.partner_id, task.project_id.partner_id, "Task created in a project billed on 'task rate' should have the same customer as the one from the project") @@ -369,9 +373,10 @@ class TestProjectBilling(TestCommonSaleTimesheet): self.assertEqual(task.sale_line_id, timesheet1.so_line, "The timesheet should be linked to the SOL associated to the task since the pricing type of the project is task rate.") # create a subtask - subtask = Task.with_context(default_project_id=self.project_subtask.id).create({ + subtask = Task.with_context(default_project_id=self.project_task_rate.id).create({ 'name': 'first subtask task', 'parent_id': task.id, + 'display_project_id': self.project_subtask.id, }) self.assertEqual(subtask.partner_id, subtask.parent_id.partner_id, "Subtask should have the same customer as the one from their mother") @@ -379,30 +384,27 @@ class TestProjectBilling(TestCommonSaleTimesheet): # log timesheet on subtask timesheet2 = Timesheet.create({ 'name': 'Test Line on subtask', - 'project_id': subtask.project_id.id, + 'project_id': subtask.display_project_id.id, 'task_id': subtask.id, 'unit_amount': 50, 'employee_id': self.employee_user.id, }) - - self.assertEqual(subtask.project_id, timesheet2.project_id, "The timesheet is in the subtask project") + self.assertEqual(subtask.display_project_id, timesheet2.project_id, "The timesheet is in the subtask project") self.assertFalse(timesheet2.so_line, "The timesheet should not be linked to SOL as it's a non billable project") # move task and subtask into task rate project task.write({ 'project_id': self.project_employee_rate.id, }) - task._onchange_project() subtask.write({ - 'project_id': self.project_employee_rate.id, + 'display_project_id': self.project_employee_rate.id, }) - subtask._onchange_project() - self.assertEqual(task.sale_line_id, task.project_id.sale_line_id, "Task moved in a employee rate billable project have the SOL of the project") - self.assertEqual(task.partner_id, task.project_id.partner_id, "Task created in a project billed on 'employee rate' should have the same customer as the one from the project") + self.assertEqual(task.sale_line_id, self.project_task_rate.sale_line_id, "Task moved in a employee rate billable project should keep its SOL because the partner_id has not changed too.") + self.assertEqual(task.partner_id, self.project_task_rate.partner_id, "Task created in a project billed on 'employee rate' should have the same customer as the one from its initial project.") - self.assertEqual(subtask.sale_line_id, task.project_id.sale_line_id, "Subask moved in a employee rate billable project have the SOL of the project") - self.assertEqual(subtask.partner_id, task.project_id.partner_id, "Subask created in a project billed on 'employee rate' should have the same customer as the one from the project") + self.assertEqual(subtask.sale_line_id, subtask.parent_id.sale_line_id, "Subtask moved in a employee rate billable project should have the SOL of its parent since it keep its partner_id and this partner is different than the one in the destination project.") + self.assertEqual(subtask.partner_id, subtask.parent_id.partner_id, "Subtask moved in a project billed on 'employee rate' should keep its initial customer, that is the one of its parent.") def test_customer_change_in_project(self): """ Test when the user change the customer in a project diff --git a/addons/sale_timesheet/tests/test_sale_service.py b/addons/sale_timesheet/tests/test_sale_service.py index eb737f78d6a..53d54c0b40b 100644 --- a/addons/sale_timesheet/tests/test_sale_service.py +++ b/addons/sale_timesheet/tests/test_sale_service.py @@ -536,7 +536,7 @@ class TestSaleService(TestCommonSaleTimesheet): self.assertEqual(task2.partner_id, so_line_deliver_new_task_project.order_partner_id, "A new task in a billable project should have the same SO line as its project") # moving subtask in another project - subtask.write({'project_id': self.project_global.id}) + subtask.write({'display_project_id': self.project_global.id}) self.assertEqual(subtask.sale_line_id, task.sale_line_id, "A child task should always have the same SO line as its mother, even when changing project") self.assertEqual(subtask.sale_line_id, so_line_deliver_new_task_project) From 4d96a03c439729c17f80675ce886f093d9e08638 Mon Sep 17 00:00:00 2001 From: "Xavier BOL (xbo)" Date: Mon, 26 Apr 2021 09:38:54 +0200 Subject: [PATCH 2/3] [IMP] project: get the display_project_id if parent is subtask Before this commit, when the user creates a subtask in subtask then we take the project_id but if the user has changed the display_project_id, the subtask of this task has not the correct project. This commit gives the project_id by default if the parent task is not a subtask or the display_project_id of this parent task is not defined otherwise it gives the display_project_id. task-2232042 closes #51471 --- addons/project/views/project_views.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addons/project/views/project_views.xml b/addons/project/views/project_views.xml index d07d2f43965..822afa8e6aa 100644 --- a/addons/project/views/project_views.xml +++ b/addons/project/views/project_views.xml @@ -720,7 +720,7 @@ - + From f79c84e2e9620ac3a5cf5a58964ca9f3b320ae0e Mon Sep 17 00:00:00 2001 From: "Xavier BOL (xbo)" Date: Mon, 3 May 2021 10:53:44 +0200 Subject: [PATCH 3/3] [IMP] project: display commercial_partner_id in project kanban view Before this commit, when the partner_id in the project, we display the partner_id selected in this project in the kanban card of it. This committ displays the commercial_partner_id rather than the partner_id in the kanban card of each project in the kanban view to be consist with task's kanban card. task-2232042 closes #51471 --- addons/project/models/project.py | 1 + addons/project/views/project_views.xml | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/addons/project/models/project.py b/addons/project/models/project.py index 7f097d973d4..3ba2692ff60 100644 --- a/addons/project/models/project.py +++ b/addons/project/models/project.py @@ -178,6 +178,7 @@ class Project(models.Model): partner_phone = fields.Char( compute='_compute_partner_phone', inverse='_inverse_partner_phone', string="Phone", readonly=False, store=True, copy=False) + commercial_partner_id = fields.Many2one(related="partner_id.commercial_partner_id") company_id = fields.Many2one('res.company', string='Company', required=True, default=lambda self: self.env.company) currency_id = fields.Many2one('res.currency', related="company_id.currency_id", string="Currency", readonly=True) analytic_account_id = fields.Many2one('account.analytic.account', string="Analytic Account", copy=False, ondelete='set null', diff --git a/addons/project/views/project_views.xml b/addons/project/views/project_views.xml index 822afa8e6aa..f48de48afed 100644 --- a/addons/project/views/project_views.xml +++ b/addons/project/views/project_views.xml @@ -506,6 +506,7 @@ + @@ -529,7 +530,7 @@
- +