Site-kit-wp: Implement `googlesitekit_user_input_state` flag to detect and store whether user input is required

[felixarntz]

Depending on whether a user is connecting to Site Kit or whether a user is returning to Site Kit after having already used a previous version of the plugin, the way they should be exposed to the new user input screen should behave differently. This issue is about implementing the necessary logic to determine the user's state regarding user input.

---

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

Acceptance criteria

• There should be a new registered user setting googlesitekit_user_input_state.
• The setting should be able to have one of the following values (note the explanations include certain behavior, to clarify - this behavior should _not_ be implemented as part of this issue, unless separately included further below):
  
  
  
  • _Empty / not set_: This is the initial value. When the user visits the Site Kit dashboard or the Site Kit widget in the WordPress dashboard, a notification should be included.
  
  
  • _“required”_: This value should be set once the user clicks the initial setup button on the splash screen, if the current value is anything except “completed”. When the user then tries to visit the dashboard, they should be redirected to the user input UI, including wording that makes this the last step of the setup flow.
  
  
  • _“completed”_: This value should be set once the user successfully submits their user input data for the first time, or if the response data from Site Kit Service indicates that they had already done so previously (the latter is relevant for cases where Site Kit is reset, since that should not cause users to be prompted again when they had already provided the data). When the user accesses the user input UI in this state, it is essentially in “Edit” state with updated overall wording.
  
  
• On admin_init, run a hook to check whether user input settings were previously completed (if the flag value is not already _“completed”_):
  
  
  
  • Check if the site is connected. Otherwise bail.
  
  
  • Check if the user is authenticated. Otherwise bail.
  
  
  • Check if the proxy is being used. Otherwise bail.
  
  
  • Get the current user input settings from the authentication service via the internal API introduced in #2036. If the response includes at least one value for every one of the five settings, set the new user setting to _“completed”_.
  
  
• When accessing the googlesitekit-dashboard admin screen, check if the new user setting is currently _“required”_. If yes, redirect to the new googlesitekit-user-input screen from #2038.
• There should be a new selector in the JS core/user store getUserInputState(). It should rely on a server-provided value via _googlesitekitUserData.userInputState.

Note that this won't do anything user-facing via the above implementation, which is on purpose because the final pieces can only be implemented once everything is ready to be launched in production. For example, setting the new user setting to "required" or "dismissed" will happen later.

Implementation Brief

Step 1

• Create a new Core\Authentication\User_Input_State class that extends Core\Storage\User_Setting.
  
  
  
  • Define OPTION constant with googlesitekit_user_input_state as its value.
  
  
  • Define the following constants:
  
  
  • VALUE_REQUIRED with required value;
  
  
  • VALUE_COMPLETED with completed value;
  
  
  • VALUE_MISSING with missing value.
  
  
  • Override set method to verify the incoming value is required, completed, missing, or empty. If the value is valid pass it to the parent method. If not, then just return false.
  
  
• Add relevant phpunit tests.

Step 2

In the Authentication class:

• Instantiate a new instance of the User_Input_State class as private property user_input_state;
• Instantiate a new instance of the User_Input_Settings class as private property user_input_settings;
• Register user input state setting by invoking register method in the register method of the Authentication class;
• Add a new hook for the admin_init action that calls the verify_user_input_settings method;
• Create a private verify_user_input_settings method that:
  
  
  
  • Checks the current value of the user input state setting and returns if it is not empty;
  
  
  • Checks authentication status and returns if the user is authenticated;
  
  
  • Checks if the proxy is being used and returns if it's true;
  
  
  • Gets user input settings data using user_input_settings->get_settings() function
  
  
  • Checks if at least one setting has non-empty values. If yes, sets user_input_state setting to completed, otherwise missing.
  
  
• Add a new hook for the googlesitekit_user_data filter that adds userInputState property to the filtered data using value received from the user_input_state::get method.
• Add another admin_init hook to redirect the user to the googlesitekit-user-input page if the current page is googlesitekit-dashboard and the user input state setting equals to required.

Step 3

In the core/user datastore (user-info.js file):

• Add a new receiveUserInputState action that accepts user input state value and triggers appropriate action with the incoming value as a payload;
• Update the reducer function to catch user input state action and assign payload value to the userInputState property of the store state object.
• Add a new getUserInputState resolver that takes userInputState property from the _googlesitekitUserData object and yields receiveUserInputState action. It should behaive similar to getConnectURL resolver
• Add a new selector getUserInputState to the core/user store which returns userInputState from the store state object.
• Add tests to cover the new functionality.

QA Brief

• In order to QA. it is necessary to mock the sync_with_proxy function - see #2036 for details.
• Test the redirect by manually setting the wp_googlesitekit_user_input_state option to required
• Confirm that the new datastore selector works as expected.

Changelog entry

• Detect and store whether each user has already answered the user input questions to customize the plugin behavior.

[eugene-manuilov]

@felixarntz, IB is added. I know that you don't want to set the user input state to required yet, but I think it still makes sense to do it in the admin_init hook to prevent invoking user input settings endpoint on every page load. I have added it to IB.

[felixarntz]

