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

Update compatibility checks #8101

Closed
3 tasks done
aaemnnosttv opened this issue Jan 18, 2024 · 9 comments
Closed
3 tasks done

Update compatibility checks #8101

aaemnnosttv opened this issue Jan 18, 2024 · 9 comments
Labels
Good First Issue Good first issue for new engineers P2 Low priority Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jan 18, 2024

Feature Description

There are a few changes we should make to our compatibility checks that run before connecting SK.


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

Acceptance criteria

  • The check for testing if SK is running on a version of WP below 5.0 should be removed entirely (this was from when SK supported older versions but some secondary functionality would not work; SK requires WP 5.2 now)
  • The help link for the staging environment cases should be displayed below both variants (it's currently only present in the installed case).
    See this diff for where it changed

Implementation Brief

  • Remove the checkWPVersion functions from assets/js/components/setup/CompatibilityChecks/checks.js

  • Remove the import and use of checkWPVersion from assets/js/components/setup/CompatibilityChecks/index.js

  • Update the assets/js/components/setup/CompatibilityChecks/CompatibilityErrorNotice.js ERROR_FETCH_FAIL condition by copying the object containing the GetHelpLink and using createInterpolateElement to render both the message and the help link.

Test Coverage

QA Brief

  • Set up a new staging environment site without the SK helper plugin installed, trigger the ERROR_FETCH_FAIL compatibility error and you should see the warning message now includes a help link at the end of the message: Looks like this may be a staging environment. If so, you’ll need to install a helper plugin and verify your production site in Search Console. [Help Link Here].
  • The help link title should be "Install" and it should link here.

Changelog entry

  • Improve compatibility checks code, including removing legacy notices for WordPress 4.x users.
@aaemnnosttv aaemnnosttv added P2 Low priority Type: Enhancement Improvement of an existing feature Good First Issue Good first issue for new engineers labels Jan 18, 2024
@benbowler benbowler assigned benbowler and unassigned benbowler Jan 19, 2024
@eugene-manuilov eugene-manuilov self-assigned this Jan 19, 2024
@eugene-manuilov
Copy link
Collaborator

  • Remove the checkWPVersion functions from assets/js/components/setup/CompatibilityChecks/checks.js
  • Remove the import and use of checkWPVersion from assets/js/components/setup/CompatibilityChecks/index.js

@benbowler, we also need to remove all that is relevant for the wpVersion variable since it is no longer needed. See for reference: https://github.com/search?q=repo%3Agoogle%2Fsite-kit-wp%20wpVersion&type=code

@benbowler
Copy link
Collaborator

benbowler commented Jan 22, 2024

@eugene-manuilov I've looked into this a little more this morning and the wpVersion and getWPVersion are still used by the hasMinimumWordPressVersion function:

/**
* Determines whether the current WordPress site has the minimum required version.
* Currently, the minimum required version is 5.2.
*
* @since 1.85.0
*
* @param {string} minimumWPVersion The minimum required WordPress version.
* @return {(boolean|undefined)} `true` if the WordPress site's version is greater than or equal to the minimum required version, `false` if not. Returns `undefined` if not loaded.
*/
hasMinimumWordPressVersion: createRegistrySelector(
( select ) => ( state, minimumWPVersion ) => {
invariant( minimumWPVersion, 'minimumWPVersion is required.' );
const { major, minor } = select( CORE_SITE ).getWPVersion() || {};
if ( major === undefined || minor === undefined ) {
return undefined;
}
const [ minimumMajor, minimumMinor = 0 ] = minimumWPVersion
.split( '.' )
.map( ( v ) => parseInt( v, 10 ) );
return (
minimumMajor < major ||
( minimumMajor === major && minimumMinor <= minor )
);
}
),

Therefore, I believe my original list of references to remove were correct.

@eugene-manuilov
Copy link
Collaborator

Ah.. ok, I missed that. Then it should be good to go. IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Jan 22, 2024
@benbowler benbowler self-assigned this Jan 24, 2024
@benbowler benbowler removed their assignment Jan 26, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Jan 26, 2024
@tofumatt tofumatt assigned tofumatt and benbowler and unassigned tofumatt Jan 30, 2024
@benbowler benbowler assigned tofumatt and unassigned benbowler Jan 30, 2024
@tofumatt tofumatt removed their assignment Jan 31, 2024
@wpdarren wpdarren self-assigned this Feb 2, 2024
@wpdarren
Copy link
Collaborator

wpdarren commented Feb 5, 2024

QA Update: ⚠️

@benbowler I am able to create the message that is in the QAB, but I am seeing a configure link rather than a get help link. Please could you tell me what the link text should be and where should it point to? When I click it, I am taken to the helper plugin settings. Is this expected?

image

@benbowler
Copy link
Collaborator

@wpdarren I re-reviewed the code and QAB and I had it right way around when it was first written. The test state is when the developer plugin is not installed on the staging site. I've added details about what CTA you should see and I've updated the expected text for the alert.

@benbowler benbowler removed their assignment Feb 6, 2024
@wpdarren
Copy link
Collaborator

wpdarren commented Feb 7, 2024

@benbowler thank you for updating the QAB. I see the correct message Looks like this may be a staging environment. If so, you’ll need to install a helper plugin and verify your production site in Search Console. but the CTA is not as per the QAB. I have an Install (or if its installed but not active, an Activate link appears) link which takes me to install or active the plugin.

In the past (which was a long time ago) to trigger the ERROR_FETCH_FAIL compatibility error, I had to use the console and blocked one or all of the wp-json network events. As you can see from my screencast this is what I have done this time.

Is there another way to trigger ERROR_FETCH_FAIL? Maybe I am doing this part wrong which is why I am not seeing the CTA correctly. What do you think?

error.mp4

@wpdarren
Copy link
Collaborator

wpdarren commented Feb 7, 2024

QA Update: ❌

@benbowler I have been working a bit more on this ticket and I found that a learn more link does appear but only for a second then changes to install. You can see it on the screenshot below. The link text does not match the QAB too, but there is something amiss here.

trim_ElFKUK.mp4

@wpdarren wpdarren removed their assignment Feb 7, 2024
@benbowler
Copy link
Collaborator

Hey @wpdarren, this was another issue with my QAB, sorry.

The "Install" link is correct linking here as you saw in your videos. I have tested this myself and can confirm this is expected.

The issue with this specific ticket was that I've been writing my QABs without directly running and logging the code which has led to all of this back and forward. I'll make sure for future QABs that I've tested the flow and give more accurate and repeatable steps.

@benbowler benbowler assigned wpdarren and unassigned benbowler Feb 7, 2024
@wpdarren
Copy link
Collaborator

wpdarren commented Feb 8, 2024

QA Update: ✅

Thanks @benbowler - yes, for these types of testing it does need a bit of an 'idiots guide' because I aren't an engineer, so rely a bit more direction. Thank you for updating the QAB, this looks good!

Verified:

  • Without the SK helper plugin installed, and triggering the ERROR_FETCH_FAIL compatibility error you see the warning message now includes a text link at the end of the message: Looks like this may be a staging environment. If so, you’ll need to install a helper plugin and verify your production site in Search Console.
  • The link title is "Install" and it links to /wp-admin/update.php?action=install-plugin&plugin=google-site-kit-dev-settings&_wpnonce=9d4ecfc5fa

Note: there is a flicker of the link text but a user would only notice it if they were on a very slow 3G connection.

image

@wpdarren wpdarren removed their assignment Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P2 Low priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants