Implements possibility to unsubscribe from email reports #13214

sgiehl · 2018-07-26T14:57:26Z

This is a POC for report unsubscriptions. Maybe someone could have a look if the implementation is fine that way.

Would add tests and some more doc blocks and so on after a first review...

mattab · 2018-07-26T16:46:03Z

Thanks for the PR! Feedback after a quick review:

overall looks good to me, maybe @diosmosis or @tsteur have further thoughts?
maybe we need 2 tokens in the unsubscribe URL to be secure, otherwise one could write a script looping over all possible token values and unsubscribe everyone (after a very long time)? for example a hash of the idreport could maybe do..
instead of updating the subscriptions when sending emails, could we have the migration script do it once, and then have the other updateReport addReport etc. APIs update the list of subscriptions?
left few other comments

sgiehl · 2018-07-26T18:48:28Z

@mattab I'll implement a Nonce in the unsubscription form, that should help avoiding iterating automatically over possible tokens (which have up to 100 chars).

For sure we could also update the subscription when the reports are added/updated. I've Implemented it that way to ensure a subscription is created/available if an email is sent. Otherwise it might happen that for example third party plugins somehow manipulate reports without calling a method that updates the subscriptions.

diosmosis · 2018-07-26T21:54:33Z

Came up w/ one question that probably only @mattab can answer: Is it important for admins to see who unsubscribed?

sgiehl · 2018-07-27T07:33:26Z

@diosmosis Thought about that as well. Will add an event triggered when someone unsubscribes from a report. That way we can at least show it in the Activity Log

mattab · 2018-07-27T11:13:40Z

It may be needed to keep trace of those who unsubscribed. Would it maybe work to have a column in the table "subscribed" or so and set it to 0 when not? This way we could have the data who has unsubscribed, which later we may show in the UI maybe.

But probably only showing this info in the activity log is required for now?

sgiehl · 2018-08-01T15:31:41Z

@mattab I've pushed an update. The table now also includes two columns with timestamps for subscription and unsubscription date. When someone unsubscribes the unsubscription date will be set, and the token will be nulled. If the email is readded in the report this entry will be overwritten with a new one (with unset unsubscribe date). That way we could later show a list of emails that unsubscribed or maybe even block readding unsubscribed emails in report admin.

mattab · 2018-07-26T16:37:41Z

plugins/ScheduledReports/SubscriptionModel.php

