From 7eeeb52f44a65cb58d1430311f2febb45cf4ad62 Mon Sep 17 00:00:00 2001 From: Jay Savaliya Date: Mon, 4 Dec 2023 15:11:09 +0100 Subject: [PATCH 1/4] [IMP] crm: add tests for assign / stage update dates Just to see how it behaves currently, as we are going to fix some unwanted changes. Notably * setting user_id to the same value as before should not update the date_open value; * setting stage_id to the same value as before should not update the last stage update value; * triggers generate chain update of those fields (changing team_id changes user_id that changes date_open, ...); Task-3515225 X-original-commit: a1c72fff151524f636fae3f253d33b8228d0add9 Part-of: odoo/odoo#161918 --- addons/crm/tests/test_crm_lead.py | 91 +++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/addons/crm/tests/test_crm_lead.py b/addons/crm/tests/test_crm_lead.py index 3e975ffb5bc..75126ea567b 100644 --- a/addons/crm/tests/test_crm_lead.py +++ b/addons/crm/tests/test_crm_lead.py @@ -3,6 +3,7 @@ from datetime import datetime from freezegun import freeze_time +from unittest.mock import patch from odoo import fields from odoo.addons.base.tests.test_format_address_mixin import FormatAddressCase @@ -636,6 +637,96 @@ class TestCRMLead(TestCrmCommon): self.assertEqual(self.contact_company_1.email, 'broken') self.assertEqual(self.contact_company_1.phone, 'alsobroken') + @users('user_sales_manager') + def test_crm_lead_update_dates(self): + """ Test date_open / date_last_stage_update update, check those dates + are not erased too often """ + first_now = datetime(2023, 11, 6, 8, 0, 0) + with patch.object(self.env.cr, 'now', lambda: first_now), \ + freeze_time(first_now): + leads = self.env['crm.lead'].create([ + { + 'email_from': 'testlead@customer.company.com', + 'name': 'Lead_1', + 'team_id': self.sales_team_1.id, + 'type': 'lead', + 'user_id': False, + }, { + 'email_from': 'testopp@customer.company.com', + 'name': 'Opp_1', + 'type': 'opportunity', + 'user_id': self.user_sales_salesman.id, + }, + ]) + leads.flush_recordset() + for lead in leads: + self.assertEqual(lead.date_last_stage_update, first_now, + "Stage updated at create time with default value") + self.assertEqual(lead.stage_id, self.stage_team1_1) + self.assertEqual(lead.team_id, self.sales_team_1) + self.assertFalse(leads[0].date_open, "No user -> no assign date") + self.assertFalse(leads[0].user_id) + self.assertEqual(leads[1].date_open, first_now, "Default user assigned") + self.assertEqual(leads[1].user_id, self.user_sales_salesman, "Default user assigned") + + # changing user_id may change team_id / stage_id; update date_open and + # maybe date_last_stage_update + updated_time = datetime(2023, 11, 23, 8, 0, 0) + with patch.object(self.env.cr, 'now', lambda: updated_time), \ + freeze_time(updated_time): + leads.write({"user_id": self.user_sales_salesman.id}) + leads.flush_recordset() + for lead in leads: + self.assertEqual(lead.stage_id, self.stage_team1_1) + self.assertEqual(lead.team_id, self.sales_team_1) + self.assertEqual( + leads[0].date_last_stage_update, updated_time, + 'FIXME: set same stage when changing user_id, should not update') + self.assertEqual( + leads[0].date_open, updated_time, + 'User assigned -> assign date updated') + self.assertEqual( + leads[1].date_last_stage_update, updated_time, + 'FIXME: set same stage when changing user_id, should not update') + self.assertEqual( + leads[1].date_open, updated_time, + 'FIXME: Should not update date_open, was already the same user_id') + + # set won changes stage -> update date_last_stage_update + newer_time = datetime(2023, 11, 26, 8, 0, 0) + with patch.object(self.env.cr, 'now', lambda: newer_time), \ + freeze_time(newer_time): + leads[1].action_set_won() + leads[1].flush_recordset() + self.assertEqual( + leads[1].date_last_stage_update, newer_time, + 'Mark as won updates stage hence stage update date') + self.assertEqual(leads[1].stage_id, self.stage_gen_won) + + # merge may change user_id and then may change team_id / stage_id; in this + # case no real value change is happening + last_time = datetime(2023, 11, 29, 8, 0, 0) + with patch.object(self.env.cr, 'now', lambda: last_time), \ + freeze_time(last_time): + leads.merge_opportunity( + user_id=self.user_sales_salesman.id, + auto_unlink=False, + ) + leads.flush_recordset() + self.assertEqual(leads[0].date_last_stage_update, updated_time) + self.assertEqual(leads[0].date_open, updated_time) + self.assertEqual(leads[0].stage_id, self.stage_team1_1) + self.assertEqual(leads[0].team_id, self.sales_team_1) + self.assertEqual( + leads[1].date_last_stage_update, last_time, + 'FIXME: should not rewrite when setting same stage') + self.assertEqual( + leads[1].date_open, last_time, + 'FIXME: should not rewrite when setting same user_id') + self.assertEqual(leads[1].stage_id, self.stage_gen_won) + self.assertEqual(leads[1].team_id, self.sales_team_1) + self.assertEqual(leads[1].user_id, self.user_sales_salesman) + @users('user_sales_manager') def test_crm_team_alias(self): new_team = self.env['crm.team'].create({ From 1e45ec7b367bc628f7d228ca8d9405a197ac5104 Mon Sep 17 00:00:00 2001 From: Jay Savaliya Date: Mon, 4 Dec 2023 16:32:52 +0100 Subject: [PATCH 2/4] [FIX] crm: fix date_{last_stage_update/open} update issues Before this commit: * when a user changes 'user_id' to set the same previous 'user_id', the assign date 'date_open' is updated but it should not as the responsible did not change; * when a user changes the salesperson 'user_id' of a crm lead, it triggers a recompute of 'team_id' that triggers a recompute of 'stage_id' that updates 'date_last_stage_update' even if the stage does not change, which happens frequently when changing leads within a given team (new assign, salesperson on holidays, ...) * when merging opportunities, 'user_id' can be set on the main opportunity which triggers a recomputation of both 'date_last_stage_update' and 'date_open' as explained in above points; Reason: The 'date_last_stage_update' field depends on 'stage_id' which depends on 'team_id' which depends on 'user_id'. As a result, when 'user_id' changes, 'date_last_stage_update' also updates. Moreover those fields are implemented using editable stored computed fields which are triggered everytime a value is given to those fields, even when the same value is given. After this commit: 'date_last_stage_update' and 'date_open' will only update when there are real changes. Task-3515225 X-original-commit: 8dc18806847e5240dba6f05bdd80d313bf466ebf Part-of: odoo/odoo#161918 --- addons/crm/models/crm_lead.py | 44 +++++++++++++++++++++++-------- addons/crm/tests/test_crm_lead.py | 20 +++++++------- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index 068f1623bdd..b93151994ec 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -297,7 +297,8 @@ class Lead(models.Model): continue team_domain = [('use_leads', '=', True)] if lead.type == 'lead' else [('use_opportunities', '=', True)] team = self.env['crm.team']._get_default_team_id(user_id=user.id, domain=team_domain) - lead.team_id = team.id + if lead.team_id != team: + lead.team_id = team.id @api.depends('user_id', 'team_id', 'partner_id') def _compute_company_id(self): @@ -345,12 +346,14 @@ class Lead(models.Model): @api.depends('user_id') def _compute_date_open(self): for lead in self: - lead.date_open = self.env.cr.now() if lead.user_id else False + if not lead.date_open and lead.user_id: + lead.date_open = self.env.cr.now() @api.depends('stage_id') def _compute_date_last_stage_update(self): for lead in self: - lead.date_last_stage_update = self.env.cr.now() + if not lead.date_last_stage_update: + lead.date_last_stage_update = self.env.cr.now() @api.depends('create_date', 'date_open') def _compute_day_open(self): @@ -767,13 +770,27 @@ class Lead(models.Model): if vals.get('website'): vals['website'] = self.env['res.partner']._clean_website(vals['website']) - stage_updated, stage_is_won = vals.get('stage_id'), False - # stage change: update date_last_stage_update - if stage_updated: - stage = self.env['crm.stage'].browse(vals['stage_id']) - if stage.is_won: - vals.update({'probability': 100, 'automated_probability': 100}) - stage_is_won = True + now = self.env.cr.now() + stage_updated, stage_is_won = False, False + # stage change (or reset): update date_last_stage_update if at least one + # lead does not have the same stage + if 'stage_id' in vals: + stage_updated = any(lead.stage_id.id != vals['stage_id'] for lead in self) + if stage_updated: + vals['date_last_stage_update'] = now + if stage_updated and vals.get('stage_id'): + stage = self.env['crm.stage'].browse(vals['stage_id']) + if stage.is_won: + vals.update({'probability': 100, 'automated_probability': 100}) + stage_is_won = True + # user change; update date_open if at least one lead does not + # have the same user + if 'user_id' in vals and not vals.get('user_id'): + vals['date_open'] = False + elif vals.get('user_id'): + user_updated = any(lead.user_id.id != vals['user_id'] for lead in self) + if user_updated: + vals['date_open'] = now # stage change with new stage: update probability and date_closed if vals.get('probability', 0) >= 100 or not vals.get('active', True): @@ -1432,7 +1449,12 @@ class Lead(models.Model): if merged_data.get('stage_id') not in team_stage_ids.ids: merged_data['stage_id'] = team_stage_ids[0].id if team_stage_ids else False - # write merged data into first opportunity + # write merged data into first opportunity; remove some keys if already + # set on opp to avoid useless recomputes + if 'user_id' in merged_data and opportunities_head.user_id.id == merged_data['user_id']: + merged_data.pop('user_id') + if 'team_id' in merged_data and opportunities_head.team_id.id == merged_data['team_id']: + merged_data.pop('team_id') opportunities_head.write(merged_data) # delete tail opportunities diff --git a/addons/crm/tests/test_crm_lead.py b/addons/crm/tests/test_crm_lead.py index 75126ea567b..adc584d7180 100644 --- a/addons/crm/tests/test_crm_lead.py +++ b/addons/crm/tests/test_crm_lead.py @@ -680,17 +680,17 @@ class TestCRMLead(TestCrmCommon): self.assertEqual(lead.stage_id, self.stage_team1_1) self.assertEqual(lead.team_id, self.sales_team_1) self.assertEqual( - leads[0].date_last_stage_update, updated_time, - 'FIXME: set same stage when changing user_id, should not update') + leads[0].date_last_stage_update, first_now, + 'Setting same stage when changing user_id, should not update') self.assertEqual( leads[0].date_open, updated_time, 'User assigned -> assign date updated') self.assertEqual( - leads[1].date_last_stage_update, updated_time, - 'FIXME: set same stage when changing user_id, should not update') + leads[1].date_last_stage_update, first_now, + 'Setting same stage when changing user_id, should not update') self.assertEqual( leads[1].date_open, updated_time, - 'FIXME: Should not update date_open, was already the same user_id') + 'Should not update date_open, was already the same user_id, but done in batch so ...') # set won changes stage -> update date_last_stage_update newer_time = datetime(2023, 11, 26, 8, 0, 0) @@ -713,16 +713,16 @@ class TestCRMLead(TestCrmCommon): auto_unlink=False, ) leads.flush_recordset() - self.assertEqual(leads[0].date_last_stage_update, updated_time) + self.assertEqual(leads[0].date_last_stage_update, first_now) self.assertEqual(leads[0].date_open, updated_time) self.assertEqual(leads[0].stage_id, self.stage_team1_1) self.assertEqual(leads[0].team_id, self.sales_team_1) self.assertEqual( - leads[1].date_last_stage_update, last_time, - 'FIXME: should not rewrite when setting same stage') + leads[1].date_last_stage_update, newer_time, + 'Should not rewrite when setting same stage') self.assertEqual( - leads[1].date_open, last_time, - 'FIXME: should not rewrite when setting same user_id') + leads[1].date_open, updated_time, + 'Should not rewrite when setting same user_id') self.assertEqual(leads[1].stage_id, self.stage_gen_won) self.assertEqual(leads[1].team_id, self.sales_team_1) self.assertEqual(leads[1].user_id, self.user_sales_salesman) From f21801ec9388d868b9fcf51ca43ee6f10fb40367 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 6 Dec 2023 12:17:20 +0100 Subject: [PATCH 3/4] [FIX] crm: do not update assign date when converting a lead to opp 'date_open' is the date when a user is assigned to a lead / opportunity. It should not be set when converting a lead to an opportunity, as those two flows are different. Only setting a responsible should update it. Task-3515225 X-original-commit: a3dbe23b83e7aae108ff72737c69c783e059f603 Part-of: odoo/odoo#161918 --- addons/crm/models/crm_lead.py | 1 - addons/crm/tests/test_crm_lead.py | 18 +++++++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index b93151994ec..4d11d354b29 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -1708,7 +1708,6 @@ class Lead(models.Model): new_team_id = team_id if team_id else self.team_id.id upd_values = { 'type': 'opportunity', - 'date_open': self.env.cr.now(), 'date_conversion': self.env.cr.now(), } if customer != self.partner_id: diff --git a/addons/crm/tests/test_crm_lead.py b/addons/crm/tests/test_crm_lead.py index adc584d7180..6627838f352 100644 --- a/addons/crm/tests/test_crm_lead.py +++ b/addons/crm/tests/test_crm_lead.py @@ -562,11 +562,23 @@ class TestCRMLead(TestCrmCommon): @users('user_sales_manager') def test_crm_lead_stages(self): - lead = self.lead_1.with_user(self.env.user) - self.assertEqual(lead.team_id, self.sales_team_1) + first_now = datetime(2023, 11, 6, 8, 0, 0) + with patch.object(self.env.cr, 'now', lambda: first_now), \ + freeze_time(first_now): + self.lead_1.write({'date_open': first_now}) - lead.convert_opportunity(self.contact_1) + lead = self.lead_1.with_user(self.env.user) + self.assertEqual(lead.date_open, first_now) self.assertEqual(lead.team_id, self.sales_team_1) + self.assertEqual(lead.user_id, self.user_sales_leads) + + second_now = datetime(2023, 11, 8, 8, 0, 0) + with patch.object(self.env.cr, 'now', lambda: second_now), \ + freeze_time(second_now): + lead.convert_opportunity(self.contact_1) + self.assertEqual(lead.date_open, first_now) + self.assertEqual(lead.team_id, self.sales_team_1) + self.assertEqual(lead.user_id, self.user_sales_leads) lead.action_set_won() self.assertEqual(lead.probability, 100.0) From 4672b525aaab1d608b195e9be09688db7d89b9b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 11 Apr 2024 14:10:43 +0200 Subject: [PATCH 4/4] [FIX] crm: fix demo data date_open date_open should not be set when there is no user_id set. Let us be coherent so that we can see the effect of assigning users that updates the date_open field. Task-3515225 X-original-commit: 3720e5aefd7bf9971d0d7b6fc93fc90c1eeecad3 Part-of: odoo/odoo#161918 --- addons/crm/data/crm_lead_demo.xml | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/addons/crm/data/crm_lead_demo.xml b/addons/crm/data/crm_lead_demo.xml index 8ef7a215796..1c49f1180b1 100644 --- a/addons/crm/data/crm_lead_demo.xml +++ b/addons/crm/data/crm_lead_demo.xml @@ -134,6 +134,7 @@ 2 + @@ -173,6 +174,7 @@ Contact: +1 813 494 5005

]]> 2 + @@ -195,6 +197,7 @@ Contact: +1 813 494 5005

]]> 0 + @@ -216,6 +219,7 @@ Contact: +1 813 494 5005

]]> 1 + @@ -251,6 +255,7 @@ ESM Expert
]]> 2 + @@ -272,6 +277,7 @@ ESM Expert
]]> 2 + @@ -307,6 +313,7 @@ Andrew

]]> 2 + @@ -356,6 +363,7 @@ Andrew

]]> + @@ -382,6 +390,7 @@ Andrew

]]> + @@ -405,6 +414,7 @@ Andrew

]]> + @@ -494,6 +504,7 @@ Andrew

]]> + @@ -519,6 +530,7 @@ Andrew

]]> + @@ -552,6 +564,7 @@ Andrew

]]> + @@ -602,6 +615,7 @@ Andrew

]]> + @@ -626,6 +640,7 @@ Andrew

]]> + @@ -646,6 +661,7 @@ Andrew

]]> 2 + @@ -665,6 +681,7 @@ Andrew

]]> 1 + @@ -683,6 +700,7 @@ Andrew

]]> + @@ -703,6 +721,7 @@ Andrew

]]> + @@ -727,6 +746,7 @@ Andrew

]]> + @@ -750,6 +770,7 @@ Andrew

]]> + @@ -816,7 +837,7 @@ Andrew

]]> +33 1 25 54 45 69 1 - + @@ -840,7 +861,7 @@ Andrew

]]> +32 22 33 54 07 1 - +