[IMP] crm: clean code of lead / opportunity convert wizards

PURPOSE

As crm will soon evolve (onchange -> compute, code improvements, better
management of sales teams) cleaning and improving tests is necessary to avoid
regressions.

SPECIFICATIONS

Purpose of this commit is to rewrite code located in convert wizards. Indeed
it holds unnecessary variables, dictionaries manipulation that hides the
real business flow, badly named variables, unclear methods, ...

Logic of those wizards seem quite simple but is hidden behind a wall of
bloated code.

LINKS

Task ID 2056759 (remove crm.partner.binding mixin)
Task ID 2088565 (convert onchange / default to compute)
Community PR odoo#41955
Enterprise PR odoo/enterprise#7322

addons/crm/models/crm_lead.py

@@ -1020,31 +1020,22 @@ def _find_matching_partner(self):
 
         return False
 
-    def handle_partner_assignation(self, action='create', partner_id=False):
-        """ Handle partner assignation during a lead conversion.
-
-        If action is 'create', create new partner with contact and assign lead to
-        new partner_id. Otherwise assign lead to the specified partner_id
-
-        TDE FIXME: docstring does not match code... code seems a bit random.
-
-        :param string action: what has to be done regarding partners (create it, assign an existing one, or nothing)
-        :param int partner_id: partner to assign if any
-        :return: dict(lead_id: partner_id)
+    def handle_partner_assignation(self, force_partner_id=False, create_missing=True):
+        """ Update customer (partner_id) of leads. Purpose is to set the same
+        partner on most leads; either through a newly created partner either
+        through a given partner_id.
+
+        :param int force_partner_id: if set, update all leads to that customer;
+        :param create_missing: for leads without customer, create a new one
+          based on lead information;
         """
-        partner_ids = {}
         for lead in self:
-            if partner_id:
-                lead.partner_id = partner_id
-            if lead.partner_id:
-                partner_ids[lead.id] = lead.partner_id.id
-                continue
-            if action == 'create':
+            if force_partner_id:
+                lead.partner_id = force_partner_id
+            if not lead.partner_id and create_missing:
                 partner = lead._create_lead_partner()
-                partner_id = partner.id
-                partner.team_id = lead.team_id
-            partner_ids[lead.id] = partner_id
-        return partner_ids
+                lead.partner_id = partner.id
+        return True
 
     def allocate_salesman(self, user_ids=None, team_id=False):
         """ Assign salesmen and salesteam to a batch of leads.  If there are more

addons/crm/tests/test_crm_lead.py

@@ -78,7 +78,6 @@ def test_mailgateway(self):
             subtype_xmlid='mail.mt_comment')
         self.assertEqual(message.author_id, self.user_sales_manager.partner_id)
 
-        new_partner_id = new_lead.handle_partner_assignation(action='create')[new_lead.id]
-        new_partner = self.env['res.partner'].with_user(self.user_sales_manager).browse(new_partner_id)
-        self.assertEqual(new_partner.email, 'unknown.sender@test.example.com')
-        self.assertEqual(new_partner.team_id, self.sales_team_1)
+        new_lead.handle_partner_assignation(create_missing=True)
+        self.assertEqual(new_lead.partner_id.email, 'unknown.sender@test.example.com')
+        self.assertEqual(new_lead.partner_id.team_id, self.sales_team_1)

addons/crm/tests/test_crm_lead_convert.py

@@ -8,6 +8,10 @@
 
 @tagged('lead_manage')
 class TestLeadConvert(crm_common.TestLeadConvertCommon):
+    """
+    TODO: created partner (handle assignation) has team of lead
+    TODO: create partner has user_id  coming from wizard
+    """
 
     @classmethod
     def setUpClass(cls):
@@ -297,7 +301,7 @@ def test_mass_convert_internals(self):
         self.assertEqual(mass_convert.user_id, self.user_sales_salesman)
         self.assertEqual(mass_convert.team_id, self.sales_team_convert)
 
-        mass_convert.mass_convert()
+        mass_convert.action_mass_convert()
         for lead in self.lead_1 | self.lead_w_partner:
             self.assertEqual(lead.type, 'opportunity')
             if lead == self.lead_w_partner:
@@ -313,7 +317,7 @@ def test_mass_convert_internals(self):
         mass_convert.write({
             'user_ids': self.user_sales_salesman.ids,
         })
-        mass_convert.mass_convert()
+        mass_convert.action_mass_convert()
         self.assertEqual(self.lead_w_partner.user_id, self.user_sales_salesman)
         self.assertEqual(self.lead_1.user_id, self.user_sales_leads)  # existing value not forced
 
