Emails: real unsubscribe functionality

[DJPaul]

Most subscriptions emails have a unsubscribe link. For 2.5, we set this link to point to the user's notifications page. If someone signs up someone else with a fraudulent email address (for example), however, the recipient may not know the account's details to log in to change the setting.

We should add a special unsubscribe link that will unsubscribe the current type of email (or all emails -- TBC) without requiring the user to authenticate.

[DJPaul]

@tharsheblows started working on this at WordCamp London 2016 contributor day, and made great progress. I expect this'll get done, we have plenty of time for 2.6.

[tharsheblows]

Here's the first go through.

The idea of the unsubscribe link is that it works with one click and nearly no matter what; a user doesn't need to be logged in to unsubscribe using the link. The only time it won't work is if you're logged in as someone else. In that case, it does not automatically unsubscribe but has a link to the user's settings page.

It's also permanent and not dependent on the WP salts which are very occasionally changed.

The emails which have the unsubscribe link are the same ones as on the email settings page with the addition of this one: ​https://buddypress.trac.wordpress.org/ticket/7029 (I think it was simply forgotten on there as those emails do check for that user setting).

[tharsheblows]

adds in unsubscribe tokens to unsubscribable emails

[DJPaul]

I'll pick this up in a couple weeks and progress it forward.

[DJPaul]

Oh, and this is great work, @tharsheblows. :) It just needs another quick review and then the logic being copy/pasted for all the other types of emails. :)

[tharsheblows]

:) Thanks! I did all the emails on the email settings page + this one: ​https://buddypress.trac.wordpress.org/ticket/7029 -- if there are others, let me know.

I was going with the idea that some shouldn't have a link to unsubscribe, eg password re-set emails, and everything which does have a link to unsubscribe should have a way to re-subscribe (I can't be the only one with fat fingers who accidentally unsubscribes to things, right?)

Also I'm not sure on the landing pages (need to not 404 even if logged out) or completely sure on how I handled strings to be translated (I copied this approach: ​https://bbpress.trac.wordpress.org/ticket/2912)

[boonebgorges]

@tharsheblows Thanks so much for your work on this! It's a great feature, and I think the basic workflow is good.

I have a number of comments about implementation details (function names and signatures, boring stuff like that), but before addressing them, I wanted to draw attention to the list of strings provided in bp_emails_unsubscribe() - 'You will no longer...' etc. It seems to me that these are properties of email types, so I wonder if we might change the logic of how these strings are generated somewhat. Briefly:

• The strings are registered alongside the email types in bp_email_schema() or bp_email_get_type_schema(). I don't know if this will mean saving strings to the database or pulling them from somewhere at runtime.
• Instead of using new identifiers for the notification type ("nt"), use the email type handles. Ie 'activity-at-message' instead of 'notification_activity_new_mention'.
• On unsubscription, pull the string from the schema to display on the unsubscribe page.

[tharsheblows]

@boonebgorges That makes sense! I could combine bp_email_schema and bp_email_get_type_schema so it looks something like

