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

[ADD] event_crm: create leads from attendees #43845

Open
wants to merge 6 commits into
base: master
from

Conversation

@jeh-odoo
Copy link
Contributor

jeh-odoo commented Jan 23, 2020

Purpose

This new module allows to create rules in order to generate leads from event's registrations.

Specifications

A new menu, "Lead Generation", in the configuration tab of an event allows to create rules.
This rules are defined by conditions (event, event categories, domain on registration, ...) and the creation of rule can be done in 2 ways :

  • Per Attendee, which creates a lead for each registration (B2C)
  • Per Order, which creates a lead for a set of registrations (B2B) (visible only when event_sale or website_event is installed)

The flow of creation is as follow:
When registrations are created we will check if any rules applies to them. To be applied :

  • The rule must be active
  • If the company is set on the rule, it must match
  • If the event category and the event aren't specified, it's always OK
  • If only one of them is set, it must match
  • If both of them are set, one must match
  • If a domain on the registration is set, it must match

When the conditions are matched, it will create the lead with information based on the rule (lead type, salesperson, sales team, tags) and the registration (name, contact_name, email, ...)

Once a lead is created for an event registration, a stat button displays the number of leads created for the event of the registration and on the lead form, a stat button displays all registrations linked to the lead.

See sub commit for more detail.

Task ID : 2166679

@C3POdoo C3POdoo added the RD label Jan 23, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from 6b9985b to ac4f479 Jan 23, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 23, 2020
Copy link
Contributor

dbeguin left a comment

Hola Muchacho !
First pass of review. I'll continue tomorrow :)

event_id = fields.Many2one('event.event')

registration_ids = fields.Many2many('event.registration')
attendees_count = fields.Integer(compute='_compute_attendees_count')

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

As the lead is the attendee, I think this would better be named registration_count.+ align compute naming

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done


class EventCrm(models.Model):
_name = "event.crm"
_description = "Event CRM"

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

Can you add some more information about what is the exact purpose of this model, when it's used and how it works ?
Also, I think the naming is not really self speaking.
Proposition :

class EventCrmRule(models.Model):
    """
    Rules to generate crm.lead from event registration (+ add some more details)
    """
    _name = "event.crm.rule"
    _description = "Event CRM Rules"

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

renaming not done

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

renaming not done

create_lead = fields.Selection([
('one_per_attendee','One per Attendee'),('one_per_order', 'One per Order')],
default='one_per_attendee', required=True,
help="Select the type that this rule will apply to create leads.\n"
"- One per Attendee will create a lead for each attendee.\n"
"- One per Order will create a lead for a group of attendees.")
Comment on lines 13 to 18

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

rename this to type or rule_type or even lead_creation_type or something that reflects more the purpose. create_lead looks more like a boolean.

You can use the examples from the specs, I think this would be more clear:

One per Attendee : One lead is created for each attendee created (default) 
e.g. : I go to the "Salon de l'Auto" page and register for me and 3 friends, 4 attendees are created and 4 leads are created. [Rather B2C]
One per Order : We create a single lead per "registration" (="ticket batch")
e.g. : I go to the Odoo Experience page and register for me and 3 colleagues, 4 attendees are created but a single lead is created.  [Rather B2B]

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

for record in self:
record.leads_count = len(record.lead_ids)

def action_get_lead(self):

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

rename this to "action_view_event_leads"

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

@api.model
def create(self, vals):
registration = super(EventRegistration, self).create(vals)
event_crm = self.env['event.crm'].search([])

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

registration_rules = ...

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 27, 2020

Contributor

well, I was talking about event_crm line. (event registration are not = to registration rules :) )

results = super(EventRegistration, self).create(vals)
registration_rules = self.env['event.crm'].search([])
def _check_rules(self, event_id, registration_ids):
"""Check if the rule applied for the registration_ids created
and send the parameters to create leads."""
if len(self.sudo().registration_ids) == 0 or not self.active:
return
if event_id.company_id == self.company_id or not self.company_id:
if (
(not self.event_type_ids and not self.event_id) or
(event_id.event_type_id in self.event_type_ids and not self) or
(not self.event_type_ids and self.event_id == event_id) or
(event_id.event_type_id in self.event_type_ids or self.event_id == event_id)
):
values = self._prepare_create_leads_values()
self.sudo()._create_leads(registration_ids, values)
Comment on lines 64 to 77

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

