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.23k stars 284 forks source link

Ensure that `serviceSetupV2` feature flag remains enabled even when site is disconnected from service #4865

Closed felixarntz closed 2 years ago

felixarntz commented 2 years ago

When requesting remote-controlled features, the site always has to be connected. Currently, when resetting the plugin all enabled features get reset. This is a problem in particular for the upcoming rollout of the serviceSetupV2 feature flag, which affects the setup flow, in which case it right now would never be enabled, even if at some point before it got enabled remotely.

The service has implemented the rollout in a way that it will only grant the feature flag after a completed setup flow, i.e. users won't suddenly switch from the V1 to V2 UI mid-setup flow. This issue is to ensure that the feature flag remains enabled even when the site loses its credentials (e.g. after a reset).


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

Acceptance criteria

Additional ACs (after QA)

Implementation Brief

Test Coverage

QA Brief

Secondary ACs

Changelog entry

eugene-manuilov commented 2 years ago

IB :heavy_check_mark:

wpdarren commented 2 years ago

QA Update: ⚠️

@felixarntz having fun with this ticket! 😄 I am unable to recreate this

Access the dashboard again so that the enabled feature flags are refreshed. Verify that the serviceSetupV2 feature flag is now active (e.g. in Site Health).

Oddly, what I see is the unifiedDashboard feature flag enabled. I have gone through the steps a few times on a few new test sites, created today. I am using the Transients Manager plugin to delete googlesitekit_remote_features

Any ideas why I'm not able to recreate this?

This is what I see in Site Health image

This is the tester plugin which is active. image

felixarntz commented 2 years ago

@wpdarren Ahh sorry, I forgot one crucial detail. You need to be using a Site Kit version 1.69.0 (or higher) in order for the feature to become available from the proxy. Of course that version doesn't exist yet, but you can "fake" it by editing the plugin code to have that version:

Let me know if you run into any issues with that.

wpdarren commented 2 years ago

QA Update: ⚠️

@felixarntz unfortunately, I am still unable to get this to work as per the QAB. Here are the steps that I took:

I was unable to verify that the serviceSetupV2 feature flag is now active.
All I noticed was that Unified Dashboard was now enabled although it was not activated in the tester plugin.

Are you able to get this to work? Am I missing anything in the steps above?

felixarntz commented 2 years ago

@wpdarren The steps look right - what could be is that the trailing slash in the proxy URL is the problem, i.e. it should be define( 'GOOGLESITEKIT_PROXY_URL', 'https://site-kit-dev.appspot.com' );.

Also, make sure you change that URL immediately, i.e. not only after going through the setup flow. It's probably easiest to change it at the very beginning, together with adjusting the version numbers.

wpdarren commented 2 years ago

@felixarntz thanks. I've just run through the process on https://pozenu.us.instawp.xyz/ but unfortunatly, seeing the same as before. Went to Site health and the serviceSetupV2 feature flag is still not set.

felixarntz commented 2 years ago

@wpdarren I realize what went wrong here - the necessary code was no longer present on staging when you tested, as it had since been overwritten by another change 🤦‍♂️

I've just redeployed it, could you please try again (on staging, as before)? All the rest in your testing looks correct, I've also verified your test site from above shows up correctly on the proxy (with version 1.69.0), so that should be all good. Sorry about that!

wpdarren commented 2 years ago

@felixarntz no problem! I went through the test again with the steps and came up against the same problem. serviceSetupV2 feature flag is still not set. I used this site https://mexipi.us.instawp.xyz/ After waiting 10 minutes, deleting the transients and then refreshing the dashboard, the Site health still showed it as disabled. I went back to Transients though for googlesitekit_remote_features and did see that serviceSetupV2 was enabled.

image

image

I then decided to reset the site and see if serviceSetupV2 setup flow appeared but it didn't.

For the setup flow, I've only been activating Search console, so I ran through the test with setting up Analytics, in case that was the cause but same problem. Any other ideas? I can be around for 30 minutes after the engineering standup if that helps, since I know the timezones are tricky and we'll need to get this into the release.

wpdarren commented 2 years ago

QA Update: ⚠️

So close @felixarntz - I finally got to verify that the serviceSetupV2 feature flag is now active (e.g. in Site Health). When I reset the plugin. I see the new (V2) UI for the setup, even though the site is no longer connected.

