Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create First-party mode base class, settings, and REST API controller #9625

Closed
hussain-t opened this issue Nov 6, 2024 · 7 comments
Closed
Labels
P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@hussain-t
Copy link
Collaborator

hussain-t commented Nov 6, 2024

Feature Description

Develop the foundational structure for First-party mode by implementing a base class, settings class, and REST API controller.

This includes:

A First_Party_Mode_Settings class to define and manage the First-party mode settings.
A REST_First_Party_Mode_Settings_Controller class to handle REST API endpoints.
A First_Party_Mode base class that will serve as the main entry point, registering and instantiating the settings and REST controller classes.

See the design doc for more details.


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

Acceptance criteria

  • Namespace: The infrastructure for handling First-party mode should be organized within the Google\Site_Kit\Core\Tags\First_Party_Mode namespace.
  • A First_Party_Mode_Settings class should be created to define the googlesitekit_first_party_mode settings object, including:
    • An isEnabled boolean property to indicate if First-party mode is on or off.
    • An isFPMHealthy boolean property to track the status of the FPFE health check.
    • An isScriptAccessEnabled boolean property to track whether direct script access is blocked.
    • Default values and sanitization callbacks for each setting should be defined.
  • A REST_First_Party_Mode_Settings_Controller class should be created to handle REST API requests for First-party mode settings with the following endpoints under core/site/data:
    • GET fpm-settings: Retrieves the current First-party mode settings.
    • POST fpm-settings: Updates the First-party mode settings, allowing users to enable/disable the mode.
    • Note: The fpm-server-requirement-status endpoint will be implemented in a separate issue.
  • A First_Party_Mode base class should be created and used to instantiate and register the First_Party_Mode_Settings and REST_First_Party_Mode_Settings_Controller classes, ensuring they are loaded properly within Site Kit.
    • The First_Party_Mode base class should handle any additional setup or hooks necessary for managing First-party mode.
  • The base class should be instantiated and registered in the main Site Kit plugin file only if the firstPartyMode feature flag is enabled.

Implementation Brief

  • The PHP classes defined in the AC should be implemented following Site Kit's standard patterns and conventions.
  • The Consent Mode classes can serve as a reference:
  • For the instantiation and registration of the First_Party_Mode class, see that of the Consent_Mode class for an example. It should however be wrapped in a check for the firstPartyMode feature flag being set. To retrieve the state of the feature flag, use Feature_Flags::enabled( 'firstPartyMode' ).

Test Coverage

  • Add PHPUnit test coverage for the First_Party_Mode_Settings and REST_First_Party_Mode_Settings_Controller classes.

QA Brief

  • Enable the firstPartyMode feature flag.
  • To verify the fpm-settings endpoint is preloaded, open the browser console and execute _googlesitekitAPIFetchData.preloadedData.
  • Verify it contains /google-site-kit/v1/core/site/data/fpm-settings in the object.
  • To execute the fpm-settings REST API endpoints, go to the Analytics settings page and open the browser developer tools "Network" tab.
  • Refresh the page if you don't see a request to conversion-tracking in the network tab.
  • Right-click on the conversion-tracking request and select "Copy->Copy as fetch":
    Image
  • Open the browser console and paste the copied fetch command.
  • Replace the conversion-tracking part of the URL with fpm-settings and execute the command:
    "http://sitekit.10uplabs.com/wp-json/google-site-kit/v1/core/site/data/fpm-settings?_locale=user"
  • Verify that the response contains the default First-party mode settings:
    {
        "isEnabled": null,
        "isFPMHealthy": null,
        "isScriptAccessEnabled": null
    }
  • To verify the POST fpm-settings endpoint, edit the Analytics settings and enable/disable the "Enable enhanced conversion tracking" toggle, and save the changes.
  • Copy the fetch command from the conversion-tracking request as described above and replace the URL with fpm-settings.
  • From the body object, change the enabled property to isEnabled and set it to true or false:
    "body": "{\"data\":{\"settings\":{\"isEnabled\":true}}}",
  • Execute the command and verify that the response contains the updated First-party mode settings.
  • Verify that the googlesitekit_first_party_mode option is updated with the new settings in the wp_options table in the database.
  • Disable the firstPartyMode feature flag and verify that the First-party mode settings routes are no longer available.

