[IMP] crm: redefine won/lost and apply rules in PLS frequencies/views

Purpose
=======
Ribbons and buttons display + PLS won/lost frequency rules should be aligned.
 --> Redefine Won / Lost heuristic and apply it everywhere.

Specification
=============

"New heuristic":
Lost: Proba = 0 AND archived (and not in won stage)
Won: won stage AND 100% (was previously: won stage and active)
	(Won stage implies 100%.)

That means that even if a lead is archived, it is still considered as won if
he is in won stage. That also mean that we should block the edition of
probability in won stage as it should always be 100%.
In the same way, we should block edition on probability if the lead is lost.

Won / lost ribbons display rules should be aligned.

That also implies to change a bit the rules to handle won/lost frequencies

- Set to Won Stage (>WS) : Reach Won
	If probability was 0 and active was False: Leave Lost
- Move out from Won stage (<WS) : Leave Won
	If active was False and Proba = 0 -> This should never happen
- Archive: If set proba=0 or proba was 0: Reach Lost
- Restore: Leave Lost

Special cases :
< WS + Archive : "Leave Won". Proba will be recomputed and if P = 0%,
	this will lead to set active = False and that will trigger "Reach Lost".
> WS + Archive : Reach Won
	If proba set to 0 -> Should never happen
< WS + Restore: Leave Won
> WS + Restore: Reach Won
	Ff proba was 0 : Leave Lost

Why oh why ?
============

Before this change, the old won/lost rules where the following:
Lost: Proba = 0 OR archived
Won: won stage AND active

Which means that both rules could have coexisted. If the lead were in won stage,
active but probability = 0%. (that should not happens but was still allowed).
The objective with the new rules is to use the probability as a necessary
but not sufficient criteriat. Probability makes it impossible to be both
lost and won. But probability is not sufficient to classify a lead as won or
lost.
0% does not mean it's lost, it must also be archived.
100% does not mean it's won, it must also be in won stage.

That will avoid having automatically lost or won leads.
If Predictive lead scoring computes 0% or 100% chance of winning, the lead will
not be flagged yet as lost or won.

Task-2654916
COM PR #77089

addons/crm/models/crm_lead.py

@@ -215,6 +215,7 @@ class Lead(models.Model):
     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")
     # Won/Lost
+    is_won = fields.Boolean(related='stage_id.is_won')
     lost_reason = fields.Many2one(
         'crm.lost.reason', string='Lost Reason',
         index=True, ondelete='restrict', tracking=True)
@@ -671,7 +672,7 @@ def write(self, vals):
         elif 'probability' in vals:
             vals['date_closed'] = False
 
-        if any(field in ['active', 'stage_id'] for field in vals):
+        if any(field in ['active', 'stage_id', 'probability'] for field in vals):
             self._handle_won_lost(vals)
 
         write_result = super(Lead, self).write(vals)
@@ -765,7 +766,12 @@ def _handle_won_lost(self, vals):
         - From lost to Won : We need to decrement corresponding lost count + increment corresponding won count
         in scoring frequency table.
         - From won to lost : We need to decrement corresponding won count + increment corresponding lost count
-        in scoring frequency table."""
+        in scoring frequency table.
+
+        A lead is WON when in won stage (and probability = 100% but that is implied)
+        A lead is LOST when active = False AND probability = 0
+        In every other case, the lead is not won nor lost.
+        """
         Lead = self.env['crm.lead']
         leads_reach_won = Lead
         leads_leave_won = Lead
@@ -774,19 +780,35 @@ def _handle_won_lost(self, vals):
         won_stage_ids = self.env['crm.stage'].search([('is_won', '=', True)]).ids
         for lead in self:
             if 'stage_id' in vals:
-                if vals['stage_id'] in won_stage_ids:
-                    if lead.probability == 0:
+                if vals['stage_id'] in won_stage_ids:  # set to won stage
+                    if lead.probability == 0 and not lead.active:
                         leads_leave_lost |= lead
                     leads_reach_won |= lead
-                elif lead.stage_id.id in won_stage_ids and lead.active:  # a lead can be lost at won_stage
+                elif lead.stage_id.id in won_stage_ids:  # remove from won stage
                     leads_leave_won |= lead
             if 'active' in vals:
-                if not vals['active'] and lead.active:  # archive lead
-                    if lead.stage_id.id in won_stage_ids and lead not in leads_leave_won:
-                        leads_leave_won |= lead
+                if not vals['active'] and lead.active and (lead.probability == 0 or vals.get('probability', 1) == 0):  # archive lead
+                    if lead.stage_id.id in won_stage_ids:  # a lead archived in won stage is still won.
+                        continue
                     leads_reach_lost |= lead
-                elif vals['active'] and not lead.active:  # restore lead
-                    leads_leave_lost |= lead
+            # As "mark as lost" in writing active = False first and then probability afterwards, handle reach lost.
+            if vals.get('probability', 1) == 0 and not lead.active and not lead.stage_id.id in won_stage_ids:
+                leads_reach_lost |= lead
+
+            # Leaving lost is a bit tricky as writing active = True and probability = x
+            # will not be done at same time in most cases. As lost need both conditions (active=False AND P=0), changing
+            # one of the two should trigger 'leave lost'. But this should not be triggered a second time if we modify
+            # the other condition. So the lead should leave lost only if both conditions are valid before writing and
+            # not after (if lead is lost before writing and not after).
+            # But this is only valid outside of the won_stage as the lead has already been removed from lost when
+            # arriving in won stage.
+
+            # restore and set p=x OR set p=x and is archived OR restore and p was 0
+            if (lead.stage_id.id not in won_stage_ids or lead not in leads_leave_lost) and \
+                    vals.get('probability', 0) != 0 and ('active' in vals and vals['active']) or \
+                    vals.get('probability', 0) != 0 and not lead.active or \
+                    ('active' in vals and vals['active']) and lead.probability == 0:
+                leads_leave_lost |= lead
 
         leads_reach_won._pls_increment_frequencies(to_state='won')
         leads_leave_won._pls_increment_frequencies(from_state='won')
@@ -883,14 +905,14 @@ def toggle_active(self):
         return res
 
     def action_set_lost(self, **additional_values):
-        """ Lost semantic: probability = 0 or active = False """
+        """ Lost semantic: probability = 0 AND active = False """
         res = self.action_archive()
         if additional_values:
             self.write(dict(additional_values))
         return res
 
     def action_set_won(self):
-        """ Won semantic: probability = 100 (active untouched) """
+        """ Won semantic: probability = 100 AND stage.is_won """
         self.action_unarchive()
         # group the leads by team_id, in order to write once by values couple (each write leads to frequency increment)
         leads_by_won_stage = {}

addons/crm/static/tests/tours/crm_forecast_tour.js

