11.0 mig mass_mailing_partner #277

ernestotejeda · 2018-05-19T03:46:21Z

rafaelbn · 2018-06-05T05:43:48Z

chienandalu

Thanks for the work, @ernestotejeda !

One important thing you have to count with in v11 mass_mailing migrations is that the relation between contacts and lists has changed: so now there should be a contact for every mail address and that contact is subscribed to n lists.

For what this module is concerned:

The limitation of creating a partner or not by model list may not fit quite well already. What do you think here, @yajo ?
You should check if a contact with a partner already exists, and add the lists to the contact ones.
Adding a contact to a list should not create a new contact if a contact associated with that partner already exists.

yajo

Some complex old code is making it harder to migrate and review, so I added mostly simplification suggestions.

yajo · 2018-06-05T08:12:42Z

mass_mailing_partner/README.rst

+* Javier Iniesta <javieria@antiun.com>
+* Jairo Llopis <jairo.llopis@tecnativa.com>
+* David Vidal <david.vidal@tecnativa.com>
+


Add yourself @ernestotejeda

yajo · 2018-06-05T08:13:02Z

mass_mailing_partner/__manifest__.py

+    "version": "11.0.1.0.0",
+    "author": "Tecnativa, "
+              "Odoo Community Association (OCA)",
+    "website": "https://www.tecnativa.com",


https://github.com/OCA/social

yajo · 2018-06-05T08:17:55Z

mass_mailing_partner/models/mail_mass_mailing_contact.py

+                                 domain=[('email', '!=', False)])
+
+    @api.constrains('partner_id', 'list_ids')
+    def _check_partner_id_lists_ids(self):


"_check_partner_id_list_ids"

yajo · 2018-06-05T08:19:02Z

mass_mailing_partner/models/mail_mass_mailing_contact.py

+                ])
+                if contact.list_ids & other_contact.mapped('list_ids'):
+                    raise ValidationError(_("Partner already exists in one of "
+                                            "these mailing lists"))


Isn't it easier to just put an unique constraint on the partner_id field? Since now a contact has several lists, there's no need to have more than 1.

With that, you are limiting a lot the possibilities, so I prefer to not do it.

I think the same as @pedrobaeza , besides, in this way it is guaranteed that a partner is not repeated in a list and at the same time I do not limit things so much

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

mass_mailing_partner/models/mail_mass_mailing_contact.py

+            self.name = self.partner_id.name
+            self.email = self.partner_id.email
+            if self.partner_id.title:
+                self.title_id = self.partner_id.title.id


When doing assignment with this kind of notation, it's safer to do instead:

self.title_id = self.partner_id.title

Because the ORM will check both records belong to the same model, and the rest will work equally.

yajo · 2018-06-05T08:33:14Z

mass_mailing_partner/models/res_partner.py

+
+    @api.constrains('email')
+    def _check_email_mass_mailing_contacts(self):
+        for partner in self:


Try just:

if self.sudo().filtered("mass_mailing_contact_ids.list_ids"): raise ...

I'm thinking that it's best to not limiting this, as it will lead to constant problems

email field of contact is required and it depend on the email of the partner associated. So, i think, we should not permit that someone delete the email of the partner that has some contact associated.

yajo · 2018-06-05T08:33:34Z

mass_mailing_partner/models/res_partner.py

+                            "mailing lists. Email must be assigned."
+                        ) % partner.name)
+
+    @api.multi


It's implicit, remove it.

yajo · 2018-06-05T08:34:07Z

mass_mailing_partner/models/res_partner.py

+            partner.mass_mailing_contacts_count = mapped_data.get(partner.id,
+                                                                  0)
+
+    @api.multi


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

mass_mailing_partner/models/res_partner.py

+            # Using sudo because ACLs shouldn't produce data inconsistency
+            self.env["mail.mass_mailing.contact"].sudo().search([
+                ("partner_id", "in", self.ids),
+            ]).write(mm_vals)


Can't you replace all of this by calling self.mass_mailing_contact_ids._onchange_partner() after writing?

yajo · 2018-06-05T08:38:19Z

mass_mailing_partner/models/mail_mass_mailing.py

+    partner_mandatory = fields.Boolean(string="Mandatory Partner",
+                                       default=False)
+    partner_category = fields.Many2one(comodel_name='res.partner.category',
+                                       string="Partner Tag")


Provide help on this field. Its purpose is unclear.

rafaelbn

Tested in runbot functionally 👍
Please @ernestotejeda attend @yajo and @chienandalu comments. Thanks!

ernestotejeda · 2018-06-17T16:11:03Z

It's true, now a contact has several lists, but, if we put an unique constraint on the partner_id field, then when a contact is unsubscribed, he/she will not receive any mass-mail sent to any list asociated with him/her. But with the python constraint, we can have an unsubscribed contact 'x' with a partner asociated to a sub-set of lists 'SS1', and another contact 'y' with the same partner asociated with other sub-set of lists 'SS2' but not unsubscribed. So, the email asociated with this contacts 'x' and 'y' can receive mass mailing that will be send to 'SS2' and not receive mass mailing that will be send to 'SS1'.

