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

[8.0][IMP][mail_tracking] Speed installation time and discard concurrent events #82

Merged
merged 6 commits into from
Sep 9, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mail_tracking/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@

from . import models
from . import controllers
from .hooks import post_init_hook
from .hooks import pre_init_hook
4 changes: 2 additions & 2 deletions mail_tracking/__openerp__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
{
"name": "Email tracking",
"summary": "Email tracking system for all mails sent",
"version": "8.0.2.0.0",
"version": "8.0.2.0.1",
"category": "Social Network",
"website": "http://www.tecnativa.com",
"author": "Tecnativa, "
Expand All @@ -28,5 +28,5 @@
"qweb": [
"static/src/xml/mail_tracking.xml",
],
"post_init_hook": "post_init_hook",
"pre_init_hook": "pre_init_hook",
}
42 changes: 27 additions & 15 deletions mail_tracking/hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,34 @@
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

import logging
from openerp import api, SUPERUSER_ID
from psycopg2.extensions import AsIs

_logger = logging.getLogger(__name__)


def post_init_hook(cr, registry):
with api.Environment.manage():
env = api.Environment(cr, SUPERUSER_ID, {})
# Recalculate all partner tracking_email_ids
partners = env['res.partner'].search([
('email', '!=', False),
])
emails = partners.mapped('email')
_logger.info(
"Recalculating 'tracking_email_ids' in 'res.partner' "
"model for %d email addresses", len(emails))
for email in emails:
env['mail.tracking.email'].tracking_ids_recalculate(
'res.partner', 'email', 'tracking_email_ids', email)
def column_exists(cr, table, column):
cr.execute("""
SELECT column_name
FROM information_schema.columns
WHERE table_name = %s AND column_name = %s""", (table, column))
return bool(cr.fetchall())


def column_add_with_value(cr, table, column, field_type, value):
if not column_exists(cr, table, column):
cr.execute("""
ALTER TABLE %s
ADD COLUMN %s %s""", (AsIs(table), AsIs(column), AsIs(field_type)))
cr.execute("""
UPDATE %s SET %s = %s""", (AsIs(table), AsIs(column), value))


def pre_init_hook(cr):
_logger.info("Creating res.partner.tracking_emails_count column "
"with value 0")
column_add_with_value(
cr, "res_partner", "tracking_emails_count", "integer", 0)
_logger.info("Creating res.partner.email_score column "
"with value 50.0")
column_add_with_value(
cr, "res_partner", "email_score", "double precision", 50.0)
39 changes: 35 additions & 4 deletions mail_tracking/models/mail_tracking_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@

_logger = logging.getLogger(__name__)

EVENT_OPEN_DELTA = 10
Copy link
Member

Choose a reason for hiding this comment

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

Comment that this is in seconds

EVENT_CLICK_DELTA = 5


class MailTrackingEmail(models.Model):
_name = "mail.tracking.email"
Expand Down Expand Up @@ -104,7 +107,7 @@ def tracking_ids_recalculate(self, model, email_field, tracking_field,
obj.write({
tracking_field: [(5, False, False)]
})
return True
return objects

@api.model
def _tracking_ids_to_write(self, email):
Expand Down Expand Up @@ -256,13 +259,41 @@ def _event_prepare(self, event_type, metadata):
_logger.info('Unknown event type: %s' % event_type)
return False

def _concurrent_events(self, event_type, metadata):
m_event = self.env['mail.tracking.event']
self.ensure_one()
concurrent_event_ids = False
if event_type in {'open', 'click'}:
ts = metadata.get('timestamp', time.time())
delta = EVENT_OPEN_DELTA if event_type == 'open' \
else EVENT_CLICK_DELTA
domain = [
('timestamp', '>=', ts - delta),
('timestamp', '<=', ts + delta),
('tracking_email_id', '=', self.id),
('event_type', '=', event_type),
]
if event_type == 'click':
domain.append(('url', '=', metadata.get('url', False)))
concurrent_event_ids = m_event.search(domain)
return concurrent_event_ids

@api.multi
def event_create(self, event_type, metadata):
event_ids = self.env['mail.tracking.event']
for tracking_email in self:
vals = tracking_email._event_prepare(event_type, metadata)
if vals:
event_ids += event_ids.sudo().create(vals)
other_ids = tracking_email._concurrent_events(event_type, metadata)
if not other_ids:
vals = tracking_email._event_prepare(event_type, metadata)
if vals:
event_ids += event_ids.sudo().create(vals)
partners = self.tracking_ids_recalculate(
'res.partner', 'email', 'tracking_email_ids',
tracking_email.recipient_address)
if partners:
partners.email_score_calculate()
else:
_logger.info("Concurrent event '%s' discarded", event_type)
Copy link
Member

Choose a reason for hiding this comment

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

Please, all of these messages as debug, not info

return event_ids

@api.model
Expand Down
11 changes: 5 additions & 6 deletions mail_tracking/models/res_partner.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,12 @@ class ResPartner(models.Model):
string="Tracking emails count", store=True, readonly=True,
compute="_compute_tracking_emails_count")
email_score = fields.Float(
string="Email score",
compute="_compute_email_score", store=True, readonly=True)
string="Email score", readonly=True, default=50.0)

@api.one
@api.depends('tracking_email_ids.state')
def _compute_email_score(self):
self.email_score = self.tracking_email_ids.email_score()
@api.multi
def email_score_calculate(self):
Copy link
Member

Choose a reason for hiding this comment

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

Why do you remove the compute method? This way, you have the risk of not having updated the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is causing a inter-block in PostgreSQL, because it's trying to read tracking email table inside the write of field state.

Now email score calculation is made in event_create method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments added to remember why we changed compute method

for partner in self:
partner.email_score = partner.tracking_email_ids.email_score()

@api.one
@api.depends('tracking_email_ids')
Expand Down
75 changes: 73 additions & 2 deletions mail_tracking/tests/test_mail_tracking.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@

import mock
import base64
import time
from openerp.tests.common import TransactionCase
from openerp.addons.mail_tracking.controllers.main import \
MailTrackingController, BLANK
from ..controllers.main import MailTrackingController, BLANK

mock_request = 'openerp.http.request'
mock_send_email = ('openerp.addons.base.ir.ir_mail_server.'
Expand Down Expand Up @@ -114,6 +114,77 @@ def test_mail_send(self):
res = controller.mail_tracking_open(db, tracking.id)
self.assertEqual(image, res.response[0])

def test_concurrent_open(self):
mail, tracking = self.mail_send()
ts = time.time()
metadata = {
'ip': '127.0.0.1',
'user_agent': 'Odoo Test/1.0',
'os_family': 'linux',
'ua_family': 'odoo',
'timestamp': ts,
}
# First open event
tracking.event_create('open', metadata)
opens = tracking.tracking_event_ids.filtered(
lambda r: r.event_type == 'open'
)
self.assertEqual(len(opens), 1)
# Concurrent open event
metadata['timestamp'] = ts + 2
tracking.event_create('open', metadata)
opens = tracking.tracking_event_ids.filtered(
lambda r: r.event_type == 'open'
)
self.assertEqual(len(opens), 1)
# Second open event
metadata['timestamp'] = ts + 350
tracking.event_create('open', metadata)
opens = tracking.tracking_event_ids.filtered(
lambda r: r.event_type == 'open'
)
self.assertEqual(len(opens), 2)

def test_concurrent_click(self):
mail, tracking = self.mail_send()
ts = time.time()
metadata = {
'ip': '127.0.0.1',
'user_agent': 'Odoo Test/1.0',
'os_family': 'linux',
'ua_family': 'odoo',
'timestamp': ts,
'url': 'https://www.example.com/route/1',
}
# First click event (URL 1)
tracking.event_create('click', metadata)
opens = tracking.tracking_event_ids.filtered(
lambda r: r.event_type == 'click'
)
self.assertEqual(len(opens), 1)
# Concurrent click event (URL 1)
metadata['timestamp'] = ts + 2
tracking.event_create('click', metadata)
opens = tracking.tracking_event_ids.filtered(
lambda r: r.event_type == 'click'
)
self.assertEqual(len(opens), 1)
# Second click event (URL 1)
metadata['timestamp'] = ts + 350
tracking.event_create('click', metadata)
opens = tracking.tracking_event_ids.filtered(
lambda r: r.event_type == 'click'
)
self.assertEqual(len(opens), 2)
# Concurrent click event (URL 2)
metadata['timestamp'] = ts + 2
metadata['url'] = 'https://www.example.com/route/2'
tracking.event_create('click', metadata)
opens = tracking.tracking_event_ids.filtered(
lambda r: r.event_type == 'click'
)
self.assertEqual(len(opens), 3)

def test_smtp_error(self):
with mock.patch(mock_send_email) as mock_func:
mock_func.side_effect = Warning('Test error')
Expand Down
28 changes: 12 additions & 16 deletions mail_tracking_mailgun/models/mail_tracking_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,12 @@ def _mailgun_event_type_mapping(self):
'dropped': 'reject',
}

@property
def _mailgun_supported_event_types(self):
return self._mailgun_event_type_mapping.keys()

def _mailgun_event_type_verify(self, event):
event = event or {}
mailgun_event_type = event.get('event')
if mailgun_event_type not in self._mailgun_supported_event_types:
_logger.info("Mailgun: event type '%s' not supported",
mailgun_event_type)
if mailgun_event_type not in self._mailgun_event_type_mapping:
_logger.error("Mailgun: event type '%s' not supported",
mailgun_event_type)
return False
# OK, event type is valid
return True
Expand All @@ -66,12 +62,12 @@ def _mailgun_signature_verify(self, event):
event = event or {}
api_key = self.env['ir.config_parameter'].get_param('mailgun.apikey')
if not api_key:
_logger.info("No Mailgun api key configured. "
"Please add 'mailgun.apikey' to System parameters "
"to enable Mailgun authentication webhoook requests. "
"More info at: "
"https://documentation.mailgun.com/user_manual.html"
"#webhooks")
_logger.warning("No Mailgun api key configured. "
"Please add 'mailgun.apikey' to System parameters "
"to enable Mailgun authentication webhoook "
"requests. More info at: "
"https://documentation.mailgun.com/"
"user_manual.html#webhooks")
else:
timestamp = event.get('timestamp')
token = event.get('token')
Expand All @@ -89,8 +85,8 @@ def _db_verify(self, event):
odoo_db = event.get('odoo_db')
current_db = self.env.cr.dbname
if odoo_db != current_db:
_logger.info("Mailgun: Database '%s' is not the current database",
odoo_db)
_logger.error("Mailgun: Database '%s' is not the current database",
odoo_db)
return False
# OK, DB is current
return True
Expand Down Expand Up @@ -124,7 +120,7 @@ def _mailgun_metadata(self, mailgun_event_type, event, metadata):
metadata[k] = event[v]
# Special field mapping
metadata.update({
'mobile': event.get('device-type') in ('mobile', 'tablet'),
'mobile': event.get('device-type') in {'mobile', 'tablet'},
'user_country_id': self._country_search(
event.get('country', False)),
})
Expand Down
2 changes: 1 addition & 1 deletion mail_tracking_mass_mailing/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

from . import models
from .hooks import post_init_hook
from .hooks import pre_init_hook
4 changes: 2 additions & 2 deletions mail_tracking_mass_mailing/__openerp__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
{
"name": "Mail tracking for mass mailing",
"summary": "Improve mass mailing email tracking",
"version": "8.0.1.0.0",
"version": "8.0.1.0.1",
"category": "Social Network",
"website": "http://www.tecnativa.com",
"author": "Tecnativa, "
Expand All @@ -24,5 +24,5 @@
"views/mail_mass_mailing_view.xml",
"views/mail_mass_mailing_contact_view.xml",
],
"post_init_hook": "post_init_hook",
"pre_init_hook": "pre_init_hook",
}
26 changes: 7 additions & 19 deletions mail_tracking_mass_mailing/hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,14 @@
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

import logging
from openerp import api, SUPERUSER_ID
from openerp.addons.mail_tracking.hooks import column_add_with_value
Copy link
Member

Choose a reason for hiding this comment

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

Protect the import

Copy link
Contributor Author

@antespi antespi Sep 2, 2016

Choose a reason for hiding this comment

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

This addon depends on mail_tracking. I think this is not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is. This causes server doesn't start if you don't have both modules accessible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then should applies to every openerp.addons.xxx import. I see lots of them in odoo and OCA and none protected

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's the question. Remember @yajo's PRs for protecting them.

Copy link
Member

Choose a reason for hiding this comment

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

Please change this

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking about adding the protection directly in the __init__.py file of the addon in the template, to skip these errors in the future more automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you give me one sample as reference?

Copy link
Member

Choose a reason for hiding this comment

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

You have to put:

try:
    from openerp.addons.mail_tracking.hooks import column_add_with_value
except ImportError:
    column_add_with_value = 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.

Done


_logger = logging.getLogger(__name__)


def post_init_hook(cr, registry):
with api.Environment.manage():
env = api.Environment(cr, SUPERUSER_ID, {})
# Recalculate all mass_mailing contacts tracking_email_ids
contacts = env['mail.mass_mailing.contact'].search([
('email', '!=', False),
])
emails = contacts.mapped('email')
_logger.info(
"Recalculating 'tracking_email_ids' in "
"'mail.mass_mailing.contact' model for %d email addresses",
len(emails))
for n, email in enumerate(emails):
env['mail.tracking.email'].tracking_ids_recalculate(
'mail.mass_mailing.contact', 'email', 'tracking_email_ids',
email)
if n % 500 == 0: # pragma: no cover
_logger.info(" Recalculated %d of %d", n, len(emails))
def pre_init_hook(cr):
_logger.info("Creating mail.mass_mailing.contact.email_score column "
"with value 50.0")
column_add_with_value(
cr, 'mail_mass_mailing_contact', 'email_score', 'double precision',
50.0)
11 changes: 5 additions & 6 deletions mail_tracking_mass_mailing/models/mail_mass_mailing_contact.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,12 @@ class MailMassMailingContact(models.Model):
string="Tracking emails", comodel_name="mail.tracking.email",
readonly=True)
email_score = fields.Float(
string="Email score",
compute="_compute_email_score", store=True, readonly=True)
string="Email score", readonly=True, default=50.0)

@api.one
@api.depends('tracking_email_ids', 'tracking_email_ids.state')
def _compute_email_score(self):
self.email_score = self.tracking_email_ids.email_score()
@api.multi
def email_score_calculate(self):
for contact in self:
contact.email_score = contact.tracking_email_ids.email_score()

@api.multi
def write(self, vals):
Expand Down
11 changes: 11 additions & 0 deletions mail_tracking_mass_mailing/models/mail_tracking_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,14 @@ def create(self, vals):
'mail.mass_mailing.contact', 'email', 'tracking_email_ids',
tracking.recipient_address, new_tracking=tracking)
return tracking

@api.multi
def event_create(self, event_type, metadata):
res = super(MailTrackingEmail, self).event_create(event_type, metadata)
for tracking_email in self:
contacts = self.tracking_ids_recalculate(
'mail.mass_mailing.contact', 'email', 'tracking_email_ids',
tracking_email.recipient_address)
if contacts:
contacts.email_score_calculate()
return res
Loading