Do not show user input flow after having already completed it

[felixarntz]

Via #2316, when a user starts the Site Kit setup flow, the user input state flag is now set to required whenever it is not already set to completed. This needs to be refined as it results in unexpected behavior when the user disconnects themselves or resets the plugin because:

• The user input state flag gets wiped upon both disconnect and reset.
• Afterwards the user is no longer connected so it is impossible to request accurate user input data from the authentication service, which currently results in the user input state flag simply being set as required.
  
  There is no case where somebody would access the setup screen unless they're disconnected. So the current logic basically is pointless, as it will always result in the user input state flag set to required.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• The check for the user input state flag from before starting the setup flow should be removed (i.e. it shouldn't be set to required in any situation). Also the mode=user_input query parameter should never be added to the proxy setup URL.
• Instead, as soon as the user is authenticated, if the userInput feature flag is enabled, the plugin should check to refresh the user input state flag (which would basically happen right when getting redirected back from the proxy). If that check results in anything other than completed (or required), the user input state flag should be updated to say required. This will then result in the redirect to the user input flow, similar to today.
• In order for the UI on the proxy to still be displayed correctly if completing the user input flow is required, the OAuth_Client::get_proxy_setup_supports() method should be expanded to include a feature "user_input_flow". The proxy will then determine that on its own.

Implementation Brief

• Remove Authentication::require_user_input call from within the Google_Proxy::ACTION_SETUP callback in the same class’ Authentication::register
• Add Authentication::require_user_input as a callback on the googlesitekit_authorize_user action (using method proxy) in Authentication::register
• Update Authentication::require_user_input:
  • Call User_Input_Settings::set_settings( null ) to refresh the user input state from the proxy before reading the user_input_state (but only if the userInput flag is enabled)
  • Remove filter on googlesitekit_proxy_setup_url_params

    site-kit-wp/includes/Core/Authentication/Authentication.php

    Lines 1109 to 1115 in 8b83400

    	// Set the `mode` query parameter in the proxy setup URL.
    	add_filter(
    	'googlesitekit_proxy_setup_url_params',
    	function ( $params ) {
    	return array_merge( $params, array( 'mode' => 'user_input' ) );
    	}
    	);

• Update the OAuth_Client::get_proxy_setup_supports to include a new user_input_flow unconditionally

Test Coverage

• Add tests to AuthenticationTest to cover the new behavior hooked onto googlesitekit_authorize_user

Visual Regression Changes

• N/A

QA Brief

• Use a development build to enable the user input feature
• Setup a new site
• Upon returning to the site you should be prompted for user input – complete the answers
• Once completed, disconnect your user from the user menu in the Site Kit header
• Reconnect on the following page
• Upon returning to the site you should not be prompted for user input
• Visit the Site Kit settings page – on the Admin Settings tab you should see your answers provided previously

Changelog entry

• Only require going through the user input flow after setup if it has not been completed by the user before.

[aaemnnosttv]

@felixarntz IB is ready for review, but I just wanted to point out that it seems there is a contradiction in the ACs:

The check for the user input state flag from before starting the setup flow should be removed (i.e. it shouldn't be set to required in any situation).

This seems to be contradicted by:

If that check results in missing, the user input state flag should be updated to say required.

Would you please clarify?

Also, it seemed that adding a user-specific flag (user setting) didn't seem to be necessary for this purpose, but perhaps I'm misunderstanding what's needed.

[felixarntz]

@aaemnnosttv I don't think there's a contradiction in the ACs, maybe they were slightly unclear: We should basically move the entire check from before starting setup to after completing setup. We can almost leave Authentication::require_user_input as is, but it should be called on googlesitekit_authorize_user. If the value is not completed at this point, the user will have to go through the flow, so we need to set it to required. (If it was already required for some reason, we technically wouldn't need to update the option of course, but that doesn't really matter.)

Also, it seemed that adding a user-specific flag (user setting) didn't seem to be necessary for this purpose, but perhaps I'm misunderstanding what's needed.

Yes this is indeed not necessary, I've updated the ACs.

Regarding your IB:

Remove Authentication::require_user_input method and reference in hook in the same class' register method

Instead of removing require_user_input and adding a new method, we could make the existing method the actual hook for googlesitekit_authorize_user. We would remove it from the 'admin_action_' . Google_Proxy::ACTION_SETUP callback. We'd need to add to it the tiny change to enforce the check (i.e. make the request even if there is a transient), and we'd need to remove the add_filter call to modify the URL.

Update the OAuth_Client::get_proxy_setup_supports to include a new user_input_flow item if userInput is enabled

This is correct according to what my original ACs said, however, given what we realized the other week in our team call, this proxy-specific feature flag should not be based on the remote-controlled feature flag. We can only determine the latter during the setup flow, so anything sent before may not be accurate. Let's use this user_input_flow flag to only indicate that the current Site Kit version "in theory" is capable of rendering the user input flow. Whether the user input flow is actually going to be triggered, will be decided by the proxy, based on whether the Site Kit version supports the flag. By the time googlesitekit_authorize_user is invoked, Site Kit will already have received the remotely enabled features (via #2533), so the logic in Authentication::require_user_input doesn't need any extra tweaks and can simply check for the userInput feature flag. TLDR, let's add the user_input_flow entry in the proxy supported features unconditionally.

[aaemnnosttv]

@felixarntz thanks for clarifying, although your last comment seems to conflict with the last point in the current ACs which says that the entry should only be added to the supported features if the feature flag is set. I'll go with what you said and add it unconditionally, but please confirm and update the ACs if necessary.

IB updated and ready for another pass 👍

[felixarntz]

@aaemnnosttv Good catch, I've updated this in the ACs too.

IB almost ✅, but it's missing a note on removing the bit about the user_input query parameter, which would probably be good to have for completeness.

[aaemnnosttv]

it's missing a note on removing the bit about the user_input query parameter, which would probably be good to have for completeness.

@felixarntz ah you're right – updated 👍

I assume this is good to go now since you moved it forward to the backlog?

[felixarntz]

Yes, all good - was expecting that update to be trivial :)

[wpdarren]

QA Update: Engineer confirmation ⚠️

@aaemnnosttv this is a pass for a brand new site, based on the QAB. I decided to do a little more testing on a site that previously had Site Kit installed, but was disconnected and reset so all settings should be cleared. When reconnecting, I was expecting the user input questions to appear, but they didn't. I looked in the admin settings and remembered I completed the questions previously (a week or so ago).

So, when you reset Site Kit, the user input questions are not being removed. Should they be?

Probably not, but would like to check.

Verified:

• For a new site, user input questions appear, and are completed. The site is then disconnected. When reconnected, the user input screens does not appear. When I look in the admin settings tab, the answers provided are displayed.

[felixarntz]

@wpdarren This is intended behavior. The user input questionnaire answers are stored on the Site Kit service outside of the plugin, so that they are truly persistent even beyond reset/disconnects. In other words, you only respond once per WordPress site per user, ever.

The idea is, when you reset your site, you often do it to try fixing an issue or so. It would be tedious to be always asked for the responses again in such a case.

[wpdarren]

@felixarntz thank you, I did wonder if that was the case but did not want to make any assumptions.

QA Update: Pass ✅

Verified: For a new site, user input questions appear, and are completed. The site is then disconnected. When reconnected, the user input screens does not appear. When I look in the admin settings tab, the answers provided are displayed.