From 79e3636b71bcf89c67cd3e82dbae32a26e0f115c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 25 Jan 2021 16:04:31 +0000 Subject: [PATCH 1/4] [IMP] crm: split results-oriented from perf-oriented assign tests Purpose is to split assign tests related to results (leads being assigned, with deduplication, compared to team and member domains) from tests related to performance (query counters). Assign process is a random process: randomizing teams leads to searching, assigning and de-duplicating leads in various order. As a lot of search are implied during assign process query counters may vary from run to run. "Heavy" performance test included here ranged from 6K to 6.3K queries. Either we set high counters maximum which makes those tests less useful. Either we avoid random if possible which is what we decided to do by setting the seed of random in tests. SPECIFICATIONS For results oriented tests remove query counters. Those tests should simply ensure results of assign process, notably that even with random choice between team and users we always have leads assigned with right domain and counts. For performance oriented tests using query counters set random seed to have a reproducible scenario and have less random in query counters. LINKS Task ID-2446759 (lead merge priority field management) Task ID-2446883 (query counters fix) PR odoo/odoo#65016 --- addons/crm/tests/__init__.py | 1 + addons/crm/tests/common.py | 3 +- addons/crm/tests/test_crm_lead_assignment.py | 35 ++-- .../crm/tests/test_crm_lead_convert_mass.py | 4 +- addons/crm/tests/test_performances.py | 188 ++++++++++++++++++ 5 files changed, 214 insertions(+), 17 deletions(-) create mode 100644 addons/crm/tests/test_performances.py diff --git a/addons/crm/tests/__init__.py b/addons/crm/tests/__init__.py index bbc44d6dff9..647f4cd0edf 100644 --- a/addons/crm/tests/__init__.py +++ b/addons/crm/tests/__init__.py @@ -10,3 +10,4 @@ from . import test_crm_lead_merge from . import test_crm_activity from . import test_crm_ui from . import test_crm_pls +from . import test_performances diff --git a/addons/crm/tests/common.py b/addons/crm/tests/common.py index 252b06d87ae..1295806a8fd 100644 --- a/addons/crm/tests/common.py +++ b/addons/crm/tests/common.py @@ -421,7 +421,8 @@ class TestLeadConvertCommon(TestCrmCommon): if member.crm_team_id.assignment_domain: self.assertEqual( member_leads.filtered_domain(literal_eval(member.crm_team_id.assignment_domain)), - member_leads + member_leads, + 'Assign domain not matching: %s' % member.crm_team_id.assignment_domain ) class TestLeadConvertMassCommon(TestLeadConvertCommon): diff --git a/addons/crm/tests/test_crm_lead_assignment.py b/addons/crm/tests/test_crm_lead_assignment.py index 750e3df705d..67b04fbb5a9 100644 --- a/addons/crm/tests/test_crm_lead_assignment.py +++ b/addons/crm/tests/test_crm_lead_assignment.py @@ -8,15 +8,15 @@ from unittest.mock import patch from odoo import fields from odoo.addons.crm.tests.common import TestLeadConvertCommon from odoo.tests.common import tagged +from odoo.tools import mute_logger @tagged('lead_assign') -class TestLeadAssign(TestLeadConvertCommon): - """ Test lead assignment feature added in saas-13.5. """ +class TestLeadAssignCommon(TestLeadConvertCommon): @classmethod def setUpClass(cls): - super(TestLeadAssign, cls).setUpClass() + super(TestLeadAssignCommon, cls).setUpClass() cls._switch_to_multi_membership() cls._switch_to_auto_assign() @@ -52,6 +52,11 @@ class TestLeadAssign(TestLeadConvertCommon): self.assertEqual(self.sales_team_convert_m1.lead_month_count, 0) self.assertEqual(self.sales_team_convert_m2.lead_month_count, 0) + +@tagged('lead_assign') +class TestLeadAssign(TestLeadAssignCommon): + """ Test lead assignment feature added in saas-14.2 """ + def test_assign_configuration(self): now_patch = datetime(2020, 11, 2, 10, 0, 0) @@ -98,7 +103,10 @@ class TestLeadAssign(TestLeadConvertCommon): self.assertFalse(self.assign_cron.active) self.assertEqual(self.assign_cron.nextcall, datetime(2020, 11, 1, 10, 0, 0)) - def test_crm_team_assign_duplicates(self): + @mute_logger('odoo.models.unlink') + def test_assign_duplicates(self): + """ Test assign process with duplicates on partner. Allow to ensure notably + that de duplication is effectively performed. """ leads = self._create_leads_batch( lead_type='lead', user_ids=[False], @@ -115,8 +123,7 @@ class TestLeadAssign(TestLeadConvertCommon): lead.probability = (idx + 1) * 10 * ((int(lead.priority) + 1) / 2) with self.with_user('user_sales_manager'): - with self.assertQueryCount(user_sales_manager=467): # crm only: 460 - self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) + self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) # teams assign leads = self.env['crm.lead'].search([('id', 'in', leads.ids)]) # ensure order @@ -148,7 +155,8 @@ class TestLeadAssign(TestLeadConvertCommon): ]) self.assertEqual(len(new_assigned_leads_wpartner), 2) - def test_crm_team_assign_no_duplicates(self): + @mute_logger('odoo.models.unlink') + def test_assign_no_duplicates(self): leads = self._create_leads_batch( lead_type='lead', user_ids=[False], @@ -165,8 +173,7 @@ class TestLeadAssign(TestLeadConvertCommon): lead.probability = (idx + 1) * 10 * ((int(lead.priority) + 1) / 2) with self.with_user('user_sales_manager'): - with self.assertQueryCount(user_sales_manager=206): # crm only: 201 - self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) + self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) # teams assign leads = self.env['crm.lead'].search([('id', 'in', leads.ids)]) # ensure order @@ -183,10 +190,11 @@ class TestLeadAssign(TestLeadConvertCommon): self.assertMemberAssign(self.sales_team_convert_m1, 2) # 30 max on 15 self.assertMemberAssign(self.sales_team_convert_m2, 4) # 60 max on 15 - def test_crm_team_assign_populated(self): + @mute_logger('odoo.models.unlink') + def test_assign_populated(self): """ Test assignment on a more high volume oriented test set in order to - have more insights on query counts. """ - # create leads enough to have interesting counters + test more real life use cases. """ + # create leads enough to assign one month of work _lead_count, _email_dup_count, _partner_count = 500, 50, 150 leads = self._create_leads_batch( lead_type='lead', @@ -240,8 +248,7 @@ class TestLeadAssign(TestLeadConvertCommon): lead.probability = (idx + 1) * 10 * ((int(lead.priority) + 1) / 2) with self.with_user('user_sales_manager'): - with self.assertQueryCount(user_sales_manager=6289): # crm only: 6165 - self.env['crm.team'].browse(sales_teams.ids)._action_assign_leads(work_days=30) + self.env['crm.team'].browse(sales_teams.ids)._action_assign_leads(work_days=30) self.members.invalidate_cache(fnames=['lead_month_count']) self.assertMemberAssign(self.sales_team_1_m1, 45) # 45 max on one month diff --git a/addons/crm/tests/test_crm_lead_convert_mass.py b/addons/crm/tests/test_crm_lead_convert_mass.py index f22679f8ef0..9e379ac3d8c 100644 --- a/addons/crm/tests/test_crm_lead_convert_mass.py +++ b/addons/crm/tests/test_crm_lead_convert_mass.py @@ -24,7 +24,7 @@ class TestLeadConvertMass(crm_common.TestLeadConvertMassCommon): with self.assertQueryCount(user_sales_manager=0): test_leads = self.env['crm.lead'].browse(test_leads.ids) - with self.assertQueryCount(user_sales_manager=254): # crm only: 252 + with self.assertQueryCount(user_sales_manager=255): # crm only: 252 test_leads._handle_salesmen_assignment(user_ids=user_ids, team_id=False) self.assertEqual(test_leads.team_id, self.sales_team_convert | self.sales_team_1) @@ -42,7 +42,7 @@ class TestLeadConvertMass(crm_common.TestLeadConvertMassCommon): with self.assertQueryCount(user_sales_manager=0): test_leads = self.env['crm.lead'].browse(test_leads.ids) - with self.assertQueryCount(user_sales_manager=220): # crm only: 218 + with self.assertQueryCount(user_sales_manager=223): # crm only: 218 test_leads._handle_salesmen_assignment(user_ids=user_ids, team_id=team_id) self.assertEqual(test_leads.team_id, self.sales_team_convert) diff --git a/addons/crm/tests/test_performances.py b/addons/crm/tests/test_performances.py new file mode 100644 index 00000000000..b02c37f6028 --- /dev/null +++ b/addons/crm/tests/test_performances.py @@ -0,0 +1,188 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +import random + +from odoo.addons.crm.tests.test_crm_lead_assignment import TestLeadAssignCommon +from odoo.tests.common import tagged +from odoo.tools import mute_logger + + +@tagged('lead_assign') +class TestLeadAssignPerf(TestLeadAssignCommon): + """ Test performances of lead assignment feature added in saas-14.2 + + Assign process is a random process: randomizing teams leads to searching, + assigning and de-duplicating leads in various order. As a lot of search + are implied during assign process query counters may vary from run to run. + "Heavy" performance test included here ranged from 6K to 6.3K queries. Either + we set high counters maximum which makes those tests less useful. Either we + avoid random if possible which is what we decided to do by setting the seed + of random in tests. + """ + + @classmethod + def setUpClass(cls): + super(TestLeadAssignPerf, cls).setUpClass() + random.seed(2042) + + @mute_logger('odoo.models.unlink', 'odoo.addons.crm.models.crm_team', 'odoo.addons.crm.models.crm_team_member') + def test_assign_perf_duplicates(self): + """ Test assign process with duplicates on partner. Allow to ensure notably + that de duplication is effectively performed. """ + leads = self._create_leads_batch( + lead_type='lead', + user_ids=[False], + partner_ids=[self.contact_1.id, self.contact_2.id, False, False, False], + count=50 + ) + self.assertInitialData() + + # assign probability to leads (bypass auto probability as purpose is not to test pls) + leads = self.env['crm.lead'].search([('id', 'in', leads.ids)]) # ensure order + for idx in range(0, 5): + sliced_leads = leads[idx:len(leads):5] + for lead in sliced_leads: + lead.probability = (idx + 1) * 10 * ((int(lead.priority) + 1) / 2) + + with self.with_user('user_sales_manager'): + with self.assertQueryCount(user_sales_manager=470): # crm only: 465 + self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) + + # teams assign + leads = self.env['crm.lead'].search([('id', 'in', leads.ids)]) # ensure order + leads_st1 = leads.filtered_domain([('team_id', '=', self.sales_team_1.id)]) + leads_stc = leads.filtered_domain([('team_id', '=', self.sales_team_convert.id)]) + self.assertEqual(len(leads_st1), 10) # 2 * 2 * 75 / 30.0 + self.assertEqual(len(leads_stc), 12) # 2 * 2 * 90 / 30.0 + + # salespersons assign + self.members.invalidate_cache(fnames=['lead_month_count']) + self.assertMemberAssign(self.sales_team_1_m1, 3) # 45 max on 2 days + self.assertMemberAssign(self.sales_team_1_m2, 1) # 15 max on 2 days + self.assertMemberAssign(self.sales_team_1_m3, 1) # 15 max on 2 days + self.assertMemberAssign(self.sales_team_convert_m1, 2) # 30 max on 15 + self.assertMemberAssign(self.sales_team_convert_m2, 4) # 60 max on 15 + + # run a second round to finish leads + with self.with_user('user_sales_manager'): + with self.assertQueryCount(user_sales_manager=131): # crm only: 123 + self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) + + # teams assign: everything should be done due to duplicates + leads = self.env['crm.lead'].search([('id', 'in', leads.ids)]) # ensure order + self.assertTrue(len(leads.filtered_domain([('team_id', '=', False)])) == 0) + + # deduplicate should have removed all duplicated linked to contact_1 and contact_2 + new_assigned_leads_wpartner = self.env['crm.lead'].search([ + ('partner_id', 'in', (self.contact_1 | self.contact_2).ids), + ('id', 'in', leads.ids) + ]) + self.assertEqual(len(new_assigned_leads_wpartner), 2) + + @mute_logger('odoo.models.unlink', 'odoo.addons.crm.models.crm_team', 'odoo.addons.crm.models.crm_team_member') + def test_assign_perf_no_duplicates(self): + leads = self._create_leads_batch( + lead_type='lead', + user_ids=[False], + partner_ids=[False], + count=50 + ) + self.assertInitialData() + + # assign probability to leads (bypass auto probability as purpose is not to test pls) + leads = self.env['crm.lead'].search([('id', 'in', leads.ids)]) # ensure order + for idx in range(0, 5): + sliced_leads = leads[idx:len(leads):5] + for lead in sliced_leads: + lead.probability = (idx + 1) * 10 * ((int(lead.priority) + 1) / 2) + + with self.with_user('user_sales_manager'): + with self.assertQueryCount(user_sales_manager=210): # crm only: 206 + self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) + + # teams assign + leads = self.env['crm.lead'].search([('id', 'in', leads.ids)]) # ensure order + leads_st1 = leads.filtered_domain([('team_id', '=', self.sales_team_1.id)]) + leads_stc = leads.filtered_domain([('team_id', '=', self.sales_team_convert.id)]) + self.assertEqual(len(leads_st1), 10) # 2 * 2 * 75 / 30.0 + self.assertEqual(len(leads_stc), 12) # 2 * 2 * 90 / 30.0 + + # salespersons assign + self.members.invalidate_cache(fnames=['lead_month_count']) + self.assertMemberAssign(self.sales_team_1_m1, 3) # 45 max on 2 days + self.assertMemberAssign(self.sales_team_1_m2, 1) # 15 max on 2 days + self.assertMemberAssign(self.sales_team_1_m3, 1) # 15 max on 2 days + self.assertMemberAssign(self.sales_team_convert_m1, 2) # 30 max on 15 + self.assertMemberAssign(self.sales_team_convert_m2, 4) # 60 max on 15 + + @mute_logger('odoo.models.unlink', 'odoo.addons.crm.models.crm_team', 'odoo.addons.crm.models.crm_team_member') + def test_assign_perf_populated(self): + """ Test assignment on a more high volume oriented test set in order to + have more insights on query counts. """ + # create leads enough to have interesting counters + _lead_count, _email_dup_count, _partner_count = 500, 50, 150 + leads = self._create_leads_batch( + lead_type='lead', + user_ids=[False], + partner_count=_partner_count, + country_ids=[self.env.ref('base.be').id, self.env.ref('base.fr').id, False], + count=_lead_count, + email_dup_count=_email_dup_count) + self.assertInitialData() + # assign for one month, aka a lot + self.env.ref('crm.ir_cron_crm_lead_assign').write({'interval_type': 'days', 'interval_number': 30}) + self.env['ir.config_parameter'].set_param('crm.assignment.bundle', '20') + # create a third team + sales_team_3 = self.env['crm.team'].create({ + 'name': 'Sales Team 3', + 'sequence': 15, + 'alias_name': False, + 'use_leads': True, + 'use_opportunities': True, + 'company_id': False, + 'user_id': False, + 'assignment_domain': [('country_id', '!=', False)], + }) + sales_team_3_m1 = self.env['crm.team.member'].create({ + 'user_id': self.user_sales_manager.id, + 'crm_team_id': sales_team_3.id, + 'assignment_max': 60, + 'assignment_domain': False, + }) + sales_team_3_m2 = self.env['crm.team.member'].create({ + 'user_id': self.user_sales_leads.id, + 'crm_team_id': sales_team_3.id, + 'assignment_max': 60, + 'assignment_domain': False, + }) + sales_team_3_m3 = self.env['crm.team.member'].create({ + 'user_id': self.user_sales_salesman.id, + 'crm_team_id': sales_team_3.id, + 'assignment_max': 15, + 'assignment_domain': [('probability', '>=', 10)], + }) + sales_teams = self.sales_teams | sales_team_3 + self.assertEqual(sum(team.assignment_max for team in sales_teams), 300) + self.assertEqual(len(leads), 550) + + # assign probability to leads (bypass auto probability as purpose is not to test pls) + leads = self.env['crm.lead'].search([('id', 'in', leads.ids)]) # ensure order + for idx in range(0, 5): + sliced_leads = leads[idx:len(leads):5] + for lead in sliced_leads: + lead.probability = (idx + 1) * 10 * ((int(lead.priority) + 1) / 2) + + with self.with_user('user_sales_manager'): + with self.assertQueryCount(user_sales_manager=6246): # crm only: 6237 + self.env['crm.team'].browse(sales_teams.ids)._action_assign_leads(work_days=30) + + self.members.invalidate_cache(fnames=['lead_month_count']) + self.assertMemberAssign(self.sales_team_1_m1, 45) # 45 max on one month + self.assertMemberAssign(self.sales_team_1_m2, 15) # 15 max on one month + self.assertMemberAssign(self.sales_team_1_m3, 15) # 15 max on one month + self.assertMemberAssign(self.sales_team_convert_m1, 30) # 30 max on one month + self.assertMemberAssign(self.sales_team_convert_m2, 60) # 60 max on one month + self.assertMemberAssign(sales_team_3_m1, 60) # 60 max on one month + self.assertMemberAssign(sales_team_3_m2, 60) # 60 max on one month + self.assertMemberAssign(sales_team_3_m3, 15) # 15 max on one month From ec382636b35fb6273ce2b2e8679d6c4424fc6a22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 27 Jan 2021 08:54:01 +0000 Subject: [PATCH 2/4] [IMP] crm: add some test about _merge_opportunity internals Purpose is to test internals of lead merge method, notably about values of final opportunity. That way we ensure future changes will be reflected in tests and regressions will be avoided. LINKS Task ID-2446759 (lead merge priority field management) Task ID-2446883 (query counters fix) PR odoo/odoo#65016 --- addons/crm/tests/common.py | 79 ++++++++++++++++++++++++- addons/crm/tests/test_crm_lead_merge.py | 18 ++++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/addons/crm/tests/common.py b/addons/crm/tests/common.py index 1295806a8fd..62ebbd5c141 100644 --- a/addons/crm/tests/common.py +++ b/addons/crm/tests/common.py @@ -2,13 +2,15 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from ast import literal_eval +from contextlib import contextmanager from unittest.mock import patch +from odoo.addons.crm.models.crm_lead import CRM_LEAD_FIELDS_TO_MERGE from odoo.addons.mail.tests.common import MailCase, mail_new_test_user from odoo.addons.phone_validation.tools import phone_validation from odoo.addons.sales_team.tests.common import TestSalesCommon from odoo.fields import Datetime -from odoo import tools +from odoo import models, tools INCOMING_EMAIL = """Return-Path: {return_path} X-Original-To: {to} @@ -44,6 +46,14 @@ Somebody.""" class TestCrmCommon(TestSalesCommon, MailCase): + FIELDS_FIRST_SET = [ + 'name', 'partner_id', 'campaign_id', 'company_id', 'country_id', + 'team_id', 'state_id', 'stage_id', 'medium_id', 'source_id', 'user_id', + 'title', 'city', 'contact_name', 'description', 'mobile', 'partner_name', + 'phone', 'probability', 'expected_revenue', 'street', 'street2', 'zip', + 'create_date', 'date_action_last', 'email_from', 'email_cc', 'website' + ] + @classmethod def setUpClass(cls): super(TestCrmCommon, cls).setUpClass() @@ -299,6 +309,71 @@ class TestCrmCommon(TestSalesCommon, MailCase): new_leads.flush() # compute notably probability return new_leads + @contextmanager + def assertLeadMerged(self, opportunity, leads, **expected): + """ Assert result of lead _merge_opportunity process. This is done using + a context manager in order to save original opportunity (master lead) + values. Indeed those will be modified during merge process. We have to + ensure final values are correct taking into account all leads values + before merging them. + + :param opportunity: final opportunity + :param leads: merged leads (including opportunity) + """ + self.assertIn(opportunity, leads) + + # save opportunity value before being modified by merge process + fields_all = self.FIELDS_FIRST_SET + ['type', 'priority'] + # ensure tests are synchronized with crm code + self.assertTrue(all(field in fields_all for field in CRM_LEAD_FIELDS_TO_MERGE)) + original_opp_values = dict( + (fname, opportunity[fname]) + for fname in fields_all + ) + + def _find_value(lead, fname): + if lead == opportunity: + return original_opp_values[fname] + return lead[fname] + + def _first_set(fname): + values = [_find_value(lead, fname) for lead in leads] + return next((value for value in values if value), False) + + def _get_type(): + values = [_find_value(lead, 'type') for lead in leads] + return 'opportunity' if 'opportunity' in values else 'lead' + + def _get_description(): + values = [_find_value(lead, 'description') for lead in leads] + return '\n\n'.join(value for value in values if value) + + def _get_priority(): + return original_opp_values['priority'] + + try: + # merge process will modify opportunity + yield + finally: + # support specific values caller may want to check in addition to generic tests + for fname, expected in expected.items(): + self.assertEqual(opportunity[fname], expected) + + # classic fields: first not void wins + for fname in fields_all: + opp_value = opportunity[fname] + if fname == 'description': + self.assertEqual(opp_value, _get_description()) + elif fname == 'type': + self.assertEqual(opp_value, _get_type()) + elif fname == 'priority': + self.assertEqual(opp_value, _get_priority()) + else: + self.assertEqual( + opp_value if opp_value or not isinstance(opp_value, models.BaseModel) else False, + _first_set(fname) + ) + class TestLeadConvertCommon(TestCrmCommon): @@ -454,6 +529,7 @@ class TestLeadConvertMassCommon(TestLeadConvertCommon): cls.lead_w_partner = cls.env['crm.lead'].create({ 'name': 'New1', 'type': 'lead', + 'priority': '0', 'probability': 10, 'user_id': cls.user_sales_manager.id, 'stage_id': False, @@ -481,6 +557,7 @@ class TestLeadConvertMassCommon(TestLeadConvertCommon): cls.lead_w_email = cls.env['crm.lead'].create({ 'name': 'LeadEmailAsContact', 'type': 'lead', + 'priority': '2', 'probability': 15, 'email_from': 'contact.email@test.example.com', 'user_id': cls.user_sales_salesman.id, diff --git a/addons/crm/tests/test_crm_lead_merge.py b/addons/crm/tests/test_crm_lead_merge.py index 1bcf1c2a640..f9e534dabe8 100644 --- a/addons/crm/tests/test_crm_lead_merge.py +++ b/addons/crm/tests/test_crm_lead_merge.py @@ -141,3 +141,21 @@ class TestLeadMerge(TestLeadConvertMassCommon): # self.assertEqual(merge_opportunity.team_id, self.sales_team_1) # TDE FIXME: BUT team_id is computed after checking stage, based on wizard's team_id self.assertEqual(merge_opportunity.stage_id, self.stage_team_convert_1) + + @users('user_sales_manager') + def test_merge_method(self): + """ In case of mix, opportunities are on top, and result is an opportunity + + lead_1 -------------------opp----seq=1 + lead_w_partner_company ---opp----seq=1 (ID greater) + lead_w_contact -----------lead---seq=30 + lead_w_email -------------lead---seq=3 + lead_w_partner -----------lead---seq=False + """ + # ensure initial data + (self.lead_w_partner_company | self.lead_1).write({'type': 'opportunity'}) + leads = self.env['crm.lead'].browse(self.leads.ids)._sort_by_confidence_level(reverse=True) + with self.assertLeadMerged(self.lead_1, leads, + name='Nibbler Spacecraft Request', + partner_id=self.contact_company_1): + leads._merge_opportunity(auto_unlink=False, max_length=None) From 3fff6f6a946fe3800d4a1c7b06c758426566b720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 27 Jan 2021 08:55:14 +0000 Subject: [PATCH 3/4] [REF] crm: improve code of value computation in _merge_opportunity Purpose of this commit is to make clearer how final opportunity value are computed or chosen when merging leads. Functionally nothing should change with this commit. LINKS Task ID-2446759 (lead merge priority field management) Task ID-2446883 (query counters fix) PR odoo/odoo#65016 --- addons/crm/models/crm_lead.py | 141 ++++++++++-------- addons/crm/tests/common.py | 8 +- addons/crm/tests/test_crm_lead_merge.py | 3 +- addons/website_crm/models/crm_lead.py | 8 +- .../models/crm_lead.py | 9 +- 5 files changed, 91 insertions(+), 78 deletions(-) diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index 8b7cc6c5978..b82d6708071 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -12,41 +12,50 @@ from odoo.addons.phone_validation.tools import phone_validation from odoo.exceptions import UserError, AccessError from odoo.osv import expression from odoo.tools.translate import _ -from odoo.tools import email_re, email_split, safe_eval +from odoo.tools import email_re, email_split from . import crm_stage _logger = logging.getLogger(__name__) CRM_LEAD_FIELDS_TO_MERGE = [ - 'name', - 'partner_id', + # UTM mixin 'campaign_id', - 'company_id', - 'country_id', - 'team_id', - 'state_id', - 'stage_id', 'medium_id', 'source_id', + # Mail mixin + 'email_cc', + # description + 'name', 'user_id', - 'title', - 'city', - 'contact_name', - 'description', - 'mobile', - 'partner_name', - 'phone', - 'probability', + 'company_id', + 'team_id', + # pipeline + 'stage_id', + # revenues 'expected_revenue', + # dates + 'create_date', + 'date_action_last', + # partner / contact + 'partner_id', + 'title', + 'partner_name', + 'contact_name', + 'email_from', + 'mobile', + 'phone', + 'website', + # address 'street', 'street2', 'zip', - 'create_date', - 'date_action_last', - 'email_from', - 'email_cc', - 'website'] + 'city', + 'state_id', + 'country_id', + # probability + 'probability', +] # Subset of partner fields: sync any of those PARTNER_FIELDS_TO_SYNC = [ @@ -99,6 +108,10 @@ class Lead(models.Model): help='UX: Limit to lead company or all if no company') user_email = fields.Char('User Email', related='user_id.email', readonly=True) user_login = fields.Char('User Login', related='user_id.login', readonly=True) + team_id = fields.Many2one( + 'crm.team', string='Sales Team', check_company=True, index=True, tracking=True, + domain="['|', ('company_id', '=', False), ('company_id', '=', company_id)]", + compute='_compute_team_id', readonly=False, store=True) company_id = fields.Many2one( 'res.company', string='Company', index=True, compute='_compute_company_id', readonly=False, store=True) @@ -109,13 +122,10 @@ class Lead(models.Model): ('lead', 'Lead'), ('opportunity', 'Opportunity')], index=True, required=True, tracking=15, default=lambda self: 'lead' if self.env['res.users'].has_group('crm.group_use_lead') else 'opportunity') + # Pipeline management priority = fields.Selection( crm_stage.AVAILABLE_PRIORITIES, string='Priority', index=True, default=crm_stage.AVAILABLE_PRIORITIES[0][0]) - team_id = fields.Many2one( - 'crm.team', string='Sales Team', check_company=True, index=True, tracking=True, - domain="['|', ('company_id', '=', False), ('company_id', '=', company_id)]", - compute='_compute_team_id', readonly=False, store=True) stage_id = fields.Many2one( 'crm.stage', string='Stage', index=True, tracking=True, compute='_compute_stage_id', readonly=False, store=True, @@ -134,7 +144,7 @@ class Lead(models.Model): 'crm.tag', 'crm_tag_rel', 'lead_id', 'tag_id', string='Tags', help="Classify and analyze your lead/opportunity categories like: Training, Service") color = fields.Integer('Color Index', default=0) - # Opportunity specific + # Revenues expected_revenue = fields.Monetary('Expected Revenue', currency_field='company_currency', tracking=True) prorated_revenue = fields.Monetary('Prorated Revenue', currency_field='company_currency', store=True, compute="_compute_prorated_revenue") recurring_revenue = fields.Monetary('Recurring Revenues', currency_field='company_currency', groups="crm.group_use_recurring_revenues") @@ -205,11 +215,13 @@ class Lead(models.Model): compute='_compute_probabilities', readonly=False, store=True) automated_probability = fields.Float('Automated Probability', compute='_compute_probabilities', readonly=True, store=True) is_automated_probability = fields.Boolean('Is automated probability?', compute="_compute_is_automated_probability") - # External records - meeting_count = fields.Integer('# Meetings', compute='_compute_meeting_count') + # Won/Lost lost_reason = fields.Many2one( 'crm.lost.reason', string='Lost Reason', index=True, ondelete='restrict', tracking=True) + # Statistics + meeting_count = fields.Integer('# Meetings', compute='_compute_meeting_count') + # UX ribbon_message = fields.Char('Ribbon message', compute='_compute_ribbon_message') _sql_constraints = [ @@ -931,15 +943,7 @@ class Lead(models.Model): # MERGE AND CONVERT LEADS / OPPORTUNITIES # ------------------------------------------------------------ - def _merge_get_result_type(self): - """ Define the type of the result of the merge. If at least one of the - element to merge is an opp, the resulting new element will be an opp. - Otherwise it will be a lead. """ - if any(record.type == 'opportunity' for record in self): - return 'opportunity' - return 'lead' - - def _merge_data(self, fields): + def _merge_data(self, fnames=None): """ Prepare lead/opp data into a dictionary for merging. Different types of fields are processed in different ways: - text: all the values are concatenated @@ -950,38 +954,37 @@ class Lead(models.Model): :param fields: list of fields to process :return dict data: contains the merged values of the new opportunity """ + if fnames is None: + fnames = self._merge_get_fields() + fcallables = self._merge_get_fields_specific() + # helpers def _get_first_not_null(attr, opportunities): + value = False for opp in opportunities: - val = opp[attr] - if val: - return val - return False + if opp[attr]: + value = opp[attr].id if isinstance(opp[attr], models.BaseModel) else opp[attr] + break + return value - def _get_first_not_null_id(attr, opportunities): - res = _get_first_not_null(attr, opportunities) - return res.id if res else False - - # process the fields' values + # process the field's values data = {} - for field_name in fields: + for field_name in fnames: field = self._fields.get(field_name) if field is None: continue - if field.type in ('many2many', 'one2many'): - continue - elif field.type == 'many2one': - data[field_name] = _get_first_not_null_id(field_name, self) # take the first not null - elif field.type == 'text': - data[field_name] = '\n\n'.join(it for it in self.mapped(field_name) if it) - else: - data[field_name] = _get_first_not_null(field_name, self) - # define the resulting type ('lead' or 'opportunity') - data['type'] = self._merge_get_result_type() + fcallable = fcallables.get(field_name) + if fcallable and callable(fcallable): + data[field_name] = fcallable(field_name, self) + elif not fcallable and field.type in ('many2many', 'one2many'): + continue + else: + data[field_name] = _get_first_not_null(field_name, self) # take the first not null + return data - def _merge_notify_get_merged_fields_message(self, fields): + def _merge_notify_get_merged_fields_message(self): """ Generate the message body with the changed values :param fields : list of fields to track @@ -992,7 +995,7 @@ class Lead(models.Model): title = "%s : %s\n" % (_('Merged opportunity') if lead.type == 'opportunity' else _('Merged lead'), lead.name) body = [title] _fields = self.env['ir.model.fields'].search([ - ('name', 'in', fields or []), + ('name', 'in', self._merge_get_fields()), ('model_id.model', '=', lead._name), ]) for field in _fields: @@ -1022,12 +1025,10 @@ class Lead(models.Model): """ # TODO JEM: mail template should be used instead of fix body, subject text self.ensure_one() - # mail message's subject - result_type = opportunities._merge_get_result_type() - merge_message = _('Merged leads') if result_type == 'lead' else _('Merged opportunities') + merge_message = _('Merged leads') if self.type == 'lead' else _('Merged opportunities') subject = merge_message + ": " + ", ".join(opportunities.mapped('name')) # message bodies - message_bodies = opportunities._merge_notify_get_merged_fields_message(list(CRM_LEAD_FIELDS_TO_MERGE)) + message_bodies = opportunities._merge_notify_get_merged_fields_message() message_body = "\n\n".join(message_bodies) return self.message_post(body=message_body, subject=subject) @@ -1084,7 +1085,6 @@ class Lead(models.Model): include `self` which is the target crm.lead being the result of the merge. """ self.ensure_one() - self._merge_notify(opportunities) self._merge_opportunity_history(opportunities) self._merge_opportunity_attachments(opportunities) @@ -1122,7 +1122,7 @@ class Lead(models.Model): # merge all the sorted opportunity. This means the value of # the first (head opp) will be a priority. - merged_data = opportunities._merge_data(list(CRM_LEAD_FIELDS_TO_MERGE)) + merged_data = opportunities._merge_data(self._merge_get_fields()) # force value for saleperson and Sales Team if user_id: @@ -1130,6 +1130,8 @@ class Lead(models.Model): if team_id: merged_data['team_id'] = team_id + # log merge message + opportunities_head._merge_notify(opportunities_tail) # merge other data (mail.message, attachments, ...) from tail into head opportunities_head._merge_dependences(opportunities_tail) @@ -1149,6 +1151,15 @@ class Lead(models.Model): return opportunities_head + def _merge_get_fields_specific(self): + return { + 'description': lambda fname, leads: '\n\n'.join(desc for desc in leads.mapped('description') if desc), + 'type': lambda fname, leads: 'opportunity' if any(lead.type == 'opportunity' for lead in leads) else 'lead', + } + + def _merge_get_fields(self): + return list(CRM_LEAD_FIELDS_TO_MERGE) + list(self._merge_get_fields_specific().keys()) + def _convert_opportunity_data(self, customer, team_id=False): """ Extract the data from a lead to create the opportunity :param customer : res.partner record diff --git a/addons/crm/tests/common.py b/addons/crm/tests/common.py index 62ebbd5c141..9aecfad9411 100644 --- a/addons/crm/tests/common.py +++ b/addons/crm/tests/common.py @@ -49,7 +49,7 @@ class TestCrmCommon(TestSalesCommon, MailCase): FIELDS_FIRST_SET = [ 'name', 'partner_id', 'campaign_id', 'company_id', 'country_id', 'team_id', 'state_id', 'stage_id', 'medium_id', 'source_id', 'user_id', - 'title', 'city', 'contact_name', 'description', 'mobile', 'partner_name', + 'title', 'city', 'contact_name', 'mobile', 'partner_name', 'phone', 'probability', 'expected_revenue', 'street', 'street2', 'zip', 'create_date', 'date_action_last', 'email_from', 'email_cc', 'website' ] @@ -323,9 +323,9 @@ class TestCrmCommon(TestSalesCommon, MailCase): self.assertIn(opportunity, leads) # save opportunity value before being modified by merge process - fields_all = self.FIELDS_FIRST_SET + ['type', 'priority'] + fields_all = self.FIELDS_FIRST_SET + ['description', 'type', 'priority'] # ensure tests are synchronized with crm code - self.assertTrue(all(field in fields_all for field in CRM_LEAD_FIELDS_TO_MERGE)) + self.assertTrue(all(field in fields_all for field in CRM_LEAD_FIELDS_TO_MERGE + list(self.env['crm.lead']._merge_get_fields_specific().keys()))) original_opp_values = dict( (fname, opportunity[fname]) for fname in fields_all @@ -359,7 +359,7 @@ class TestCrmCommon(TestSalesCommon, MailCase): for fname, expected in expected.items(): self.assertEqual(opportunity[fname], expected) - # classic fields: first not void wins + # classic fields: first not void wins or specific computation for fname in fields_all: opp_value = opportunity[fname] if fname == 'description': diff --git a/addons/crm/tests/test_crm_lead_merge.py b/addons/crm/tests/test_crm_lead_merge.py index f9e534dabe8..f32381089d8 100644 --- a/addons/crm/tests/test_crm_lead_merge.py +++ b/addons/crm/tests/test_crm_lead_merge.py @@ -157,5 +157,6 @@ class TestLeadMerge(TestLeadConvertMassCommon): leads = self.env['crm.lead'].browse(self.leads.ids)._sort_by_confidence_level(reverse=True) with self.assertLeadMerged(self.lead_1, leads, name='Nibbler Spacecraft Request', - partner_id=self.contact_company_1): + partner_id=self.contact_company_1, + priority='0'): leads._merge_opportunity(auto_unlink=False, max_length=None) diff --git a/addons/website_crm/models/crm_lead.py b/addons/website_crm/models/crm_lead.py index 9612c9995a4..c46da2d7de4 100644 --- a/addons/website_crm/models/crm_lead.py +++ b/addons/website_crm/models/crm_lead.py @@ -37,11 +37,11 @@ class Lead(models.Model): action['context'] = {'search_default_group_by_page': '1'} return action - def _merge_data(self, fields): - merged_data = super(Lead, self)._merge_data(fields) + def _merge_get_fields_specific(self): + fields_info = super(Lead, self)._merge_get_fields_specific() # add all the visitors from all lead to merge - merged_data['visitor_ids'] = [(6, 0, self.visitor_ids.ids)] - return merged_data + fields_info['visitor_ids'] = lambda fname, leads: [(6, 0, leads.visitor_ids.ids)] + return fields_info def website_form_input_filter(self, request, values): values['medium_id'] = values.get('medium_id') or \ diff --git a/addons/website_crm_partner_assign/models/crm_lead.py b/addons/website_crm_partner_assign/models/crm_lead.py index 5a9ffe10f16..261ffe30090 100644 --- a/addons/website_crm_partner_assign/models/crm_lead.py +++ b/addons/website_crm_partner_assign/models/crm_lead.py @@ -26,10 +26,6 @@ class CrmLead(models.Model): copy=True, readonly=False, store=True, help="Last date this case was forwarded/assigned to a partner") - def _merge_data(self, fields): - fields += ['partner_latitude', 'partner_longitude', 'partner_assigned_id', 'date_partner_assign'] - return super(CrmLead, self)._merge_data(fields) - @api.depends("partner_assigned_id") def _compute_date_partner_assign(self): for lead in self: @@ -38,6 +34,11 @@ class CrmLead(models.Model): else: lead.date_partner_assign = fields.Date.context_today(lead) + def _merge_get_fields(self): + fields_list = super(CrmLead, self)._merge_get_fields() + fields_list += ['partner_latitude', 'partner_longitude', 'partner_assigned_id', 'date_partner_assign'] + return fields_list + def assign_salesman_of_assigned_partner(self): salesmans_leads = {} for lead in self: From 9d0965926df6b7d175e5b7027b004bde4062de02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 26 Jan 2021 15:48:08 +0000 Subject: [PATCH 4/4] [IMP] crm: take highest priority when merging leads When merging leads final priority is currently the one of the lead considered as the master lead. This is based on ``_sort_by_confidence_level`` who sorts on confidence but not on priority. We think final priority should be the highest one from merged leads. Indeed if a top priority lead is merged into a low priority one we should not loose the priority information. It is better to keep the final lead with an high priority and let salesmen move it downwards. LINKS Task ID-2446759 (lead merge priority field management) Task ID-2446883 (query counters fix) PR odoo/odoo#65016 --- addons/crm/models/crm_lead.py | 1 + addons/crm/tests/common.py | 3 ++- addons/crm/tests/test_crm_lead_merge.py | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index b82d6708071..deb0df7cdee 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -1155,6 +1155,7 @@ class Lead(models.Model): return { 'description': lambda fname, leads: '\n\n'.join(desc for desc in leads.mapped('description') if desc), 'type': lambda fname, leads: 'opportunity' if any(lead.type == 'opportunity' for lead in leads) else 'lead', + 'priority': lambda fname, leads: max(leads.mapped('priority')) if leads else False, } def _merge_get_fields(self): diff --git a/addons/crm/tests/common.py b/addons/crm/tests/common.py index 9aecfad9411..e8052378d6e 100644 --- a/addons/crm/tests/common.py +++ b/addons/crm/tests/common.py @@ -349,7 +349,8 @@ class TestCrmCommon(TestSalesCommon, MailCase): return '\n\n'.join(value for value in values if value) def _get_priority(): - return original_opp_values['priority'] + values = [_find_value(lead, 'priority') for lead in leads] + return max(values) try: # merge process will modify opportunity diff --git a/addons/crm/tests/test_crm_lead_merge.py b/addons/crm/tests/test_crm_lead_merge.py index f32381089d8..de4aa826b37 100644 --- a/addons/crm/tests/test_crm_lead_merge.py +++ b/addons/crm/tests/test_crm_lead_merge.py @@ -158,5 +158,5 @@ class TestLeadMerge(TestLeadConvertMassCommon): with self.assertLeadMerged(self.lead_1, leads, name='Nibbler Spacecraft Request', partner_id=self.contact_company_1, - priority='0'): + priority='2'): leads._merge_opportunity(auto_unlink=False, max_length=None)