Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP][MIG] mass_mailing_custom_unsubscribe: Migration to 11.0 #279

Merged

Conversation

chienandalu
Copy link
Member

@chienandalu chienandalu commented May 25, 2018

Needs:

WIP as the contacts relation to the lists has changed quite a bit (now one mailing contact can belong to multiple lists) so some parts must be reworked.

cc @Tecnativa

@rafaelbn rafaelbn added this to the 11.0 milestone May 25, 2018
@rafaelbn rafaelbn requested review from yajo and rafaelbn May 25, 2018 17:50
@yajo
Copy link
Member

yajo commented May 28, 2018

Check those ❌ please

@chienandalu
Copy link
Member Author

This WIP yet 😄 The module has to be reworked as the contact<->list relation has changed in v11

@chienandalu
Copy link
Member Author

@yajo This bug odoo/odoo#24969 has to be fixed in order to get the tours working

@rafaelbn
Copy link
Member

rafaelbn commented Jun 1, 2018

@yajo @chienandalu this FIX is needed yes odoo/odoo#24969 and we should propose it in OCB if Odoo don't accept it. Do we have an OPW?

@@ -14,7 +14,15 @@
position="attributes">
<attribute name="class" value="container o_unsubscribe_form_custom"/>
</xpath>

<!---->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@@ -31,5 +36,7 @@ def update_opt_out(self, email, res_ids, value):
"unsubscriber_id": "%s,%d" % (one._name, one.id),
"action": action,
})
if model._name == 'mail.mass_mailing.contact':
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

action = "unsubscription" if value else "subscription"
records = self.env[self.mailing_model_real].browse(res_ids)
records = self.env[model._name].browse(res_ids)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

model.browse?

@@ -12,13 +12,18 @@ class MailMassMailing(models.Model):
def update_opt_out(self, email, res_ids, value):
"""Save unsubscription reason when opting out from mailing."""
self.ensure_one()
model = self.env[self.mailing_model_real].with_context(
active_test=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 why the context?

@@ -39,7 +39,7 @@ odoo.define("mass_mailing_custom_unsubscribe.partner_tour",
},
{
content: "Successfully unsubscribed",
trigger: "body:not(:has(#reason_form)) .alert-success:contains('Your changes have been saved.')",
trigger: "body:not(:has(#reason_form)) .alert-success:contains('You have been successfully unsubscribed!')",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you are subscribing? Shouldn't this be kept?

<li class="list-group-item">
<input type="checkbox" class="mail_list_checkbox" name="list_ids"
t-att-value="contact_list['id']"
t-att-checked="'checked' if contact['opt_out'] == False else None"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is instead of ==

contact.list_id <= mailing.contact_list_ids
not any(contact.list_ids.mapped(
'not_cross_unsubscriptable')) or
contact.list_ids <= mailing.contact_list_ids
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that, given upstream basically broke this in v11, you can just add a new qcontext key like this:

"lists": contact.list_ids | request.env["mail.mass_mailing.list"].search([["not_cross_unsubscriptable", "=", False]]),

Then iterate over those lists in the view and render 1 checkbox for each of them if contact is subscribed to any of them.

Then, when user checks some and unchecks others, record 1 mail.mass_mailing.unsubscription per contact+list combination.

antespi and others added 12 commits June 11, 2018 16:46
…ption (OCA#58)

* [8.0][IMP][mass_mailing_custom_unsubscribe] Get reasons for unsubscription.
- Imported last updates from v8.
- Adapted to v9.
- Added a saner default to `mass_mailing.salt` configuration parameter by
  reusing `database.secret` if available, hoping that some day
  odoo/odoo#12040 gets merged.
- Updated README.
- Increase security, drop backwards compatibility.
  Security got improved upstream, which would again break compatibility among current addon and future master upstream.
  I choose to break it now and keep it secured future-wise, so I drop the backwards compatibility features.
- Includes tour tests.
- Removes outdated tests.
- Extends the mailing list management form when unsubscriber is a contact.
- Adds a reason form even if he is not.
- Avoids all methods that were not model-agnostic.

[FIX][mass_mailing_custom_unsubscribe] Reasons noupdate

After this fix, when you update the addon, you will not lose your customized reasons.

[FIX] Compatibilize with mass_mailing_partner

Current test code was based on the assumption that the `@api.model` decorator on `create()` ensured an empty recordset when running the method, but that's not true. This was causing an incompatibility betwee these tests and the `mass_mailing_partner` addon, which works assuming 0-1 recordsets.

Now records are created from an empty recordset, and thus tests work everywhere.

Update instructions

If the user does not add the unsubscribe snippet, nothing will happen, so it's added to README to avoid confusion when testing/using the addon.

[FIX] Use the right operator to preserve recordsets order

Using `|=` sorts records at will each time (treating them as Python's `set`).
Using `+=` always appends a record to the end of the set.
Since we are using the record position in the set, this caused the test to work sometimes and fail other times. Now it works always.
* [IMP] mass_mailing_custom_unsubscribe: GDPR compliance

- Record resubscriptions too.
- Record action metadata.
- Make ESLint happy.
- Quick color-based action distinction in tree view.
- Add useful quick groupings.
- Display (un)subscription metadata.
- Pivot & graph views.
@chienandalu chienandalu force-pushed the 11.0-mig-mass_mailing_custom_unsubscribe branch from 578c624 to 6e64405 Compare June 11, 2018 16:44
@pedrobaeza
Copy link
Member

Travis is still red

@chienandalu chienandalu force-pushed the 11.0-mig-mass_mailing_custom_unsubscribe branch from f5a749a to 358b0e4 Compare June 12, 2018 12:39
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

# split in several contacts
vals = contact._convert_to_write(contact._cache)
vals.pop('list_ids', None)
for list in contact.list_ids:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should use upper level methods for this, and don't override builtin list:

for contact in contacts:
    if len(contact.list_ids) <= 1:
        continue
    list_1 = contact.list_ids[0]
    for list_ in contact.list_ids - list_1:
        contact.copy({"list_ids": [(6, 0, list_.ids)]})
    contact.list_ids = list_1

@@ -0,0 +1,11 @@
* As version 11 has introduced a new relation type between mailing lists and
contacts that has multiple usability issues that are being reworked by Odoo
to land in version 12, this module fallsback to the version 10 behaviour in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

falls back

* As version 11 has introduced a new relation type between mailing lists and
contacts that has multiple usability issues that are being reworked by Odoo
to land in version 12, this module fallsback to the version 10 behaviour in
wich one contact belonged to just one list.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which

@rafaelbn
Copy link
Member

all is 💚 here! Great! @yajo last technical review? I will test it functionally

@yajo
Copy link
Member

yajo commented Jun 12, 2018

It's just above 😉

@chienandalu chienandalu force-pushed the 11.0-mig-mass_mailing_custom_unsubscribe branch from 358b0e4 to 41c5f6d Compare June 12, 2018 15:41
@chienandalu
Copy link
Member Author

@yajo Fixed

@rafaelbn
Copy link
Member

@chienandalu 🤣 fixed? It was 🍏 now is 🔴

@chienandalu
Copy link
Member Author

@rafaelbn 💚 as 🍏

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@pedrobaeza pedrobaeza merged commit 9815862 into OCA:11.0 Jun 13, 2018
@pedrobaeza pedrobaeza deleted the 11.0-mig-mass_mailing_custom_unsubscribe branch June 13, 2018 08:23
@pedrobaeza pedrobaeza mentioned this pull request Jun 13, 2018
26 tasks
SiesslPhillip pushed a commit to grueneerde/OCA-social that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/social (12.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants