[12.0][MIG] mass_mailing_partner: Migration to v12.0 #323

sergio-teruel · 2018-11-09T12:26:04Z

cc @Tecnativa

chienandalu

Tests are failing on external ids of partner which were completely changed in v12

mass_mailing_partner/views/mail_mass_mailing_contact_view.xml

mass_mailing_partner/wizard/partner_mail_list_wizard.py

yajo

OK when bots get ✔️

mass_mailing_partner/models/res_partner.py

chienandalu

Thanks @ernestotejeda ! A final comment. As I stated before: wouldn't it make more sense to add mailing lists to the partner main (and unique?) contact instead of creating a new one everytime we run the wizard?

ernestotejeda · 2018-11-13T15:04:44Z

As I said in PR #324 , I think it is feasible to add a contact _sql_constraints in mailing contact (mail.mass_mailing.contact) to add a unique (email).
If you agree with this, then all the constraints in this module would no longer be necessary to prevent more than one partner (res.partner) from being in the same mailing list more than once, just by making this module dependent of mass_mailing_unique that would be solved, because avoiding that an email is repeated in a list, implies that a contact in a list will not be repeated either. This implies that each partner is associated only with a mailing contact (Many2one instead of Many2many).
What do you think about this?

pedrobaeza · 2018-11-13T15:19:16Z

You both should take into account that we sometimes want to have same the same partner in several mass mailing contacts. For example, with mass_mailing_list_dynamic, unless we put another implementation for that one. Check the v11 PR for that where I put several comments about this topic.

chienandalu · 2018-11-13T16:31:01Z

To have the PR @pedrobaeza comments in consideration: #284 (abandoned?)

Indeed in v11 there's a problem in the way the unsubscription was managed due to an incomplete frontend support. I'd say that's now solved in v12 (it supports it both ways):

I'd go for allowing both ways of having contacts. Although I still think the logic way for the partner is to have a unique relation to a single contact.

ernestotejeda · 2018-11-14T03:35:55Z

As i said in PR #324 , i think is better to leave that migration as it is now, so i think we should leave this PR as it is now too.

rafaelbn · 2018-11-15T02:50:04Z

Rebuilding runbot for testing

About #323 (comment) from @ernestotejeda