this check method should not create the leads.
You can have

  • one method to check if the rule must apply (return True or False)
  • one to prepare the values (if overridden somewhere, else, prepare the values directly in create_leads method)
  • and one to create the leads.

But in fact, everything should be done in create_leads method:

def _create_leads():
    if self._check_rules():
        values = self._prepare_create_leads_values() (or init values dict directly here)
        # create the leads

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

return description, first_registration

def _create_leads(self, registration_ids, vals):
self.ensure_one()

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

You should instead create a list of values dict and create everything at the end of the loop.
We should avoid making access to DB in a loop as much as possible.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 27, 2020

Contributor

with the ensure_one, it won't be possible to make only one create for each rule.
You don't need to specify the event and the registration_ids as they are on self.
(e.g. event_id is self.event_id)
It could be cool to make something like this:

def _create_leads(self):
    vals_list = []
    for rule in self:
        event_id = rule.event_id
        registrations = rule.registration_ids (a priori, it's not necessary to make the intersection (self.registration_ids & registration_ids) as self.registration_ids is computed based on the rule configuration and should already be correct)
        if self.lead_creation_type == 'one_per_attendee':
            for registration in registrations:
                  ...
                 vals_list.add(lead_values)
                 ...
       else:
             ...
             vals_list.add(lead_values)
              ...
    lead = self.env['crm.lead'].create(vals_list)

Note that you don't need to write the lead_id on the registration as you can set the registration_id on lead_values. (many2many will link in both ways)

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

done, I still need to get the registrations since it's possible that the module can be install later. So if there was registrations created before the installation of a module or a rule creation, it will create lead for this registration or mix them with the actual registrations.

for record in self:
record.attendees_count = len(record.registration_ids)

def action_get_attendee(self):

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

rename to action_view_lead_registrations

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

def _onchange_user_id(self):
if self.user_id and self.user_id.sale_team_id:
self.sales_team_id = self.user_id.sale_team_id
else:
self.sales_team_id = False
Comment on lines 57 to 61

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

self.sales_team_id = self.user_id.sale_team_id if self.user_id else False

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

removed, Luc doesn't want an onchange on this field

def _get_description_lead(self, registration_ids):
description = _("Other Participants:\n")
first_registration = False
for registration in self.registration_ids & registration_ids:
if not first_registration:
first_registration = registration_ids[0]
description += "\t%s %s %s\n" % (registration.name, registration.email, registration.phone)
description = self._additional_content_description(registration, description)
return description, first_registration
Comment on lines 102 to 110

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 23, 2020

Contributor

I think first registration should not be set in the function (especially as it's always the first one, and it has no impact on description).
Only compute description.
_additional_content_description is not needed. if you want to add something in the description in other modules, just override _get_description_lead.
also I would rename the method to _get(_order)_leads_description.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

Copy link
Contributor

dbeguin left a comment

Second part of review.
watch out that many fields you add have missing string, help, etc.. property.
Those are needed to understand directly the purpose of the field.

  • Add docstring in every method or override that do something functional or that are not self speaking.
  • try to avoid search/create in loop -> better create a domain/dict or list with everything you need and call only once the search/create (need create multi : to check)
  • and just for me and to honor the dbe linter, check that every file has a empty line at the end.

Courage !

addons/event_crm_sale/__init__.py Show resolved Hide resolved
addons/website_event_crm/__init__.py Outdated Show resolved Hide resolved

return registrations

@http.route()

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 24, 2020

Contributor

what's the route ?

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

from odoo.http import request

class WebsiteEventCrmSaleController(WebsiteEventSaleController):
@http.route()

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 24, 2020

Contributor

For route override, we prefer to re-write the complete route decorator, it's easier to understand the usage, without having to go to super

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

registration = super(EventRegistration, self).create(vals)
registration.web_registration = True
return registration
Comment on lines 11 to 13

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 24, 2020

Contributor

Better to update vals with web_registration before calling super, to avoid create, then write.
We try to limit the number of DB access as much as possible.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done, moved in controller instead of the create method

<span class="o_stat_text" attrs="{'invisible':[('leads_count','&gt;',1)]}"> Lead</span>
<span class="o_stat_text" attrs="{'invisible':[('leads_count','&lt;=',1)]}"> Leads</span>
Comment on lines 13 to 14

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 24, 2020

Contributor

same here : use Leads no matter what

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

'campaign_id': registration.sudo().campaign_id.id,
'source_id': registration.sudo().source_id.id,
'medium_id': registration.sudo().medium_id.id,
Comment on lines 12 to 14

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 24, 2020

Contributor

I think the sudo thing should not be done here but should be done by caller instead. It's the responsibility of the caller to know if the user should have access or not to the models in certains conditions

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

registrations_before = self.env['event.registration'].search([])
res = super(SaleOrderLine, self)._update_registrations(confirm, cancel_to_draft, registration_data)
registrations_after = self.env['event.registration'].search([])
registrations = registrations_after - registrations_before

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 24, 2020

Contributor

rename in new_registrations

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

registrations = registrations_after - registrations_before
event_crm = self.env['event.crm'].search([])
if len(registrations) > 0:
if not registrations[0].web_registration:

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 24, 2020

Contributor

is it possible to mix registration that are web_registration with registration that are not ?

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

not done yet

if not registrations[0].web_registration:
for so_line in self.filtered('event_id'):
for r in event_crm:
r._check_rules(event_id=so_line.event_id, registration_ids=registrations & registrations.search([('event_id','=',so_line.event_id.id)]))

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 24, 2020

Contributor

first build a domain to search only once the registrations based on all so_line.
Then call the _create_leads method for each rules.

Or we could call create_lead, search for all the rules inside the create_leads method, make a dict of lead to create
(adding leads_values for each rules) then call the create only once (need create multi.. if not 'possible' to apply create multi, call create for each lead to create)

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from ac4f479 to 459f3b9 Jan 27, 2020
@robodoo robodoo removed the CI 🤖 label Jan 27, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch 3 times, most recently from 86c1a49 to d8bd5d9 Jan 27, 2020
Copy link
Contributor Author

jeh-odoo left a comment

part1 of reviews

event_id = fields.Many2one('event.event')

registration_ids = fields.Many2many('event.registration')
attendees_count = fields.Integer(compute='_compute_attendees_count')

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

for record in self:
record.attendees_count = len(record.registration_ids)

def action_get_attendee(self):

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done


class EventCrm(models.Model):
_name = "event.crm"
_description = "Event CRM"

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

renaming not done

create_lead = fields.Selection([
('one_per_attendee','One per Attendee'),('one_per_order', 'One per Order')],
default='one_per_attendee', required=True,
help="Select the type that this rule will apply to create leads.\n"
"- One per Attendee will create a lead for each attendee.\n"
"- One per Order will create a lead for a group of attendees.")

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

event_type_ids = fields.Many2many('event.type',
string='Event Categories',
help='Only the categories selected will be impacted by this rule.')
event_id = fields.Many2one('event.event',
string='Event',
help='The event selected will be impacted by this rule.')

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

if not registrations[0].web_registration:
for so_line in self.filtered('event_id'):
for r in event_crm:
r._check_rules(event_id=so_line.event_id, registration_ids=registrations & registrations.search([('event_id','=',so_line.event_id.id)]))

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

addons/website_event_crm/__init__.py Outdated Show resolved Hide resolved

return registrations

@http.route()

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

from odoo.http import request

class WebsiteEventCrmSaleController(WebsiteEventSaleController):
@http.route()

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done

registration = super(EventRegistration, self).create(vals)
registration.web_registration = True
return registration

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 27, 2020

Author Contributor

done, moved in controller instead of the create method

@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch 2 times, most recently from e4b63a0 to 10f04f9 Jan 27, 2020
Copy link
Contributor

dbeguin left a comment

Already much better.
waiting for you to apply all the 'not done' part :)
courage again!

if (!el['display_name']) {
el['display_name'] = el['name'];
}
Comment on lines 15 to 22

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 27, 2020

Contributor

Not sure you need to create a new widget for that.
default display_name = name 'by default'.
So if a model does not specify a display_name, display_name will = name.
I think using many2many_tags widget directly will do the job.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

I create the widget because when you add a question and answers, the display name is computed when you click on the button save on the event form. So with many2many_tags, when you create a question and answers, it displays a tag with nothing inside, since display_name is not set.

lead_type = fields.Selection([
('lead', 'Lead'), ('opportunity', 'Opportunity')], required=True,
default=lambda self: 'lead' if self.env['res.users'].has_group('crm.group_use_lead') else 'opportunity',
help="Select the type of lead that will be created when this rule applied.")
sales_team_id = fields.Many2one('crm.team', string='Sales Team')
user_id = fields.Many2one('res.users', string='Salesperson')
tag_ids = fields.Many2many('crm.lead.tag', string='Tags')

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 27, 2020

Contributor

I meant something like

# Rule configuration fields
lead_creation_type = ...
event_type_ids = ...
event_id = ...
company_ids = ...
registration_filter_domain = ...
registration_ids = ...

# Lead default_value fields
lead_type = ...
lead_sales_team_id = ...
lead_user_id = ...
lead_tag_ids = ...

@api.depends('event_type_ids','event_id','registration_filter_domain')
def _compute_registration_ids(self):
"""Compute the attendees impacted by the rule to compare later in the creation of leads"""

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 27, 2020

Contributor

compute the registrations *

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

done

addons/event_crm/models/event_crm.py Outdated Show resolved Hide resolved
description += "\t%s %s %s\n" % (registration.name, registration.email, registration.phone)
return description

def _create_leads(self, event_id, registration_ids):

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 27, 2020

Contributor

I would put this method higher in the file (juste below compute* methods).
The methods used by _create_leads can be placed after as they are 'technical/intermediate/tools/... methods.
The idea is to read the file and read the main methods first, then , if you want some detail, go and see further.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

done

return description, first_registration

def _create_leads(self, registration_ids, vals):
self.ensure_one()

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 27, 2020

Contributor

with the ensure_one, it won't be possible to make only one create for each rule.
You don't need to specify the event and the registration_ids as they are on self.
(e.g. event_id is self.event_id)
It could be cool to make something like this:

def _create_leads(self):
    vals_list = []
    for rule in self:
        event_id = rule.event_id
        registrations = rule.registration_ids (a priori, it's not necessary to make the intersection (self.registration_ids & registration_ids) as self.registration_ids is computed based on the rule configuration and should already be correct)
        if self.lead_creation_type == 'one_per_attendee':
            for registration in registrations:
                  ...
                 vals_list.add(lead_values)
                 ...
       else:
             ...
             vals_list.add(lead_values)
              ...
    lead = self.env['crm.lead'].create(vals_list)

Note that you don't need to write the lead_id on the registration as you can set the registration_id on lead_values. (many2many will link in both ways)

@api.model
def create(self, vals):
registration = super(EventRegistration, self).create(vals)
event_crm = self.env['event.crm'].search([])

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 27, 2020

Contributor

well, I was talking about event_crm line. (event registration are not = to registration rules :) )

results = super(EventRegistration, self).create(vals)
registration_rules = self.env['event.crm'].search([])
registrations = self.env['event.registration'].search([('sale_order_line_id', 'in', self.ids)])
for so_line in self.filtered('event_id'):
existing_registrations = registrations.filtered(lambda self: self.sale_order_line_id.id == so_line.id)
if existing_registrations[0].lead_id.description:

This comment has been minimized.

Copy link
@dbeguin

dbeguin Jan 27, 2020

Contributor

existing_registrations[0] will crash if len(existing_registrations) < 1

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

done

@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from 10f04f9 to 27cf0ef Jan 27, 2020
@robodoo robodoo added the CI 🤖 label Jan 27, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from 27cf0ef to 7b7c489 Jan 28, 2020
@robodoo robodoo removed the CI 🤖 label Jan 28, 2020
Copy link
Contributor Author

jeh-odoo left a comment

part 2 of review


@api.depends('event_type_ids','event_id','registration_filter_domain')
def _compute_registration_ids(self):
"""Compute the attendees impacted by the rule to compare later in the creation of leads"""

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

done

description += "\t%s %s %s\n" % (registration.name, registration.email, registration.phone)
return description

def _create_leads(self, event_id, registration_ids):

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

done

return description, first_registration

def _create_leads(self, registration_ids, vals):
self.ensure_one()

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

done, I still need to get the registrations since it's possible that the module can be install later. So if there was registrations created before the installation of a module or a rule creation, it will create lead for this registration or mix them with the actual registrations.

registrations = self.env['event.registration'].search([('sale_order_line_id', 'in', self.ids)])
for so_line in self.filtered('event_id'):
existing_registrations = registrations.filtered(lambda self: self.sale_order_line_id.id == so_line.id)
if existing_registrations[0].lead_id.description:

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

done

addons/website_event_crm/__init__.py Outdated Show resolved Hide resolved
if (!el['display_name']) {
el['display_name'] = el['name'];
}

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Jan 28, 2020

Author Contributor

I create the widget because when you add a question and answers, the display name is computed when you click on the button save on the event form. So with many2many_tags, when you create a question and answers, it displays a tag with nothing inside, since display_name is not set.

@robodoo robodoo added the CI 🤖 label Jan 28, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from 7b7c489 to 9ca16c4 Jan 29, 2020
@robodoo robodoo removed the CI 🤖 label Jan 29, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from 9ca16c4 to 87e403d Jan 29, 2020
@robodoo robodoo added the CI 🤖 label Jan 29, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from 87e403d to cd04021 Jan 29, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 29, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from 10179b5 to eedbea7 Mar 4, 2020
@robodoo robodoo removed the CI 🤖 label Mar 4, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from eedbea7 to 98be39e Mar 5, 2020
@robodoo robodoo added the CI 🤖 label Mar 5, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from 98be39e to a380de8 Mar 5, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 5, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from a380de8 to f4549df Mar 24, 2020
@robodoo robodoo removed the CI 🤖 label Mar 24, 2020
@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch 2 times, most recently from 20c2296 to c20debe Mar 24, 2020
Copy link
Contributor

tde-banana-odoo left a comment

Technical review: architecture seems ok. Code should be cleaned a bit further, notably to have a smoother code flow and better located methods.

Side note: maybe you will hate me, but why not renaming event.crm.rule to event.lead.rule ?

class Lead(models.Model):
_inherit = 'crm.lead'

event_id = fields.Many2one('event.event', string="Event linked to the lead")

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 24, 2020

Contributor

Isn't it a bit verbose in a form view ?

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done, replaced by "Related Event"

@api.depends('lead_ids')
def _compute_leads_count(self):
for record in self:
record.leads_count = len(record.lead_ids)

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 24, 2020

Contributor

You should use a read_group, better for performances normally .

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

addons/event_crm/models/event_registration.py Outdated Show resolved Hide resolved
@@ -49,6 +49,8 @@ class EventRegistration(models.Model):
('draft', 'Unconfirmed'), ('cancel', 'Cancelled'),
('open', 'Confirmed'), ('done', 'Attended')],
string='Status', default='draft', readonly=True, copy=False, tracking=True)
# technical field
main_registration_id = fields.Many2one('event.registration', string="Main Registration", help="Contains the first registration when a batch is created", copy=False)

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 24, 2020

Contributor

Linter: set help as last parameter to have main parameters available at beginning of definition :)

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

@api.depends('registration_ids')
def _compute_registration_count(self):
for record in self:
record.registration_count = len(record.registration_ids)

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 24, 2020

Contributor

You can use a read_group instead

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

@tde-banana-odoo I'm not really familiar with read_group, is it possible to use read_group in this case since registration_ids and lead_ids are many2many field because I can't groupby with these fields.

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

Indeed, forgot it was a m2m. Well then we can leave it as it is.

Otherwise you have to do an SQL query directly on relationship table as there is no model. It is better to avoid writing manual SQL, will see if optimization is necessary.

"""
Build the description for the lead when a rule (per order) matchs for registrations.
"""
return "\t%s %s %s\n" % (self.name, self.email if self.email else '', self.phone if self.phone else '')

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 24, 2020

Contributor

Why don't we set name as name, email as email_from and phone as phone ? and/or log a message ?

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

I made this because when we create a registration by hand with a new partner create on the fly, we have an email and a phone equals to False which is displayed as "False" in the description.

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

Ok, but my question was more about the lead description utility actually. Those fields are tracked in lead model anyway, so what's the point of adding them in description ? For me it is unnecessary redundant information storage.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 27, 2020

Author Contributor

Haaa, I asked Luc about the same thing and I think it was because internally we do that and as this feature will be used internally he wanted to have the same thing and also that was less clicks to get the information.

But ultimately, we will have to do something about this description because the next task which will add new triggers for a rule must normally edit the description to add info about the attendee who trigger it. So if a user edit the description during the process, the formatted description will be broken, unless we erase and recompute it but that means also erase potential info that the user has written, which is not good. To be honest I didn't like this idea of description from the start.

vals_list.append(lead_values)
else:
# only one registration
for lead in registrations.main_registration_id.lead_ids:

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 24, 2020

Contributor

Some explanations are requires. We have one registration linked to a main registration -> it means we don't create a lead but update existing, is it what you want to do ? In which case you should filter lead on registration_rule_id itself, we could have several "per order" rules creating leads if we accept the multi-creation mode.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done, I added a comment above to explain this part. Indeed I forgot the case where there are multiple "per order" rule matching.

lead.write(vals)
return registration

def _get_registration_lead_values(self, rule_type, registration_ids=False):

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 24, 2020

Contributor

Should work on a recordset (self) and not take registration_ids. Simpler to understand if it works on a record set.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

}
if rule_type == 'attendee':
registration = registration_ids if registration_ids else self
registration = self.search([('id', '=', registration.id)])

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 24, 2020

Contributor

Seems unnecessary, unless you want to filter something, in which case it should be done explicitly (and done in batch to avoid queries :) ).

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

registration_lead_values.update({
'name': "%s - %s" % (self.event_id.name, registration.partner_id.name),
'partner_id': registration.partner_id.id,
'email_from': registration.partner_id.email,

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 24, 2020

Contributor

You should first set the partner (and no other info as computed field will populate other information), and if not set other fields. That way you will have less code and rely more on existing computed fields :) .

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from c20debe to ef4984e Mar 25, 2020
@robodoo robodoo added the CI 🤖 label Mar 25, 2020
Copy link
Contributor Author

jeh-odoo left a comment

Application of the review
I was unsure if there is a proper way to use the read_group for many2many fields or if it's possible.

@@ -49,6 +49,8 @@ class EventRegistration(models.Model):
('draft', 'Unconfirmed'), ('cancel', 'Cancelled'),
('open', 'Confirmed'), ('done', 'Attended')],
string='Status', default='draft', readonly=True, copy=False, tracking=True)
# technical field
main_registration_id = fields.Many2one('event.registration', string="Main Registration", help="Contains the first registration when a batch is created", copy=False)

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

class Lead(models.Model):
_inherit = 'crm.lead'

event_id = fields.Many2one('event.event', string="Event linked to the lead")

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done, replaced by "Related Event"

addons/event_crm/models/crm_lead.py Outdated Show resolved Hide resolved
@api.depends('registration_ids')
def _compute_registration_count(self):
for record in self:
record.registration_count = len(record.registration_ids)

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

@tde-banana-odoo I'm not really familiar with read_group, is it possible to use read_group in this case since registration_ids and lead_ids are many2many field because I can't groupby with these fields.

help='Filter the attendees to include those of this specific event. If not set, no event restriction will be applied.')
company_id = fields.Many2one('res.company', string='Company',
help="Restrict the trigger of this rule to events belonging to a specific company.\nIf not set, no company restriction will be applied.")
registration_filter_domain = fields.Text(string="Registrations Domain", default='[]', required=True,

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

"""
Build the description for the lead when a rule (per order) matchs for registrations.
"""
return "\t%s %s %s\n" % (self.name, self.email if self.email else '', self.phone if self.phone else '')

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

I made this because when we create a registration by hand with a new partner create on the fly, we have an email and a phone equals to False which is displayed as "False" in the description.

@@ -0,0 +1,3 @@
id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
access_event_crm_manager,event.crm.manager,model_event_crm_rule,event.group_event_manager,1,1,1,1
access_event_crm_user,event.crm.user,model_event_crm_rule,event.group_event_user,1,0,0,0

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

@@ -0,0 +1,34 @@
<?xml version="1.0"?>
<odoo>
<record id="action_view_lead_registrations" model="ir.actions.act_window">

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

</field>
</record>

<record id="action_event_crm" model="ir.actions.act_window">

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

</field>
</record>

<menuitem name="Lead Generation" id="menu_event_crm"

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 25, 2020

Author Contributor

done

@jeh-odoo jeh-odoo force-pushed the odoo-dev:master-event_crm-create-leads-from-attendees-jeh branch from ef4984e to d2f795a Mar 26, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 26, 2020
@jeh-odoo jeh-odoo changed the title [IMP] event_crm: create leads from attendees [ADD] event_crm: create leads from attendees Mar 27, 2020
Copy link
Contributor

tde-banana-odoo left a comment

Technical review, part 2 !

<field name="lead_tag_ids" eval="[(6, 0, [ref('sales_team.categ_oppor8')])]"/>
</record>

<record id="event_registration_0_rule_0" model="event.registration">

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

Side note: why not either using existing demo, either adding your demo directly to event and keep only event_crm demo here (aka, rules) ? That way we keep a consistent crmeventdemo set.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 27, 2020

Author Contributor

Because the registrations in event are created before event_crm, so I need to create registration after the rule to be able to get the demo working.

</record>
<record id="event_registration_1_rule_0" model="event.registration">
<field name="name">Jean-Luc</field>
<field name="email">jean-luc@test.com</field>

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

Is test.com a fake domain like example ?

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 27, 2020

Author Contributor

Yes

class Lead(models.Model):
_inherit = 'crm.lead'

event_id = fields.Many2one('event.event', string="Related Event")

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

HumanLinter: no need to have spaces between fields definition (but some long definitions may be spanned on two lines, like registration_ids, for easier reading)

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 27, 2020

Author Contributor

done

@api.depends('registration_ids')
def _compute_registration_count(self):
for record in self:
record.registration_count = len(record.registration_ids)

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

Indeed, forgot it was a m2m. Well then we can leave it as it is.

Otherwise you have to do an SQL query directly on relationship table as there is no model. It is better to avoid writing manual SQL, will see if optimization is necessary.

# Part of Odoo. See LICENSE file for full copyright and licensing details.

from odoo import fields, models
from ast import literal_eval

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

HumanLinter: imports should be ordered (first std lib, while line, odoo lib, one per line)

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 27, 2020

Author Contributor

done

'security/ir.model.access.csv',
'views/event_views.xml',
'views/crm_lead_views.xml',
'views/event_crm_views.xml',

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

Du coup, event_lead_views ?

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 27, 2020

Author Contributor

done

# -*- coding: utf-8 -*-
# Part of Odoo. See LICENSE file for full copyright and licensing details.

from . import event_crm

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

Du coup, event_lead_rule.py so that file matches model that is inside ?

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 27, 2020

Author Contributor

done

@@ -48,7 +48,7 @@ def create(self, vals_list):
self.env['sale.order.line'].browse(vals['sale_order_line_id'])
)
vals.update(so_line_vals)
registrations = super(EventRegistration, self).create(vals)
registrations = super(EventRegistration, self).create(vals_list)

This comment has been minimized.

Copy link
@tde-banana-odoo
for count in range(int(so_line.product_uom_qty) - len(existing_registrations)):
registration_vals = {}
if registration_data:
registration_vals = registration_data.pop()
# TDE CHECK: auto confirmation
registration_vals['sale_order_line_id'] = so_line.id
Registration.create(registration_vals)
if not first_registration:
so_registrations = Registration.search([('sale_order_id', '=', so_line.order_id.id), ('event_id', '=', so_line.event_id.id)])

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 27, 2020

Contributor

Some computation could be done outside of the count loop, otherwise we compute several times the same thing.

This comment has been minimized.

Copy link
@jeh-odoo

jeh-odoo Mar 27, 2020

Author Contributor

done

<field name="inherit_id" ref="event_crm.event_crm_rule_view_tree"/>