Use Site Kit as SDK in order to detect if user can read data of module by leonidasmi · Pull Request #22120 · Yoast/wordpress-seo

leonidasmi · 2025-03-21T09:59:04Z

Context

Summary

This PR can be summarized in the following changelog entry:

Improves the code that detects if a user has viewing rights or the owner of a Site Kit module.

Relevant technical choices:

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

For starters, let's re-test 469 site kit set up add alerts for user with no install plugin capabilities or dashboard viewing rights #22091
As the admin that installed and configured the Site kit plugin make sure you still see all our widgets.
Give a new user viewing permissions to both search console and GA and make sure the correct widgets show up.

For checking #22157 (multiple admins):

Set up Site Kit (without setting up Analytics) with user A
Don't give viewing rights to any role
Go to Yoast dashboard with user B and confirm that you don't see the Site Kit widgets
With user A, give viewing rights to Search Console to admins
User B now can see the Search Console widgets in Yoast dashboard
Revert viewing rights to none, to continue our tests
With user B, go and set up Site Kit with a different account
Go to Yoast dashboard with user B and confirm that you see the Site Kit widgets that are for Seach Console
With user A, go and set up Analytics but don't give viewing rights to it, to any role
Go to Yoast dashboard with user A and confirm that you see the Organic Sessions widget too
Go to Yoast dashboard with user B and confirm that you see see the Organic Sessions widget too but you get a You don't have permission error
If user A gives viewing rights to admin for Analytics and user refreshes the Yoast dashboard, they will see the Organic Sessions widget with no errors. Revert the viewing rights of Analytics to no role.
With user B, go and set up Analytics (first you will need to go to the Analytics account of user A and give access to your account as per this: https://support.google.com/analytics/answer/9305788?hl=en#Add&zippy=%2Cin-this-article)
- After you gave access to your account as per the article, go to Site Kit->Settings, edit the Analytics settings and save your account
Go to Yoast dashboard with user B and confirm that you see the Organic Sessions widget with no errors
Now repeat the test in a fresh site, but this time set up Site Kit with user A and set up Analytics at the beginning (before doing anything with user B). User B should not see any widgets until either user A gives viewing rights or user B sets up Site Kit with their own Google account:
- First have user A give viewing rights for Search Console to admins
- Go to Yoast dashboard with user B and confirm you see all widgets with no errors except Organic Sessions which you can't see at all
- Then have user A give viewing rights for Analytics to admins
- Go to Yoast dashboard with user B and confirm you see all widgets including Organic Sessions
- Revoke viewing rights for Search Console
- Go to Yoast dashboard with user B and confirm you see only the Organic Sessions widget with no errors there
- Now revoke viewing rights for Analytics too
- GO to Yoast dashboard with user B and confirm you see no Site Kit related widgets
- Have user B set up Site Kit with their own Google account and then confirm that in the Yoast dashboard they see all Site Kit widgets.

For checking #22154 (no fatal errors in that sub-case):

We have to repeat the replication steps from that bug.
There is a fresh site with two agents
Agent 1 set up Site Kit
Agent 2 set up Site Kit
Agent 1 added Who can manage view access to any admin
Visit Yoast SEO Dashboard and confirm everything works and no fatal errors

For checking SEO Manager capabilities:

Make sure you have a user with SEO manager role
Let the admin user give the SEO manager viewing capabilities in the Google Site Kit plugin
Go to Yoast SEO -> General
- Without this PR all the Site Kit widgets would show an error like the one below
- With this PR the Site Kit Widgets will show the actual data

Relevant test scenarios

Changes should be tested with the browser console open
Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
Changes should be tested on different browsers
Changes should be tested on multisite

Test instructions for QA when the code is in the RC

QA should use the same steps as above.

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

In Yoast dashboard, check wpseoScriptData.dashboard.siteKitConfiguration in the console and it should be the same with this PR and without. Check multiple cases:
- When Site Kit is installed and not
- When Site Kit is active and not
- When Site Kit is set up and not
- When Analytics has been set up in the site and not
- or any combination of the above

UI changes

This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

I have tested this code to the best of my abilities.
During testing, I had activated all plugins that Yoast SEO provides integrations for.
I have added unit tests to verify the code works as intended.
If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
I have written this PR in accordance with my team's definition of done.
I have checked that the base branch is correctly set.

Innovation

No innovation project is applicable for this PR.
This PR falls under an innovation project. I have attached the innovation label.
I have added my hours to the WBSO document.

Fixes https://github.com/Yoast/reserved-tasks/issues/492

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk

leonidasmi

CR: 🏗️

I actually didn't finish my CR, but we decided with @thijsoo to pause it for now, so that he adds some more stuff that fix additional parts of our integration which is nice!

leonidasmi · 2025-04-01T08:13:08Z


 		$site_kit_setup_url = \self_admin_url( 'admin.php?page=googlesitekit-splash' );

+		$preload_paths = \apply_filters( 'googlesitekit_apifetch_preload_paths', [] );


Let's move the below in a separate function(s), this is too much cognitive load for a single function.

We can opt into create private properties for the $ga_module and $search_console_module variables, if that was the problem.

github-actions · 2025-04-01T11:49:57Z

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

…ulate it is a boolean

…lated.

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk # Conflicts: # src/dashboard/infrastructure/integrations/site-kit.php # tests/Unit/Dashboard/Infrastructure/Integrations/Site_Kit_To_Array_Test.php

leonidasmi

CR: 🚧

Aside from the points in the code, I have a last reservation:

On instances where Site Kit is not active/installed (which will be the most usual case), on dashboard page load, we do multiple calls to the Site Kit REST endpoints that will inevitably fail (albeit gracefully).
We should make our flow likeso that we return early and not perform the REST calls at all.
Maybe build a default value for all script data that are dependant on Site Kit being active and resort to that when Site Kit is not active

leonidasmi · 2025-04-04T06:54:36Z

+			if ( $module['slug'] === 'analytics-4' ) {
+				$this->ga_module['owner']     = $module['owner'];
+				$this->ga_module['connected'] = $module['connected'];
+				if ( isset( $modules_permissions['googlesitekit_read_shared_module_data::["analytics-4"]'] ) ) {
+					$this->ga_module['permissions'] = $is_authenticated || $modules_permissions['googlesitekit_read_shared_module_data::["analytics-4"]'];
+				}
+			}
+			if ( $module['slug'] === 'search-console' ) {
+				$this->search_console_module['owner'] = $module['owner'];
+				if ( isset( $modules_permissions['googlesitekit_read_shared_module_data::["search-console"]'] ) ) {
+					$this->search_console_module['permissions'] = $is_authenticated || $modules_permissions['googlesitekit_read_shared_module_data::["search-console"]'];


There's a lot of code duplication here begging for some abstraction, but I'm not sure if it's worth it, what do you think @thijsoo?

Maybe we can bypass the fact that 'connected' is only a thing in GA and go ahead with some?

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk

coveralls · 2025-04-10T07:55:45Z

Pull Request Test Coverage Report for Build 090a767182f28908a7fe209412e2f713a4c1cabb

Details

28 of 89 (31.46%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-5.3%) to 52.428%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dashboard/user-interface/configuration/site-kit-capabilities-integration.php	14	15	93.33%
src/dashboard/infrastructure/connection/site-kit-is-connected-call.php	0	20	0.0%
src/dashboard/infrastructure/integrations/site-kit.php	14	54	25.93%

Totals
Change from base Build 8b259d176f9a2ec35d9403e7a4b74add03e477c6:	-5.3%
Covered Lines:	29060
Relevant Lines:	56478

💛 - Coveralls

github-actions · 2025-04-10T21:41:21Z

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

leonidasmi · 2025-04-11T09:19:54Z

Code Review: ✅
Acceptance test: ❌

As per test instructions, I tried to regression test #22091, but it looks like our new approach doesn't work for SEO managers as is, so half of those test steps fail.

The reason is that SEO managers don't have sufficient permissions to do the REST requests so the script data.

Let's have a discussion (also with product) to see if we want to give those permissions to SEO managers, or figure out a different solution. The impact of giving those permissions is not negligible, so we'll need to have that discussion.

As for the other parts of the test instructions, I didn't test them because I expect our next steps to fix the above will make us have to test the whole PR again anyway.

github-actions · 2025-04-14T10:25:23Z

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk # Conflicts: # src/dashboard/infrastructure/integrations/site-kit.php # tests/Unit/Dashboard/Infrastructure/Integrations/Site_Kit_To_Array_Test.php

github-actions · 2025-04-15T09:46:57Z

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk # Conflicts: # src/dashboard/infrastructure/integrations/site-kit.php # tests/Unit/Dashboard/Infrastructure/Integrations/Site_Kit_To_Array_Test.php

pls78

CR && Acc: ✅

leonidasmi and others added 4 commits March 21, 2025 11:54

Use Site Kit as an SDK to detect if user can read data for a module

ad92334

Fix check for owner

853cf9b

WIP - Start using Site Kit's REST endpoints for script data

1bbe962

Update to use internal api calls

8970fd2

thijsoo added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Mar 26, 2025

thijsoo added 4 commits March 27, 2025 12:24

cs.

5ebfeeb

Merge branch 'trunk' of github.com:Yoast/wordpress-seo into 492-impro…

a9de99e

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk

Fix tests.

9b0567e

CS

dfc5c52

thijsoo marked this pull request as ready for review March 31, 2025 10:33

Mark test skipped.

14edd6f

leonidasmi commented Apr 1, 2025

View reviewed changes

github-actions Bot added the merge conflict label Apr 1, 2025

thijsoo and others added 4 commits April 2, 2025 12:01

Small refactor.

0dd4005

Fix call about permissions

3f0676f

Fix type array element since the share_module_data we then use to pop…

5cee1a9

…ulate it is a boolean

Fix flow for multiple users

91e14bf

thijsoo added 3 commits April 3, 2025 13:34

Refactor a bit to not run site kit code when site kit is not enabled.

83e3a41

Check if the owner is filled in to know if the object is already popu…

5f2209c

…lated.

Merge branch 'trunk' of github.com:Yoast/wordpress-seo into 492-impro…

7ad7b69

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk # Conflicts: # src/dashboard/infrastructure/integrations/site-kit.php # tests/Unit/Dashboard/Infrastructure/Integrations/Site_Kit_To_Array_Test.php

github-actions Bot removed the merge conflict label Apr 3, 2025

Update tests

05dba54

leonidasmi mentioned this pull request Apr 4, 2025

Rework search console and analytics 4 calls to internally use a rest request #22146

Merged

18 tasks

leonidasmi commented Apr 4, 2025

View reviewed changes

pls78 reviewed Apr 4, 2025

View reviewed changes

Comment thread src/dashboard/infrastructure/integrations/site-kit.php Outdated

Merge branch 'trunk' of github.com:Yoast/wordpress-seo into 492-impro…

d451e51

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk

thijsoo added 3 commits April 10, 2025 12:44

Refactor and simplyfy unit test.

0fb8bb1

Update namespace.

4399d0c

Change use statement and some more defensive coding.

feee83e

github-actions Bot added the merge conflict label Apr 10, 2025

Merge trunk

00d9a92

github-actions Bot removed the merge conflict label Apr 11, 2025

Fix PHPCS

6ea4175

github-actions Bot added the merge conflict label Apr 14, 2025

thijsoo added 2 commits April 14, 2025 15:38

Add capabilities for SEO managers

033cb53

Merge branch 'trunk' of github.com:Yoast/wordpress-seo into 492-impro…

9e2475c

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk # Conflicts: # src/dashboard/infrastructure/integrations/site-kit.php # tests/Unit/Dashboard/Infrastructure/Integrations/Site_Kit_To_Array_Test.php

github-actions Bot removed the merge conflict label Apr 14, 2025

github-actions Bot added the merge conflict label Apr 15, 2025

thijsoo force-pushed the 492-improve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk branch from bad7474 to 9f8ded5 Compare April 16, 2025 08:11

Add capabilities for SEO managers

6e8c36c

thijsoo force-pushed the 492-improve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk branch from 9f8ded5 to 6e8c36c Compare April 16, 2025 08:12

Merge branch 'trunk' of github.com:Yoast/wordpress-seo into 492-impro…

4e1ff99

…ve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk # Conflicts: # src/dashboard/infrastructure/integrations/site-kit.php # tests/Unit/Dashboard/Infrastructure/Integrations/Site_Kit_To_Array_Test.php

github-actions Bot removed the merge conflict label Apr 16, 2025

fix test

1eb9d16

pls78 approved these changes Apr 16, 2025

View reviewed changes

pls78 added this to the 25.0 milestone Apr 16, 2025

pls78 merged commit ac5953b into trunk Apr 16, 2025
28 of 29 checks passed

pls78 deleted the 492-improve-site_kitcan_read_data-so-that-it-uses-site-kit-as-an-sdk branch April 16, 2025 13:55

leonidasmi mentioned this pull request May 5, 2025

Fix issues from disconnecting accounts via Site Kit dashboard #22232

Merged

18 tasks


		$site_kit_setup_url = \self_admin_url( 'admin.php?page=googlesitekit-splash' );

		$preload_paths = \apply_filters( 'googlesitekit_apifetch_preload_paths', [] );

Conversation

leonidasmi commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Summary

Relevant technical choices:

Test instructions

Test instructions for the acceptance test before the PR gets merged

Relevant test scenarios

Test instructions for QA when the code is in the RC

Impact check

UI changes

Other environments

Documentation

Quality assurance

Innovation

Uh oh!

leonidasmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leonidasmi Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 1, 2025

Uh oh!

leonidasmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leonidasmi Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 090a767182f28908a7fe209412e2f713a4c1cabb

Details

💛 - Coveralls

Uh oh!

github-actions Bot commented Apr 10, 2025

Uh oh!

leonidasmi commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 14, 2025

Uh oh!

github-actions Bot commented Apr 15, 2025

Uh oh!

pls78 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leonidasmi commented Mar 21, 2025 •

edited

Loading

coveralls commented Apr 10, 2025 •

edited

Loading

leonidasmi commented Apr 11, 2025 •

edited

Loading