#6932 closed enhancement (fixed)
Emails: real unsubscribe functionality
Reported by: | DJPaul | Owned by: | tharsheblows |
---|---|---|---|
Milestone: | 2.7 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Emails | Keywords: | has-patch |
Cc: | jjjay@… |
Description
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.
Attachments (10)
Change History (43)
#2
@
9 years ago
- Milestone changed from Future Release to 2.6
- Owner set to tharsheblows
- Status changed from new to assigned
#3
@
9 years ago
- Keywords has-patch needs-unit-tests added
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).
#5
@
9 years ago
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. :)
#6
@
9 years ago
:) 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)
This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.
9 years ago
#8
@
9 years ago
@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()
orbp_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.
#9
@
9 years ago
@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?
#10
@
9 years ago
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 .
#11
@
9 years ago
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!
This ticket was mentioned in Slack in #buddypress by tharsheblows. View the logs.
9 years ago
#13
follow-up:
↓ 14
@
9 years ago
@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! :)
#14
in reply to:
↑ 13
@
9 years ago
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 eitherurldecode()
orintval()
. 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-arraysbp_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 turnbp_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 ofbp_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 thanbp_check()
:) bp_get_request_unsubscribe
doesn't appear to be hooked to anything. I would suggest thatbp_emails_unsubscribe
(also not a great function name!) could be hooked tobp_template_redirect
, or perhapsbp_actions
.
#15
@
9 years ago
@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. ;)
#16
@
9 years ago
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!
#18
@
9 years ago
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". ??
#19
@
9 years ago
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. :)
#20
@
9 years ago
Fixes above. I'm still not quite sure on how to unit test so that's not there (yet)
#21
@
9 years ago
Oh! I forgot that I added in #7045 from @r-a-y so it has a default unsubscribe link if there is none in the notification function which calls bp_send_email.
(Edited to give the correct ticket number)
This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.
9 years ago
This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.
9 years ago
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
9 years ago
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
9 years ago
#26
@
9 years ago
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.
This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.
9 years ago
#28
@
9 years ago
- Milestone changed from 2.6 to 2.7
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.
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
9 years ago
#30
@
9 years ago
@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.
#31
@
8 years ago
- Keywords needs-unit-tests removed
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.
@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.