The functional purpose of this module is relate res.partner with mail.mass_mailing.contact (the README is clear
The NEW thing / change in v12 (@sergio-teruel I think you are aware) is that now ONE single mail.mass_mailing.contact can be related to N mail.mass_mailing.list. En v11 and before you have to create ONE mail.mass_mailing.contact PER mail.mass_mailing.list . THIS means for example that now you cannot group by mailing list int the list of mass mailing contact
The POINT This module must not prevent that 2 or more different mail.mass_mailing.contact has the same email for this purpose we have mass_mailing_unique (could be use toguether o not). We should be aware about:
- ONE res.partner can be related to ONE mail.mass_mailing.contact.
- If you create a NEW res.partner and add it to a mass mailing list then create a NEW mail.mass_mailing.contact in that list
- If you take and EXISTIGN res.partner which already is related to a mail.mass_mailing.contact and you want to add it to another mass mailing list then DON'T create a NEW mail.mass_mailing.contact take the existing mail.mass_mailing.contact related to the res.partner and ADD it to another mass mailing list
- Can you create N res.partners with the same email so you will have N mass.mailing.contract with the same email? YES (I guess this will be never forbidden)
- Can you ADD to the same Mass Mailing Lins to N res.partner with the same email? YES if you don't want the use mass_mailing_unique
- In the CASE you have a mass mailing list and you create a new mail.mass_mailing.contact it will check if there is a partner with that email and related before creating a new one if there is not such a partner with that email then if this option is enabled https://github.com/OCA/social/pull/323/files#diff-1d6f3cba6dabcedb707195bcf524ad25R35 will create a new partner from the mail.mass_mailing.contact
Because you named in this PR... mass_mailing_list_dynamic should change name to mass_mailing_partner_list_dynamic in v12
About mass_mailing_partner_list_dynamic and [12.0][MIG] mass_mailing_partner: Migration to v12.0 #323 (comment) we will take care migrating it to v12. Because now in v12 the game has change and the migration of date will be hard (we will have to merge al mass.mailing.contact of a single res.partner in ONE single mass mailing contact

VERY IMPORTANT ISSUE WE HAD and @chienandalu SOLVED is when the user (customer) MERGE res.partner . I will test this case to see what happends.

rafaelbn

Please review that:

All fields from res.partner are filled in the mass_mailng.contact when it gets created
All filelds from mass_maling.contact are set in the res.partner when created
Which are the possibilities to have this fields updated over the time?

rafaelbn · 2018-11-15T03:01:50Z

mass_mailing_partner/README.rst

+
+* When creating or saving a mass-mailing contact, partners are matched through
+  email, linking matched partner, or creating a new one if no match and the
+  maling list partner mandatory field is checked.


@ernestotejeda I have found a BUG. Module creates a NEW res.partner always and it shouldn't.

Modules should check always if there is a res.partner with the same email to match but

Only if the mail.mass_mailing.contact belongs to a mail.mass_mailing.list which has set to TRUE the field "Mandatory Partner" must create a NEW res.partner

@rafaelbn Yay!

@rafaelbn I tested on the runbot and I could not reproduce that error

mass_mailing_partner/models/res_partner.py

rafaelbn · 2018-11-15T03:18:28Z

mass_mailing_partner/README.rst

+* When creating or saving a mass-mailing contact, partners are matched through
+  email, linking matched partner, or creating a new one if no match and the
+  maling list partner mandatory field is checked.
+* Mailing contacts smart button in partner form.


Yes, but I've find a new smart-button "Mailing contact" in res.partner. Why are we allowing this? In my side doesn't have any sense as I said and we should FIX this. In the migration from v10 or v11 to v12 then OpenUpgrade should merge all mass_mailing.contact related to a single res.partner to a single mass_mailing.contact

ernestotejeda · 2018-11-16T14:53:44Z

Well, I don't want to extend the debate any longer, but I think the summary that @rafaelbn has made is very good, and I take the opportunity to respond to these aspects because I think that before making any change to the code, we must have everything clearly

@rafaelbn, I agree with almost everything he has put in, but I do not understand what would be the problem with allowing a partner to be associated with more than one contact.
Since this does not contradict the functional purpose of this module (as long as all the contacts associated with this partner belong to different lists).
Indeed We could restrict that a partner is associated with a single contact (as you said), and that contact associate all the lists that we want to relate with the partner, but this, I think, instead of making everything simpler, it would complicate things, because:

An idea would be in the create method, call write method in case the contact we are creating match with an existing partner that already has an associated contact, so that the lists are added to this, and return that record instead of calling super. About this we were debating in the PR 12.0 mig mass_mailing_unique #324 but referring to the email field instead partner_id field. But the problem is that we should also overwrite the write method when a contact is modified by changing the partner for one that already has an associated contact, we can add the lists to that contact, but then we would have to call the unlink method to eliminate the contact that was being modified, and here in this last case it does not behave in the expected way, I already implemented it to make sure, and for that reason I discard that option.
Another idea would be to put a constraint to avoid always having two contacts associated with the same partner. But then we should make modifications in other places so there is no conflict with this. I have already found these:

1- the wizard that exists to associate several partners to a list:

For each partner, it should be checked that if this already has a contact associated, that contact would be added to the selected list, otherwise a new one would be created. (This has already been mentioned before)

2- In res.partner:

The Many2many with mail.mass_mailing.contact would not make sense, therefore, it would be then a many2one.
The smart-button "Mailing contact" would not make sense either, but it could be changed by one called "Mailing list" in order to see the mailing lists associated with the partner's contact.

3- In the mass_mailing_list_dynamic module (in case you were to migrate to v12)

During the synchronization, after finding the partners that matches with the domain, you would only have to create a new contact if this partner no longer has a contact associated with it.
This would be a change that would be made when the module is migrated.

4- As I said in the PR #324 . The module website_mass_mailing 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 this list, this module creates a new contact mailing although there is another contact with that e-mail. So, if there is an AA contact with an email aa@example.com, each time that email subscribes to a list through the website, a contact will be created with that email and therefore associated with the AA partner.
What I can think of to solve this is to create an auto_install = True module that depends on website_mass_mailing and mass_mailing_partner, this module would redefine the 'subscribe' method in the snippet controller to create a contact only if one does not already exist with the same partner.

Anyway, I think that if the fact that a partner can be related to more than one contact is not wrong as a concept, it would be better to leave it as is, so as not to have to intervene in the aforementioned aspects.

ernestotejeda · 2018-11-16T15:16:02Z

the domain domain = [('opt_out', '=', False)] in the field mass_mailing_contact_ids was removed because the mail.mass_mailing.contact doesn't have that field now in v12, mail.mass_mailing class. list_contact_rel is the class that has this field now.

chienandalu · 2018-11-16T15:23:46Z

Good analysis @ernestotejeda I'm on keeping both ways of behaviour so we avoid those issues. So:

I'd go for the wizard approach for adding a list to the main partner contact. The code is almost done 😄 : https://github.com/OCA/social/pull/323/files#diff-e7229f99dcf57bbe7c6c20faef2c10cfR27
(maybe as a post-PR improvement) I'd provide a tool to merge partner contacts into a single one. Something like:

list_ids = partner.contact_ids.mapped('list_ids')
partner.contact_ids[1:].unlink()
partner.contact_ids[:1].list_ids |= list_ids  # Or maybe on the relation model to keep the subscription dates and states

The last one could be even triggered in the res.partner write()

ernestotejeda · 2018-11-16T15:32:03Z

I agree with you @chienandalu, thanks

ernestotejeda · 2018-11-16T15:32:08Z

@rafaelbn when a contact is associated with a partner the following contact fields:
name, email, title_id, company_name, country_id and tag_ids are read-only and each time one of these fields is modified in the related partner, the corresponding field is updated in the contact. That is the way in which the module keeps the fields updated at the moment. The only problem is that the company is not updating well, I have to fix that.

sergio-teruel · 2019-04-18T15:01:55Z

@ernestotejeda @chienandalu Whats happend with this? 😄

ernestotejeda · 2019-04-18T15:42:31Z

Well, @sergio-teruel , I think all the suggestions have been resolved.

rafaelbn

This migration is critical I need to analyze deeper. It could be a disaster when migrating a customer from v10 to v12 for example. I cannot approve with out testing it deeper.

Need to test it deeper again

victormmtorres

I would try finish this PR as I have next module depends on

chienandalu · 2019-06-21T12:36:02Z

I would try finish this PR as I have next module depends on

Thanks, @victormmtorres I'd say this was almost ready but I lost track of it. I you'd review if all the comments were honored and this is ready to review again let me know and I do my final one :)

