11.0 mig mass_mailing_unique #276

ernestotejeda · 2018-05-19T03:40:25Z

cc @Tecnativa

rafaelbn · 2018-06-05T05:42:59Z

cc @Tecnativa

chienandalu

Here there's a problem with the new Odoo mailing lists implementation as well. Here's the thing (ping @yajo ):

Two contacts with the same email address will receive just one email, so one of them is just ignored.
Maybe we should just add a _sql_constraints by email.

yajo · 2018-06-05T08:47:44Z

mass_mailing_unique/__manifest__.py

+    "summary": "Avoids duplicate mailing lists and contacts",
+    "version": "11.0.1.0.0",
+    "category": "Marketing",
+    "website": "https://tecnativa.com",


https://github.com/OCA/social

yajo · 2018-06-05T08:48:20Z

mass_mailing_unique/models/mass_mailing.py

+    _inherit = "mail.mass_mailing.contact"
+
+    @api.constrains('partner_id', 'list_ids')
+    def _check_partner_id_lists_ids(self):


Just add a unique constraint per email and remove this method.

yajo · 2018-06-05T08:49:25Z

mass_mailing_unique/models/mass_mailing.py

+class MailMassMailingContact(models.Model):
+    _inherit = "mail.mass_mailing.contact"
+
+    @api.constrains('partner_id', 'list_ids')


If I'm not wrong, partner_id field doesn't exist.

yajo · 2018-06-05T08:50:43Z

mass_mailing_unique/hooks.py

+    cr.execute("""SELECT c.email, l.name, COUNT(c.id)
+                  FROM
+                    mail_mass_mailing_contact AS c
+                    INNER JOIN mail_mass_mailing_contact_list_rel AS cl


All you need to check, if you follow my suggestion below, is to check there are no duplicate emails in mail_mass_mailing_contact. No joins needed.

rafaelbn · 2018-06-13T14:46:35Z

This PR depends on @chienandalu approval.

yajo · 2018-06-13T15:04:20Z

Maybe we should just add a _sql_constraints by email.

This can't be possible, because it would break mass_mailing_custom_unsubscribe, which is currently much more needed.

Two contacts with the same email address will receive just one email, so one of them is just ignored.

We just checked with @rafaelbn that it sends duplicated emails, just the 2nd (and next ones) are set in exception state. Not sure why that happens, but it can be confusing.

At the end, seeing current v11 implementation, all we need here is a python constraint that avoids 2 contacts to exist for 1 list.

ernestotejeda · 2018-06-17T16:15:14Z

the situation here is similar to 11.0 mig mass_mailing_partner #277 , i had comment about that

pedrobaeza · 2018-06-17T19:14:52Z

mass_mailing_unique/readme/CONTRIBUTORS.rst

@@ -0,0 +1,6 @@
+* `Tecnativa <https://www.tecnativa.com>`_:
+
+  * Jairo Llopis <jairo.llopis@tecnativa.com>


Remove emails

chienandalu

I think case variations should be consider to avoid duplicates due to user errors. For instance, these two email:

Are going to be sent to the same recipient, but is going to pass over the duplicates filter this module tries to avoid.

chienandalu · 2018-06-18T14:31:53Z

mass_mailing_unique/models/mass_mailing.py

+    def _check_email_list_ids(self):
+        for contact in self:
+            other_contact = self.search([
+                ('email', '=', contact.email),


To avoid duplucates due to case variations:

('email', '=', contact.email.lower()),

chienandalu · 2018-06-18T14:37:16Z

mass_mailing_unique/models/mass_mailing.py

+class MailMassMailingList(models.Model):
+    _inherit = "mail.mass_mailing.list"
+    _sql_constraints = [
+        ("unique_name", "UNIQUE(name)",


Same problem is going to happen here with case variations. For instance:

- My list of contacts - my list of contacts - My List Of Contacts - MY LIST OF CONTACTS ...

They all are going to pass over the constrain.

With postgre, i think you can not make an insensitive unique constraint to not distinguish between uppercase and lowercase. To solve that, we can make an UNIQUE INDEX, for example:
CREATE UNIQUE INDEX "name_unique_idx" ON "mail_mass_mailing_list" (LOWER ('name'))
But then we must catch an IntegrityError exception in the create and write method or in another place. Actually, I do not know if it's worth doing all of this for this specific case

Fortunately it's possible to add a constraint case insensitive. Just put UNIQUE (LOWER(email)). See for example: http://shuber.io/case-insensitive-unique-constraints-in-postgres/

In this page it says that Unfortunately, Postgres does now allow us to define unique constraint using LOWER and according to this page one of the solutions is to make a 'UNIQUE INDEX'. That's what I said above and then it can be more work

Right, I read it bad :(

OK, let's keep it that way. @chienandalu please give your final review

chienandalu · 2018-06-18T14:39:29Z

mass_mailing_unique/hooks.py

+            _("{0} appears {2} times in list {1}.").format(*result))
+
+    # Search for duplicates in list's name
+    cr.execute("""SELECT name, COUNT(id)


If we want to avoid case duplicates:

SELECT LOWER(name) AS n, COUNT(id) [...] GROUP BY n

chienandalu · 2018-06-18T14:59:48Z

mass_mailing_unique/hooks.py

+    errors = list()
+
+    # Search for duplicates in emails
+    cr.execute("""SELECT c.email, l.name, COUNT(c.id)


We should check avoid email case variations. Something like:

SELECT LOWER(c.email) AS e, l.name, COUNT(c.id) [...] GROUP BY l.name, e, c.id

pedrobaeza · 2018-06-20T09:59:42Z

@ernestotejeda please rebase and attend latest comments

This new module avoids duplicates in mass mailing lists. Now you cannot have 2 lists with the same name, and you cannot have the same email repeated in a list. This will avoid sending the same mail to a contact repeated times, which will disturb him and most probably cause you to be blocked as spam.

pedrobaeza · 2018-06-28T14:42:17Z

@ernestotejeda next time please drop a comment when you make the rebase, because no notification gets to our inboxes if not.

pedrobaeza

@yajo please finish your review with latest changes

rafaelbn requested review from yajo, chienandalu and rafaelbn June 5, 2018 05:42

rafaelbn added the needs review label Jun 5, 2018

rafaelbn added this to the 11.0 milestone Jun 5, 2018

chienandalu reviewed Jun 5, 2018

View reviewed changes

yajo requested changes Jun 5, 2018

View reviewed changes

rafaelbn added work in progress and removed needs review labels Jun 13, 2018

pedrobaeza requested changes Jun 17, 2018

View reviewed changes

chienandalu reviewed Jun 18, 2018

View reviewed changes

Jairo Llopis and others added 7 commits June 20, 2018 16:23

[9.0] [MIG] mass_mailing_unique module

6fd013d

[IMP] mass_mailing_unique: Add tests

79a590d

OCA Transbot updated translations from Transifex

961d616

[MIG] mass_mailing_unique: Migration to 10.0

d8d3ca9

OCA Transbot updated translations from Transifex

fc23d65

[MIG] mass_mailing_unique: Migration to 11.0

5682580

ernestotejeda force-pushed the 11.0-mig-mass_mailing_unique branch from 45d9fef to 5682580 Compare June 20, 2018 20:25

pedrobaeza mentioned this pull request Jun 20, 2018

Migration to version 11.0 #203

Closed

26 tasks

pedrobaeza removed the work in progress label Jun 28, 2018

pedrobaeza approved these changes Jun 28, 2018

View reviewed changes

yajo approved these changes Jun 29, 2018

View reviewed changes

yajo merged commit 1274d12 into OCA:11.0 Jun 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

11.0 mig mass_mailing_unique #276

11.0 mig mass_mailing_unique #276

ernestotejeda commented May 19, 2018 •

edited

Loading

rafaelbn commented Jun 5, 2018

chienandalu left a comment

yajo Jun 5, 2018

yajo Jun 5, 2018

yajo Jun 5, 2018

yajo Jun 5, 2018

rafaelbn commented Jun 13, 2018

yajo commented Jun 13, 2018

ernestotejeda commented Jun 17, 2018

pedrobaeza Jun 17, 2018

chienandalu left a comment

chienandalu Jun 18, 2018

chienandalu Jun 18, 2018

ernestotejeda Jun 20, 2018

pedrobaeza Jun 20, 2018

ernestotejeda Jun 20, 2018

pedrobaeza Jun 21, 2018

chienandalu Jun 18, 2018

chienandalu Jun 18, 2018

pedrobaeza commented Jun 20, 2018

pedrobaeza commented Jun 28, 2018

pedrobaeza left a comment

		@@ -0,0 +1,6 @@
		* `Tecnativa <https://www.tecnativa.com>`_:

		* Jairo Llopis <jairo.llopis@tecnativa.com>

11.0 mig mass_mailing_unique #276

11.0 mig mass_mailing_unique #276

Conversation

ernestotejeda commented May 19, 2018 • edited Loading

rafaelbn commented Jun 5, 2018

chienandalu 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

rafaelbn commented Jun 13, 2018

yajo commented Jun 13, 2018

ernestotejeda commented Jun 17, 2018

Choose a reason for hiding this comment

chienandalu 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

pedrobaeza commented Jun 20, 2018

pedrobaeza commented Jun 28, 2018

pedrobaeza left a comment

Choose a reason for hiding this comment

ernestotejeda commented May 19, 2018 •

edited

Loading