Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMP] crm: smooth lead assign #154857

Closed

Conversation

jeh-odoo
Copy link
Contributor

We make sure that a user don't receive more than his daily
lead quota if the cron run often. The lead assignment now
only work on a daily basis. The work_days params is replaced
by force_quota which when set to True reassigns the daily quota
ignoring the lead already assigned.

The lead assignment do the following steps:

  • Split the member per saleteams
  • Find all leads to be assigned for a team
  • Sort members based on the leads received in the last 24 hours
  • Assign the lead in a round robin manner:
    • Find the first member possible
    • Assign the lead and move the member to the end of the list
      if quota is not reach or remove it if it is

task-3514687

@robodoo
Copy link
Contributor

robodoo commented Feb 21, 2024

@C3POdoo C3POdoo added the RD research & development, internal work label Feb 21, 2024
@C3POdoo C3POdoo requested a review from a team February 21, 2024 11:02
@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch 2 times, most recently from 082fa6a to facdbb6 Compare February 26, 2024 11:11
Copy link
Contributor

@Odoonan Odoonan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jeh-odoo ,

It looks pretty good to me, I've left a few remarks but mostly small things.

Thanks for having a look & cheers!

addons/crm/models/crm_lead.py Outdated Show resolved Hide resolved
addons/crm/models/crm_team.py Outdated Show resolved Hide resolved
addons/crm/models/crm_team.py Outdated Show resolved Hide resolved
@@ -26,16 +25,39 @@ class TeamMember(models.Model):
lead_month_count = fields.Integer(
'Leads (30 days)', compute='_compute_lead_month_count',
help='Lead assigned to this member those last 30 days')
lead_day_count = fields.Integer(
'Leads (last days)', compute='_compute_lead_day_count',
help='Lead assigned to this member this last days')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

days -> day

This method follows the following heuristic
* split member per team
* find all the lead to be assigned in the team
* Sort member per lead receive in the last 24h
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lead receive -> number of leads received

