Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#6915 closed defect (bug) (fixed)

BP Email template: the unsubscribe link & the account activation email

Reported by: imath's profile imath Owned by:
Milestone: 2.5 Priority: normal
Severity: normal Version:
Component: Core Keywords: dev-feedback has-patch
Cc:

Description

While testing the patch on #6913 i've noticed the "Unsubscribe" link was inserted inside the activation email although the user is not activated yet.

For regular WordPress config : clicking on the unsubscribe link displays the login form, when trying to login it displays an error "please activate your account".

For all other configs (BP_SIGNUPS_SKIP_USER_CREATION && multisite) the unsubscribe link has an empty href attribute.

I wonder if it's possible to avoid displaying this unsubscribe link if the email type is core-user-registration-with-blog or core-user-registration, and if we should actually do it ?

Attachments (2)

6915.01.patch (4.5 KB) - added by r-a-y 8 years ago.
6915.mustache.patch (38.7 KB) - added by r-a-y 8 years ago.

Download all attachments as: .zip

Change History (12)

#1 @DJPaul
8 years ago

unsubscribe will be empty if the recipient can't be found by get_user_by( 'email', ... ). There's no support for using tokens in a conditional.

We could set the default value for this to wp-login.php, so at the very least, people arrive at the log-in form, which is better than a broken link, because they get a message telling them to activate their account first.

Until if/when we have a "real" unsubscribe link that doesn't require you to be logged in, this is probably the easiest and least messy solution I can think of. What do you think?

#2 @imath
8 years ago

wp-login.php

+1 :)

#3 @r-a-y
8 years ago

Attached patch passes the 'To' email info into the email template for checking so we can determine if the "Unsubscribe" link should show in emails.

I had to move the 'to' parameter higher up to the bp_get_email() function and I had to stash the parameter in the buddypress() object in order so we can access this info in the email template. Yeah, I know...

To determine if a user should see the "Unsubscribe" link, I created an email template function called bp_email_is_recipient_to_singular_user() (pardon the crappy name for now!). In this function, I'm using bp_is_user_active(), which I've patched to determine if a user is pending as well.

If we like this technique, we could also move the tokens to bp_get_email() so we could potentially do other checks in the email template.

Last edited 8 years ago by r-a-y (previous) (diff)

@r-a-y
8 years ago

#4 @DJPaul
8 years ago

The idea with bp_get_email() was that it encapsulates the query to fetch a WP_Post and handles that (which is why it sets subject/content/template, because that data comes from the Post). My initial thoughts are that I don't like the idea of moving any other data into that doesn't directly come from that Post. I like the separation of concerns. The recipient details come from another place.

There's no reason why a more custom implementation of our email system could be made by someone who chooses to not use bp_get_email(). Perhaps they are creating their own bp_send_email for use in their own plugin, for some reason, and bypass bp_get_email() entirely (maybe they aren't using Posts to store email content). While this isn't my strongest argument ever, moving any part of recipient information or tokens into bp_get_email() just means more work for someone to copy out of our functions and paste it somewhere else (and be baffled why all they need isn't in one place to copy from).

If the goal of patch idea is to provide conditional logic around token rendering (plus the blocks of markup around them), I'd rather take the time to consider replacing the basic/simple token parsing logic with a real Mustache template parser. For example, see https://mustache.github.io/mustache.5.html under "Sections" ({{#person}} etc).

#5 @r-a-y
8 years ago

  • Keywords has-patch added

I'd rather take the time to consider replacing the basic/simple token parsing logic with a real Mustache template parser.

I think this would be very cool and quite easy to integrate.

Only question is what Mustache parser we would want to use.

  • mustache.php is linked from the official mustache Github page. Could use Composer for this, but library is 176kb.
  • LightnCandy claims to be 2-7 times faster than mustache.php, but requires PHP 5.3 due to namespaces. Library is also 196kb.

For 6915.mustache.patch, I decided to pick a lesser-known library, php-mustache, that is only 40kb and licensed under the BSD. We can pick and choose any library, I just chose one that was relatively small.

To address the issue in this ticket, I've included some unit tests for bp_core_replace_tokens_in_text() and have changed the email template to use a mustache section for the unsubscribe block -- {{#unsubscribe}}. The 'unsubscribe' token is only added if the user is active. See the changes made to bp_email_set_default_tokens() and bp_is_user_active().

Now, the unsubscribe link will not show up in registration and activation emails.

@r-a-y
8 years ago

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


8 years ago

#7 @djpaul
8 years ago

In 10611:

Emails: set default value of unsubscribe token to wp-login.php.

If the email recipient can't be found by get_user_by( 'email', ... ) on multisite configurations or when BP_SIGNUPS_SKIP_USER_CREATION is enabled, the unsubscribe link in the emails is empty.

In a future release, we may introduce logic in the template to not render this token if it is valueless, but for 2.5, we're setting it to the log in form. If a user who hasn't activated their account tries to log in, they will see a message informing them they need to activate their account first, which is good enough.

See #6915

#8 @DJPaul
8 years ago

I've made #6921 to carry on discussing Mustache parsers.

#9 @DJPaul
8 years ago

  • Resolution set to fixed
  • Status changed from new to closed

#10 @DJPaul
8 years ago

  • Component changed from API to Core
Note: See TracTickets for help on using tickets.