@sgiehl opened this Pull Request on July 26th 2018 Member

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...

fixes #2197

@mattab commented on July 26th 2018 Member

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 commented on July 26th 2018 Member

@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 commented on July 26th 2018 Member

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

@sgiehl commented on July 27th 2018 Member

@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 commented on July 27th 2018 Member

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 commented on August 1st 2018 Member

@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 commented on August 2nd 2018 Member

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

This Pull Request was closed on August 6th 2018
Powered by GitHub Issue Mirror