@@ -78,9 +78,8 @@ tour.register('crm_forecast', {
         content: "wait for date_picker to disappear",
         run: function () {},
     }, {
-        trigger: "input[name=probability]",
-        content: "max out probability",
-        run: "text 100"
+        trigger: "button[name=action_set_won_rainbowman]",
+        content: "win the lead",
     }, {
         trigger: '.o_back_button',
         content: 'navigate back to the kanban view',

addons/crm/tests/test_crm_pls.py

@@ -238,9 +238,14 @@ def test_predictive_lead_scoring(self):
         self.assertEqual(lead_9_country_freq.lost_count, 0.0)  # frequency does not exist
         self.assertEqual(lead_9_email_state_freq.lost_count, 2.1)
 
+        # NOTE: if we touch to active, stage_id and probability, the cache needs to be invalidated in order to get the
+        # correct lead values during the won/lost increment. This is because increment calls _pls_get_lead_pls_values()
+        # that uses SQL queries and not the ORM cache.
+
         # B. Test Live Increment
         leads[4].action_set_lost()
         leads[9].action_set_won()
+        leads.invalidate_cache()
 
         # re-get frequencies that did not exists before
         lead_9_country_freq = LeadScoringFrequency.search([('team_id', '=', leads[9].team_id.id), ('variable', '=', 'country_id'), ('value', '=', leads[9].country_id.id)])
@@ -273,6 +278,7 @@ def test_predictive_lead_scoring(self):
 
         # Restore -> Should decrease lost
         leads[4].toggle_active()
+        leads[4].invalidate_cache()
         self.assertEqual(lead_4_stage_0_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_stage_won_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_country_freq.won_count, 0.1)  # unchanged
@@ -293,6 +299,7 @@ def test_predictive_lead_scoring(self):
 
         # set to won stage -> Should increase won
         leads[4].stage_id = won_stage_id
+        leads[4].invalidate_cache()
         self.assertEqual(lead_4_stage_0_freq.won_count, 2.1)  # + 1
         self.assertEqual(lead_4_stage_won_freq.won_count, 2.1)  # + 1
         self.assertEqual(lead_4_country_freq.won_count, 1.1)  # + 1
@@ -302,50 +309,67 @@ def test_predictive_lead_scoring(self):
         self.assertEqual(lead_4_country_freq.lost_count, 1.1)  # unchanged
         self.assertEqual(lead_4_email_state_freq.lost_count, 2.1)  # unchanged
 
-        # Archive (was won, now lost) -> Should decrease won and increase lost
+        # Archive in won stage -> Should NOT decrease won NOR increase lost
+        # as lost = archived + 0% and WON = won_stage (+ 100%)
         leads[4].toggle_active()
-        self.assertEqual(lead_4_stage_0_freq.won_count, 1.1)  # - 1
-        self.assertEqual(lead_4_stage_won_freq.won_count, 1.1)  # - 1
-        self.assertEqual(lead_4_country_freq.won_count, 0.1)  # - 1
-        self.assertEqual(lead_4_email_state_freq.won_count, 1.1)  # - 1
-        self.assertEqual(lead_4_stage_0_freq.lost_count, 3.1)  # + 1
-        self.assertEqual(lead_4_stage_won_freq.lost_count, 1.1)  # consider stages with <= sequence when lostand as stage is won.. even won_stage lost_count is increased by 1
-        self.assertEqual(lead_4_country_freq.lost_count, 2.1)  # + 1
-        self.assertEqual(lead_4_email_state_freq.lost_count, 3.1)  # + 1
+        leads[4].invalidate_cache()
+        self.assertEqual(lead_4_stage_0_freq.won_count, 2.1)  # unchanged
+        self.assertEqual(lead_4_stage_won_freq.won_count, 2.1)  # unchanged
+        self.assertEqual(lead_4_country_freq.won_count, 1.1)  # unchanged
+        self.assertEqual(lead_4_email_state_freq.won_count, 2.1)  # unchanged
+        self.assertEqual(lead_4_stage_0_freq.lost_count, 2.1)  # unchanged
+        self.assertEqual(lead_4_stage_won_freq.lost_count, 0.1)  # unchanged
+        self.assertEqual(lead_4_country_freq.lost_count, 1.1)  # unchanged
+        self.assertEqual(lead_4_email_state_freq.lost_count, 2.1)  # unchanged
 
-        # Move to original stage -> Should do nothing (as lead is still lost)
+        # Move to original stage -> lead is not won anymore but not lost as probability != 0
         leads[4].stage_id = stage_ids[0]
+        leads[4].invalidate_cache()
+        self.assertEqual(lead_4_stage_0_freq.won_count, 1.1)  # -1
+        self.assertEqual(lead_4_stage_won_freq.won_count, 1.1)  # -1
+        self.assertEqual(lead_4_country_freq.won_count, 0.1)  # -1
+        self.assertEqual(lead_4_email_state_freq.won_count, 1.1)  # -1
+        self.assertEqual(lead_4_stage_0_freq.lost_count, 2.1)  # unchanged
+        self.assertEqual(lead_4_stage_won_freq.lost_count, 0.1)  # unchanged
+        self.assertEqual(lead_4_country_freq.lost_count, 1.1)  # unchanged
+        self.assertEqual(lead_4_email_state_freq.lost_count, 2.1)  # unchanged
+
+        # force proba to 0% -> as already archived, will be lost (lost = archived AND 0%)
+        leads[4].probability = 0
+        leads[4].invalidate_cache()
         self.assertEqual(lead_4_stage_0_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_stage_won_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_country_freq.won_count, 0.1)  # unchanged
         self.assertEqual(lead_4_email_state_freq.won_count, 1.1)  # unchanged
-        self.assertEqual(lead_4_stage_0_freq.lost_count, 3.1)  # unchanged
-        self.assertEqual(lead_4_stage_won_freq.lost_count, 1.1)  # unchanged
-        self.assertEqual(lead_4_country_freq.lost_count, 2.1)  # unchanged
-        self.assertEqual(lead_4_email_state_freq.lost_count, 3.1)  # unchanged
+        self.assertEqual(lead_4_stage_0_freq.lost_count, 3.1)  # +1
+        self.assertEqual(lead_4_stage_won_freq.lost_count, 0.1)  # unchanged - should not increase lost frequency of won stage. + consider stages with <= sequence when lost
+        self.assertEqual(lead_4_country_freq.lost_count, 2.1)  # +1
+        self.assertEqual(lead_4_email_state_freq.lost_count, 3.1)  # +1
 
         # Restore -> Should decrease lost - at the end, frequencies should be like first frequencyes tests (except for 0.0 -> 0.1)
         leads[4].toggle_active()
+        leads[4].invalidate_cache()
         self.assertEqual(lead_4_stage_0_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_stage_won_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_country_freq.won_count, 0.1)  # unchanged
         self.assertEqual(lead_4_email_state_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_stage_0_freq.lost_count, 2.1)  # - 1
-        self.assertEqual(lead_4_stage_won_freq.lost_count, 1.1)  # unchanged - consider stages with <= sequence when lost
+        self.assertEqual(lead_4_stage_won_freq.lost_count, 0.1)  # unchanged - consider stages with <= sequence when lost
         self.assertEqual(lead_4_country_freq.lost_count, 1.1)  # - 1
         self.assertEqual(lead_4_email_state_freq.lost_count, 2.1)  # - 1
 
         # Probabilities should only be recomputed after modifying the lead itself.
         leads[3].stage_id = stage_ids[0]  # probability should only change a bit as frequencies are almost the same (except 0.0 -> 0.1)
         leads[8].stage_id = stage_ids[0]  # probability should change quite a lot
+        leads.invalidate_cache()
 
         # Test frequencies (should not have changed)
         self.assertEqual(lead_4_stage_0_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_stage_won_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_country_freq.won_count, 0.1)  # unchanged
         self.assertEqual(lead_4_email_state_freq.won_count, 1.1)  # unchanged
         self.assertEqual(lead_4_stage_0_freq.lost_count, 2.1)  # unchanged
-        self.assertEqual(lead_4_stage_won_freq.lost_count, 1.1)  # unchanged
+        self.assertEqual(lead_4_stage_won_freq.lost_count, 0.1)  # unchanged
         self.assertEqual(lead_4_country_freq.lost_count, 1.1)  # unchanged
         self.assertEqual(lead_4_email_state_freq.lost_count, 2.1)  # unchanged
 
@@ -360,6 +384,7 @@ def test_predictive_lead_scoring(self):
 
         # Continue to test probability computation
         leads[3].probability = 40
+        leads[3].invalidate_cache()
 
         self.assertEqual(leads[3].is_automated_probability, False)
         self.assertEqual(leads[8].is_automated_probability, True)

addons/crm/views/crm_lead_views.xml

@@ -10,13 +10,16 @@
                             type="object" class="oe_highlight" data-hotkey="w" title="Mark as won"
                             attrs="{'invisible': ['|','|', ('active','=',False), ('probability', '=', 100), ('type', '=', 'lead')]}"/>
                         <button name="%(crm.crm_lead_lost_action)d" string="Lost" data-hotkey="l" title="Mark as lost"
-                            type="action" context="{'default_lead_id': active_id}" attrs="{'invisible': ['|', ('type', '=', 'lead'),('active', '=', False),('probability', '&lt;', 100)]}"/>
+                            type="action" context="{'default_lead_id': active_id}"
+                                attrs="{'invisible': ['|', '|', ('type', '=', 'lead'), ('is_won', '=', True),
+                                                    '&amp;', ('active', '=', False), ('probability', '&lt;', 100)]}"/>
                         <button name="%(crm.action_crm_lead2opportunity_partner)d" string="Convert to Opportunity" type="action" help="Convert to Opportunity"
                             class="oe_highlight" attrs="{'invisible': ['|', ('type', '=', 'opportunity'), ('active', '=', False)]}" data-hotkey="v"/>
                         <button name="toggle_active" string="Restore" type="object" data-hotkey="z"
                             attrs="{'invisible': ['|', ('probability', '&gt;', 0), ('active', '=', True)]}"/>
                         <button name="action_set_lost" string="Lost" type="object" data-hotkey="l" title="Mark as lost"
-                            attrs="{'invisible': ['|', ('type', '=', 'opportunity'), '&amp;', ('probability', '=', 0), ('active', '=', False)]}"/>
+                            attrs="{'invisible': ['|', '|', ('type', '=', 'opportunity'), ('is_won', '=', True),
+                                                 '&amp;', ('probability', '=', 0), ('active', '=', False)]}"/>
                         <field name="stage_id" widget="statusbar"
                             options="{'clickable': '1', 'fold_field': 'fold'}"
                             domain="['|', ('team_id', '=', team_id), ('team_id', '=', False)]"
@@ -45,8 +48,8 @@
                                 </div>
                             </button>
                         </div>
-                        <widget name="web_ribbon" title="Lost" bg_color="bg-danger" attrs="{'invisible': ['|', ('probability', '&gt;', 0), ('active', '=', True)]}"/>
-                        <widget name="web_ribbon" title="Won" attrs="{'invisible': [('probability', '&lt;', 100)]}" />
+                        <widget name="web_ribbon" title="Lost" bg_color="bg-danger" attrs="{'invisible': [('probability', '&gt;', 0), ('active', '=', True)]}"/>
+                        <widget name="web_ribbon" title="Won" attrs="{'invisible': [('probability', '&lt;', 100), ('is_won', '=', False)]}" />
                         <div class="oe_title">
                             <label for="name" string="Lead" attrs="{'invisible': [('type', '=', 'opportunity')]}"/>
                             <label for="name" attrs="{'invisible': [('type', '=', 'lead')]}"/>