Changelog entry

  • Add REST endpoints for First-Party Mode module settings.
@hussain-t hussain-t added Type: Enhancement Improvement of an existing feature Team M Issues for Squad 2 labels Nov 6, 2024
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Nov 6, 2024
@techanvil techanvil self-assigned this Nov 7, 2024
@techanvil
Copy link
Collaborator

techanvil commented Nov 7, 2024

Hey @hussain-t, thanks for drafting the AC. In general I prefer to avoid these IB-like ACs but sometimes it's unavoidable and this seems like one of those times.

Arguably, the IB could simply state "implement as per the AC", but it could also expand on the detail, we can leave that for the IB author to decide.

The only question I have relates to the one I raised in the design doc about the is_fpm_health_check_failed and is_script_access_disabled setting names, I would prefer to invert them to make them a bit easier to comprehend, but it's manageable enough to keep them as they are. Please review my comment on the design doc, and update these AC accordingly if we do make that change.

@hussain-t
Copy link
Collaborator Author

hussain-t commented Nov 8, 2024

Thanks, @techanvil! I agree with avoiding IB-like ACs. In cases like this, though, a bit more specificity helps provide a clear understanding. I have updated the AC with the renamed properties.

@hussain-t hussain-t assigned techanvil and unassigned hussain-t Nov 8, 2024
@techanvil
Copy link
Collaborator

Thanks @hussain-t, yup makes sense.

The updated AC LGTM, cheers!

AC ✅

@techanvil techanvil assigned techanvil and unassigned techanvil Nov 8, 2024
@hussain-t hussain-t added the P0 High priority label Nov 11, 2024
@hussain-t hussain-t self-assigned this Nov 11, 2024
@hussain-t
Copy link
Collaborator Author

Thanks, @techanvil. The IB almost looks good. However, I'd suggest adding the individual accessors for the settings for completion and bumping the estimate up to 19 to avoid overages.

@hussain-t hussain-t assigned techanvil and unassigned hussain-t Nov 11, 2024
@techanvil
Copy link
Collaborator

techanvil commented Nov 11, 2024

Thanks for the feedback @hussain-t. I am a bit hesitant though with regard to the accessors, including them just for the sake of completion might result in us maintaining dead code if we don't end up using them. Can you point to where we know they will be used? When I had a browse of the issues for this epic it didn't look like they will end up being needed, although I could be wrong there.

Either way, I'd be happy to bump this to a 19 if you still think it needs it. Please see what you think.

@techanvil techanvil assigned hussain-t and unassigned techanvil Nov 11, 2024
@hussain-t
Copy link
Collaborator Author

Thanks, @techanvil, that sounds reasonable. We can add them as needed. Let’s go ahead and bump it to 19.

IB ✅

@hussain-t hussain-t assigned techanvil and unassigned hussain-t Nov 13, 2024
techanvil added a commit that referenced this issue Nov 13, 2024
…ngs-rest-classes

Enhancement/#9625 - Create FPM base class, settings, and REST API controller
@techanvil techanvil removed their assignment Nov 13, 2024
@kelvinballoo kelvinballoo self-assigned this Nov 22, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Tested this and results are as expected.
Moving ticket to approval.

  • fpm-settings endpoint is preloaded

    Image

  • Copy as fetch for the conversion-tracking request was successful and the response contains the default FPM settings

    Image

    Image

  • Copy as fetch for POST fpm-settings endpoint was successful and the response contained the updated FPM settings, whether it was True or false.

    Image

    Image

    Image

  • The googlesitekit_first_party_mode option is updated accordingly in the wp_options table in the database, based on what was set in the command.

    Image

    Image

  • When the firstPartyMode feature flag is disabled, the First-party mode settings routes are no longer available and will hit 404 not found error.

    Image

    Image

@kelvinballoo kelvinballoo removed their assignment Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

4 participants