@@ -338,7 +342,7 @@ def test_mass_convert_w_salesmen(self):
         })
 
         # TDE FIXME: what happens if we mix people from different sales team ? currently nothing, to check
-        mass_convert.mass_convert()
+        mass_convert.action_mass_convert()
 
         for idx, lead in enumerate(self.leads - self.lead_w_email_lost):
             self.assertEqual(lead.type, 'opportunity')

addons/crm/wizard/crm_lead_to_opportunity.py

@@ -90,70 +90,53 @@ def view_init(self, fields):
                 raise UserError(_("Closed/Dead leads cannot be converted into opportunities."))
         return False
 
-    def _convert_opportunity(self, vals):
+    def _convert_opportunity(self, leads, user_ids, team_id=False):
         self.ensure_one()
 
-        res = False
-
-        leads = self.env['crm.lead'].browse(vals.get('lead_ids'))
         for lead in leads:
-            self_def_user = self.with_context(default_user_id=self.user_id.id)
-
-            partner_id = False
-            if self.action != 'nothing':
-                partner_id = self_def_user._create_partner(
-                    lead.id, self.action, vals.get('partner_id') or lead.partner_id.id)
+            if lead.active and self.action != 'nothing':
+                self._apply_convert_action(
+                    lead, self.action, self.partner_id.id or lead.partner_id.id)
 
-            res = lead.convert_opportunity(partner_id, [], False)
-        user_ids = vals.get('user_ids')
+            lead.convert_opportunity(lead.partner_id.id, [], False)
 
         leads_to_allocate = leads
         if self._context.get('no_force_assignation'):
             leads_to_allocate = leads_to_allocate.filtered(lambda lead: not lead.user_id)
 
         if user_ids:
-            leads_to_allocate.allocate_salesman(user_ids, team_id=(vals.get('team_id')))
-
-        return res
+            leads_to_allocate.allocate_salesman(user_ids, team_id=team_id)
 
     def action_apply(self):
         """ Convert lead to opportunity or merge lead and opportunity and open
             the freshly created opportunity view.
         """
         self.ensure_one()
-        values = {
-            'team_id': self.team_id.id,
-        }
-
-        if self.partner_id:
-            values['partner_id'] = self.partner_id.id
 
         if self.name == 'merge':
-            leads = self.with_context(active_test=False).opportunity_ids.merge_opportunity()
-            if not leads.active:
-                leads.write({'active': True, 'activity_type_id': False, 'lost_reason': False})
-            if leads.type == "lead":
-                values.update({'lead_ids': leads.ids, 'user_ids': [self.user_id.id]})
-                self.with_context(active_ids=leads.ids)._convert_opportunity(values)
-            elif not self._context.get('no_force_assignation') or not leads.user_id:
-                values['user_id'] = self.user_id.id
-                leads.write(values)
+            result_opportunity = self.with_context(active_test=False).opportunity_ids.merge_opportunity()
+            if not result_opportunity.active:
+                result_opportunity.write({'active': True, 'activity_type_id': False, 'lost_reason': False})
+
+            if result_opportunity.type == "lead":
+                self._convert_opportunity(result_opportunity, [self.user_id.id], team_id=self.team_id.id)
+            elif not self._context.get('no_force_assignation') or not result_opportunity.user_id:
+                result_opportunity.write({
+                    'user_id': self.user_id.id,
+                    'team_id': self.team_id.id,
+                })
         else:
-            leads = self.env['crm.lead'].browse(self._context.get('active_ids', []))
-            values.update({'lead_ids': leads.ids, 'user_ids': [self.user_id.id]})
-            self._convert_opportunity(values)
-
-        return leads[0].redirect_lead_opportunity_view()
-
-    def _create_partner(self, lead_id, action, partner_id):
-        """ Create partner based on action.
-            :return dict: dictionary organized as followed: {lead_id: partner_assigned_id}
-        """
-        #TODO this method in only called by Lead2OpportunityPartner
-        #wizard and would probably diserve to be refactored or at least
-        #moved to a better place
-        if action == 'each_exist_or_create':
-            partner_id = self.env['crm.lead'].browse(lead_id)._find_matching_partner()
-            action = 'create'
-        result = self.env['crm.lead'].browse(lead_id).handle_partner_assignation(action, partner_id)
-        return result.get(lead_id)
+            result_opportunities = self.env['crm.lead'].browse(self._context.get('active_ids', []))
+            self._convert_opportunity(result_opportunities, [self.user_id.id], team_id=self.team_id.id)
+            result_opportunity = result_opportunities[0]
+
+        return result_opportunity.redirect_lead_opportunity_view()
+
+    def _apply_convert_action(self, lead, action, partner_id):
+        # used to propagate user_id (salesman) on created partners during conversion
+        return lead.with_context(
+            default_user_id=self.user_id.id
+        ).handle_partner_assignation(
+            force_partner_id=partner_id,
+            create_missing=(action == 'create')
+        )

addons/crm/wizard/crm_lead_to_opportunity_mass.py

@@ -55,20 +55,18 @@ def _onchange_deduplicate(self):
 
         self.opportunity_ids = self.env['crm.lead'].browse(leads_with_duplicates)
 
-    def _convert_opportunity(self, vals):
+    def _convert_opportunity(self, leads, user_ids, team_id=False):
         """ When "massively" (more than one at a time) converting leads to
             opportunities, check the salesteam_id and salesmen_ids and update
             the values before calling super.
         """
         self.ensure_one()
-        salesteam_id = self.team_id.id if self.team_id else False
         salesmen_ids = []
         if self.user_ids:
             salesmen_ids = self.user_ids.ids
-        vals.update({'user_ids': salesmen_ids, 'team_id': salesteam_id})
-        return super(Lead2OpportunityMassConvert, self)._convert_opportunity(vals)
+        return super(Lead2OpportunityMassConvert, self)._convert_opportunity(leads, salesmen_ids, team_id=team_id)
 
-    def mass_convert(self):
+    def action_mass_convert(self):
         self.ensure_one()
         if self.name == 'convert' and self.deduplicate:
             merged_lead_ids = set()
@@ -88,3 +86,9 @@ def mass_convert(self):
             self = self.with_context(active_ids=list(active_ids))  # only update active_ids when there are set
         no_force_assignation = self._context.get('no_force_assignation', not self.force_assignation)
         return self.with_context(no_force_assignation=no_force_assignation).action_apply()
+
+    def _apply_convert_action(self, lead, action, partner_id):
+        if self.action == 'each_exist_or_create':
+            partner_id = lead._find_matching_partner()
+            action = 'create'
+        return super(Lead2OpportunityMassConvert, self)._apply_convert_action(lead, action, partner_id)

addons/crm/wizard/crm_lead_to_opportunity_mass_views.xml

@@ -43,7 +43,7 @@
                     </group>
                 </group>
                 <footer>
-                    <button name="mass_convert" string="Convert to Opportunities" type="object" class="btn-primary"/>
+                    <button string="Convert to Opportunities" name="action_mass_convert" type="object" class="btn-primary"/>
                     <button string="Cancel" class="btn-secondary" special="cancel"/>
                 </footer>
             </form>
@@ -56,7 +56,6 @@
         res_model="crm.lead2opportunity.partner.mass"
         binding_model="crm.lead" binding_views="list"
         view_mode="form" target="new"
-        context="{'mass_convert' : True}"
         view_id="view_crm_lead2opportunity_partner_mass"
         groups="sales_team.group_sale_salesman"
     />

addons/sale_crm/wizard/crm_opportunity_to_quotation.py

@@ -46,16 +46,6 @@ def action_apply(self):
         """
         self.ensure_one()
         if self.action != 'nothing':
-            self.lead_id.write({
-                'partner_id': self.partner_id.id if self.action == 'exist' else self._create_partner()
-            })
+            self.lead_id.handle_partner_assignation(force_partner_id=self.partner_id.id, create_missing=(self.action == 'create'))
             self.lead_id._onchange_partner_id()
         return self.lead_id.action_new_quotation()
-
-    def _create_partner(self):
-        """ Create partner based on action.
-            :return int: created res.partner id
-        """
-        self.ensure_one()
-        result = self.lead_id.handle_partner_assignation(action='create')
-        return result.get(self.lead_id.id)

addons/website_crm/models/website_visitor.py

@@ -50,11 +50,8 @@ def _message_composer_prepare(self):
             sorted_leads = self.lead_ids._sort_by_confidence_level(reverse=True)
             partners = sorted_leads.mapped('partner_id')
             if not partners:
-                main_lead = self.lead_ids[0]
-                partner_id = main_lead.handle_partner_assignation(action='create')[lead.id]
-                if not main_lead.partner_id:
-                    main_lead.partner_id = partner_id
-                self.partner_id = partner_id
+                self.lead_ids[0].handle_partner_assignation(create_missing=True)
+                self.partner_id = self.lead_ids[0].partner_id.id
             return True
         return res