Address feedback from code review.

Co-authored-by: Matthew Riley MacPherson <hi@tofumatt.com>

Author: shervElmi

includes/Modules/Sign_In_With_Google.php

@@ -679,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 ) {
@@ -799,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;
 		}
@@ -859,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' )
 		);
 	}
 
@@ -980,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.
 	 */
@@ -994,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/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
 	 *
@@ -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 ),
@@ -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 );
 	}
 }

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

@@ -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_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() {