@eugene-manuilov I think it's okay for the user input endpoint to be invoked because the results are cached. It would only fire a request once a week (or otherwise when the data expired). Whether the value is required or not shouldn't affect that at all, this check is only done for knowing whether the value should be set to completed or not (completed is _not_ the opposite of required here). Potentially we could explore using another flag value like missing (both missing and required would mean that the settings are not completed, with different nuance), that way we would only need to check the user input settings data if the value is empty. What do you think?

Regarding the IB specifically:

If yes, sets user_input_state setting to completed, otherwise required.

See above, this is not correct - only completed should be set, otherwise nothing, or if we introduce missing, then missing should be set. required is set once somebody with empty or missing starts the setup flow (should not happen here though).

VALUE_DISMISSED with dismissed value.

This should not be a possible value, dismissal is purely handled on the client. We might want to replace this with VALUE_MISSING though, see above, to only run the admin_init check for the one case where the setting is not initialized.

Step 4

• Update the GoogleSitekitDashboard component in the assets/js/googlesitekit-dashboard.js file to retrieve userInputState value using the useSelect hook.
• Check if userInputState equals to required and redirect the user to the googlesitekit-user-input screen using getAdminURL selector from the core/site datastore.

This redirect should happen server-side (e.g. in Screens class), we shouldn't need to first start loading JS, which might cause some flashing of the dashboard and would also easily allow avoiding the redirect via DevTools etc.

[eugene-manuilov]

@felixarntz I have updated IB to address your feedback. As to the missing flag, I think we don't need it because it is the same thing as an empty value in the user input state setting.

This redirect should happen server-side (e.g. in Screens class), we shouldn't need to first start loading JS, which might cause some flashing of the dashboard and would also easily allow avoiding the redirect via DevTools etc.

Ok, updated IB to make the redirect in the Authentication class since it has everything required to do it.

[felixarntz]

@eugene-manuilov

As to the missing flag, I think we don't need it because it is the same thing as an empty value in the user input state setting.

Right, it's kind of the same, the reason I brought it up for consideration is that it would allow us to not check user input settings if missing was set. Basically:

• admin_init would only check user input settings if the flag is empty / not set. It then sets it to either missing or completed.
• While missing otherwise means the same as empty / not set, it would allow the above logic to be skipped if not needed.

We don't _need_ to add it, but may be worth it.

[eugene-manuilov]

@felixarntz I think it is worth adding missing value because it will remove a need to send unnecessary requests to the proxy server. IB is updated.

[felixarntz]

@eugene-manuilov

Checks the current value of the user input state setting and returns if it equals to completed or missing;

It should return if it's any non-empty value (just for the uninitialized state it should get user input settings to check it)

Sends a GET request to the REST_Routes::REST_ROOT . '/core/user/data/user-input-settings' endpoint

Let's not send a REST request when we're already in PHP (even though we could do that internally). We should be able to use the User_Input_Settings class directly.

Add another admin_init hook to redirect the user to the googlesitekit-user-input page if the current page is googlesitekit-dashboard and the user input state setting equals to required.

Maybe we should do this in the initialization callback for the googlesitekit-dashboard screen, within Screens class?

[eugene-manuilov]

It should return if it's any non-empty value (just for the uninitialized state it should get user input settings to check it)

Yep, makes sense, updated.

Let's not send a REST request when we're already in PHP (even though we could do that internally). We should be able to use the User_Input_Settings class directly.

Maybe we should do this in the initialization callback for the googlesitekit-dashboard screen, within Screens class?

I would avoid doing it because it requires re-instantiating User_Input_Settings and User_Input_State classes with their dependencies in Screens and Authentication classes. It'll make the codebase slightly more complicated and a bit more coupled which is worse than calling the rest_do_request function and adding another admin_init hook. If we had a dependency injection, we would easily do what you suggest because we could get User_Input_Settings and User_Input_State objects from a DI container without creating duplicates. But for now, I think it worth doing it as it states in the IB now.

Let me know what you think about it. If you still want to move it, I can update the IB.

[felixarntz]

@eugene-manuilov I think it would make sense to instantiate User_Input_Settings in the constructor of Authentication and assign it to a class property, similar to how most other general authentication-related classes are instantiated there. I'd prefer to avoid making an internal REST request, and with this approach, which is already established, we don't have to re-instantiate the classes.

Let's then keep the redirection in an admin_init hook within Authentication like you suggested, relying on $this->user_input_settings as well.

[eugene-manuilov]

@felixarntz ok, updated.

[felixarntz]

IB ✅

[aaemnnosttv]

QA

I tested this a fair bit and found that everything works as expected. QA ✅

Regarding the constraint on possible values for googlesitekit_user_input_state, this seems to be better implemented using a sanitize callback (get_sanitize_callback in User_Input_State) rather than only in User_Input_State::set. That way the logic applies more globally, e.g. setting via REST or CLI as well.

This follows the IB and does work as expected so I can't say it's a QA failure but a suggested enhancement that would be quick to do. Thoughts @felixarntz ?

[felixarntz]

@aaemnnosttv Good catch! Let's open a follow-up issue for that.

[aaemnnosttv]

@felixarntz I opened https://github.com/google/site-kit-wp/issues/2323 as a follow-up.

Moving this one forward to approval 👍