[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

victormmtorres · 2019-06-21T14:20:14Z

@chienandalu I kind of lost on that amount of comments on many people on this PR, I fix one I found is clear. But other maybe already done by @ernestotejeda on his last commit ca74186

mass_mailing_partner/models/mail_mass_mailing_contact.py

For each partner, if already has a contact it's added to the selected list, otherwise a new one is created

Or infinite recursions will happen on other `write` overwrites, like the one that happens on `mass_mailing_partner`.

pedrobaeza · 2019-06-26T18:49:18Z

/ocabot merge

OCA-git-bot · 2019-06-26T18:49:41Z

This PR looks fantastic, let's merge it!
Rebased to 12.0-ocabot-merge-pr-323-by-pedrobaeza-bump-no, awaiting test results.

Signed-off-by pedrobaeza

OCA-git-bot · 2019-06-26T19:44:52Z

Congratulations, your PR was merged at 7081297. Thanks a lot for contributing to OCA. ❤️

PS: Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged into 12.0.

chienandalu reviewed Nov 9, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

pedrobaeza added this to the 12.0 milestone Nov 9, 2018

chienandalu suggested changes Nov 9, 2018

View reviewed changes

mass_mailing_partner/views/mail_mass_mailing_contact_view.xml Show resolved Hide resolved

mass_mailing_partner/wizard/partner_mail_list_wizard.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

yajo added the needs fixing label Nov 12, 2018

yajo approved these changes Nov 12, 2018

View reviewed changes

chienandalu reviewed Nov 12, 2018

View reviewed changes

mass_mailing_partner/models/res_partner.py Outdated Show resolved Hide resolved

mass_mailing_partner/models/res_partner.py Outdated Show resolved Hide resolved

chienandalu reviewed Nov 13, 2018

View reviewed changes

rafaelbn self-requested a review November 15, 2018 02:03

rafaelbn added needs review and removed needs fixing labels Nov 15, 2018

rafaelbn requested changes Nov 15, 2018

View reviewed changes

emagdalenaC2i mentioned this pull request Dec 29, 2018

Migration to version 12.0 #306

Closed

24 tasks

rafaelbn previously approved these changes May 8, 2019

View reviewed changes

rafaelbn reviewed May 9, 2019

View reviewed changes

rafaelbn self-requested a review May 9, 2019 00:16

victormmtorres reviewed Jun 21, 2019

View reviewed changes

Javier Iniesta and others added 9 commits June 21, 2019 16:14

mass_mailing_partner

dff0581

OCA Transbot updated translations from Transifex

6585fc1

Link mail statistics to partner (OCA#104)

206801a

[IMP] mass_mailing_partner: Link mail statistics to partner

[MIG] mass_mailing_partner: Migration to 10.0

a7af6de

OCA Transbot updated translations from Transifex

52432d0

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

0e673d9

- 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

victormmtorres force-pushed the 12.0-mig-mass_mailing_partner branch from 156e441 to ca74186 Compare June 21, 2019 14:15

victormmtorres reviewed Jun 25, 2019

View reviewed changes

mass_mailing_partner/models/mail_mass_mailing_contact.py Show resolved Hide resolved

ernestotejeda and others added 4 commits June 26, 2019 20:45

[MIG] mass_mailing_partner: Migration to 11.0

92e1a52

[12.0][MIG] mass_mailing_partner: Migration to v12.0

128d350

mass_mailing_partner: Improve wizard behavior

3699250

For each partner, if already has a contact it's added to the selected list, otherwise a new one is created

[FIX] mail_tracking: Don't call write inside other write

007d8ad

Or infinite recursions will happen on other `write` overwrites, like the one that happens on `mass_mailing_partner`.

pedrobaeza force-pushed the 12.0-mig-mass_mailing_partner branch from d6fb85e to 007d8ad Compare June 26, 2019 18:45

OCA-git-bot merged commit 007d8ad into OCA:12.0 Jun 26, 2019

OCA-git-bot added a commit that referenced this pull request Jun 26, 2019

Merge PR #323 into 12.0

7081297

Signed-off-by pedrobaeza

OCA-git-bot added the merged 🎉 label Jun 26, 2019

pedrobaeza deleted the 12.0-mig-mass_mailing_partner branch June 26, 2019 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12.0][MIG] mass_mailing_partner: Migration to v12.0 #323

[12.0][MIG] mass_mailing_partner: Migration to v12.0 #323

sergio-teruel commented Nov 9, 2018

chienandalu left a comment

This comment has been minimized.

This comment has been minimized.

yajo left a comment

chienandalu left a comment

ernestotejeda commented Nov 13, 2018 •

edited

Loading

pedrobaeza commented Nov 13, 2018

chienandalu commented Nov 13, 2018

ernestotejeda commented Nov 14, 2018

rafaelbn commented Nov 15, 2018

rafaelbn left a comment •

edited

Loading

rafaelbn Nov 15, 2018

chienandalu Nov 15, 2018

ernestotejeda Nov 16, 2018 •

edited

Loading

rafaelbn Nov 15, 2018

ernestotejeda commented Nov 16, 2018 •

edited

Loading

ernestotejeda commented Nov 16, 2018 •

edited

Loading

chienandalu commented Nov 16, 2018

ernestotejeda commented Nov 16, 2018

ernestotejeda commented Nov 16, 2018

sergio-teruel commented Apr 18, 2019

ernestotejeda commented Apr 18, 2019

rafaelbn left a comment

victormmtorres left a comment

chienandalu commented Jun 21, 2019

victormmtorres commented Jun 21, 2019

pedrobaeza commented Jun 26, 2019

OCA-git-bot commented Jun 26, 2019

OCA-git-bot commented Jun 26, 2019

[12.0][MIG] mass_mailing_partner: Migration to v12.0 #323

[12.0][MIG] mass_mailing_partner: Migration to v12.0 #323

Conversation

sergio-teruel commented Nov 9, 2018

chienandalu left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

yajo left a comment

Choose a reason for hiding this comment

chienandalu left a comment

Choose a reason for hiding this comment

ernestotejeda commented Nov 13, 2018 • edited Loading

pedrobaeza commented Nov 13, 2018

chienandalu commented Nov 13, 2018

ernestotejeda commented Nov 14, 2018

rafaelbn commented Nov 15, 2018

rafaelbn left a comment • edited Loading

Choose a reason for hiding this comment

rafaelbn Nov 15, 2018

Choose a reason for hiding this comment

chienandalu Nov 15, 2018

Choose a reason for hiding this comment

ernestotejeda Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

rafaelbn Nov 15, 2018

Choose a reason for hiding this comment

ernestotejeda commented Nov 16, 2018 • edited Loading

ernestotejeda commented Nov 16, 2018 • edited Loading

chienandalu commented Nov 16, 2018

ernestotejeda commented Nov 16, 2018

ernestotejeda commented Nov 16, 2018

sergio-teruel commented Apr 18, 2019

ernestotejeda commented Apr 18, 2019

rafaelbn left a comment

Choose a reason for hiding this comment

victormmtorres left a comment

Choose a reason for hiding this comment

chienandalu commented Jun 21, 2019

victormmtorres commented Jun 21, 2019

pedrobaeza commented Jun 26, 2019

OCA-git-bot commented Jun 26, 2019

OCA-git-bot commented Jun 26, 2019

ernestotejeda commented Nov 13, 2018 •

edited

Loading

rafaelbn left a comment •

edited

Loading

ernestotejeda Nov 16, 2018 •

edited

Loading

ernestotejeda commented Nov 16, 2018 •

edited

Loading

ernestotejeda commented Nov 16, 2018 •

edited

Loading