image

When I did not select Analytics on the V2 splash screen the set up flow was successful.

When I selected Analytics on the splash screen and went through the flow, it got to Step 4 and an error appeared. It looks to be on WordPress core rather than Site Kit but wanted to mention it.

image

I tested on https://yitale.us.instawp.xyz/

These are the steps I went through to generate the error above. The URL it errors out on is https://yitale.us.instawp.xyz/wp-admin/admin.php?page=googlesitekit-module-analytics&slug=analytics&reAuth=true

https://user-images.githubusercontent.com/73545194/155369556-97979b83-e3bc-4483-8382-167496910550.mov

If I click back and then on step 4 click next again, I am redirected back to the WordPress dashboard with this error.

image

wpdarren commented 2 years ago

QA Update: ❌

Sending this back to execution as per our Engineering sync meeting.

felixarntz commented 2 years ago

@wpdarren Thanks for the details! One follow-up question: When you land on that "Sorry, you are not allowed to access this page." screen, can you try accessing the Site Kit dashboard manually (e.g. by typing the URL) instead of going back? My hunch would be that at that point you're still successfully connected, and the only thing would be that Analytics is not fully configured yet.

Update: Confirmed by @wpdarren in Slack thread

The problem here is basically that the redirect URL for the Analytics setup is stored before going through the setup, so the unifiedDashboard feature flag is not enabled. But after the setup, it is enabled, so then the stored URL is no longer working (since the module pages don't exist anymore with the unified dashboard). We can fix it by implementing the redirects to the dashboard that we were thinking about a couple days ago already.

Since this has come up now as a means to fix the problem here, let's do that as part of this issue and not open a new one. I've added a point to the ACs for that.

cc @eugene-manuilov @aaemnnosttv

felixarntz commented 2 years ago

@eugene-manuilov Assigning to you per our call. Please create a PR against main.

wpdarren commented 2 years ago

QA Update: ❌

@felixarntz unfortunately, I do not see the serviceSetupV2 feature flag as active. Went through the steps as previously, the only thing I didn't do this time is change the version wp-admin/plugin-editor.php?plugin=google-site-kit%2Fgoogle-site-kit.php&Submit=Select because it was already set to 1.69.0 since I am on the main branch.

I also ran another test, same steps, but this time I edited the version to 1.70.0 and still do not see the serviceSetupV2 feature flag as active.

1.69.0 test site. https://gigipa.us.instawp.xyz 1.70.0 test site. https://gorapo.us.instawp.xyz/

felixarntz commented 2 years ago

@wpdarren It looks like something went wrong in your testing with setting the proxy constant. I checked for your two sites in our systems, and both of them were only used with the production version of the proxy, so in that case it's expected this is not working yet. There is no record of these sites on the staging proxy, which needs to be used here. Can you double-check that the constant to use the staging proxy is set correctly?

wpdarren commented 2 years ago

@felixarntz I went through the process again and I am still not seeing servicesetupV2 getting enabled. At this point I am not sure why it's no longer working. I am on the main branch, and have gone through the steps are previously documented. Are you able to recreate the same issue?

The latest site I've used is https://faxahe.us.instawp.xyz/

Screenshots ![image](https://user-images.githubusercontent.com/73545194/155955448-f3c8a32b-72f8-4000-b592-ec3ee2d5c231.png) ![image](https://user-images.githubusercontent.com/73545194/155955487-5da3fc98-62dd-4aa6-a63b-7c89b13cbceb.png) ![image](https://user-images.githubusercontent.com/73545194/155955511-fefc0d3c-58fd-49b6-98bf-58ccd10a52fc.png) This is what I see when I delete the transients. ![image](https://user-images.githubusercontent.com/73545194/155955572-1b1240ba-aa68-44a0-b36b-dead54de151b.png) ![image](https://user-images.githubusercontent.com/73545194/155955590-49db8ac7-982e-43d8-90e8-5aaf86e9b2ea.png)
wpdarren commented 2 years ago

QA Update: ✅

Verified:

image

The URL for testing this is https://furoki.us4.instawp.xyz/ (note: I have been playing around with the site to test the secondary ACs, so hopefully it hasn't caused an issue with the logs!