Create REST endpoints to store and fetch user selected Key_Metrics settings

[jimmymadon]

Feature Description

Following up from #6308, a new class should be created to register the new REST endpoint to store and fetch user selected metrics as well as any other user data related to the Key Metrics widget, such as the flag to toggle the visibility.

The REST controller class should make use of Key_Metrics_Settings class created in #6308 which deals with persistently storing the user selected key metrics in the database.

The architecture and purpose of the new classes created can be similar to the existing Dismissals (Key_Metrics), Dismissed_Items (Key_Metrics_Settings) and REST_Dismissals_Controller (REST_Key_Metrics_Controller) classes.

---

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

Acceptance criteria

• A new REST_Key_Metrics_Controller class should be created in the Google\Site_Kit\Core\Key_Metrics namespace similar to other REST_*_Controller classes.
• It should register a new route for core/user/data/key-metrics in the Site Kit REST API namespace
  • A POST endpoint should be registered for creating/updating the widget slugs picked by the user.
    • the endpoint should require at least one of the Permissions::VIEW_SPLASH or the Permissions::VIEW_DASHBOARD capabilities (similar to $can_list_data here).
    • the request should require a data parameter which should be merged into the current settings similar to modules/:slug/data/settings.
    • the data request should be of the following shape:

        {
            widgetSlugs,    // Object of widget slug strings,
            isWidgetHidden    // boolean value
        }

    • its response should return the same shape as the request after the merge (updated settings).
  • A GET endpoint should be registered for fetching the widget slugs picked by the user.
    • the endpoint should require the Permissions::VIEW_SPLASH or the Permissions::VIEW_DASHBOARD capabilities (similar to $can_list_data here).
    • the response should return the same shape as the POST endpoint above.
    • this route should also be pre-fetched, i.e. added to the googlesitekit_apifetch_preload_paths filter.
• Test coverage should be added for both the new endpoints.

Implementation Brief

• Please wait until Create the entry point Key_Metrics and Key_Metrics_Settings classes #6308 is merged.
• In includes/Core/Key_Metrics/Key_Metrics_Settings.php file:
  • Create a public method named merge(), that should update the key metrics settings partially, i.e. call set() and update only the passed settings, leaving other existing settings intact. This can be similar to the merge() method in includes/Core/Modules/Module_Settings.php.
• In the includes/Core/Key_Metrics folder, create a new file named REST_Key_Metrics_Controller.php. In this file:
  • While writing this class, includes/Core/User_Input/REST_User_Input_Controller.php can largely be used as an example.
  • Create a new class named REST_Key_Metrics_Controller.php. Its namespace should be Google\Site_Kit\Core\Key_Metrics.
  • The class constructor should receive an instance of the Key_Metrics_Settings class as a parameter, and it should be assigned to a private/protected class property named $key_metrics_settings.
  • Create a new private method named get_rest_routes(). In this method:
    • Create a new variable to check capabilities of the user to use route(s) created here, say $can_select_key_metrics. This variable should return true if either Permissions::VIEW_SPLASH or the Permissions::VIEW_DASHBOARD is truthy (similar to $can_list_data here).
    • Return an array containing a REST_ROUTE defined for the new core/user/data/key-metrics route. This route should accept requests for both GET and POST methods. Here are the specifications for each method:
      • GET:
        • The permission_callback property should be the $can_select_key_metrics variable.
        • The method property should be WP_REST_Server::READABLE.
        • The callback property should be a function that returns the response of the get() method of $key_metrics_settings class property as an object, wrapped with rest_ensure_response().
      • POST:
        • The permission_callback property should be the $can_select_key_metrics variable.
        • The method property should be WP_REST_Server::CREATABLE.
        • The callback property should be a function that accepts the $request (of type WP_REST_Request) parameter. Extract the data parameter from $request using its get_param() method, assinging it to a new $data variable. Check if the $data variable is not set, or if it is not an array. If that is the case, return a 400 WP_Error with rest_missing_callback_param as the code and a translatable Missing settings data. as the error message. Call the new merge() method of $key_metrics_settings class property, passing $data['settings'] to it. Return the response of the get() method of $key_metrics_settings class property as an object, wrapped with rest_ensure_response().
        • Add an args property which should be an array with the data property. The data property should also be an array with the following properties:
          • type of object.
          • description of Metrics to set (translatable).
          • A validate callback function which should check if the value is an array.
  • Create a new public method named register(). In this method:
    • Add a function to the filter hook googlesitekit_rest_routes, that merges the get_rest_routes() method to the existing routes.
    • Add a function to the filter hook googlesitekit_apifetch_preload_paths, that merges the new core/user/data/key-metrics route to the existing list of pre-fetched routes.