addons/crm/models/crm_team_member.py Outdated Show resolved Hide resolved
to_assign = self.assignment_max * assign_ratio
compensation = max(0, self.assignment_max - (self.lead_month_count + to_assign)) * 0.2
return round(to_assign + compensation)
quota = math.floor(self.assignment_max / 30.0 + 0.5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes the rounding. Actually the default round may be doing this: https://en.wikipedia.org/wiki/Rounding#:~:text=Whenever%20the%20fractional%20part%20is,round%20down%2C%20and%20so%20on.
But I guess this is arbitrary to have those with 45 have as much as those with 75 (rounded 2 both) instead of having 1 more, so I guess your way is clearer :p

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah indeed ^^ Kind of weird why they do that imo

Yes I saw that in the test and didn't make really sense to have the same value for this much difference in the assignment max

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It demonstrates a monthly quota is not really helpful for a day-based assign. Should we keep this configuration or move towards a "member should have X leads / day + maximum Y leads / month" configuration ? As it works for us maybe I am going too far and we should not really care, but I feel like we end up giving 1 or 2 leads to people anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed we will end up with 1 or 2 lead per members in most cases. I'm not sure it's worth it to add another field (for leads / day) in the quota computation. For now the leads / month should be good enough as people will generally based their goals on a monthly basis instead of a daily one. I think that it will add too much complexity for the daily quota purpose just to get in the end probably the same number.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I always struggle with those rounding stuff, could you explain 1/ what is the impact of our rounding (aka what it is going to give as result) 2/ why this +0.5 :D ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so here (x being an integer number) the rounding will round up for any value going for [x.5 : x.999...] and round down for [x : x.499...]. It's the + 0.5 that allows this rounding as floor([x.5 : x.999...] + 0.5) will always result in x+1 and floor([x : x.499...] + 0.5) to x. That way we ensure that the rounding always act the same for the same decimals.
Before, we tried round() but the x.5 value was rounded in a round robin manner alternating between a round up and down each time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this kind of stuff on the worldwide internets; same odoo floats internals use copysign (but far from being a decimal representation expert ^^)

def _round(x):
    return int(x + 0.5 * copysign(1, x))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up for copysign question

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems it works so at this point I think we can use the float_round tools instead then

with self.with_user('user_sales_manager'):
teams_data, members_data = self.sales_team_1._action_assign_leads(work_days=4)
teams_data, members_data = self.sales_team_1._action_assign_leads(True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably write force_quota=True to make test reading easier if you don't mind (especially this one :p)
Same below

self.assertGreaterEqual(len(leads_st1), 75) # 75 * 600 / 300 * 0.5 (because random)
self.assertGreaterEqual(len(leads_st2), 90) # 90 * 600 / 300 * 0.5 (because random)
self.assertGreaterEqual(len(leads_st3), 135) # 135 * 600 / 300 * 0.5 (because random)
self.assertEqual(len(leads_st1), 165) # 75 * 600 / 300 * 1.5 (because random)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, the comments are not correct anymore. Side note: the randomness now relies on the team allocation only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The seed gives those numbers but I wonder why we checked it to be between 0.5 and 1.5 times the expected value before? Was this arbitrary? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I forgot to remove them. Seems it was arbitrary and the remnants of the test without the seed. So the comments can be remove

addons/crm/tests/test_crm_lead_assignment.py Show resolved Hide resolved
@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch from facdbb6 to 994340e Compare February 28, 2024 14:19
Copy link
Contributor Author

@jeh-odoo jeh-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Odoonan

Thanks for having a look.
Changes done.

Cheers!

addons/crm/models/crm_lead.py Outdated Show resolved Hide resolved
addons/crm/models/crm_team.py Outdated Show resolved Hide resolved
addons/crm/models/crm_team_member.py Outdated Show resolved Hide resolved
to_assign = self.assignment_max * assign_ratio
compensation = max(0, self.assignment_max - (self.lead_month_count + to_assign)) * 0.2
return round(to_assign + compensation)
quota = math.floor(self.assignment_max / 30.0 + 0.5)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah indeed ^^ Kind of weird why they do that imo

Yes I saw that in the test and didn't make really sense to have the same value for this much difference in the assignment max

self.assertGreaterEqual(len(leads_st1), 75) # 75 * 600 / 300 * 0.5 (because random)
self.assertGreaterEqual(len(leads_st2), 90) # 90 * 600 / 300 * 0.5 (because random)
self.assertGreaterEqual(len(leads_st3), 135) # 135 * 600 / 300 * 0.5 (because random)
self.assertEqual(len(leads_st1), 165) # 75 * 600 / 300 * 1.5 (because random)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I forgot to remove them. Seems it was arbitrary and the remnants of the test without the seed. So the comments can be remove

addons/crm/tests/test_crm_lead_assignment.py Show resolved Hide resolved
addons/crm_iap_enrich/models/crm_lead.py Outdated Show resolved Hide resolved
@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch from 994340e to af1226c Compare February 28, 2024 14:55
@Odoonan
Copy link
Contributor

Odoonan commented Feb 29, 2024

Hey @jeh-odoo ,
I think we can move this one forward :)
Thanks for last changes !

Copy link
Contributor

@awa-odoo awa-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hoy @jeh-odoo 👋

Saw this one while passing by and left some comments & suggestions.

I do not assign myself as official reviewer though as I know a certain someone that is an expert on this topic and that will want to look at the PR 😄

Thanks for your work & cheers!

addons/crm/models/crm_team.py Show resolved Hide resolved

def _get_lead_from_date(self, date_from):
Lead = self.env['crm.lead'].with_context(active_test=False)
return {(g[0].id, g[1].id): g[2] for g in Lead._read_group(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return {(user.id, team.id): count for user, team, count in Lead._read_group( maybe? A bit more readable.

@api.depends('user_id', 'crm_team_id')
def _compute_lead_day_count(self):
day_date = fields.datetime.now() - datetime.timedelta(hours=24)
groups_1_days = self._get_lead_from_date(day_date)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"groups_1_days" -> "daily_leads_counts" ?


@api.depends('user_id', 'crm_team_id')
def _compute_lead_month_count(self):
month_date = fields.Datetime.now() - datetime.timedelta(days=30)
groups_30_days = self._get_lead_from_date(month_date)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"groups_30_days" -> "monthly_leads_counts" ?

@@ -26,16 +25,39 @@ class TeamMember(models.Model):
lead_month_count = fields.Integer(
'Leads (30 days)', compute='_compute_lead_month_count',
help='Lead assigned to this member those last 30 days')
lead_day_count = fields.Integer(
'Leads (last days)', compute='_compute_lead_day_count',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Leads (last 24h)" would maybe be more accurate based on its computation?

addons/crm/models/crm_team_member.py Outdated Show resolved Hide resolved
not member.assignment_optout and
member.assignment_max > 0 and
quota_per_member.get(member, 0) > 0
).sorted(key=lambda m: quota_per_member.get(m, 0), reverse=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"m" -> "member".

if not team_members:
continue
leads_per_member = {
member: self.env["crm.lead"].search(member._get_assignment_domain())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I see the need to extract "_get_assignment_domain" to a separate method.

Comment on lines 128 to 132
leads = self.env["crm.lead"].search([
('user_id', '=', False),
('date_open', '=', False),
('team_id', '=', team.id),
], order='probability DESC, id')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm.

If we're going to fetch all the leads per team, isn't it more efficient to fetch them all at once before the loop and make a dict "leads_per_team" ?

And then if we're going to do that, do we need to search in the members loop, or can we change to:

leads_per_member = {
    member: leads_per_team.get(member.crm_team_id.id, self.env['crm.lead']).filtered_domain(
                   literal_eval(member.assigment_domain or '[]')
}

(No need to re-apply the rest of the domain as it's part of the initial domain of "leads_per_team").

Seems like it would greatly lower the necessary amount of searches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea indeed!

Comment on lines 134 to 135
if not team_members:
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was already "continued" here above, does not seem necessary?

@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch from af1226c to f7c0447 Compare March 6, 2024 13:16
Copy link
Contributor

@tde-banana-odoo tde-banana-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zboing ! Some Friday reading, left some thoughts and pray... comments. Did not check tests, will be part of a "pre merge polish" probably :) . Feel free to comment or yell at comments, to be sure I understood everything !

@@ -193,68 +193,34 @@ def _alias_get_creation_values(self):
# ------------------------------------------------------------

@api.model
def _cron_assign_leads(self, work_days=None):
def _cron_assign_leads(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could get a force_quota=False parameter, so that you can easily tweak the cron call (i.e. code of ir_cron_crm_lead_assign) if you want.

addons/crm/models/crm_team.py Show resolved Hide resolved
addons/crm/models/crm_team.py Show resolved Hide resolved
@@ -384,6 +355,7 @@ def _allocate_leads(self, work_days=1):
* without team, without user -> not assigned;
* not in a won stage, and not having False/0 (lost) or 100 (won)
probability) -> live leads;
* created in the last week
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be explained: why are we doing this ? I suppose purpose is to avoid activating old stuff, but coudl be great to know why. Also (see other comment) I would make this as a parameter, as I feel like this depends on your business (could imagine that sometimes one week is not enough)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but not sure about the name to use for this one, for now I put from_nb_days but meh...

addons/crm/models/crm_team.py Outdated Show resolved Hide resolved
addons/crm/tests/test_crm_lead_assignment.py Show resolved Hide resolved
to_assign = self.assignment_max * assign_ratio
compensation = max(0, self.assignment_max - (self.lead_month_count + to_assign)) * 0.2
return round(to_assign + compensation)
quota = math.floor(self.assignment_max / 30.0 + 0.5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh

lead.partner_id,
user_ids=member.user_id.ids
)
result_data[member]['assigned'] |= lead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would use a "+" as we should not have duplicates, and it helps avoiding non deterministic behaviors. Unless we could have duplicates issues ? Looking at how it is computed, don't think so.

Suggested change
result_data[member]['assigned'] |= lead
result_data[member]['assigned'] += lead

# ------------------------------------------------------------
# LEAD ASSIGNMENT
# ------------------------------------------------------------

def _assign_and_convert_leads(self, work_days=1):
def _assign_and_convert_leads(self, force_quota=False):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in this new implementation, it is more a per-team assign than a per-member. Previously we made a population of members on which we have to assign leads, based on weight. Now it is more running on teams, take available members, and give them leads.

Not sure there is still something that is really member-based, can't we move the method on the "crm.team" model instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now in the crm.team model

to_assign = self.assignment_max * assign_ratio
compensation = max(0, self.assignment_max - (self.lead_month_count + to_assign)) * 0.2
return round(to_assign + compensation)
quota = math.floor(self.assignment_max / 30.0 + 0.5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It demonstrates a monthly quota is not really helpful for a day-based assign. Should we keep this configuration or move towards a "member should have X leads / day + maximum Y leads / month" configuration ? As it works for us maybe I am going too far and we should not really care, but I feel like we end up giving 1 or 2 leads to people anyway.

@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch 3 times, most recently from c2a1190 to 58c7fc8 Compare April 25, 2024 11:55
Copy link
Contributor Author

@jeh-odoo jeh-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bloups @tde-banana-odoo
Some changes and answers 🙂
In summary:

  • add method parameter for cron (and some config parameter removed)
  • move the teams members assignation in the crm.team model
  • simplify a bit that method
  • some docstring changes
    (And hopefully, runbot will stop crying ^^)
    Cheers!

).sorted(key=lambda member: quota_per_member.get(member, 0), reverse=True)
if not team_members:
continue
leads_per_member = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I try to simplify a bit so no more team_members and leads_per_member

# ------------------------------------------------------------
# LEAD ASSIGNMENT
# ------------------------------------------------------------

def _assign_and_convert_leads(self, work_days=1):
def _assign_and_convert_leads(self, force_quota=False):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now in the crm.team model

to_assign = self.assignment_max * assign_ratio
compensation = max(0, self.assignment_max - (self.lead_month_count + to_assign)) * 0.2
return round(to_assign + compensation)
quota = math.floor(self.assignment_max / 30.0 + 0.5)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed we will end up with 1 or 2 lead per members in most cases. I'm not sure it's worth it to add another field (for leads / day) in the quota computation. For now the leads / month should be good enough as people will generally based their goals on a monthly basis instead of a daily one. I think that it will add too much complexity for the daily quota purpose just to get in the end probably the same number.

addons/crm/tests/test_crm_lead_assignment.py Show resolved Hide resolved
@@ -28,7 +28,8 @@ def _compute_show_enrich_button(self):

@api.model
def _iap_enrich_leads_cron(self):
timeDelta = fields.datetime.now() - datetime.timedelta(hours=1)
ENRICH_DELAY = float(self.env['ir.config_parameter'].sudo().get_param('crm.enrich.delay', default=1))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to say more, I don't like config_parameter either 🙂

@@ -384,6 +355,7 @@ def _allocate_leads(self, work_days=1):
* without team, without user -> not assigned;
* not in a won stage, and not having False/0 (lost) or 100 (won)
probability) -> live leads;
* created in the last week
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but not sure about the name to use for this one, for now I put from_nb_days but meh...

addons/crm/models/crm_team.py Show resolved Hide resolved
Copy link
Contributor

@tde-banana-odoo tde-banana-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for update, added some additional small comments on code, but globally LGTM :) still did not check tests, will probably do next week, so that we close it soon :) . Thanks for retaking it !

addons/crm/models/crm_team.py Show resolved Hide resolved
:param float work_days: see ``CrmTeam.action_assign_leads()``;
:param bool force_quota: Assign the full daily quota without taking into account
the leads already assigned today
:param int from_nb_days: Take into account all leads created in the last nb days (by default 7)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_age_days ? creation_timespan_days ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propositions led me to creation_delta_days, any thoughts ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allez, vendu.

@@ -457,6 +434,7 @@ def _allocate_leads(self, work_days=1):
lead_domain = expression.AND([
literal_eval(team.assignment_domain or '[]'),
[('create_date', '<=', max_create_dt)],
[('create_date', '>', fields.datetime.now() - datetime.timedelta(days=from_nb_days))],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use self.env.cr.now() to be coherent with the other create_date condition (we use cr.now to be sure to avoid non deterministic issues)

def _iap_enrich_leads_cron(self):
timeDelta = fields.datetime.now() - datetime.timedelta(hours=1)
def _iap_enrich_leads_cron(self, enrich_hours_delay=1):
timeDelta = fields.datetime.now() - datetime.timedelta(hours=enrich_hours_delay)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are it, can't we add a batch_size=1000 (because 1000 is cool) parameter, used when searching leads ? That way if you install it on a 2M db you don't try to enrich 2M leads in a single run 🥸

I know it is not really related but hey, cleaning cron one by one is also a good objective.

addons/crm/models/res_config_settings.py Outdated Show resolved Hide resolved
to_assign = self.assignment_max * assign_ratio
compensation = max(0, self.assignment_max - (self.lead_month_count + to_assign)) * 0.2
return round(to_assign + compensation)
quota = math.floor(self.assignment_max / 30.0 + 0.5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I always struggle with those rounding stuff, could you explain 1/ what is the impact of our rounding (aka what it is going to give as result) 2/ why this +0.5 :D ?

addons/crm/tests/test_crm_lead_assignment.py Show resolved Hide resolved
addons/crm/models/crm_team.py Show resolved Hide resolved
@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch from 58c7fc8 to 29071a1 Compare April 26, 2024 15:16
Copy link
Contributor Author

@jeh-odoo jeh-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bloups @tde-banana-odoo
Some changes before the weekend 🙂
Cheers!

to_assign = self.assignment_max * assign_ratio
compensation = max(0, self.assignment_max - (self.lead_month_count + to_assign)) * 0.2
return round(to_assign + compensation)
quota = math.floor(self.assignment_max / 30.0 + 0.5)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so here (x being an integer number) the rounding will round up for any value going for [x.5 : x.999...] and round down for [x : x.499...]. It's the + 0.5 that allows this rounding as floor([x.5 : x.999...] + 0.5) will always result in x+1 and floor([x : x.499...] + 0.5) to x. That way we ensure that the rounding always act the same for the same decimals.
Before, we tried round() but the x.5 value was rounded in a round robin manner alternating between a round up and down each time.

:param float work_days: see ``CrmTeam.action_assign_leads()``;
:param bool force_quota: Assign the full daily quota without taking into account
the leads already assigned today
:param int from_nb_days: Take into account all leads created in the last nb days (by default 7)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propositions led me to creation_delta_days, any thoughts ?

Copy link
Contributor

@tde-banana-odoo tde-banana-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked a bit the code, added some new comments à tete reposée (vaguement), just to be pixel perfect :) .

A number of leads will be assigned each time depending on the daily leads
already assigned.
This allows the assignation process based on the cron to work on a daily basis
without allocating to much leads on members if the cron is executed multiple
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Suggested change
without allocating to much leads on members if the cron is executed multiple
without allocating too much leads on members if the cron is executed multiple

@@ -457,6 +434,7 @@ def _allocate_leads(self, work_days=1):
lead_domain = expression.AND([
literal_eval(team.assignment_domain or '[]'),
[('create_date', '<=', max_create_dt)],
[('create_date', '>', self.env.cr.now() - datetime.timedelta(days=creation_delta_days))],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would say that if we give creation_delta_days=0 we skip the criterion ? Would allow to bootstrap the assignment for example whatever the creation date.

to_assign = self.assignment_max * assign_ratio
compensation = max(0, self.assignment_max - (self.lead_month_count + to_assign)) * 0.2
return round(to_assign + compensation)
quota = math.floor(self.assignment_max / 30.0 + 0.5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up for copysign question

quota = math.floor(self.assignment_max / 30.0 + 0.5)
if force_quota:
return quota
return quota - self.lead_day_count
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real question: lead_day_count seems to count all leads assigned to the member. Shoudln't we exclude lost one ? If purpose is to have a pipe of leads to work on, I suppose we should not count those that are lost as the member won't work on it anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum I suppose it makes sense as if a lead was lost the same day, someone did not really work on it. So I think we could exclude the lost one for the daily count indeed.

counter += 1
member_found = next((
member for member in members_to_assign
if lead in leads_to_assign.filtered_domain(literal_eval(member.assignment_domain or '[]'))), False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that if we have a lot of leads and members, we could do this filtered domain a lot of times. We could probably pre-compute the filtered domai and store it in a given variable / structure so that we don't compute it every time we have to use it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, just to be sure, do you mean a leads_per_member kind of thing like before then ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, seems that way we avoid computing it several times in the loop, isn't it ?

members_to_assign.append(member_found)

if auto_commit and counter % commit_bundle_size == 0:
self._cr.commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: shouldn't we use self.env.cr, to avoid using the internal shortcuts ?

self._cr.commit()

if auto_commit:
self._cr.commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch 2 times, most recently from f372324 to e157dca Compare May 13, 2024 14:31
Copy link
Contributor Author

@jeh-odoo jeh-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bloups @tde-banana-odoo
Changes and answers 🙂
Cheers!

quota = math.floor(self.assignment_max / 30.0 + 0.5)
if force_quota:
return quota
return quota - self.lead_day_count
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum I suppose it makes sense as if a lead was lost the same day, someone did not really work on it. So I think we could exclude the lost one for the daily count indeed.

to_assign = self.assignment_max * assign_ratio
compensation = max(0, self.assignment_max - (self.lead_month_count + to_assign)) * 0.2
return round(to_assign + compensation)
quota = math.floor(self.assignment_max / 30.0 + 0.5)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems it works so at this point I think we can use the float_round tools instead then

counter += 1
member_found = next((
member for member in members_to_assign
if lead in leads_to_assign.filtered_domain(literal_eval(member.assignment_domain or '[]'))), False)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, just to be sure, do you mean a leads_per_member kind of thing like before then ?

@tde-banana-odoo
Copy link
Contributor

@robodoo override=ci/security

Reason: just moving a getattr, used to know if we are in a testing env or not

@tde-banana-odoo
Copy link
Contributor

@robodoo r+ rebase-ff

@robodoo
Copy link
Contributor

robodoo commented May 14, 2024

@tde-banana-odoo you may want to rebuild or fix this PR as it has failed CI.

@robodoo
Copy link
Contributor

robodoo commented May 14, 2024

Merge method set to rebase and fast-forward.

@tde-banana-odoo
Copy link
Contributor

@jeh-odoo runbot keeps red, I think it requires a rebase :)

@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch from e157dca to 2eb0a83 Compare May 14, 2024 13:40
@tde-banana-odoo
Copy link
Contributor

@robodoo r+ rebase-ff

@robodoo
Copy link
Contributor

robodoo commented May 14, 2024

Merge method set to rebase and fast-forward.

@tde-banana-odoo
Copy link
Contributor

Seems planning is not happy .. not sure to understand why 🤔

@tde-banana-odoo
Copy link
Contributor

I see the error on some other builds so it is maybe an nondeterministic one you just achieved to reproduce ^^ ?

@jeh-odoo
Copy link
Contributor Author

I tried locally but the test seems to work so I don't have a clue for now. I will recheck again tomorrow morning and pray the non deterministic god

@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch from 2eb0a83 to 2a47480 Compare May 15, 2024 07:09
@tde-banana-odoo
Copy link
Contributor

Seems we will have to dive into the issue 😬

tfr-odoo and others added 2 commits May 15, 2024 15:46
We make sure that a user does not receive more than his daily
lead quota if the cron runs often. The lead assignment now
only works on a daily basis. The `work_days` params is replaced
by `force_quota` which when set to True reassigns the daily quota
ignoring the lead already assigned.

The lead assignment does the following steps:
- Get quota per members
- Find all leads to be assigned per team
- Sort list of members based on the leads received in the last 24 hours
- Assign the lead in a round robin manner:
	- Find the first member with domain compatible
	- Assign the lead and move the member to the end of the list
	if quota is not reached or remove it if it is
	- Move to the next lead

task-3514687
Add a method parameter for the delay applied for the create date
on the _iap_enrich_leads_cron.

task-3514687
@jeh-odoo jeh-odoo force-pushed the master-crm-smooth-lead-assign-tfr-jeh branch from 2a47480 to b15e06b Compare May 15, 2024 13:46
@tde-banana-odoo
Copy link
Contributor

@robodoo r+ rebase-ff

@robodoo
Copy link
Contributor

robodoo commented May 15, 2024

Merge method set to rebase and fast-forward.

robodoo pushed a commit that referenced this pull request May 15, 2024
We make sure that a user does not receive more than his daily
lead quota if the cron runs often. The lead assignment now
only works on a daily basis. The `work_days` params is replaced
by `force_quota` which when set to True reassigns the daily quota
ignoring the lead already assigned.

The lead assignment does the following steps:
- Get quota per members
- Find all leads to be assigned per team
- Sort list of members based on the leads received in the last 24 hours
- Assign the lead in a round robin manner:
	- Find the first member with domain compatible
	- Assign the lead and move the member to the end of the list
	if quota is not reached or remove it if it is
	- Move to the next lead

task-3514687

Part-of: #154857
robodoo pushed a commit that referenced this pull request May 15, 2024
Add a method parameter for the delay applied for the create date
on the _iap_enrich_leads_cron.

task-3514687

closes #154857

Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
robodoo pushed a commit that referenced this pull request May 15, 2024
We make sure that a user does not receive more than his daily
lead quota if the cron runs often. The lead assignment now
only works on a daily basis. The `work_days` params is replaced
by `force_quota` which when set to True reassigns the daily quota
ignoring the lead already assigned.

The lead assignment does the following steps:
- Get quota per members
- Find all leads to be assigned per team
- Sort list of members based on the leads received in the last 24 hours
- Assign the lead in a round robin manner:
	- Find the first member with domain compatible
	- Assign the lead and move the member to the end of the list
	if quota is not reached or remove it if it is
	- Move to the next lead

task-3514687

Part-of: #154857
robodoo pushed a commit that referenced this pull request May 15, 2024
Add a method parameter for the delay applied for the create date
on the _iap_enrich_leads_cron.

task-3514687

closes #154857

Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
@robodoo robodoo closed this May 15, 2024
@robodoo robodoo added the 17.3 label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
17.3 RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants