[MIG] mail_tracking: Migration to 11.0 #244

chienandalu · 2018-03-19T15:50:37Z

Mail tracking

This module shows email notification tracking status for any messages in
mail thread (chatter). Each notified partner will have an intuitive icon just
right to his name.

cc @Tecnativa

rafaelbn · 2018-03-21T10:31:47Z

thanks @chienandalu ! please could you review travis? is in red

chienandalu · 2018-03-21T10:34:42Z

@rafaelbn I'm on it. I had an old v11 instance in wich test worked. So I have to debug what change in mail is making the tests fail

chienandalu · 2018-03-21T16:17:07Z

So travis won't pass due to this issue: odoo/odoo#23808 Wich affects to mail_digest module here: https://github.com/OCA/social/blob/11.0/mail_digest/models/res_partner.py#L80

Without that module installed tests pass with no problem at all in my local...

pedrobaeza · 2018-03-21T16:55:03Z

@simahawk maybe you can find a workaround for making green again the branch

simahawk · 2018-03-22T07:42:02Z

@pedrobaeza branch 11.0 is green AFAIS. For the issue here I've no clue ATM.

@chienandalu can you report here which domain do you get and which is the offending leaf here?

Then you could improve the test cases or add a new one here so that we are sure that this case too is covered w/ tests.

BTW I'll have to work on mail_digest for some improvements in the next 2 weeks. I will have a look at this if not solved yet.

chienandalu · 2018-03-27T10:30:09Z

Hi @simahawk You can test it like this:

Add a normal partner to the setup.
Add this partner to the message.
Assert the partner is found (it won't be due to the bug)

def test_notify_domains_only_recipients(self):
    # we don't set recipients
    # because we call `_get_notify_by_email_domain` directly
    self.partner1.real_user_id.notification_type = 'email'
    self.partner2.real_user_id.notification_type = 'email'
    partners = self.partner1 + self.partner2
    # followers
    self.partner3.message_subscribe(self.partner2.ids)
    # partner1 is the only recipient
    message = self.message_model.create({
        'body': 'My Body',
        'res_id': self.partner3.id,
        'model': 'res.partner',
        'partner_ids': [(4, self.partner1.id), (4, self.partner_alt.id)]
    })
    domain = partners._get_notify_by_email_domain(message)
    # we find both of them since partner2 is a follower
    self._assert_found(self.partner1, domain)
    self._assert_found(self.partner2, domain)
    self._assert_found(self.partner_alt, domain)
    # no one here in digest mode
    domain = partners._get_notify_by_email_domain(message, digest=True)
    self._assert_found(self.partner1, domain, not_found=True)
    self._assert_found(self.partner2, domain, not_found=True)

simahawk · 2018-03-27T15:50:40Z

@chienandalu if w/ "normal partner" you mean a partner w/ no user... then you are right. It does not pass. I'm checking.

chienandalu · 2018-04-02T07:42:10Z

@simahawk Sure, that's what I meant 😅

simahawk · 2018-04-04T15:57:55Z

@chienandalu review wlcm :) #253

pedrobaeza · 2018-04-05T00:44:08Z

Please rebase

chienandalu · 2018-04-05T07:35:17Z

Rebased and a little commit squash

pedrobaeza · 2018-04-05T10:45:10Z

@yajo please review

simahawk · 2018-04-05T12:45:48Z

mail_tracking/__manifest__.py

@@ -0,0 +1,34 @@
+# -*- coding: utf-8 -*-


drop coding headers

simahawk · 2018-04-05T12:46:18Z

mail_tracking/controllers/main.py

+                reg = registry(db)
+            except OperationalError:
+                _logger.warning("Selected BD '%s' not found", db)
+            except:  # pragma: no cover


except Exception: and remove the comment

simahawk · 2018-04-05T12:49:05Z

mail_tracking/models/ir_mail_server.py

+        if match:
+            try:
+                tracking_email_id = int(match.group(1))
+            except:  # pragma: no cover


except Exception or ValueError if is related to int call

simahawk · 2018-04-05T12:50:16Z

mail_tracking/models/ir_mail_server.py

+            mail_server = mail_server_ids[0] if mail_server_ids else None
+        if mail_server:
+            smtp_server_used = mail_server.smtp_host
+        else:  # pragma: no cover


comment needed for?

simahawk · 2018-04-05T12:54:04Z

mail_tracking/models/mail_message.py

+                if tracking.partner_id:
+                    partners_already |= tracking.partner_id
+            # Search all recipients for this message
+            if message.partner_ids:


recordsets operation can be slow compared to normal sets operations.
You could build a set or a dict ID: name (the only values that you are going to use below)

simahawk · 2018-04-05T12:55:44Z

mail_tracking/models/mail_tracking_email.py

+    @api.model
+    def email_is_bounced(self, email):
+        if email:
+            return len(self._email_score_tracking_filter([


wouldn't be better to use search_count here, especially if - as stated above - we gonna have tons of records?

simahawk · 2018-04-05T12:59:10Z

mail_tracking/models/mail_tracking_email.py

+        if event and event.recipient_address:
+            recipients.append(event.recipient_address)
+        else:
+            recipients = list(


list() needed for? mapped already returns an iterable...

This oddities tend to come from 2to3. I don't know exactly attending to wich rule. Fixed anyway.

simahawk · 2018-04-05T13:03:32Z

mail_tracking/static/src/js/mail_tracking.js

+            this.bind_events();
+        },
+        render: function(messages, options) {
+            var MessageModel = new data.DataSetSearch(this, 'mail.message', session.context);


DataSet* are deprecate and models too. You should use _rpc

pedrobaeza · 2018-04-07T00:50:13Z

@chienandalu please finish this so that we can merge it.

chienandalu · 2018-04-09T10:24:57Z

Thanks for your comments @simahawk Take a look now

simahawk

@chienandalu small glitches but I'm approving so that I won't block you anymore once you've addressed them 😉

simahawk · 2018-04-09T14:33:03Z

mail_tracking/models/mail_message.py

@@ -35,8 +34,8 @@ def tracking_status(self):
        res = {}
        for message in self:
            partner_trackings = []
-            partners_already = self.env['res.partner']
-            partners = self.env['res.partner']
+            partners_already = []


you don't need this list, the dict is enough. lambda x: x.id not in partners will work.

simahawk · 2018-04-09T14:33:11Z

mail_tracking/controllers/main.py

@@ -22,7 +21,7 @@ def _env_get(db, callback, tracking_id, event_type, **kw):
                reg = registry(db)
            except OperationalError:
                _logger.warning("Selected BD '%s' not found", db)
-            except:  # pragma: no cover
+            except Exception:  # pragma: no cover


remove pragma comment

simahawk · 2018-04-09T14:35:26Z

mail_tracking/models/mail_message.py

+                partners[p.id] = p.name
+            # If there is partners not included, then status is 'unknown'
+            partner_trackings += [
+                ('unkown', False, partners[x], x) for x in partners.keys()]


('unkown', False, pid, name) for pid, name in partners.items()

rafaelbn · 2018-04-13T23:29:29Z

Please @yajo make a review here in order to merge. Thank you!

pedrobaeza · 2018-04-14T00:02:10Z

Take into account #257

yajo

The migration looks good overall, but there are some points where things have changed with no good reason for it, and there is another where a change would be really appreciated. More details below.

Thanks!

yajo · 2018-04-16T07:16:01Z

mail_tracking/models/mail_message.py

+                partners[p.id] = p.name
+            for p in message.needaction_partner_ids.filtered(
+                    lambda x: x.id not in partners_already):
+                partners[p.id] = p.name


If I'm not wrong, with the change in this method you're doing the same thing as before, but avoiding the use of recordsets. However recordsets are not harmful; they help making code more understandable.

Would you mind leaving it as it was? Thanks 😊

yajo · 2018-04-16T07:20:57Z

mail_tracking/models/mail_tracking_email.py

@@ -102,10 +101,10 @@ def _email_score_tracking_filter(self, domain, order='time desc',
    @api.model
    def email_is_bounced(self, email):
        if email:
-            return len(self._email_score_tracking_filter([
+            return self.search_count([


For what I can see here, we have 2 options:

Remove the _email_score_tracking_filter method, since it is not being used now.

Change the _email_score_tracking_filter signature to be (*args, **kwargs), which get passed down to self.search(*args, **kwargs).

Then, call here to _email_score_tracking_filter, we add count=True as an argument, which acts just like search_count, but makes the count in the DB side, which is also more performant.

I think you should check if any submodule overwrites the method, to make the sane choice.

Seems that nobody is using this. I chose to erase the method

yajo · 2018-04-16T07:23:49Z

mail_tracking/models/mail_tracking_email.py

@@ -197,7 +196,7 @@ def _partners_email_bounced_set(self, reason, event=None):
        if event and event.recipient_address:
            recipients.append(event.recipient_address)
        else:
-            recipients = list(filter(None, self.mapped('recipient_address')))
+            recipients = [x for x in self.mapped('recipient_address') if x]


I see no benefit on this change.

Just for the sake of clarity

yajo · 2018-04-16T07:29:04Z

mail_tracking/static/src/js/mail_tracking.js

+            };
+            this.do_action(action);
+        },
+        bind_events: function () {


It's better to remove this method and add the events as a class property using the extension method you can see here.

This way we leverage Odoo framework to do the binding...

yajo · 2018-04-16T07:29:18Z

mail_tracking/static/src/js/mail_tracking.js

+        },
+        start: function () {
+            this._super();
+            this.bind_events();


... and we avoid the need to overwrite this method too.

yajo · 2018-04-16T07:41:53Z

mail_tracking/static/src/js/mail_tracking.js

+            });
+            self._rpc({
+                model: 'mail.message',
+                method: 'tracking_status',


This thing you're doing here is not exactly wrong, but think about it: every time the chat thread is rendered (which can happens i.e. when removing a message), you're making an additional RPC call and waiting for it to resolve until you actually render the thread.

Since reading mail threads is so common in Odoo, they designed it in a way that uses the web bus and caches messages internally, so it reduces RPC calls and improves responsiveness. Here, we're introducing a bottleneck that honestly I'd prefer to avoid.

By digging into the made calls, you can see that the web client ultimately calls the model's message_fetch method. Then, it would be much better to simply overwrite that method in the backend and avoid these RPC calls. The JS code would be much less too now.

This would be a really big performance improvement. Would you do it?

Well, I'd say that what you propose is alredy happening here: https://github.com/OCA/social/pull/244/files#diff-4e1cf268ea32e1266c1e39d18db0d429R66 Wich is a common point in the already exisiting rpc calls. Indeed, this code could be erased with no harm. Don't you think this method was made to have tighter check of the thread messages status?

I understand. For what I can see, this could be erased. Could you check please?

Seems to work as before. I guess it will lower down the refresh rate but, as you said, it will release a good bunch of unnecesary rpc calls.

rafaelbn · 2018-04-26T16:07:01Z

@chienandalu please review @yajo comments please. Thanks!

rafaelbn · 2018-04-26T16:10:22Z

@chienandalu please remember to add #257 !! Thanks!

…s and contacts to bounce (OCA#133)

…oved (OCA#132)

As regular users can't access this object.

* [FIX] Only one data-odoo-tracking-email tag in each email * [FIX] Get status even in multicompany instances

On our server, queries based on "mail_tracking_event"."tracking_email_id" improved from 501,924 ms to 1,840 ms queries based on "mail_tracking_email"."mail_message_id" improved from 167,436 ms to 3,223 ms The last ones are run several times when a thread has many messages

pedrobaeza · 2018-05-05T15:09:41Z

@yajo give your final review

rafaelbn added this to the 11.0 milestone Mar 20, 2018

rafaelbn added the needs review label Mar 20, 2018

rafaelbn requested review from yajo and rafaelbn March 20, 2018 10:52

pedrobaeza mentioned this pull request Mar 24, 2018

Migration to version 11.0 #203

Closed

26 tasks

simahawk mentioned this pull request Apr 4, 2018

mail_digest: fix notification domain for partners w/ no user #253

Merged

chienandalu force-pushed the 11.0-mig-mail_tracking branch 2 times, most recently from de57fec to 74f5229 Compare April 5, 2018 07:34

pedrobaeza approved these changes Apr 5, 2018

View reviewed changes

simahawk requested changes Apr 5, 2018

View reviewed changes

simahawk approved these changes Apr 9, 2018

View reviewed changes

yajo requested changes Apr 16, 2018

View reviewed changes

oca-transbot and others added 21 commits May 4, 2018 10:13

OCA Transbot updated translations from Transifex

0989504

[FIX] mail_tracking: Typo

f757114

[MIG] Make modules uninstallable

746765a

[MIG] mail_tracking: Migrated to 10.0

cad6522

[IMP] mail_tracking performance and bounce process (OCA#103)

07b875d

[IMP] mail_tracking: Allow to define a custom mail_tracking base url

2aad6e5

Add access to portal/public user to his own email trackings

e46f5a5

OCA Transbot updated translations from Transifex

1025e30

[8.0][FIX][mail_tracking] Use event recipient address to find partner…

73b2aa7

…s and contacts to bounce (OCA#133)

[9.0][FIX][mail_tracking] Control exception when mail_message was rem…

1d5be2e

…oved (OCA#132)

[FIX] Sender label is repeated in search fields

71e1de1

[FIX][9.0] partner buttons view on enterprise

4697928

OCA Transbot updated translations from Transifex

68b2494

[FIX] mail_tracking: Obtain outgoing server with sudo

fe24498

As regular users can't access this object.

OCA Transbot updated translations from Transifex

87b4d99

[IMP] Shut mock exceptions up

208469c

OCA Transbot updated translations from Transifex

6f259f6

[9.0][IMP] mail_tracking: Improve view with error details

d4c5a43

[FIX] mail_tracking: bugfixes (OCA#199)

bc5e239

* [FIX] Only one data-odoo-tracking-email tag in each email * [FIX] Get status even in multicompany instances

OCA Transbot updated translations from Transifex

47d8a38

chienandalu force-pushed the 11.0-mig-mail_tracking branch from f63de02 to 72ab11e Compare May 4, 2018 08:32

[FIX] mail_tracking js could break other modules using chat_manager

c7d4288

chienandalu force-pushed the 11.0-mig-mail_tracking branch from 72ab11e to fd6ecb2 Compare May 4, 2018 08:46

[MIG] mail_tracking: Migration to 11.0

a529e15

chienandalu force-pushed the 11.0-mig-mail_tracking branch from fd6ecb2 to a529e15 Compare May 7, 2018 08:22

yajo approved these changes May 7, 2018

View reviewed changes

yajo merged commit bae9999 into OCA:11.0 May 7, 2018

yajo deleted the 11.0-mig-mail_tracking branch May 7, 2018 09:22

[MIG] mail_tracking: Migration to 11.0 #244

[MIG] mail_tracking: Migration to 11.0 #244

Conversation

chienandalu commented Mar 19, 2018

Mail tracking

rafaelbn commented Mar 21, 2018

chienandalu commented Mar 21, 2018

chienandalu commented Mar 21, 2018

pedrobaeza commented Mar 21, 2018

simahawk commented Mar 22, 2018

chienandalu commented Mar 27, 2018

simahawk commented Mar 27, 2018

chienandalu commented Apr 2, 2018

simahawk commented Apr 4, 2018

pedrobaeza commented Apr 5, 2018

chienandalu commented Apr 5, 2018

pedrobaeza commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Apr 7, 2018

chienandalu commented Apr 9, 2018

simahawk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn commented Apr 13, 2018

pedrobaeza commented Apr 14, 2018

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn commented Apr 26, 2018

rafaelbn commented Apr 26, 2018

pedrobaeza commented May 5, 2018

simahawk left a comment •

edited

Loading