12.0 mig mass_mailing_unique #324
Conversation
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.
|
In v12, unlike v11, a mailing contact has a One2many with subscriptions to lists (mail.mass_mailing.list_contact_rel), so the same mailing contact (mail.mass_mailing.contact) can be related to mailing lists (mail. mass_mailing.list) from which he / she wants to receive emails and at the same time can be related to other mailing lists from which he / she doesn't want to receive mails, therefore, now in v12, to restrict that an email is no longer once in the same list, it's enough to add a contact _sql_constraints that adds a unique (email). |
|
Well, the relation in v11 was similar. Indeed, we overlooked this feature a little bit since it was not completely usable (for instance in unsubscribes) What I was noting is that when you create a new contact with the same address another contact has, now we're getting a link automatically to the lists that contact has. This is not happening in standard. And I think both options should be available. You dropped the lower case check as well so we can have duplicates just varying the email case again. Besides, this is going to clash against your migration of mass_mailing_partner, that precisely creates several contacts 😄 (anyway, as I stated there, I think subscriptions should be related to a single contact as well). |
|
Well, I haven't dug too much on this. What I want is a happy world and that everything fits the best way 😄 , so go on with the best design you think of having into account all the possible modules. |
|
I have done a bit of research about this topic, and I think we should better leave it as it is now in this PR cause it continues to meet the pursued objective of this module. If we put some restriction so there is no more than one contact with the same email, as I had said before, it would conflict with the module website_mass_mailing, because this module adds the snippet that gives the user the possibility to register in a mass mailing list from the website and each time a user uses this to subscribe to a list, this module creates a new contact mailing although there is already another contact with that email. I don't know either if exists another module out there (maybe in the enterprise edition) that does the same. We could redefine the create method, and if amount the parameters we get an email address that already exists in another contact, we should modify that contact instead of creating a new one, but I do not think it's a good idea to invoke the write method from the create method. |
Indeed it seems a clunky solution. Maybe we have to stay with current implementation and IMHO, now that you have done such research, it's a good time to open a little PR to Odoo master, so the create/write election is done via the snippet controller, and we remove this situation for v14+. Do you feel like opening that? Ping me there if you do. |
|
Well, it's not so bad solution at all. We are already doing that on certain parts and it's working without problems. It's not a problem to call a write on other records on the create, and return that record instead of calling super in that case. |
|
But I'm thinking that this would avoid the creation of explicit isolated contacts, so better to leave it as is indeed. |
mass_mailing_unique/models/mail_mass_mailing_list_contact_rel.py
Outdated
Show resolved
Hide resolved
|
|
||
| @api.constrains('email', 'list_ids') | ||
| def _check_email_list_ids(self): | ||
| for contact in self: |
There was a problem hiding this comment.
I'm afraid this for could lead to bottlenecks, mostly when using suggestions from below. How about using set()s, which allow to check for duplicates? One idea:
others = self.mapped("list_ids.contact_ids") - self
if set(self.mapped("email")) & set(others.mapped("email")):
raise ...There was a problem hiding this comment.
The problem of what you propose is that when you try to create a list with two contacts with the same email address, the exception will not be launched
There was a problem hiding this comment.
Are you sure? AFAICS that's exactly what it does... It just avoids many loops to make sure the method scales fine.
There was a problem hiding this comment.
Suppose that the write method is called to modify two contacts (contact_1 and contact_2) so that both have the same email (contact@example.com) and belong to the same lists (L_1 and L_2), which had not yet been associated to no contact. In that case, the error will not be launched, because the intesection between the sets set (contact@example.com) and set () will be empty.
There was a problem hiding this comment.
Well, you're absolutely right! Thanks 😉
|
Ok, for one side the unique contatc constraint is already implemented in Odoo standard: https://github.com/odoo/odoo/blob/12.0/addons/mass_mailing/models/mass_mailing.py#L61 So why don't we keep the original behaviour of the module so we cover all the cases? |
|
@chienandalu, the objective that this module pursues is that two contacts with the same email address can not be associated to the same mailing list. |
|
Sure, we're on the same page there! 😅 I tested it on runbot and it seems to work fin now. Anyway, the case issue still exists. I dont't know if there could be a way to solve it. |
|
I've tested this PR @chienandalu and I cannot find any issue. I don't understand the problem here #324 (review) 😢 Could you please clarify a little bit? go here , make that you will find this problem.... 🙏 Isn't this PR a trivial migration from v11 to v12? Let's go! |
There was a problem hiding this comment.
Sure @rafaelbn it's solved by now! 😄 Thanks @ernestotejeda !
|
|
||
| @api.constrains('email', 'list_ids') | ||
| def _check_email_list_ids(self): | ||
| for contact in self: |
There was a problem hiding this comment.
Well, you're absolutely right! Thanks 😉
|
This PR has the |
|
Please could you squash migration-related commits to merge? |
ernestotejeda
force-pushed
the
12.0-mig-mass_mailing_unique
branch
from
b3ad35d
to
3f09804
Compare
Cc @Tecnativa