• In includes/Core/Key_Metrics/Key_Metrics.php file:
  • In the class constructor, instantiate the new REST_Key_Metrics_Controller class passing the existing instance of the Key_Metrics_Settings class. Assign it to a protected class property named $rest_controller.
  • In the register() method:
    • Call the register() method of the $rest_controller property.

Test Coverage

• Create tests/phpunit/integration/Core/Key_Metrics/REST_Key_Metrics_ControllerTest.php. In this file:
  • Add a test case for the register() method.
  • Add test cases for the both endpoints.
  • This can be similar to tests/phpunit/integration/Core/User_Input/REST_User_Input_ControllerTest.php.

QA Brief

• This issue has no UI changes - hence, a simple smoke test of the plugin (and checking for any errors/console errors) should be enough.
• Also check that the new API work by running the following commands in your browser console to set and get settings:

  googlesitekit.api.set('core', 'user', 'key-metrics', { settings: { isWidgetHidden: true, widgetSlugs:  ['analytics']} });

  googlesitekit.api.get('core', 'user', 'key-metrics');

QA:Eng

• Another code review to ensure the ACs are met would be helpful.

Changelog entry

• Create REST endpoints to store and fetch user-selected Key Metrics settings.

[tofumatt]

ACs here look good 👍🏻

[eugene-manuilov]

Thanks, @nfmohit. IB mostly looks good, but we also need to instantiate the controller and register it to make sure that the new endpoints are available for use.

[nfmohit]

Excellent catch. I've updated the IB, thank you @eugene-manuilov!

[eugene-manuilov]

Thanks, @nfmohit. IB ✔️

[wpdarren]

QA Update: ✅

Verified:

• Completed simple smoke test of the plugin:
  • Set up Analytics.
  • Checked for any errors/console errors.
  • Checked data in the widgets on main, entity and view only dashboard.
• Checked that the new API work by running the commands in the browser console

Left for QA:Eng

[jimmymadon]

✅ QA: Eng

• Went through the code.
• Tested the REST endpoints with correct and incorrect data/params.

Screenshot

[aaemnnosttv]

⚠️ Approval

This one looks good overall, just one flag I noticed here https://github.com/google/site-kit-wp/pull/6426/files#r1089239518

[felixarntz]

Approval ⚠️

I spotted this while going through approval of #6308, and the problem here potentially could be addressed in that issue too, but it's most applicable here: We need to avoid preloading REST endpoints that are not used. The preloading here happens unconditionally without considering the userInput flag, which is not enabled for anyone right now, resulting in all sites unnecessarily preloading the new endpoint. This had recently already come up with another flag, we should pay additional attention to this for the future.

Maybe it's even safer to wrap the entire Key_Metrics class instantiation in a check for userInput. But at a minimum, we should avoid the preloading as that has real performance implications while not providing any benefit to sites without the userInput flag enabled.

[eugene-manuilov]

Good point, @felixarntz. I have update #6486 PR created by @techanvil to also wrap the Key_Metrics class instantiation into the feature flag check. I think this is the best approach for new features. Can you or @aaemnnosttv review it?