<?php
...
  $email_schema['activity-comment'] = array(
    'description' = __( 'A member has replied to an activity update that the recipient posted.', 'buddypress' ), 
    'email_content' = array( 
      'post_title'   => __( '[{{{site.name}}}] {{poster.name}} replied to one of your updates', 'buddypress' ), 
      'post_content' => __ ...
    ),
   'unsubscribe' => array(
      'meta_key' = 'notification_activity_new_reply',
      'message' = __( 'You will no longer receive emails when someone replies to an update or comment you posted.', 'buddypress' ),
      'landing_page' = trailingslashit( bp_get_root_domain() . '/' . bp_get_activity_root_slug() )
   ),
  ...
return apply_filters( 'bp_email_schema', $email_schema );

so people could even add to it and it's all in one place. But I didn't want to do that without discussing it first. What do you think?

[boonebgorges]

Thanks, @tharsheblows ! Conceptually, I think that emails and email types are separate, which I think is why the schemas are separate. It seems to me that this data is a property of the *type* (since it's not user configurable on a per-email basis), but for this I would want to get the feedback of @DJPaul .

[tharsheblows]

Yes, again, that makes sense and for backwards compatibility the bp_email_get_type_schema() has to stay; I had forgotten that. I'll do it in bp_email_get_type_schema() and then change if need be. Thanks!

[tharsheblows]

@boonebgorges is this new one about what you mean?

I have a number of comments about implementation details (function names and signatures, boring stuff like that)

Will you tell me those? Those are the kind of details I'm trying to learn. Thank you! :)

[tharsheblows]

include unsubscribe details in bp_email_get_type_schema

[tharsheblows]

like 6932.2.diff but added back in accidentally deleted link

[boonebgorges]

Replying to tharsheblows:

@boonebgorges is this new one about what you mean?

I have a number of comments about implementation details (function names and signatures, boring stuff like that)

Will you tell me those? Those are the kind of details I'm trying to learn. Thank you! :)

Sure :) I will try to take more time next week to look over the new functionality in the patch, but in the meantime, here are some smaller requests/comments about 6932.3.diff​, in no particular order:

• Could use a sweep to ensure adherence to ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/. Especially: spacing (as between (int) and the variable being cast), indentation levels, if/then bracket style, trailing commas after last array value. See also ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/; in particular, use spaces rather than tabs to align columns in docblocks.
• There are a couple places where I think you can use certain BP functions as shortcuts. For example, bp_get_activity_directory_permalink() lets you skip concatenating the activity URL.
• Always sanitize content coming from $_GET. In most cases, this will mean either urldecode() or intval().
• bp_email_get_unsubscribe_link() should have better documentation for $args. See ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#1-1-parameters-that-are-arrays
• bp_email_get_unsubscribe_link() - Use more verbose $args parameter names. 'user_id' and 'notification_type' are much easier to read. The short versions are probably acceptable when building URLs, which may have length restrictions (and should be obfuscated anyway).
• You're running URLs through esc_url() before sending them to email. Are we doing this elsewhere in BP? I think that @DJPaul and I had a discussion about it before 2.5, but I don't remember what the result was.
• The changes to bp_email_get_type_schema() are technically a compatibility break. I don't know if anyone is using this in the wild, so maybe we don't have to worry about it. To be safe, we may have to introduce a separate function that contains the full data array (as you are currently building it) and then turn bp_email_get_type_schema() into a wrapper that returns the expected (legacy) value format. Feedback from @DJPaul would be helpful here.
• It looks like you removed $link from the signature of bp_email_get_unsubscribe_link(), but then didn't fix the function :) Putting all the paramaters into a single $args array - 'link' (or maybe better 'redirect_to'), 'notification_type', 'user_id' - seems better to me than separating them out into separate arguments.
• Can we come up with a better function name than bp_check()? I know it's kinda doing two different things, but we could still have a better name than bp_check() :)
• bp_get_request_unsubscribe doesn't appear to be hooked to anything. I would suggest that bp_emails_unsubscribe (also not a great function name!) could be hooked to bp_template_redirect, or perhaps bp_actions.

[tharsheblows]

@boonebgorges Oh perfect, thanks! That's super helpful and exactly what I was hoping for. Function naming is not my strong point, but I'll try. ;)

[tharsheblows]

Here is the next version. Except for the things below, the mistakes are typos or complete misunderstandings, so if you could, please point them out explicitly again!

It looks like you removed $link from the signature of bp_email_get_unsubscribe_link(), but then didn't fix the function :) Putting all the paramaters into a single $args array - 'link' (or maybe better 'redirect_to'), 'notification_type', 'user_id' - seems better to me than separating them out into separate arguments.

I've gone back and forth on where this should be. It was in the schema (not explicitly but you could filter it in) but I think that was way too confusing and I hadn't documented it (even when I tried it was still too confusing) so it's back in the individual functions with a default of the activity directory. It's better there anyway because it has access to the user id and notification type. I have been having a long discussion with myself about this.

bp_get_request_unsubscribe doesn't appear to be hooked to anything.

This picks up action=unsubscribe in bp_get_request()

You're running URLs through esc_url() before sending them to email.

Eek, I couldn't find this discussion. I've left it in because I'd rather it fail that way than the other if that makes sense!

I've used ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards but only done the stuff I wrote.

Also, the unsubscribe functionality needs unit tests like bp_activity_at_message_notification_email_unsubscribe, is that right? I was going to do one for each notification type but tell me if that's not the way to do it.

Also also, I was looking at this ​https://buddypress.trac.wordpress.org/ticket/7036 by @r-a-y and this ticket ​https://buddypress.trac.wordpress.org/ticket/7038 and think it might make sense to put all the email details in one schema, ie combine bp_email_type_schema() and bp_email_get_schema(). This current patch does *not* do that.

And finally (!!) if the function names are bad, could you suggest new ones? I am out of ideas!

