Wizard Step: Plugin compatibility scanning

[jwold]

Feature description

As part of the onboarding the AMP plugin will check for plugin compatibility compatibility issues, and recommend actions based on what it finds.

---

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

Acceptance criteria

• User is made aware of the plugins which cause AMP validation errors.
• User can easily suppress the plugins that are causing AMP validation errors.
• User is recommended to use Reader mode or Transitional mode when errors from plugins are detected.

Implementation brief

cf. #4795

1. Add a new Onboarding Wizard step for Site Scanning as well as an area on the Settings Screen to initiate a scan.
2. Obtain validation results for URLs representative of the site. See IB on Wizard Step: Theme compatibility scanning #4795.
3. On the Template mode selection, do not recommend Standard mode if there are any validation errors detected.
4. Add an onboarding screen for Plugins that lists out the plugins which are causing problems and provide option to suppress each.

QA testing instructions

Demo

Changelog entry

[westonruter]

This step is not going to be about plugin compatibility scanning only. It will also check for the theme compatibility. If a theme is generating AMP validation errors, then this will be a signal for which template mode will be recommended to the user. For example:

• If there are no errors caused by the theme or plugins, then we could recommend Standard mode.
• If there are no errors caused by the theme but there are errors caused by plugins, we could recommend Transitional mode, along with suggesting for the offending plugins to be turned off in AMP responses.
• If there are errors caused by both the theme and plugins, then we can recommend Reader mode along with the plugin suppression suggestion.

See #4795.

If it finds incompatible plugins it will show list of plugins that are causing problems, and ask user if they want to turn off the plugins.

Turn them off specifically when generating AMP pages.

[jwold]

What about scenarios where there are theme errors, but no plugin errors?

[jwold]

[westonruter]

Yes, if there are theme errors, then we recommend Reader mode.

If there are no theme errors but there are plugin errors, then Transitional mode can recommended (especially for technical users).

If there are no theme errors nor plugin errors, then Standard mode can be recommended (especially for technical users).

I believe the scenarios are captured in the AMP Copy Configuration Table doc.

[westonruter]

Note we'll need to incorporate site scanning into the settings screen as well.

This should make the recommendations consistent between the wizard and settings screen. See #4990 (comment).

[westonruter]

There needs to be a button on the Settings screen to initiate a re-scan.

[westonruter]

Also, with #1756 when using WP-Cron, we'll want to provide a summary of the most recent site scan results when the user first lands on the Settings screen. Then they can re-scan if they want to get more up-to-date results.

[johnwatkins0]

Using this task as the jumping-off point for the various site scanning tasks. I kinda think #4795 can be closed in favor of this one, but I'll hold off for now.

[jwold]

@westonruter and @amedina I've updated the prototype based on our last conversation, and look forward to your feedback.

Video:

rTl6d5ycbu.mp4

This also accounts for #6071 with site review, so we'll go over them together. Happy to schedule a call Tuesday/Wednesday, otherwise we can go async. You can also checkout the prototype on Figma. Thanks!

cc @delawski

[westonruter]

@jwold Are the counts next to “theme issues” and “plugin issues” indicating the number of themes and plugins respectively that issues? Or is it indicating the number of issues across the themes and plugins? If the former, which seems to be indicated here, then the headings should perhaps be rather “Plugins with issues”.

Note that there will usually be one theme, or else a parent theme and child theme. So for the theme, maybe it doesn't make sense to show it collapsed like that. Maybe for the themes it should just have the theme name and not have any count. And if there is a child theme active, have a separate row for it. The parent theme and child theme would not be in expandable drawers then.

On the settings screen, wasn't there also going to be a link off to the Validated URLs screen and/or down to the Plugin Suppression table for the user to discover what the “issues” are? I think we also need to elaborate on what “issues” means. If we're just listing out the theme/plugins which have issues, I can imagine a technical user especially exclaiming, “WHAT IS WRONG WITH MY AWESOME PLUGIN I MADE WITH MY OWN TWO HANDS?!” Or a non-technical user may throw up their hands, “I PAID $100 FOR THIS PREMIUM PLUGIN AND IT HAS ISSUES!?” Maybe we should be more explicit that issues means “AMP incompatibilities”? It's tricky because we want to also de-emphasize AMP and focus more on PX, but we don't have general PX issues yet to report.

For technical users, perhaps there should be a link out to the Validated URLs screen so the user can review what is wrong so they aren't left wondering, both in the onboarding wizard step and on the settings/dashboard screen.

@delawski Side note, during site scanning we'll be wanting to evaluate the URLs as if Standard mode is enabled. Currently validation is performed based on whatever the currently-selected mode is, which is by default Reader mode currently. So we should come up with a way to temporarily override the mode, perhaps with a query parameter.

[amedina]

+1 on providing such access to advanced users.

Could the Browse site button at the end be placed on the right?

[jwold]

Are the counts next to “theme issues” and “plugin issues” indicating the number of themes and plugins respectively that issues? Or is it indicating the number of issues across the themes and plugins? If the former, which seems to be indicated here, then the headings should perhaps be rather “Plugins with issues”.

Good catch. Fixed. It's supposed to be "Plugins with issues."

Note that there will usually be one theme, or else a parent theme and child theme. So for the theme, maybe it doesn't make sense to show it collapsed like that. Maybe for the themes it should just have the theme name and not have any count. And if there is a child theme active, have a separate row for it. The parent theme and child theme would not be in expandable drawers then.

I like that. Updated: https://d.pr/i/WQy2bG

On the settings screen, wasn't there also going to be a link off to the Validated URLs screen and/or down to the Plugin Suppression table for the user to discover what the “issues” are?

Good point. That got lost in the mix. Added back in: https://d.pr/i/1OmyDm

I think we also need to elaborate on what “issues” means. If we're just listing out the theme/plugins which have issues, I can imagine a technical user especially exclaiming, “WHAT IS WRONG WITH MY AWESOME PLUGIN I MADE WITH MY OWN TWO HANDS?!” Or a non-technical user may throw up their hands, “I PAID $100 FOR THIS PREMIUM PLUGIN AND IT HAS ISSUES!?” Maybe we should be more explicit that issues means “AMP incompatibilities”? It's tricky because we want to also de-emphasize AMP and focus more on PX, but we don't have general PX issues yet to report.

What if we changed it then to "Plugins with AMP incompatibility?" That helps solve the problem in the meantime until we're ready for 2.3. Screenshot: https://d.pr/i/1OmyDm

For technical users, perhaps there should be a link out to the Validated URLs screen so the user can review what is wrong so they aren't left wondering, both in the onboarding wizard step and on the settings/dashboard screen.

We've discussed whether to include a link or not, and this feels like evidence pointing to have a link. In this case, it's probably fine for technical or non-technical. The simplest way I can think at the moment is an anchor link out on the plugin issue list. Screenshot: https://d.pr/i/STG7ps

Could the Browse site button at the end be placed on the right?

@amedina great question. My only concern with moving the button to the right is the imbalance it will now create with white space on the screen, especially since we're adding back in the paragraph above. Here's what it could look like: https://d.pr/i/KmtVNP

[jwold]

@delawski this is now unblocked: https://www.figma.com/file/SfMlDvHc5KHxJmAZN12PIM/?node-id=0%3A1

Happy to jump on a call if clarification is needed!

cc @westonruter and @amedina

[westonruter]

We've discussed whether to include a link or not, and this feels like evidence pointing to have a link. In this case, it's probably fine for technical or non-technical. The simplest way I can think at the moment is an anchor link out on the plugin issue list. Screenshot: https://d.pr/i/STG7ps

Only thing is that there shouldn't be a link for each plugin/theme, if that link is intended to take the user to a Validated URL. A given Validated URL may have validation errors from one or more plugins, and there's no 1:1 correspondence between URLs and plugins with validation errors. So instead of there being a link with each plugin, there should be one paragraph perhaps at the top that has one single CTA to review validated URLs for any AMP compatibility issues.

Humm, that being said, we could identify a list of validated URLs that have errors for a given plugin, and the plugin-specific links could link to that subset of validated URLs. In other words, instead of linking to the Validated URLs screen at /wp-admin/edit.php?post_type=amp_validated_url we could link to /wp-admin/edit.php?post_type=amp_validated_url, let's say that a given plugin's errors were detected on the amp_validated_url posts with IDs 1, 2, and 3. We could link to /wp-admin/edit.php?post_type=amp_validated_url&post__in=1,2,3 and this would just show those three Validated URLs in the post list table. (Note that post__in is not a public query var so this won't work out of the box.) If this is the case, then tooltip for the link with each item could be something like “Review 3 URLs on which this plugin has AMP compatibility issues.”

[westonruter]

Or rather than passing in the specific post IDs, instead the URL could use the plugin slug. The only caveat here is that there is no taxonomy of the sources of validation errors, so what the Plugin Suppression table has to do is it grabs the latest 100 validated URLs, and for each that is not stale, obtains all the error sources and and builds up an in-memory index to then provide that information. In other words, we can't easily query for validated URLs that have an error which has a given source. That would be made possible with the move to custom DB tables (#5553).

In any case, in the context of Site Scan, the most important thing is to list the validated URLs which were just scanned. Since we will have the full validation data on hand with the scan, then the plugin sources for the validation errors will be in memory already, and so the validated URLs with errors for a given plugin will be able to be obtained.

So yeah, going with what I proposed above with linking to a filter view that lists out specific validated URLs seems like the better route.

[westonruter]

Side note, during site scanning we'll be wanting to evaluate the URLs as if Standard mode is enabled. Currently validation is performed based on whatever the currently-selected mode is, which is by default Reader mode currently. So we should come up with a way to temporarily override the mode, perhaps with a query parameter.

See #6615

[jwold]

@delawski Alberto and I met today on the AMP config file table. We simplified a few things.

1. Each template mode will now just have bullet points, instead of some text and a notice as the designs previously indicated. Those bullet points are listed under the right column: "Template Mode Screen Copy" in the Google doc.
2. The recommended template mode will have Recommended as a badge at the top of the accordion.
3. We'll open the Recommended accordion by default, and close the other two.
4. We won't sort the template modes, but instead will have Reader on top, Transitional in the middle, and Standard on bottom.
5. Updated the description of the page with two links below the headline.

Figma: https://www.figma.com/file/SfMlDvHc5KHxJmAZN12PIM/AMP-Design?node-id=0%3A1

Happy to jump on a call if clarification is needed!

CC @amedina and @westonruter

[delawski]

@jwold Thank you for that! I've pushed a few commits as part of #6610 that introduce those changes.

Here's a sample screen shot of the updated Template Modes step:

[jwold]

You're welcome! I love it! 😀

[milindmore22]

QA

Scenario	Plugins	Theme	User Type	Recommendation	QA Status
Compatible Plugin and Theme	Yoast SEO	TwentyTwentyOne	Tech	Standard Mode recommended	✅
Compatible Plugin and Theme	Yoast SEO	TwentyTwentyOne	Non-Tech	No Recommendation	❌
Plugin has errors and Theme is compatible	Contact Form 7	TwentyTwentyOne	Tech	Transitional Mode recommended	✅
Plugin has errors and Theme is compatible	Contact Form 7	TwentyTwentyOne	Non-Tech	Transitional Mode and Reader Mode recommended	✅
Plugin is Compatible and Theme has validation errors	Yoast SEO	ColorMag	Tech	Transitional Mode and Reader Mode recommended	✅
Plugin is Compatible and Theme has validation errors	Yoast SEO	ColorMag	Non-Tech	Reader Mode recommended	✅
Plugin and Theme has validation errors	Contact Form 7	ColorMag	Tech	Reader Mode recommended	✅
Plugin and Theme has validation errors	Contact Form 7	ColorMag	Non-Tech	Reader Mode recommended	✅

Here is a contradicting recommendation where Wizard site scanner recommends Transitional mode & Reader mode and AMP settings page Site scanner recommends Standard mode when the user chooses Reader Mode Theme

Video for same: https://recordit.co/NEyBjW8fw1

[delawski]

Here is a contradicting recommendation where Wizard site scanner recommends Transitional mode & Reader mode and AMP settings page Site scanner recommends Standard mode when the user chooses Reader Mode Theme

@milindmore22 I think this works as expected but indeed may be confusing to the users.

Here's the entire flow:

• In the Wizard, a scan is done as if the site was in the Standard template mode.
• Since there's an issue with a plugin when in the Standard mode, the Reader and the Transitional modes are recommended.
• The Reader mode is selected and the user goes back to the AMP Settings screen.
• A new scan is triggered automatically since the scan results are now stale (the site is now using the Reader mode while the scan in the Wizard was done in the Standard mode). The new scan is done in the Reader mode.
• The previously offending plugin no longer causes issues in the Reader mode and so a different recommendation is offered to a user.

@westonruter What do you think about this flow? It seems to me that while technically everything works as expected, for the user this may be confusing. We may have to think of some improvements to the recommendation engine in the next release but I think we may be going down a rabbit hole here.

[westonruter]

• The previously offending plugin no longer causes issues in the Reader mode and so a different recommendation is offered to a user.

Right, I can see this particularly being an issue in the legacy Reader theme since it doesn't use standard theme hooks. Therefore if a plugin is printing something in wp_footer, it won't trigger an error in legacy Reader mode like it would if you're using a Reader theme. That's a tricky one. I agree it's something we should mull over and it doesn't need to be fixed right now.

[delawski]

@milindmore22 Given the discussion above, do you think this issue can be moved to QA Passed lane?

[milindmore22]

@delawski Not sure if this is intentional, there is one failed QA, where the user has AMP-compatible plugins and theme but didn't get any recommendation if they choose to be a non-tech user, they should be given a Standard Mode as a recommendation?

https://recordit.co/WhXdDoIKQW

[delawski]

I think it is expected (as per the template mode recommendation logic) but I'll defer to @westonruter since he's worked on the recommendation logic most recently.

[westonruter]

Yeah, that's a good observation. Basically the thinking there is that in Standard mode without loose sandboxing being enabled, it is very likely that a non-technical user could end up activating a plugin later or adding some embed code that could cause validation errors. They could then end up with a broken site without a way for visitors to access the unbroken version. So in that case, Transitional would be safer for non-technical users at the moment. We'll need to revisit this once Sandboxing graduates from experimental, as if we default to Loose then we can safely recommend Standard much more often. The ultimate goal would be to default to Standard unless the theme fails some extreme PX checks (like lacking mobile friendliness), in which case Reader would be the recommendation.