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 292 forks source link

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

Open hussain-t opened 2 weeks ago

hussain-t commented 2 weeks ago

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

Implementation Brief

Test Coverage

QA Brief

Changelog entry

techanvil commented 2 weeks ago

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 commented 2 weeks ago

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.

techanvil commented 2 weeks ago

Thanks @hussain-t, yup makes sense.

The updated AC LGTM, cheers!

AC ✅

hussain-t commented 2 weeks ago

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.

techanvil commented 2 weeks ago

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.

hussain-t commented 2 weeks ago

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

IB ✅

kelvinballoo commented 4 days ago

QA Update ✅

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