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.22k stars 277 forks source link

Install/activate the WP Consent API plugin in the background. #8521

Open techanvil opened 3 months ago

techanvil commented 3 months ago

Feature Description

As discussed on Slack, we should install and/or activate the WP Consent API plugin in the background, rather than opening the WP plugins page in a new tab to facilitate the installation/activation.

This will improve both the general UX and our capability for tracking the WP Consent API activation state, which will help to further inform how to improve the feature.


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

tofumatt commented 3 weeks ago

The ACs say we should enable the WP Consent API plugin, but they don't say when/under what conditions.

Enabling it automatically when it isn't enabled seems like a fine idea, but we shouldn't do it constantly, because then a user that explicitly disables it (for whatever reason) after we enabled it will return to the plugins page to see it "randomly" enabled again by us.

So we probably want to specify the conditions to enable it, and also a mechanism to only do so once.

tofumatt commented 3 weeks ago

Thanks, this makes sense 👍🏻

Moving to IB 👍🏻

techanvil commented 3 weeks ago

Hey @benbowler, actually the AC for this needs a revision.

It's not the case that we want the WP Consent API plugin to be installed in direct response to the user enabling Consent Mode via the switch in settings.

We still want the trigger to be a click of the Install (or Activate) button, so the user is making an informed and deliberate choice to install the plugin.

image

At present when the Install/Activate button is clicked, the plugins page is navigated to, taking the user away from Site Kit. What this issue should accomplish is to install/activate the plugin in the background when the button is clicked, showing an in-progress state while doing so (i.e. a spinner in the button).

Please can you update the AC accordingly?

techanvil commented 3 weeks ago

Hi @benbowler, thanks for updating the AC.

techanvil commented 3 weeks ago

@benbowler thanks for the update. Please note I made a small change to tweak the proposed error text and fix a couple of typos.

That said - while I like the proposed error handling, I do have some reservations; for one, handling an error with "warning" styling is a bit out of step with other errors which tend to be displayed inline using the "error" red colour.

image

Also by showing a fixed error message, the underlying error we receive from the WP API is hidden and this might contain some valuable information as to why the plugin couldn't be installed.

Lastly the proposed message doesn't handle the activation case where the more appropriate guidance would be to navigate to the plugins page on the current WP instance and try activating there.

To be honest, I think we might be better off initially taking a simple approach where we display the error received inline using the usual error styling (as seen in ErrorNotice). This is what I had imagined when writing my previous feedback, I just wanted the AC to be a bit more explicit about it.

We can then iterate on it in a subsequent issue to provide more detailed error handling along the lines of what you have specced, but looking further into the possible errors received by the WP plugin API rather than having a single fixed message.

How does that sound?

benbowler commented 3 weeks ago

Agreed @techanvil, I've simplified the AC removing implementation detail which can code in the IB based on the comments above.

techanvil commented 3 weeks ago

Great, thanks @benbowler!

AC :white_check_mark:

techanvil commented 3 weeks ago

Incidentally I don't think we'll need an explicit Retry button as the Install/Activate buttons will allow retrying...

tofumatt commented 2 weeks ago

IB ✅

mohitwp commented 16 hours ago

QA Update ❌

Issue- I use the Tweak extension to show error but error not reproduced for me. So, I use inspect element> Network settings to create error state.

**Install Plugin: Gets stuck in a loading state-** ![image](https://github.com/user-attachments/assets/485df3db-287c-4397-bfc8-091636183b6c) **Activate Plugin :** ![image](https://github.com/user-attachments/assets/8ee20502-c5a8-4859-8bd8-c89280d5a9fe) **PASS CASES** - Verified when the user clicks the install button in the Consent Mode "Install WP Consent API" panel in the Admin Settings, the WP Consent API plugin : -- Gets installed without redirecting the user to wp-admin/update.php. -- Gets activated without redirecting the user to wp-admin/plugins.php. - Verified the user see an appropriate loading state, such as a spinner button, while backend requests are taking place in the background.
https://github.com/user-attachments/assets/ff11fcd9-a282-4a70-9883-97993cc1efd7