[tharsheblows]

style updates, back compat and change who handles the redirect

[DJPaul]

#7045 is going to break this patch 😓

[DJPaul]

It seems to me that this data is a property of the *type*

Yes.

You're running URLs through esc_url() before sending them to email.

I forget the conversation, but I think we need to use esc_url_raw() here. We'll catch any issues with this in testing.

bp_email_get_type_schema(), etc

BP functions that return data should use "get" in their names. i.e. your new bp_email_type_schema(). :)
Maybe add optional parameter to existing function and use that to choose what data is returned? e.g. "old data", "new data", "everything". ??

[tharsheblows]

Ok, that sounds good about the esc_url_raw, thanks.

What about bp_email_types_get_schema()? That way would be less confusing for me if I were new to it but that is, of course, not to say it would be less confusing in general. :)

[tharsheblows]

Fixes above. I'm still not quite sure on how to unit test so that's not there (yet)

[tharsheblows]

use esc_url_raw for link, rename schema

[tharsheblows]

Oh! I forgot that I added in #7036 from @r-a-y so it has a default unsubscribe link if there is none in the notification function which calls bp_send_email.

[tharsheblows]

fix styling issues and use correct function names

[DJPaul]

I am in the process of reviewing this locally; 7.diff is the current state. From patch 6, I've tweaked a few variable names (mostly out of personal taste) and switch the esc_url_raw escaping to esc_url (it took me a while to remember why esc_url is preferred, so sorry for mis-leading advice earlier).

I am working on reviewing the changes to core functions and core options files, which is the last piece.

[DJPaul]

This is very close, but I'm moving it to the next release because it needs a couple hours more work, including final testing.
For my own future reference, I've left "djpaultodo" comments in the code indicating the remaining functions that need reviewing and/or tweaking; everything not marked is OK (some indentation errors aside).

@tharsheblows Question for you - in bp_hash_array(), there's a comment saying that order doesn't matter. Why doesn't it matter? I don't understand how a different order won't result in a different generated hash.

[tharsheblows]

@DJPaul I think that comment should be "order *shouldn't* matter". The ksort sorts it so if the keys are out of order in the array, it's still ok. I think this makes sense but am not entirely sure on that.

I've added a couple of unit tests, one for bp_hash_array and one for bp_email_get_salt. There were a couple of typos so I fixed those and changed md5 to sha1. I haven't tested overall functionality though of 6932.8.diff though.

I'd feel better if this had unit tests for each thing it unsubscribed. Is this worth doing or not necessary? I'll do it but thought I'd ask first!

Also #7045 should go be looked at to go in 2.6. The reason I added it to this was to test them together and for ease of merging.

[tharsheblows]

add in unit tests for bp_email_get_salt and bp_hash_array

[DJPaul]

I've tweaked things a bit, mostly just the TODOs I left in the previous patch, shortened some variable names, and so on. I need to test this to check it still works in its entirely, but:

I would like code review of the changes in bp-core-functions.php. Especially the hashing/salt logic.

[djpaul]

In 10941:

Emails: add unsubscribe feature.

Updates the unsubscribe token to link to a new unsubscribe action handler.

All emails from other platforms or popular websites have a unsubscribe link. For previous versions of BuddyPress, our unsubscribe link pointed to that user's notifications page. However, if someone creates an account on a BuddyPress with a fraudulent email address (for example), that email address' owner will not know the account's authentication details, and so have no way to unsubscribe from that "spam" email.

The change implements a new action handler which accepts unsubscription requests from un-authenticated requests. It adds an new option containing a dynamically generated salt which is used to generate the hash on the unsubscribe link.

Fixes #6932

Props tharsheblows, DJPaul

[djpaul]

In 10950:

Core: fix infinite loop in bp_core_clear_root_options_cache

In r10941, an email unsubscribe feature was introduced. This added a new site option that stores the verification salt. For new installations, we added the variable to bp_add_options, and for upgrades, in a new function. We generate the value by using a randomly-generated string, which uses wp_rand internally.

On WordPress older than 4.4, wp_rand's implementation was causing infinite loops in bp_core_clear_root_options_cache because that adds its own site option, and BuddyPress has some cache clearing logic in bp_core_clear_root_options_cache which is called whenever options are added or updated, which in turns re-calls bp_get_default_options.

We now un-hook/re-hook bp_core_clear_root_options_cache inside that function to prevent loops.

WordPress 4.4+ re-implemented wp_rand and doesn't cause a loop.

See #6932