-
Notifications
You must be signed in to change notification settings - Fork 801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
User invitation: Mail the newly added user with WPCom invite #35234
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty clean! We just have to handle errors very gracefully.
Client::wpcom_json_api_request_as_user( | ||
$url, | ||
'2', // Api version | ||
array( | ||
'method' => 'POST', | ||
), | ||
array( | ||
'invitees' => array( | ||
array( | ||
'email_or_username' => $email, | ||
'role' => $role, | ||
), | ||
), | ||
) | ||
); | ||
// returning false prevents the user to be notified by the core email. | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should investigate some error handling. We may want to block adding a user unless the invite works because otherwise the user won't be able to log in any way.
@crisbusquets, visually the only change here is the banner before the button. We need to inform the site owner that, when creating the user, an invitation to join WPcom will be sent. Do you have any idea regarding copy or design? 😄 |
The alert is fine: it's blue = informative. Is it possible to place the alert at the top (above the first form input)? Can we make it the same width as the content? If we can't place the alert at the top, Do we have a Support Page where we can explain why they'll receive an invite? |
Addressed @crisbusquets 🙇 |
Not exactly, but we have a general article here. Maybe we can make the message more encouraging like "This user will receive an invitation to join WordPress.com, and they will be able to log in using Secure Sign On". |
Got it! What about this? To reinforce the security part: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks terrific now. Besides the notice width (should be limited to 550px), this looks great to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very close. Left a few comments.
wp_enqueue_style( 'jetpack-sso-admin-create-user', plugins_url( 'modules/sso/jetpack-sso-admin-create-user.css', JETPACK__PLUGIN_FILE ), array(), time() ); | ||
|
||
wp_admin_notice( | ||
__( 'New users will receive an invite to join WordPress.com, so they can log in securely using [Secure Sign On].', 'jetpack' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secure Sign On should be a link to SSO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I misunderstood that, added the link 👍
@@ -55,6 +55,12 @@ private function __construct() { | |||
add_action( 'jetpack_site_before_disconnected', array( static::class, 'disconnect' ) ); | |||
add_action( 'wp_login', array( 'Jetpack_SSO', 'clear_cookies_after_login' ) ); | |||
|
|||
// If the user has no errors on creation, send an invite to WordPress.com. | |||
add_filter( 'user_profile_update_errors', array( $this, 'send_wpcom_mail_user_invite' ), 10, 3 ); | |||
// Don't send core invitation email when adding a new user via the admin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Don't send core invitation email when adding a new user via the admin. | |
// Don't send core invitation email when SSO is activated. They will get an email from WP.com. |
/* Select the second-to-last tr in the table */ | ||
#createuser .form-table tr:nth-last-child(2) { | ||
display: none; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do this and change the code comment, currently the comment just compiles the code to the reader, it should explain the reasoning.
/* Select the second-to-last tr in the table */ | |
#createuser .form-table tr:nth-last-child(2) { | |
display: none; | |
} | |
/* Hide the checkbox to send WP core invitation emails when SSO is on */ | |
#createuser .form-table tr:has( #send_user_notification ) { | |
display: none; | |
} |
$message = sprintf( | ||
'%s<a class="%s" rel="noopener noreferrer" target="_blank" href="%s">%s</a>', | ||
__( 'New users will receive an invite to join WordPress.com, so they can log in securely using', 'jetpack' ), | ||
'jetpack-sso-admin-create-user-invite-message-link-sso', | ||
esc_url( 'https://jetpack.com/support/sso/' ), | ||
__( 'Secure Sign On.', 'jetpack' ) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working great, a few notes:
- Let's use %1$s, %2$s, %3$s instead, to have more clarity.
- After that let's add the
* translators: %1$s is the message, %2$2 is ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great, what an improvement! LGTM
__( 'New users will receive an invite to join WordPress.com, so they can log in securely using %s', 'jetpack' ), | ||
sprintf( | ||
'<a class="jetpack-sso-admin-create-user-invite-message-link-sso" rel="noopener noreferrer" target="_blank" href="%s">%s</a>', | ||
'https://jetpack.com/support/sso/', | ||
__( 'Secure Sign On.', 'jetpack' ) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, this is still incorrect.
And there is no variables here at all, you can remove sprintf and put everything inside one call of __().
Fixes Automattic/wp-calypso#86737 and Automattic/wp-calypso#86738
This is part of the pdDR7T-1ja-p2
Proposed changes:
Currently, when adding a new user to a site via wp-admin, it only creates the user in the site context (locally) therefore these users a not mapped as WPCom users.
This PR:
Testing instructions:
jetpack rsync
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?