+
+    public function unsubscribe($token)
+    {
+        $details = $this->getSubscription($token);


Here need to check that the subscription details are returned?

mattab · 2018-07-26T16:39:19Z

plugins/ScheduledReports/SubscriptionModel.php

+                Request::processRequest('ScheduledReports.updateReport', [
+                    'idReport'     => $report['idreport'],
+                    'idSite'       => $report['idsite'],
+                    'description'  => $report['description'],


Is it needed to pass again all the other values like description, period, hour, etc. except idReport/idSite/parameters?

yes. the parameters are all required for the update method. See https://github.com/matomo-org/matomo/blob/3.x-dev/plugins/ScheduledReports/API.php#L139

mattab · 2018-07-26T16:40:00Z

plugins/ScheduledReports/SubscriptionModel.php

+
+    public function updateReportSubscriptions($idReport, $emails)
+    {
+        $availableSubscriptions = $this->getReportSubscriptions($idReport);


maybe return an error if subscription not found?

no. the method is also used to initially set the subscriptions.

mattab · 2018-07-26T16:42:23Z

plugins/ScheduledReports/templates/unsubscribe.twig

+    </div>
+    <nav>
+        <div class="nav-wrapper">
+            <span id="logo" class="brand-logo center">


is it possible to not duplicate the logo include code?

we could extract it into another template and include it into both. but we also have similar implementations in all layout.twig files (morpheus, updater, ...)

mattab · 2018-08-02T07:57:13Z

Update looks great @sgiehl 👍 Just left a few comments, let me know if these could be applied soon?

diosmosis · 2018-08-02T23:25:49Z

plugins/ScheduledReports/ScheduledReports.php

+
+        foreach ($subscriptions as $subscription) {
+            $tokens[$subscription['email']] = $subscription['token'];
+        }


Think this could be shortened to $tokens = array_column($subscriptions, 'token', 'email');

Not really important, just fyi

diosmosis · 2018-08-02T23:30:14Z

plugins/ScheduledReports/SubscriptionModel.php

+            return reset($reports);
+        });
+
+        if (empty($report)) {


In the case where there's no report for a subscription, should the subscription be removed?

Note: think this is an edge case, so could put this in a new ticket if important

* Implements possibility to unsubscribe from reports * Use a nonce for better security * post event if someone unsubscribes * various improvements and tests * store information about unsubscribed reports until they are resubscribed * code improvements

sgiehl added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Needs Review PRs that need a code review labels Jul 26, 2018

sgiehl added this to the 3.6.0 milestone Jul 26, 2018

sgiehl force-pushed the subscriptions branch from cee08b0 to afe7e90 Compare July 26, 2018 15:00

sgiehl force-pushed the subscriptions branch from cbb015f to 926f8ee Compare July 28, 2018 12:57

sgiehl changed the title ~~[POC] Implements possibility to unsubscribe from reports~~ Implements possibility to unsubscribe from reports Aug 1, 2018

sgiehl removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Aug 1, 2018

mattab requested changes Aug 2, 2018

View reviewed changes

diosmosis reviewed Aug 2, 2018

View reviewed changes

sgiehl added 6 commits August 5, 2018 22:32

Implements possibility to unsubscribe from reports

3099f27

Use a nonce for better security

c7a108e

post event if someone unsubscribes

ef7340d

various improvements and tests

240dc6b

store information about unsubscribed reports until they are resubscribed

0cbf484

code improvements

c3bcdb1

sgiehl force-pushed the subscriptions branch from 12c1820 to c3bcdb1 Compare August 5, 2018 20:34

diosmosis merged commit 70ca09f into 3.x-dev Aug 6, 2018

diosmosis deleted the subscriptions branch August 6, 2018 00:58

mattab added Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. labels Aug 28, 2018

mattab changed the title ~~Implements possibility to unsubscribe from reports~~ Implements possibility to unsubscribe from email reports Aug 28, 2018

mattab mentioned this pull request Sep 10, 2018

Make email report unsubscribe link look consistent with other report footer links #13330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements possibility to unsubscribe from email reports #13214

Implements possibility to unsubscribe from email reports #13214

sgiehl commented Jul 26, 2018 •

edited

mattab commented Jul 26, 2018

sgiehl commented Jul 26, 2018

diosmosis commented Jul 26, 2018

sgiehl commented Jul 27, 2018

mattab commented Jul 27, 2018 •

edited

sgiehl commented Aug 1, 2018

mattab Jul 26, 2018

mattab Jul 26, 2018

sgiehl Aug 2, 2018

mattab Jul 26, 2018

sgiehl Aug 2, 2018

mattab Jul 26, 2018

sgiehl Aug 2, 2018

mattab commented Aug 2, 2018

diosmosis Aug 2, 2018

diosmosis Aug 2, 2018

diosmosis Aug 2, 2018

diosmosis Aug 2, 2018

sgiehl Aug 5, 2018

Implements possibility to unsubscribe from email reports #13214

Implements possibility to unsubscribe from email reports #13214

Conversation

sgiehl commented Jul 26, 2018 • edited

mattab commented Jul 26, 2018

sgiehl commented Jul 26, 2018

diosmosis commented Jul 26, 2018

sgiehl commented Jul 27, 2018

mattab commented Jul 27, 2018 • edited

sgiehl commented Aug 1, 2018

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

mattab commented Aug 2, 2018

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

sgiehl commented Jul 26, 2018 •

edited

mattab commented Jul 27, 2018 •

edited