Add VIP Support Email Aliases #73
Conversation
Masks `vip-support+<username>@automattic.com` addresses to appear as `vip-support@automattic.com` instead, primarily for the Users lists and for Gravatar. Added to help clearly brand VIP staff who don't use a8c email addresses.
There was a problem hiding this comment.
Thanks for working through this @davidsword. Left a few pieces of feedback. Will do some testing and follow-up with any other notes in another review.
tests/test-user.php
Outdated
| $this->assertEquals( 'vip-support@automattic.com', User::VIP_SUPPORT_EMAIL_ADDRESS ); | ||
| } | ||
|
|
||
| function test_is_vip_support_email_alias() { |
There was a problem hiding this comment.
This should ideally be split up into two separate tests: one for valid emails and one for invalid emails.
Both should also use dataProviders: https://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.data-providers
If we do keep it as a single function, each assert should include a failure message to add details around what actually failed (otherwise phpunit reports "true is not false", which isn't very useful).
There was a problem hiding this comment.
Great points, thank you. Split up tests in 56331f9
makes it more clear when a tests fails wether it was the valid emails or invalid emails. Cleaned up structure by using dataProvider.
On fail, this test does not specify the issue, only that it failed. It lacked context and am actionable message. Clear description of the problem and instructions are now provided on fail.
breaking apart email to create a dynamic regex pattern was convoluted and not very readable. moved regex pattern to a constant
If the constant ever changes, in/valid email examples need to change as well. This test attempts to make that clear.
There was a problem hiding this comment.
@davidsword just noticed two minor things and we can merge 🙌
| * @return string | ||
| */ | ||
| public function filter_vip_support_email_aliases( $email ) { | ||
| if ( is_admin() && $this->is_vip_support_email_alias( $email ) ) { |
There was a problem hiding this comment.
Noting for myself: in a separate PR we should extend this to include the machine user email address as well.
`.*` is a bit too loose and may cause complications. switching to something a little less vague Co-Authored-By: davidsword <david@davidsword.ca>
slight alteration to regex pattern to match adjustments made to pattern in class-vip-support-user.php
…vidsword/vip-support into add/vip-support-email-aliases
Running md5() on the email is unessisary as the email and Gravatar are unlikly to ever change
No longer needed since we hard-code the Gravatar URL.
| } | ||
|
|
||
| // Get the user's email address. | ||
| if ( is_numeric( $id_or_email ) ) { |
There was a problem hiding this comment.
This conditional block could be abstracted into it's own private method to make what's happening here more obvious.
$user_email = $this->get_user_email_from_id_or_email( $id_or_email );private function get_user_email_from_id_or_email( $id_or_email ) {
if ( is_string( $id_or_email ) ) {
return $id_or_email;
}
if ( $id_or_email instanceof WP_User ) {
return $id_or_email->user_email;
}
if ( is_int( $id_or_email ) ) {
$user = get_user_by( 'id', $id_or_email );
return $user->user_email;
}
return '';
}While the method could return null, I'd rather see it keep the returned string types consistent - the isset( $user_email ) check below could be changed to ! empty( $user_email ).
is_numeric() changed to is_int() to avoid dealing with potential values like 0.01 and 10e3.
props @GaryJones
|
Noting that this doesn't work in Network Admin since the raw |
Adds support for VIP Support email aliases.
Currently a VIP Support user with an email address of
vip-support+<username>@automattic.comwould appear like this:This PR masks these addresses to appear as
vip-support@automattic.comwhen outputting, and also masks the Gravatar URL tovip-support@automattic.com.Added to help clearly brand VIP staff who don't have an a8c email addresses and will receive these support email aliases.
Note that
vip-support@automattic.comdoes not currently have a Gravatar setup, so mystery man will appear until then (but you can double check themd5()hash forvip-support@automattic.comisc83fd21f1122c4d1d8677d6a7a1291d3).