google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 291 forks source link

Add e2e coverage for user input flow #2288

Closed felixarntz closed 3 years ago

felixarntz commented 4 years ago

e2e test coverage should be added for the new user input flow.


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

Acceptance criteria

"New" user

  1. Click "Sign in with Google" on setup screen (which will internally mark user input as required before redirecting to authentication service).
  2. Come back from setup to the Site Kit dashboard URL technically, but get redirected to user input screen.
  3. Go through all five question, choose some answers.
  4. Arrive at preview screen with all questions and the given answers, click "Submit".
  5. Land on dashboard with the "Congrats" notification for the user input flow present.

"Existing" user (already connected)

  1. Visit the Site Kit dashboard, see notification prompting to fill user input.
  2. Click the CTA link in the notification, arriving on the user input screen.
  3. The rest matches steps 3.-5. for the "New" user scenario above.

Implementation Brief

use Google\Site_Kit\Core\Authentication\User_Input_State; use Google\Site_Kit\Core\REST_API\REST_Routes; use Google\Site_Kit\Core\Storage\User_Setting;

add_action( 'rest_api_init', function () { if ( ! defined( 'GOOGLESITEKIT_PLUGIN_MAIN_FILE' ) ) { return; }

    register_rest_route(
        REST_Routes::REST_ROOT,
        'e2e/auth/user-input',
        array(
            'methods'             => WP_REST_Server::EDITABLE,
            'callback'            => function ( WP_REST_Request $request ) {
                update_option(
                    User_Setting::OPTION,
                    sanitize_text_field( $request['user_input'] )
                );

                return array( 'success' => true );
            },
            'permission_callback' => '__return_true',
        )
    );
},
0

);

* Create an E2E JS Util (eg: `tests/e2e/utils/set-user-input.js`) that allows this REST endpoint to be used to set a User Input settings option value:
```js
/**
 * Internal dependencies
 */
import { wpApiFetch } from './wp-api-fetch';

/**
 * Sets the User Input option.
 *
 * @since n.e.x.t
 *
 * @param {string} value The new user input state to set.
 * @return {*} Resolved value from `apiFetch` promise.
 */
export async function setUserInput( value = 'required' ) {
    return await wpApiFetch( {
        path: 'google-site-kit/v1/e2e/auth/user-input',
        method: 'post',
        data: {
            user_input: value,
        },
    } );
}

New user flow

Existing user flow

Settings page flow

Test Coverage

Visual Regression Changes

QA Brief

Changelog entry

tofumatt commented 3 years ago

I don't think the tests themselves here should take too long to write, but given how long E2E tests can take to run I've made this an 11 estimate, as the E2E tests are harder to diagnose issues with and run repeatedly.

felixarntz commented 3 years ago

@tofumatt IB mostly LGTM, but one detail to consider here:

I noticed that this may partly depend on #2316 (see last bullet point of ACs and IB) to be done. For the new user flow, the user will only get redirected to the googlesitekit-user-input screen after setup once that is implemented, and only in a development build. I'm not actually sure what build we use for e2e, is it a development build or production build?

If the latter, we will need to find an alternate solution anyway, potentially we can write a tiny PHP utility that intercepts a request and sets the user input state flag to required before the setup flow, so that the user will be redirected even though that is not yet implemented in production code. That might make sense to decouple this issue from #2316, and then we should add a TODO comment.

tofumatt commented 3 years ago

Ah, yeah, we actually use a production build for the E2E tests

Given how straightforward #2316 is (and how low the estimate is), I'd rather make it a blocker for this issue re: the feature flags. But yeah, to force the redirect we'll want a PHP utility. I'll update the IB with an E2E plugin we can use to force the redirect.

tofumatt commented 3 years ago

I tried to sort out a more elegant way of forcing the redirect with an E2E plugin, but I couldn't quite sort one out given how nested that check is… maybe I'm missing something on the PHP side—if so let me know and we can do something better, but I think that solution should allow us to force the redirect for testing.

felixarntz commented 3 years ago

@tofumatt I think it would be better to use a solution that doesn't involve modifying the production code - I think we should be able to add an e2e plugin that introduces a simple REST API route which allows setting the googlesitekit_user_input_state user option value. The e2e test setup could then call that REST route to set that user option to required, which would then automatically trigger the redirect based on the logic.

felixarntz commented 3 years ago

@tofumatt Some of that exact code in the IB is not correct, but that goes beyond the IB review - overall the approach LGTM. I'd suggest to name the e2e REST route user-input-state instead of user-input to clarify a bit more its purpose.

Overall IB ✅

aaemnnosttv commented 3 years ago

@felixarntz I discussed this one with @tofumatt last week and this issue is not currently actionable due to limitations of our current feature flag implementation. The main reason here being that E2E currently runs in a production build context and we have no way of enabling select features for specific tests. I say specific features because currently all flags are enabled for development but I don't think simply changing E2E to run with a development build is a good solution here as it will result in many failures. I will open a new issue now to address it which this issue will depend on.

cole10up commented 3 years ago

QA ✅

Ran npm run test:e2e after pulling down the latest develop.

No issues on my end.