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

[Compatibility Tool] Better tooling for evaluating plugins #1007

Closed
postphotos opened this Issue Mar 9, 2018 · 18 comments

Comments

@postphotos
Copy link
Contributor

commented Mar 9, 2018

As a WordPress site, my Compatibility Tool should have better tooling for evaluating plugins' AMP compatibility programmatically.

  • AC1: Currently, AMP validation only occurs on plugin activation and when editing a post: This story addresses user experience. We would like to additionally allow a site admin to manually trigger a check of the plugins' AMP compatibility inside the user interface from #1006 via WP-CLI.
  • AC2: A validation test should be able to evaluate an entire site even if AMP theme support is not declared yet. (i.e. Be able to force the equivalent of AMP theme support when crawling.)
  • AC3: The compatibility tool should be able to target the various templates that now are presented to the user (as as described by @westonruter below, related #1235.)

The goal is to spawn validation process and get results programmatically for an entire site. WP-Cron and WP-CLI are in view here, as is integrating with logic in XML Sitemap generators to obtain list of URLs potentially.

@postphotos postphotos changed the title Compatibility Tool - Better tooling for evaluating plugins [Compatibility Tool] Better tooling for evaluating plugins Mar 9, 2018

@MackenzieHartung MackenzieHartung added this to the v1.0 milestone Mar 12, 2018

@MackenzieHartung MackenzieHartung added this to To do in v1.0 Mar 12, 2018

@postphotos

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2018

The main issue description has been updated with a formal Story and Acceptance Criteria.

@postphotos postphotos added this to To Do in v0.7 Mar 15, 2018

@postphotos postphotos removed this from To Do in v0.7 Mar 15, 2018

@MackenzieHartung MackenzieHartung added Sprint 2 and removed Sprint 1 labels Mar 26, 2018

@westonruter

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

Validation test on entire site even if AMP theme support is not declared yet. Be able to force amp theme support while crawling.

@postphotos

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2018

Added AC2 per our meeting yesterday.

@westonruter

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

Aside: This is useful for trying out AMP theme support for a given theme without manually doing add_theme_support('amp') in the theme. Add a new plugin (e.g. mu-plugin) that contains:

add_action( 'after_setup_theme', function() {
    if ( isset( $_REQUEST['amp_forced'] ) ) {
        add_theme_support( 'amp' );
    }
} );

Then to see how a theme works with AMP just activate the theme and access the homepage or any other page with ?amp_forced in the URL.

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2018

Question About Working On This Issue

Hi @westonruter,
Would it help if I worked on this issue, and having the validator:

...spawn the validation process and get results programmatically for an entire site.

Otherwise, I could start working on #1036.

@westonruter

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

@kienstra yes, go for it. Let's start with a WP-CLI command (with progress bar) that crawls the site and validates each URL it can find (that is, all AMP documents). As part of this, there probably needs to be something like I have in #1007 (comment) to be able to force amp theme support. When the WP-CLI command finishes then there will need to be some way to visualize the results. While all of the validation errors will be populated in the amp_validation_error posts, there should be some more helpful summarizing of the most common issues as noted in #1003 (comment)

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2018

Thanks, Working On This Now

Hi @westonruter,
Thanks, starting with creating a WP-CLI command sounds good. Your snippet above to force support for 'amp' will help.

@kienstra kienstra moved this from To do to In progress in v1.0 Apr 10, 2018

@westonruter

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

Once we have the WP-CLI command in place, then we can look at how to present that same information in the admin. We'll also then need to figure out a good way to use admin-ajax to reliably make requests to batch validate URLs without timing-out. But these things would essentially be GUI improvements over the raw work performed by WP-CLI, so good to focus on that first. On that note, most of the WP-CLI commands should be invoking logic in AMP_Validation_Utils which can be re-shared with the admin integration.

@westonruter

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

As part of this we might want to resurrect the REST API endpoint for AMP validation. There could be a amp/v1/validation-errors endpoint which returns a list of all validation error posts. Then this endpoint could maybe accept POST request with a url to check, which then could result in a validation error post being created, or maybe return 400 error when there are no validation errors on the page. That feels somewhat odd, to return an error for there not being errors. Some more thought on the REST API design should be made.

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2018

Moving Into "To Do"

Hi @westonruter,
That sounds good to have an endpoint to get AMP validation results. Like we discussed, I'm moving this issue back into "To Do" status, to signal that I'm not actively working on it.

If you're not working on it by the time I'm free, I'll probably come back to it.

Thanks!

@kienstra kienstra removed their assignment Apr 10, 2018

@kienstra kienstra moved this from In progress to To do in v1.0 Apr 10, 2018

@postphotos postphotos added the Sprint 4 label Apr 19, 2018

@postphotos postphotos added the Sprint 5 label May 3, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2018

Still Needed?

Hi @westonruter,
Could we still use the WP-CLI command that you mentioned above to check for validation errors with forced theme support? If so, I'd be happy to work on it if that would help.

@westonruter

This comment has been minimized.

Copy link
Member

commented May 11, 2018

Yes, but first we really need to finish #1093.

@westonruter

This comment has been minimized.

Copy link
Member

commented May 27, 2018

A key dependency for this is the ability to obtain validation results for a URL even when the user is not authenticated. This has been implemented via nonce in 90eff4f via #1093.

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2018

@westonruter mentioned that this should involve:

  • Getting a list of URLs of the site, and a way to iterate over them in efficient way that won't time out WordPress
  • Process the URLs in batches
  • WP-CLI
  • WP Cron
  • User-initiated: a button that makes AJAX request with spinner

Of course, feel free to edit this if these notes didn't capture the points exactly.

@westonruter

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

@postphotos New suggested AC: I think we should have the compatibility tool target the various templates that now are presented to the user. Then we can surface next to each supportable template checkbox an indicator for whether AMP is known to be supported. Each item can include a “Check compatibility“ and a link to “Review issues”. Maybe also a button to re-check compatibility for all of the templates. These template checks should be done each time the user switches themes (though the request should be queued via cron and not done synchronously, since it will take a minute or so).

It is more important that the automated site checker look at these templates instead of looking at every single URL. We should still do the scanning of all URLs periodically but when a user initiates a scan it should focus on the main site templates. /cc @kienstra

See also #1254 for an admin pointer to draw users to the admin screen. We should use the tool to help provide an onboarding experience for users to AMP theme support.

@postphotos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

Hi @westonruter - I didn't flag you here, but I added your AC from the comment above to this ticket last week.

@westonruter

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

I've tweaked the AC1 to make it explicit that this issue is regarding the initiation of a site validation scan via WP-CLI not via the WordPress admin. An admin UI would need to be proposed in a separate issue along with designs; the admin UI would have the added complexity of needing to guard against timeouts. The PR #1183 resolves the WP-CLI issue.

@westonruter westonruter moved this from In progress to Ready for QA in v1.0 Aug 29, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Sep 18, 2018

Moving To 'Ready For Merging'

If it's alright, I'm moving this to 'Ready For Merging,' as I don't think it needs functional testing beyond the 'technical QA' from PR #1183.

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Sep 18, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.