Address feedback from code review.

Author: shervElmi

includes/Modules/Sign_In_With_Google.php

@@ -146,7 +146,7 @@ public function __construct(
 	 *
 	 * @since 1.137.0
 	 * @since 1.141.0 Add functionality to allow users to disconnect their own account and admins to disconnect any user.
-	 * @since n.e.x.t Allow signed-in users to connect their existing WordPress account from the profile page.
+	 * @since n.e.x.t Allow signed-in users to connect their existing WordPress account from their own profile page.
 	 */
 	public function register() {
 		$this->register_inline_data();
@@ -174,13 +174,21 @@ function () {
 
 		add_action( 'admin_action_' . self::ACTION_DISCONNECT, array( $this, 'handle_disconnect_user' ) );
 
-		// Render the Sign in with Google profile section. The user's own profile
-		// shows connect or disconnect, other editable profiles show disconnect only.
+		// Render the Sign in with Google profile section.
+		// If this profile belongs to the current user (eg. they're editing
+		// their own profile): show the "connect a Google account" if
+		// they don't have a Google Account associated with their profile,
+		// or the "Disconnect your Google account" option if they do have
+		// a Google Account associated with their profile
+		//
+		// If this profile belongs to another user (eg. we're an admin editing
+		// another user's profile), only the "Disconnect Google Account"
+		// option is shown.
 		add_action( 'show_user_profile', $this->get_method_proxy( 'render_sign_in_with_google_profile' ) );
 		add_action( 'edit_user_profile', $this->get_method_proxy( 'render_sign_in_with_google_profile' ) );
 
 		// Output the Sign in with Google init script so the placeholder above
-		// becomes a real button on the user's own unlinked profile.
+		// becomes a real button on the user's own, unlinked profile.
 		add_action( 'admin_footer', $this->get_method_proxy( 'maybe_render_profile_signinwithgoogle' ) );
 
 		// Surface the already-connected error from the profile-page connect flow.
@@ -235,7 +243,14 @@ function ( $paths ) {
 	}
 
 	/**
-	 * Resolves the authenticator class to instantiate for an auth callback.
+	 * Get the authenticator class to use for the integration specified.
+	 *
+	 * Returns the name of the class to load for this Sign in with Google
+	 * authentication flow (eg. for WooCommerce, connecting an existing
+	 * user, etc.)
+	 *
+	 * Returns the base authenticator class's class name if no integration
+	 * matches the one supplied.
 	 *
 	 * @since n.e.x.t
 	 *
@@ -664,7 +679,7 @@ public function get_authenticated_users_count() {
 	 *
 	 * @since n.e.x.t
 	 *
-	 * @param int $user_id User ID to scope the accessor to.
+	 * @param int $user_id User ID to use to build the hashed ID.
 	 * @return Hashed_User_ID
 	 */
 	private function get_hashed_user_id( $user_id ) {
@@ -784,16 +799,16 @@ private function build_registrable_tag(): ?Web_Tag {
 
 	/**
 	 * Outputs the Sign in with Google init script that turns the placeholder
-	 * from `render_sign_in_with_google_profile()` into a real button.
+	 * `<div>` from `render_sign_in_with_google_profile()` into a real button.
 	 *
 	 * Hooks `admin_footer` so `did_action( 'show_user_profile' )` reliably
 	 * reports whether the section actually rendered.
 	 *
 	 * @since n.e.x.t
 	 */
 	private function maybe_render_profile_signinwithgoogle() {
-		// `show_user_profile` only fires on the user's own profile, so this runs
-		// only there. The connect button never renders for other users.
+		// `show_user_profile` only fires on the user's own profile.
+		// The connect button should never render for other users.
 		if ( ! did_action( 'show_user_profile' ) ) {
 			return;
 		}
@@ -844,7 +859,7 @@ private function render_profile_admin_notices() {
 
 		printf(
 			'<div class="notice notice-error is-dismissible"><p>%s</p></div>',
-			esc_html__( 'A profile is already connected to this Google account.', 'google-site-kit' )
+			esc_html__( 'Another user on this site already connected this Google account to their profile.', 'google-site-kit' )
 		);
 	}
 
@@ -965,7 +980,7 @@ public function handle_disconnect_user() {
 	 *
 	 * @since 1.141.0
 	 * @since n.e.x.t Renamed from `render_disconnect_profile` and added the
-	 *                connect-existing-profile branch.
+	 *                the ability to connect an existing profile.
 	 *
 	 * @param WP_User $user WordPress user object.
 	 */
@@ -979,8 +994,9 @@ private function render_sign_in_with_google_profile( WP_User $user ) {
 		$is_own_profile         = get_current_user_id() === $user->ID;
 		$is_unlinked            = empty( $current_user_google_id );
 
-		// Skip when there's no linked account and the connect button can't show
-		// anyway (other user's profile, or module not connected).
+		// Skip when there's no linked account and the connect button shouldn't
+		// appear (eg. when on another user's profile or Sign in with Google is
+		// not connected).
 		if ( $is_unlinked && ( ! $is_own_profile || ! $this->is_connected() ) ) {
 			return;
 		}

includes/Modules/Sign_In_With_Google/Authenticator.php

@@ -48,7 +48,7 @@ class Authenticator implements Authenticator_Interface {
 	 *
 	 * @since n.e.x.t
 	 */
-	const CONNECT_NONCE_ACTION = 'googlesitekit_connect_existing_profile';
+	const CONNECT_EXISTING_PROFILE_NONCE_ACTION = 'googlesitekit_connect_existing_profile';
 
 	/**
 	 * User options instance.

includes/Modules/Sign_In_With_Google/Profile_Authenticator.php

@@ -49,7 +49,7 @@ class Profile_Authenticator extends Authenticator {
 	 *   of the Google account's email address.
 	 * - A new user is never created. Open registration is irrelevant.
 	 * - If the Google account is already linked to another WordPress user,
-	 *   the flow errors out without mutating any user meta.
+	 *   an error is triggered.
 	 *
 	 * @since n.e.x.t
 	 *
@@ -65,7 +65,7 @@ public function authenticate_user( Input $input ): string {
 		// Tie the link request to the current WordPress session so a
 		// cross-site request can't piggyback on the user's auth cookie.
 		$nonce = $input->filter( INPUT_POST, 'connect_nonce' );
-		if ( ! wp_verify_nonce( $nonce, self::CONNECT_NONCE_ACTION ) ) {
+		if ( ! wp_verify_nonce( $nonce, self::CONNECT_EXISTING_PROFILE_NONCE_ACTION ) ) {
 			return $this->get_error_redirect_url( self::ERROR_INVALID_REQUEST );
 		}
 
@@ -78,9 +78,12 @@ public function authenticate_user( Input $input ): string {
 
 		$g_user_hid = $this->get_hashed_google_user_id( $payload );
 
-		// Block the link if another WordPress user already has this Google
-		// account stored against their profile.
-		$existing_users = get_users(
+		// Check to see if the Google user ID for the Google Account we signed in
+		// with is already in use (eg. registered to another user on the site).
+		//
+		// If this is the case, we'll trigger an error instance of associating
+		// this Google Account with the current user.
+		$existing_user = get_users(
 			array(
 				// phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_key
 				'meta_key'   => $this->user_options->get_meta_key( Hashed_User_ID::OPTION ),
@@ -92,7 +95,7 @@ public function authenticate_user( Input $input ): string {
 			)
 		);
 
-		if ( ! empty( $existing_users ) ) {
+		if ( ! empty( $existing_user ) ) {
 			return $this->get_error_redirect_url( self::ERROR_ACCOUNT_ALREADY_CONNECTED );
 		}
 
@@ -121,9 +124,9 @@ protected function get_error_redirect_url( $code ): string {
 		$user_id = get_current_user_id();
 		$target  = $user_id ? get_edit_user_link( $user_id ) : admin_url( 'profile.php' );
 
-		// A Site Kit-specific query arg avoids the empty error notice WP core
-		// emits on `wp-admin/profile.php` and `wp-admin/user-edit.php` for any
-		// unrecognized `?error=`.
+		// Do not use `error=` as a query arg here, because
+		// `wp-admin/profile.php` and `wp-admin/user-edit.php` will
+		// show an error for any unrecognized `?error=` value.
 		return add_query_arg( self::ERROR_QUERY_ARG, $code, $target );
 	}
 }

includes/Modules/Sign_In_With_Google/Web_Tag.php

@@ -191,7 +191,7 @@ protected function render() {
 		<?php if ( ! is_preview() ) : // phpcs:ignore Generic.WhiteSpace.ScopeIndent.Incorrect ?>
 		<?php if ( $this->is_connect_existing_profile ) : // phpcs:ignore Generic.WhiteSpace.ScopeIndent.Incorrect ?>
 		response.integration = 'connect_existing_profile';
-		response.connect_nonce = <?php echo wp_json_encode( wp_create_nonce( Authenticator::CONNECT_NONCE_ACTION ) ); ?>;
+		response.connect_nonce = <?php echo wp_json_encode( wp_create_nonce( Authenticator::CONNECT_EXISTING_PROFILE_NONCE_ACTION ) ); ?>;
 		<?php elseif ( $is_woocommerce && ! $this->is_wp_login ) : // phpcs:ignore Generic.WhiteSpace.ScopeIndent.Incorrect ?>
 		response.integration = 'woocommerce';
 		<?php endif; // phpcs:ignore Generic.WhiteSpace.ScopeIndent.Incorrect ?>

tests/phpunit/integration/Core/Util/Current_ScreenTest.php

@@ -25,7 +25,7 @@ public function test_returns_wp_screen_for_profile_screen() {
 		$screen = Current_Screen::get();
 
 		$this->assertInstanceOf( WP_Screen::class, $screen, 'Should return a WP_Screen instance when the profile screen is active.' );
-		$this->assertSame( 'profile', $screen->id, 'Returned screen ID should match the active profile screen.' );
+		$this->assertSame( 'profile', $screen->id, "Returned screen ID should match the current user's profile screen." );
 	}
 
 	public function test_returns_wp_screen_for_user_edit_screen() {
@@ -40,6 +40,6 @@ public function test_returns_wp_screen_for_user_edit_screen() {
 	public function test_returns_null_when_no_current_screen_is_set() {
 		unset( $GLOBALS['current_screen'] );
 
-		$this->assertNull( Current_Screen::get(), 'Should return null when no current screen has been set.' );
+		$this->assertNull( Current_Screen::get(), 'Should return null (eg. should not throw an error) when no current screen has been set.' );
 	}
 }

tests/phpunit/integration/Modules/Sign_In_With_Google/ProfileAuthenticatorTest.php

@@ -50,7 +50,7 @@ public function tear_down() {
 
 	private function do_authenticate_user( $profile_reader_data = array(), $with_valid_nonce = true ) {
 		if ( $with_valid_nonce ) {
-			$_POST['connect_nonce'] = wp_create_nonce( Authenticator::CONNECT_NONCE_ACTION );
+			$_POST['connect_nonce'] = wp_create_nonce( Authenticator::CONNECT_EXISTING_PROFILE_NONCE_ACTION );
 		}
 
 		$user_options        = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) );
@@ -127,7 +127,7 @@ public function test_returns_error_redirect_when_google_account_taken_by_other_u
 		$this->assertEquals( $expected, $actual, 'Should redirect with already-connected error when another user owns the Google account.' );
 		$this->assertEmpty(
 			get_user_option( Hashed_User_ID::OPTION, $current_user_id ),
-			'Current user should not gain a Google link when another user owns the Google account.'
+			'Current user should not be associated with a Google account already linked to another WordPress user on the site.'
 		);
 	}
 

tests/phpunit/integration/Modules/Sign_In_With_Google/Web_TagTest.php

@@ -243,7 +243,7 @@ public function test_render_tag__emits_connect_markers_only_when_flag_on() {
 		$output = $this->capture_render_tag_output();
 		$this->assertStringContainsString( "response.integration='connect_existing_profile'", $output, 'Rendered script should set the connect_existing_profile integration when the flag is on.' );
 		$this->assertStringContainsString( 'response.connect_nonce=', $output, 'Rendered script should include the connect nonce assignment when the flag is on.' );
-		$this->assertStringContainsString( wp_create_nonce( Authenticator::CONNECT_NONCE_ACTION ), $output, 'Rendered nonce should match wp_create_nonce for CONNECT_NONCE_ACTION.' );
+		$this->assertStringContainsString( wp_create_nonce( Authenticator::CONNECT_EXISTING_PROFILE_NONCE_ACTION ), $output, 'Rendered nonce should be a valid wp_create_nonce.' );
 
 		// Toggling the flag off on the same instance guards against a vacuous pass if the marker
 		// strings ever change. The positive phase above fails first when that happens, signalling
@@ -265,7 +265,7 @@ public function test_render_tag__forces_button_render_loop_on_connect_flow_even_
 		// Logged-in users normally skip the button render loop. The connect flow flips that gate
 		// so the placeholder div on the profile page can still be wired up.
 		$this->assertStringContainsString( 'google.accounts.id.renderButton', $output, 'Connect flow should force the button render loop even when the user is logged in.' );
-		$this->assertStringContainsString( 'googlesitekit-sign-in-with-google__frontend-output-button', $output, 'Render loop should target the SiwG placeholder class.' );
+		$this->assertStringContainsString( 'googlesitekit-sign-in-with-google__frontend-output-button', $output, 'Render loop should target the Sign in with Google placeholder class.' );
 	}
 
 	private function capture_render_tag_output() {

tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php

@@ -406,7 +406,7 @@ public function test_render_sign_in_with_google_profile__connect_branch_on_own_p
 		$output = $this->capture_action( 'show_user_profile', wp_get_current_user() );
 		$this->assertStringContainsString( 'Connect your account to sign in with Google.', $output, 'Connect copy should render on own profile when no Google user id stored.' );
 		$this->assertStringContainsString( 'id="googlesitekit-sign-in-with-google-profile-settings"', $output, 'Connect branch should reuse the profile-settings id.' );
-		$this->assertStringContainsString( 'googlesitekit-sign-in-with-google__frontend-output-button', $output, 'Connect branch should render the SiwG button placeholder.' );
+		$this->assertStringContainsString( 'googlesitekit-sign-in-with-google__frontend-output-button', $output, 'Connect branch should render the Sign in with Google button placeholder.' );
 		$this->assertStringContainsString( 'googlesitekit-sign-in-with-google__profile-connect-button', $output, 'Connect branch should tag the placeholder with the profile-connect class.' );
 		$this->assertStringNotContainsString( 'Disconnect Google Account', $output, 'Connect branch should not render the disconnect button.' );
 		$this->assertSame( 1, substr_count( $output, '<h2>' ), 'Connect branch should render only one <h2>.' );
@@ -422,7 +422,7 @@ public function test_render_sign_in_with_google_profile__skips_connect_branch_wh
 		// connected (no `clientID`), the connect section should not render
 		// at all because the button JS would never wire it up.
 		$output = $this->capture_action( 'show_user_profile', wp_get_current_user() );
-		$this->assertEmpty( $output, 'Connect branch should not render when SiwG module is not connected.' );
+		$this->assertEmpty( $output, 'Connect branch should not render when Sign in with Google module is not connected.' );
 	}
 
 	public function test_render_sign_in_with_google_profile__skips_connect_branch_on_other_user_profile() {
@@ -441,17 +441,17 @@ public function test_render_sign_in_with_google_profile__skips_connect_branch_on
 		// branch's other short-circuit can't fire. The only thing keeping
 		// the section out is the admin viewing someone else's profile.
 		$output = $this->capture_action( 'edit_user_profile', get_user_by( 'id', $editor_id ) );
-		$this->assertEmpty( $output, 'Section should not render on another user profile when that user has no linked Google account.' );
+		$this->assertEmpty( $output, "Section should never render on a profile that is not the current user's profile." );
 	}
 
-	public function test_render_profile_admin_notices__shows_error_on_profile_page() {
+	public function test_render_profile_admin_notices__shows_error_on_profile_page_when_profile_has_linked_google_account() {
 		$_GET[ Profile_Authenticator::ERROR_QUERY_ARG ] = Profile_Authenticator::ERROR_ACCOUNT_ALREADY_CONNECTED;
 		set_current_screen( 'profile' );
 
 		$this->module->register();
 
 		$output = $this->capture_action( 'admin_notices' );
-		$this->assertStringContainsString( 'A profile is already connected to this Google account.', $output, 'Admin notice should render the already-connected copy on profile.php.' );
+		$this->assertStringContainsString( 'A profile is already connected to this Google account.', $output, 'Admin notice should render the already-connected error.' );
 
 		unset( $_GET[ Profile_Authenticator::ERROR_QUERY_ARG ] );
 	}
@@ -463,7 +463,7 @@ public function test_render_profile_admin_notices__shows_error_on_user_edit_page
 		$this->module->register();
 
 		$output = $this->capture_action( 'admin_notices' );
-		$this->assertStringContainsString( 'A profile is already connected to this Google account.', $output, 'Admin notice should render the already-connected copy on user-edit.php.' );
+		$this->assertStringContainsString( 'A profile is already connected to this Google account.', $output, 'Admin notice should render the already-connected error.' );
 
 		unset( $_GET[ Profile_Authenticator::ERROR_QUERY_ARG ] );
 	}
@@ -475,7 +475,7 @@ public function test_render_profile_admin_notices__skips_outside_profile_pages()
 		$this->module->register();
 
 		$output = $this->capture_action( 'admin_notices' );
-		$this->assertEmpty( $output, 'Admin notice should not render outside of profile.php or user-edit.php.' );
+		$this->assertEmpty( $output, 'Errors related to a Google Account already being connected should not appear outside of profile.php or user-edit.php.' );
 
 		unset( $_GET[ Profile_Authenticator::ERROR_QUERY_ARG ] );
 	}
@@ -516,7 +516,7 @@ public function test_maybe_render_profile_signinwithgoogle__skips_when_show_user
 
 		$output = $this->capture_action( 'admin_footer' );
 
-		$this->assertStringNotContainsString( "response.integration='connect_existing_profile'", $output, 'admin_footer should not emit the connect script outside the profile page.' );
+		$this->assertStringNotContainsString( "response.integration='connect_existing_profile'", $output, 'admin_footer should not render the connect script outside the profile page.' );
 	}
 
 	public function test_maybe_render_profile_signinwithgoogle__skips_when_user_already_linked() {