@@ -85,8 +88,9 @@
                                         </small>
                                     </div>
                                     <div id="probability" class="o_row d-flex">
+                                        <field name="is_won" invisible="1"/>
                                         <field name="is_automated_probability" invisible="1"/>
-                                        <field name="probability" widget="float" class="oe_inline"/>
+                                        <field name="probability" widget="float" class="oe_inline" attrs="{'readonly': ['|', ('is_won', '=', True), '&amp;', ('active', '=', False), ('probability', '=', 0)]}"/>
                                         <span class="oe_grey"> %%</span>
                                     </div>
                                 </div>
@@ -604,6 +608,7 @@
                 </xpath>
                 <xpath expr="//kanban" position="inside">
                     <field name="date_deadline" allow_group_range_value="true"/>
+                    <field name="is_won"/>
                 </xpath>
                 <xpath expr="//field[@name='expected_revenue']" position="replace">
                     <field name="prorated_revenue"/>
@@ -612,7 +617,7 @@
                     <attribute name="sum_field">prorated_revenue</attribute>
                 </xpath>
                 <xpath expr="//t[@t-set='lost_ribbon']" position="after">
-                    <t t-set="won_ribbon" t-value="record.active.raw_value and record.probability and record.probability.raw_value == 100"/>
+                    <t t-set="won_ribbon" t-value="record.is_won.raw_value and record.probability and record.probability.raw_value == 100"/>
                 </xpath>
                 <xpath expr="//div[contains(@t-attf-class, 'oe_kanban_card')]" position="attributes">
                     <attribute name="t-attf-class">
@@ -624,7 +629,7 @@
                 <xpath expr="//div[hasclass('ribbon')]" position="replace">
                     <div class="ribbon ribbon-top-right"
                         attrs="{'invisible': ['&amp;', '|', ('probability', '&gt;', 0), ('active', '=', True),
-                            '|', ('probability', '&lt;', 100), ('active', '=', False)]}">
+                            '|', ('probability', '&lt;', 100), ('is_won', '=', False)]}">
                         <span t-if="won_ribbon" class="bg-success">Won</span>
                         <span t-elif="lost_ribbon" class="bg-danger">Lost</span>
                     </div>
@@ -962,7 +967,7 @@
                     <filter string="Creation Date" name="creation_date" date="create_date"/>
                     <filter string="Closed Date" name="close_date" date="date_closed"/>
                     <separator/>
-                    <filter string="Won" name="won" domain="['&amp;', ('active', '=', True), ('stage_id.is_won', '=', True)]"/>
+                    <filter string="Won" name="won" domain="['&amp;', ('probability', '=', 100), ('stage_id.is_won', '=', True)]"/>
                     <filter string="Lost" name="lost" domain="['&amp;', ('active', '=', False), ('probability', '=', 0)]"/>
                     <separator/>
                     <filter invisible="1" string="Overdue Opportunities" name="overdue_opp" domain="['&amp;', ('date_closed', '=', False), ('date_deadline', '&lt;', context_today().strftime('%Y-%m-%d'))]"/>