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

dbeguin · 2021-09-23T16:03:49Z

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 :

Out of WS (Won Stage) + Archive : "Leave Won"
Later, if set P = 0% -> trigger "Reach Lost".
In WS + Archive : Reach Won
If proba set to 0 -> Should never happen
Out of WS + Restore: Leave Won
In WS + Restore: Reach Won
If proba was 0 : Leave Lost

This commit adds two fields on crm.lead to ease domains to display/hide
fields, buttons and ribbons:

is_lost: computed on active=False AND proba = 0. Tracking enbaled to follow
the state of the lead in the chatter. (when it was lost or restored)
is_won: related to the stage_id.won_stage. Used to know if the lead is won.
As won = in won stage.

Review the Restore vs Unarchive:

Unarchive = set active=True
Restore = Unarchive + force automated probability (and it's recomputation).
Restore button is displayed only on lost leads.

Probabilty is set to readonly mode when the lead is won or lost. This is to
avoid inconsistencies between the state of the lead and it's probability.

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 and the user will still need to take action to
correctly classify the lead.

Task-2654916

robodoo · 2021-09-23T16:04:42Z

Pull request status dashboard.

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 odoo#77089

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" Later, if set P = 0% -> 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#77089

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" Later, if set P = 0% -> trigger "Reach Lost". > WS + Archive : Reach Won If proba set to 0 -> Should never happen < WS + Restore: Leave Won > WS + Restore: Reach Won If proba was 0 : Leave Lost This commit adds two fields on crm.lead to ease domains to display/hide fields, buttons and ribbons: - is_lost: computed on active=False AND proba = 0. Tracking enbaled to follow the state of the lead in the chatter. (when it was lost or restored) - is_won: related to the stage_id.won_stage. Used to know if the lead is won. As won = in won stage. Review the Restore vs Unarchive: - Unarchive = set active=True - Restore = Unarchive + force automated probability (and it's recomputation). Restore button is displayed only on lost leads. Probabilty is set to readonly mode when the lead is won or lost. This is to avoid inconsistencies between the state of the lead and it's probability. 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 and the user will still need to take action to correctly classify the lead. Task-2654916 COM PR odoo#77089

Odoonan · 2023-12-20T12:49:15Z

Hello @awa-odoo ,

I've updated the code to match the last discussion we had, and double checked the tests too. I think it's getting better.

I've also left in a separate commit the suggestion you made on using old_status new_status around the write call, and after the create call, for you to have a look. In that commit are also two domains (on lead duplicate detection and lead assignment) I've updated and could use some double check :p

Cheers!

awa-odoo

Hello @Odoonan

I left a few very minor additional comments while re-reading but it looks super clean now.

I much prefer the new logic inside your second commit, what do you think about it? We could probably squash both commits if we keep that one in the end.

We should be very close to merge now, may need to ask Thibault for a last check if he wants to.
I know the leads are a usual source of performance issue, but looking at the few domain changes, they seem relatively minor (and pretty much identical to what we had before), so hopefully it will be fine 🤞

Again, thank you for pushing this one to the end!

addons/crm/models/crm_stage.py

addons/crm/tests/test_crm_lead.py

addons/crm/tests/test_crm_pls.py

addons/crm/views/crm_lead_views.xml

awa-odoo · 2023-12-27T10:16:26Z

addons/crm/models/crm_lead.py

-            lead.is_won = lead.probability == 100 and lead.stage_id.is_won
+            lead.is_won = lead.stage_id.is_won and lead.probability == 100


Strange change inside this commit ^^

Will not matter if we squash commits, but I think keeping probability first is better for performance, as if it's not 100%, the code will not need to fetch the stage_id at all.

Hello @awa-odoo

I'm done, just posting a reply to this very comment. Actually, the query counters went up in two tests linked to _action_assign_leads. (1176 > 1167 and 6303 > 6066) When trying to switch stage and proba, it did not, hence the change.

I guessed this had to do with the probability being a computed field, even though stored. As we have queries in the pls flow, _compute_probabilities could lead to extra queries in comparison to reading the stage. I can investigate more if needed.

What do you think? I can at least already move it to Testing BE?

Cheers!

Fine then, just seemed strange in that commit context, but indeed computed field can add queries so switching the check order actually matters.

Greened on my side, thanks again! 🟢

addons/crm/models/crm_lead.py

addons/crm/models/crm_team.py

Purpose ======= Redefine Won / Lost heuristic, apply it everywhere in computations, and align the display (buttons and ribbons) with it. Specification ============= New heuristic: - Lost: Proba = 0 AND archived (previsously: archived) - Won: won stage AND 100% (was previously: won stage and active) (*) That means that even if a lead is archived, it is still considered as won if it is in won stage. That also mean that we should block the edition of probability in won stage as it should always be 100%. Won / lost ribbons display rules should be aligned to the new heuristics. Update on rules for won / lost frequencies: We handle won and lost frequencies in the create and write methods. Based on the old / new values and new heuristic, we check all the followings and update the frequency table each time a lead leaves / reaches a won or lost state. Several can happen simultaneously. - Set to Won Stage: Reach Won (and P will be 100) - Move out from Won stage: Leave Won - Archive: If P is 0, Reach Lost - Unarchive: If P was 0, Leave Lost Clarify the Restore action (vs unarchive): - Unarchive <=> Set active = True - Restore <=> Unarchive + Force automated probability (and its recomputation). Restore button is displayed only on lost leads. This commit adds two computed fields on crm.lead to ease domains to display/hide fields, buttons and ribbons, and to ease the reading of domains based on leads being won / lost. Both have a search method to allow to search on them and to use them in those domains: - is_lost (Tracking enbaled to follow the state of the lead in the chatter.) - is_won The new heuristic also applied in other places: - Ribbons on the lead views - Filters 'Won' / 'Lost' / 'Ongoing' on the crm.lead model - Also on the filters for portal users, on their portal. - Domains including / not including lost / won leads. Constraint (*) ============= In order to have a perfect equivalence between 'won' lead and won stage, we now add a constraint to force 100% probability for leads in a won stage. An upgrade script is added to ensure this as well. We can therefore consider that a won stage implies 100% probability 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 it 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 and the user will still need to take action to correctly classify the lead. Tests ===== Tests are updated to match the new heuristic. Some tests are clarified and comments added to ease further work New tests are added to check new fields and assert new heuristic. Task-2654916 COM PR odoo#77089 UPG PR odoo/upgrade#4074

tde-banana-odoo

Thanks for the PR and your work on it :) made a first reading, added few comments on coding style mainly. Will read again soon to be sure about the changes, but already looking good on my side.

Cheers !

tde-banana-odoo · 2024-05-06T11:55:32Z

addons/crm/models/crm_lead.py

@@ -226,6 +226,8 @@ 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_lost = fields.Boolean(string="Lost", compute='_compute_is_lost', search='_search_is_lost', tracking=True)


It is not recommended to track unstored fields ... indeed purpose of tracking is to do some audit trail on values in DB. If it is not stored it does not make much sense to track it. Maybe it works a bit by accident but a framework / ORM / implementation change could break the tracking of unstored fields (e.g. track it each time it is computed in cache). Moreover we aleady have some logging, so is it really necessary ?

tde-banana-odoo · 2024-05-06T11:56:13Z

addons/crm/models/crm_lead.py

+            return [('active', '=', False), ('probability', '=', 0)]
+        return ['|', ('active', '=', True), ('probability', '!=', 0)]
+
+    @api.depends('stage_id.is_won', 'probability')


If it is not stored, 'stage_id' is sufficient: we don't want to propagate an invalidation if someone changes the stage flag, as anyway it will be computed again soonish.

tde-banana-odoo · 2024-05-06T13:59:54Z

addons/crm/models/crm_lead.py

-        for lead, values in zip(leads, vals_list):
-            if any(field in ['active', 'stage_id'] for field in values):
-                lead._handle_won_lost(values)
+        self._handle_won_lost({}, {


Would keep a recordset-based method, like

Suggested change

self._handle_won_lost({}, {

leads._handle_won_lost({}, {

tde-banana-odoo · 2024-05-06T14:28:16Z

addons/crm/models/crm_lead.py

+        for lead_id, new_status in new_status_by_lead.items():
+            old_status = old_status_by_lead.get(lead_id, {'is_lost': False,
+                                                          'is_won': False})


Would use "self" to be a real recordset based method, like

Suggested change

for lead_id, new_status in new_status_by_lead.items():

old_status = old_status_by_lead.get(lead_id, {'is_lost': False,

'is_won': False})

for lead in self:

new_status = new_status_by_lead.get(

lead.id, {'is_lost': False, 'is_won': False}

)

old_status = old_status_by_lead.get(

lead.id, {'is_lost': False, 'is_won': False}

)

tde-banana-odoo · 2024-05-06T14:30:53Z

addons/crm/models/crm_lead.py

+                raise ValidationError(_("The lead %s cannot be won and lost at the same time.", Lead.browse(lead_id)))
+
+            if new_status['is_lost'] and not old_status['is_lost']:
+                leads_reach_lost_ids.append(lead_id)


Would use recordsets, and do "leads_reach_lost += lead", if we use real records :)

tde-banana-odoo · 2024-05-06T14:34:59Z

addons/crm/tests/common.py

            'type': 'lead',
            'user_id': cls.user_sales_leads.id,
            'team_id': cls.sales_team_1.id,
        })
        cls.lead_team_1_lost.action_set_lost()
        (cls.lead_team_1_won + cls.lead_team_1_lost).flush_recordset()

+        # make lead 1 take team history into account for its automated proba.
+        # it should now be 50% as auto proba. (1 lost 1 won for team 1)
+        cls.lead_1._compute_probabilities()


Shouldn't it be automatically be done if we flush ?

tde-banana-odoo · 2024-05-06T14:40:17Z

addons/crm/models/crm_stage.py

+        res = super().write(vals)
+        if 'is_won' in vals:
+            won_leads = self.env['crm.lead'].search([('stage_id', 'in', self.ids)])
+            if won_leads and vals.get('is_won'):


Can potentially be violent ... shouldn't we have a warning if somehow someone changes the flag ?

tde-banana-odoo · 2024-05-06T14:42:11Z

addons/crm/models/crm_lead.py

+        Lead.browse(leads_reach_lost_ids)._pls_increment_frequencies(to_state='lost')
+        Lead.browse(leads_leave_lost_ids)._pls_increment_frequencies(from_state='lost')
+
+        return leads_reach_won_ids, leads_leave_won_ids, leads_reach_lost_ids, leads_leave_lost_ids


Is the return really necessary ? Does not seems used

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 9dbf4d8 to 56d0d40 Compare September 23, 2021 16:04

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 56d0d40 to cdbf53f Compare September 23, 2021 16:05

C3POdoo requested a review from a team September 23, 2021 16:13

C3POdoo added the RD research & development, internal work label Sep 23, 2021

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from cdbf53f to 194b07f Compare September 24, 2021 14:21

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 194b07f to 71dec52 Compare September 24, 2021 14:46

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 71dec52 to bbfd440 Compare September 27, 2021 09:16

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from bbfd440 to d089601 Compare September 27, 2021 09:52

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from d089601 to 66dd9fa Compare October 18, 2021 15:06

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 66dd9fa to 3ee3f62 Compare October 21, 2021 11:58

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 6841f73 to 6aabd53 Compare October 22, 2021 12:55

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from df5a639 to 48a9c29 Compare November 5, 2021 10:42

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 48a9c29 to ccb4e70 Compare November 5, 2021 10:45

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from ccb4e70 to 12799d4 Compare November 5, 2021 10:51

dbeguin force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 12799d4 to 1f0ae95 Compare November 5, 2021 11:55

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch 2 times, most recently from 348614d to cf2d12b Compare December 19, 2023 16:33

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch from cf2d12b to 867ad28 Compare December 20, 2023 09:39

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 79061a7 to 867ad28 Compare December 20, 2023 13:23

awa-odoo reviewed Dec 27, 2023

View reviewed changes

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 867ad28 to f4c5cba Compare January 3, 2024 12:05

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch 2 times, most recently from b46f725 to d6ee9fe Compare January 3, 2024 15:41

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch 3 times, most recently from ebe4515 to 4f0de7d Compare January 24, 2024 10:28

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 4f0de7d to d9c29eb Compare February 13, 2024 10:20

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch from d9c29eb to d6ece0f Compare April 9, 2024 13:46

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch from d6ece0f to 649466b Compare April 9, 2024 14:37

Odoonan force-pushed the master-crm-pls-won-lost-redefine-dbe branch from 649466b to b3af871 Compare April 9, 2024 15:01

tde-banana-odoo reviewed May 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dbeguin commented Sep 23, 2021 •

edited

robodoo commented Sep 23, 2021

Odoonan commented Dec 20, 2023

awa-odoo left a comment

awa-odoo Dec 27, 2023

Odoonan Jan 5, 2024

awa-odoo Jan 9, 2024

tde-banana-odoo left a comment

tde-banana-odoo May 6, 2024

tde-banana-odoo May 6, 2024

tde-banana-odoo May 6, 2024

tde-banana-odoo May 6, 2024

tde-banana-odoo May 6, 2024

tde-banana-odoo May 6, 2024

tde-banana-odoo May 6, 2024

tde-banana-odoo May 6, 2024

		lead.is_won = lead.probability == 100 and lead.stage_id.is_won
		lead.is_won = lead.stage_id.is_won and lead.probability == 100

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

Are you sure you want to change the base?

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

Conversation

dbeguin commented Sep 23, 2021 • edited

Purpose

Specification

Why oh why ?

robodoo commented Sep 23, 2021

Odoonan commented Dec 20, 2023

awa-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tde-banana-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbeguin commented Sep 23, 2021 •

edited