From d08960108c9400ef4deacdf6e9418c950882f109 Mon Sep 17 00:00:00 2001 From: David Beguin Date: Thu, 23 Sep 2021 17:56:44 +0200 Subject: [PATCH] [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 ( 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 odoo/odoo#77089 --- addons/crm/models/crm_lead.py | 46 +++++++++++---- .../static/tests/tours/crm_forecast_tour.js | 5 +- addons/crm/tests/test_crm_pls.py | 57 +++++++++++++------ addons/crm/views/crm_lead_views.xml | 21 ++++--- 4 files changed, 90 insertions(+), 39 deletions(-) diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index ad463bbfc720b..1e0ecb10a84de 100644 --- a/addons/crm/models/crm_lead.py +++ b/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 = {} diff --git a/addons/crm/static/tests/tours/crm_forecast_tour.js b/addons/crm/static/tests/tours/crm_forecast_tour.js index df08fea71710f..821cc8542444f 100644 --- a/addons/crm/static/tests/tours/crm_forecast_tour.js +++ b/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', diff --git a/addons/crm/tests/test_crm_pls.py b/addons/crm/tests/test_crm_pls.py index b34cc0c682472..7e9dbea7d476f 100644 --- a/addons/crm/tests/test_crm_pls.py +++ b/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,42 +309,59 @@ 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 @@ -345,7 +369,7 @@ def test_predictive_lead_scoring(self): 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) diff --git a/addons/crm/views/crm_lead_views.xml b/addons/crm/views/crm_lead_views.xml index d6d7a18577388..8164bf04d262d 100644 --- a/addons/crm/views/crm_lead_views.xml +++ b/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')]}"/> - - + +
+ - + %%
@@ -604,6 +608,7 @@ + @@ -612,7 +617,7 @@ prorated_revenue - + @@ -624,7 +629,7 @@
+ '|', ('probability', '<', 100), ('is_won', '=', False)]}"> Won Lost
@@ -962,7 +967,7 @@ - +