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

[MIG][mass_mailing_custom_unsubscribe] Migrate to v10 #178

Merged
merged 6 commits into from
Jul 17, 2017

Conversation

yajo
Copy link
Member

@yajo yajo commented Jul 5, 2017

@yajo yajo requested a review from pedrobaeza July 5, 2017 09:08
@yajo yajo self-assigned this Jul 5, 2017
@yajo yajo added this to the 10.0 milestone Jul 5, 2017
@pedrobaeza pedrobaeza mentioned this pull request Jul 5, 2017
21 tasks
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

On Settings > Technical > Email > Emails the unsubscribe link isn't yet generated in the body preview. Any tip on how to test on runbot without real smtp config?


#. Go to *Marketing > Configuration > Unsubscription Reasons*.
Copy link
Member

Choose a reason for hiding this comment

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

Mass Mailing > Configuration > Unsubscription Reasons

<tree>
<field name="name"/>
<field name="details_required"/>
<field name="sequence" invisible="True"/>
Copy link
Member

Choose a reason for hiding this comment

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

widget="handle" would fit here quite well

<field name="date"/>
<field name="mass_mailing_id"/>
<field name="unsubscriber_id"/>
<field name="email"/>
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it weird that the email isn't get whenever the unsubscriber_id change?

Copy link
Member Author

@yajo yajo Jul 10, 2017

Choose a reason for hiding this comment

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

This is in purpose. The email where the unsusbscription came from must be stored, no matter the target unusbscriber. But in fact, this whole view should be readonly, at least for normal users, so I'll change that.

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been fixed now by adding edition permissions only to admins.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Little nitpickings but overall code review OK


- Choose which mailing lists are not cross-unsubscriptable when unsubscribing
from a different one.
- Know why and when a contact as been unsubscribed from a mass mailing.
Copy link
Member

Choose a reason for hiding this comment

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

s/as/has

<small>[EN] You can unsubscribe <a href="%(url)s">here</a></small><br/>
<small>[ES] Puedes darte de baja <a href="%(url)s">aquí</a></small>

* This module adds a security hash for mass mailing unsubscription URLs, which
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already includes in v10?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's v11: odoo/odoo#12040

Copy link
Member

Choose a reason for hiding this comment

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

Please add them a comment on code and on README for serving as reminder on v11 migration.

'category': 'Marketing',
'version': '8.0.1.0.0',
'version': '10.0.2.0.0',
Copy link
Member

Choose a reason for hiding this comment

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

10.0.1.0.0

'demo/assets.xml',
],
'images': [
'images/form.png',
],
'author': 'Antiun Ingeniería S.L., '
Copy link
Member

Choose a reason for hiding this comment

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

Remove Antiun

@yajo yajo force-pushed the 10.0-mass_mailing_custom_unsubscribe branch from 252cc69 to 4cd7485 Compare July 10, 2017 08:14
@yajo
Copy link
Member Author

yajo commented Jul 10, 2017

All changed, I also did a full rebase to get fixed some commit messages that got stripped when using method prior to https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-10.0/_compare/e6543dbec279c6108b7274b34d8d4f2f08d3f855

@pedrobaeza
Copy link
Member

Please check Travis

yajo and others added 6 commits July 17, 2017 10:44
…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.
@yajo yajo force-pushed the 10.0-mass_mailing_custom_unsubscribe branch from d598781 to 5dc6c87 Compare July 17, 2017 08:44
@yajo
Copy link
Member Author

yajo commented Jul 17, 2017

I think the problem was #181, so I rebased. Let's see...

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Perfect now! :)

@pedrobaeza pedrobaeza merged commit 78b7791 into OCA:10.0 Jul 17, 2017
@pedrobaeza pedrobaeza deleted the 10.0-mass_mailing_custom_unsubscribe branch July 17, 2017 15:30
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.

None yet

4 participants