pedrobaeza

I have squashed all the v11 history for better reviewing, but there are some comments that still apply.

pedrobaeza · 2018-06-17T18:53:50Z

mass_mailing_partner/models/mail_mass_mailing_contact.py

+                ])
+                if contact.list_ids & other_contact.mapped('list_ids'):
+                    raise ValidationError(_("Partner already exists in one of "
+                                            "these mailing lists"))

    @api.onchange('partner_id')
    def _onchange_partner(self):


Rename this to _onchange_partner_id_mass_mailing_partner for having an unique method name and avoid to choke with other modules.

pedrobaeza · 2018-06-17T18:54:29Z

mass_mailing_partner/models/mail_mass_mailing_contact.py

+        if not vals.get('email') and vals.get('partner_id'):
+            vals['email'] = vals['partner_id']
+        result = super(MailMassMailingContact, self).create(vals)
+        if not result.partner_id:


Please don't perform a create and a write. You should do it adding to vals the corresponding result. What you can do is:

def create(self, vals): if not vals.get('partner_id'): record = self.new({ 'email': vals.get('email'), 'name': vals.get('name'), }) record._set_partner(vals) record._onchange_partner() new_vals = record._convert_to_write(record._cache) vals.update(new_vals)

pedrobaeza · 2018-06-17T19:08:24Z

mass_mailing_partner/models/mail_mass_mailing_contact.py

-                vals = contact._set_partner(vals)
-            vals = contact._set_name_email(vals)
-        return super(MailMassMailingContact, self).write(vals)
+            if not contact.partner_id:


Perform something similar to create here.

pedrobaeza · 2018-06-17T19:09:04Z

mass_mailing_partner/models/mail_mass_mailing_contact.py

-        vals['email'] = partner.email
-        vals['name'] = partner.name
-        return vals
+            self.partner_id = m_partner.sudo().create({


Don't remove the _prepare_... method for having a hook on things.

pedrobaeza · 2018-06-17T19:10:35Z

mass_mailing_partner/models/mail_mass_mailing_contact.py

+    @api.constrains('partner_id', 'list_ids')
+    def _check_partner_id_list_ids(self):
+        for contact in self:
+            if contact.partner_id:


I'm thinking that it's best to not limiting this, as it will lead to constant problems

pedrobaeza · 2018-06-17T19:11:21Z

mass_mailing_partner/models/res_partner.py

+
+    @api.constrains('email')
+    def _check_email_mass_mailing_contacts(self):
+        for partner in self:


I'm thinking that it's best to not limiting this, as it will lead to constant problems

ernestotejeda · 2018-06-20T00:07:17Z

It doesn't pass the tests because the module mass_mailing_custom_unsubscribe throws an error, I have done a PR #287 that solves the problem. Can you please check it?

[IMP] mass_mailing_partner: Link mail statistics to partner

* Exclude opt_out. Now opted-out records will not be counted in the "Mailing lists" smart button in the partner form. * Avoid duplicate error. By indicating the exact `partner_id` and ensuring no contacts associated to it are found, you avoid possible duplication errors when several partners share the same name or email.

Without this patch, users without access to reading and editing mass mailing contact records are now unable to change a partner's name or email. They'd recieve an exception such as: AccessError: Sorry, you are not allowed to access this document. Only users with the following access level are currently allowed to do that: - Mass Mailing/User (Document model: mail.mass_mailing.contact) Restrictive ACLs shouldn't restrict normal user operation nor DB consistency, so using sudo mode now and testing behavior.

* [FIX+IMP] mass_mailing_list_dynamic: tests, icons, filters... * Brand new icon * Added feature of loading an existing filter as criteria * Tests as SavepointCase for optimizing times * Tests in post-install for avoiding errors on res.partner not null constraints when several modules added them. * Updated documentation. * Fix mock in test for not commiting test data. * [FIX] mass_mailing_list_dynamic: Wasn't able to create contacts in fully synced lists Syncing context was being set in the wrong object. Added to test too. * [FIX] mass_mailing_list_dynamic: Allow to write back vals from res.partner Module mass_mailing_partner writes back certain values from partner to mass_mailing_contact. Module should allow that write operation.

- In DB which use large amounts of records and intesive use of mass_mailings, not optimized compute records lead to a drastical decrease of performance

pedrobaeza · 2018-06-20T09:58:10Z

I have rebased and squashed several commits. @yajo please review again to see if we unblock this.

yajo

It looks good. Some doubts, but since they are theories, I think it's not worth it blocking this for them.

yajo · 2018-06-20T10:04:15Z

mass_mailing_partner/models/res_partner.py

+            if not partner.email and self.sudo().mass_mailing_contact_ids:
+                raise ValidationError(_(
+                    "This partner '%s' is linked to one or more mass "
+                    "mailing contact. Email must be assigned."


I'm thinking that it should discriminate when mass mailings are in queue. Otherwise, there would be no problem in removing the email, right?

But the field email in the mass_mailing contact model is still required. Setting the linked partner email to blank would raise an exception.

yajo · 2018-06-20T10:04:36Z

mass_mailing_partner/readme/DESCRIPTION.rst

+This module links mass-mailing contacts with partners.
+
+Features
+--------


Isn't this gonna crash?

Yeah, good catch. The only allowed subtitle is ~~~~~~, and I'm not sure we should keep it.

pedrobaeza

As there are too much changes difficult to see if there's going to be problems, I'm merging this and we'll try on several DBs

rafaelbn added this to the 11.0 milestone Jun 5, 2018

rafaelbn added the work in progress label Jun 5, 2018

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

chienandalu suggested changes Jun 5, 2018

View reviewed changes

yajo requested changes Jun 5, 2018

View reviewed changes

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

rafaelbn approved these changes Jun 13, 2018

View reviewed changes

damcar mentioned this pull request Jun 14, 2018

11.0 mig mass_mailing_list_dynamic #284

Closed

1 task

ernestotejeda mentioned this pull request Jun 17, 2018

11.0 mig mass_mailing_unique #276

Merged

pedrobaeza force-pushed the 11.0-mig-mass_mailing_partner branch from ea666a2 to 471d491 Compare June 17, 2018 18:19

pedrobaeza requested changes Jun 17, 2018

View reviewed changes

Javier Iniesta and others added 12 commits June 20, 2018 11:57

mass_mailing_partner

627b238

OCA Transbot updated translations from Transifex

88231e4

Link mail statistics to partner (OCA#104)

50cbb70

[IMP] mass_mailing_partner: Link mail statistics to partner

OCA Transbot updated translations from Transifex

1ff9147

OCA Transbot updated translations from Transifex

262b667

[MIG] mass_mailing_partner: Migration to 10.0

1657e77

OCA Transbot updated translations from Transifex

982c472

OCA Transbot updated translations from Transifex

78366ae

OCA Transbot updated translations from Transifex

83d16b1

[FIX] mass_mailing_partner: compute fields performance (OCA#271)

cdd5a2e

- In DB which use large amounts of records and intesive use of mass_mailings, not optimized compute records lead to a drastical decrease of performance

pedrobaeza force-pushed the 11.0-mig-mass_mailing_partner branch from fc2fa23 to acf27e7 Compare June 20, 2018 09:57

yajo approved these changes Jun 20, 2018

View reviewed changes

[MIG] mass_mailing_partner: Migration to 11.0

ac6bb81

pedrobaeza force-pushed the 11.0-mig-mass_mailing_partner branch from a81bd6a to ac6bb81 Compare June 20, 2018 22:03

pedrobaeza removed the needs fixing label Jun 20, 2018

pedrobaeza approved these changes Jun 20, 2018

View reviewed changes

pedrobaeza merged commit bab34b4 into OCA:11.0 Jun 20, 2018

pedrobaeza mentioned this pull request Jun 20, 2018

Migration to version 11.0 #203

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

11.0 mig mass_mailing_partner #277

11.0 mig mass_mailing_partner #277

ernestotejeda commented May 19, 2018 •

edited

Loading

rafaelbn commented Jun 5, 2018

chienandalu left a comment

yajo left a comment

yajo Jun 5, 2018

yajo Jun 5, 2018

yajo Jun 5, 2018

yajo Jun 5, 2018

pedrobaeza Jun 17, 2018

ernestotejeda Jun 19, 2018

yajo Jun 5, 2018

yajo Jun 5, 2018

pedrobaeza Jun 17, 2018

ernestotejeda Jun 19, 2018

yajo Jun 5, 2018

yajo Jun 5, 2018

yajo Jun 5, 2018

yajo Jun 5, 2018

rafaelbn left a comment

ernestotejeda commented Jun 17, 2018

pedrobaeza left a comment

pedrobaeza Jun 17, 2018

pedrobaeza Jun 17, 2018

pedrobaeza Jun 17, 2018

pedrobaeza Jun 17, 2018

pedrobaeza Jun 17, 2018

pedrobaeza Jun 17, 2018

ernestotejeda commented Jun 20, 2018

pedrobaeza commented Jun 20, 2018

yajo left a comment

yajo Jun 20, 2018

chienandalu Jun 20, 2018

yajo Jun 20, 2018

pedrobaeza Jun 20, 2018

pedrobaeza left a comment

11.0 mig mass_mailing_partner #277

11.0 mig mass_mailing_partner #277

Conversation

ernestotejeda commented May 19, 2018 • edited Loading

rafaelbn commented Jun 5, 2018

chienandalu left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn left a comment

Choose a reason for hiding this comment

ernestotejeda commented Jun 17, 2018

pedrobaeza 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

ernestotejeda commented Jun 20, 2018

pedrobaeza commented Jun 20, 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

pedrobaeza left a comment

Choose a reason for hiding this comment

ernestotejeda commented May 19, 2018 •

edited

Loading