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

Refactor storage of validation errors to use taxonomy terms #1093

Merged
merged 58 commits into from
May 29, 2018

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Apr 21, 2018

This will include the plumbing to be able to achieve dynamically allowing AMP responses based on validation status (#1087). It should include the ability to acknowledge a certain validation error as being ignored (#1003, #1063).

  • Add “term statuses” (via term_group) for acknowledged and ignored, with ability for user to change them.
  • Add ability to acknowledge/ignore errors from the Invalid URL post edit screen.
  • Move sources to be stored in the amp_invalid_url posts themselves. This addresses issue with validation errors having dynamic sources, such as post_id.
  • Capture inline script contents in validation errors. Fixes Capture script tag contents in AMP validator errors #1031.
  • Merge enqueued_script validation error into removed_element validation error, combining sources into latter.
  • Detect when critical validation errors are present after post-processor runs, and refresh with non-AMP version.
  • Refactor prototype code into better-organized class(es).
  • Accepted validation errors for CSS need to be applied when pulling parsed CSS from cache.
  • When serving dirty AMP, ensure document.write is disabled for sake of amp-live-list.
  • Rewrite unit tests.

Fixes #1003.
Fixes #1087.

@westonruter westonruter added this to the v1.0 milestone Apr 21, 2018
@westonruter westonruter self-assigned this Apr 21, 2018
@westonruter
Copy link
Member Author

Current view of the new validation errors, with bulk actions for acknowledge/ignore and inline actions to do the same:

image

The URLs column indicates the number of URLs that have the error, which links to a new “Invalid Pages” screen (formerly Validation Status) with the URLs listed:

image

@westonruter
Copy link
Member Author

There is also a new admin menu item for validation errors which is separate from the Invalid Pages screen:

image

@westonruter
Copy link
Member Author

There is now filtering of the validation errors by their status (new, acknowledged, or ignored):

image

The admin menu item for the Validation Errors also now has a “moderation count” for the number of new validation errors that are present (which haven't been acknowledged or ignored yet):

image

@westonruter
Copy link
Member Author

The “At a Glance” widget now shows the number of URLs that contain any new errors (which haven't yet been acknowledged or ignored):

image

Likewise, the number appearing with the “Invalid Pages” admin menu item also is now the number of invalid URLs which contain at least one new validation error.

You can also now filter Invalid Pages (URLs) by those which contain at least one new, acknowledged, or ignored error:

image

And lastly, when listing out the validation errors for a given invalid URL it will now show whether a given error has been acknowledged or ignored, or if it is new. If it is new then the error details are initially expanded. Otherwise, they are initially collapsed:

image

Next step here is to allow a user to switch the status of a given validation error inline when looking at the list of validation errors for a given invalid URL.

@ThierryA
Copy link
Collaborator

ThierryA commented Apr 23, 2018

This is awesome @westonruter, such a great step forward! First round feedback:

  1. It would be good to have a way to go back to the "blocking status". So for example a user might mark an issue as "Acknowledged" or "Ignored" but later realize there is still an issue in which case they might revert the status. Right now that status is called "New" but the naming could change.
  2. It would be good to have a way to preview a page with validation error in AMP mode which would help to debug the issues reported.
  3. What is the difference between "Acknowledged" and "Ignored" from a functional perspective?
  4. 4b34bdc breaks bulk actions, not only preventing the users from bulk deleting but also from doing any other bulk actions when validation errors still have associated invalid URLs.

@westonruter
Copy link
Member Author

@ThierryA

  1. It would be good to have a way to go back to the "blocking status". So for example a user might mark an issue as "Acknowledged" or "Ignored" but later realize there is still an issue in which case they might revert the status. Right now that status is called "New" but the naming could change.
  2. What is the difference between "Acknowledged" and "Ignored" from a functional perspective?

Actually, “acknowledge” has the same effect as “new”: both are blockers. The difference is that “new” is like unread. The idea is that maybe a user would like to default the behavior of new validation errors to be ignored by default instead of acknowledged by default. I tried to initially explain what acknowledging means in aria-label attributes:

  • Acknowledge: Acknowledging an error marks it as read. AMP validation errors prevent a URL from being served as AMP.
  • Ignore: Ignoring an error prevents it from blocking a URL from being served as AMP.

I'm not totally sold on the naming of “acknowledge” and “ignore” but I couldn't think of anything better.

It would be good to have a way to preview a page with validation error in AMP mode which would help to debug the issues reported.

Agreed. I've got this jotted down as a todo item.

@westonruter
Copy link
Member Author

westonruter commented Apr 23, 2018

4b34bdc breaks bulk actions, not only preventing the users from bulk deleting but also from doing any other bulk actions when validation errors still have associated invalid URLs.

You're right. Unfortunately WP_Terms_List_Table::column_cb() uses delete_term for whether to show the checkbox. Need to figure out an alternative.


Update: Fixed in ac8fd64 (via workaround).

@westonruter
Copy link
Member Author

Validation errors are now sorted by creation date, with default being in descending order, with the creation date column being sortable in ascending order by clicking the header:

image

Validation errors are also sortable by frequency (the number of URLs that have the error occurring):

image

@westonruter
Copy link
Member Author

Now see the error status at a glance in the list of invalid AMP URLs:

image

@westonruter
Copy link
Member Author

In the same way that there are inline row actions to acknowledge or ignore a given validation error on the edit terms screen, so too there are now the same action links provided when listing the validation errors on a given invalid AMP URL post:

image

@westonruter
Copy link
Member Author

The edit post screen for an Invalid AMP Page now displays its actual URL and as a link so you can visit the URL to review the post on the frontend. (The view URL will need to be getting an amp_forced query param when the auto-disable functionality is triggered due to a URL having new/acknowledged errors which block it from being served as AMP.) This screen also now features the counts for the various error types present on the URL:

image

The URL shown in the post list table is also now shortened to remove the scheme and host name:

image

@DavidCramer
Copy link
Contributor

@westonruter I took some time today going over the whole validation system, and it's crazy cool!

Some feedback.
I was initially confused as @ThierryA pointed out, as to the functional differences between Acknowledged and Ignore. My first thought was ignore tells the AMP plugin to ignore it and leave the invalid markup on the page. Where is Acknowledged meant that you are acknowledging it being removed.
Now reading your explanation to @ThierryA's question on functionality, I understand the difference. I still think the confirmation message is a little confusing though.

A suggestion on terminology. It seams that Ignore and Acknowledge is similar. Perhaps Dismiss would be better with an option like Dismiss for 1 hour, 1 day, 1 week, indefinitely. Kinda like, saying "ye thanks, I'm still working on it, remind me later."

The confirmation messages also made me feel a little confused Ignored 1 error. It will no longer block related URLs from being served as AMP. to me this told me that the pages will now be AMP valid. but was confused when I checked the page and it was valid since the invalid markup was being stripped.

$node->parentNode->removeChild( $node );
}
return $should_remove;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In canonical AMP, whenever $should_remove is false, then this needs to trigger dirty AMP mode, where amp attribute is removed from the html element.

…ource comments

* Introduce amp_preserve_source_comments query param.
* Rename locate_sources arg to should_locate_sources.
* Remove debug flag for preventing sanitization on all validation errors.
* Rename add_validation_hooks method to add_validation_error_sourcing.
Fix additional static analysis issues
Allow passing nonce instead of auth cookie to with amp_validate requests.
@westonruter
Copy link
Member Author

With 69055c0, it is now made explicit that accepting a validation error on one URL will apply to the same error that occurs on other URLs :

image

Let statuses be changed in bulk on invalid URL screen
@westonruter
Copy link
Member Author

I've added the rudimentary ability to preview changes to the statuses of validation errors:

image

Clicking “Preview” takes you to a URL like:

https://example.com/2018/05/25/foo/?amp_validate=47cd785376&amp_validation_error_status%5Bddecaba254d1ffcd034c9ab7add5196f%5D=1

You can see how the URL behaves with any number of validation errors accepted (sanitized).

Again, the UI for the invalid URLs screen and validation errors screen needs to be completely redone. The UI code in place now should be considered a prototype. Ideally there would be REST API endpoints added for managing the validation error status and invalid URL posts, and the current screens for managing them should be refactored to use a JS-based interface, such as with React.

@ThierryA Given these known issues with the UI and the outstanding need to update unit tests (which will likely change greatly with changes to the UI) I suggest getting this merged sooner rather than later, and then we can follow up with additional PRs to iterate on what is included here.

@westonruter westonruter changed the title [WIP] Refactor storage of validation errors to use taxonomy terms Refactor storage of validation errors to use taxonomy terms May 28, 2018
* Use wp_json_encode() instead or serialize().
* Ensure window opened for previewing validation error term status changes to have consistent name.
* Use VALIDATION_ERROR_TERM_STATUS_QUERY_VAR constant.
@westonruter
Copy link
Member Author

I've updated the design of the Invalid AMP Page screen to work a lot more like a regular edit post screen, with buttons inside the normal side metabox:

image

Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Hi @westonruter,
Great work here. This PR looks good, having done a sanity-check review. The UI for "Accepted" and "Rejected" also looks much better

There are a few points, but no blocker from my perspective.

@@ -89,7 +89,9 @@ class AMP_Autoloader {
'AMP_DOM_Utils' => 'includes/utils/class-amp-dom-utils',
'AMP_HTML_Utils' => 'includes/utils/class-amp-html-utils',
'AMP_Image_Dimension_Extractor' => 'includes/utils/class-amp-image-dimension-extractor',
'AMP_Validation_Utils' => 'includes/utils/class-amp-validation-utils',
'AMP_Validation_Manager' => 'includes/validation/class-amp-validation-manager',
Copy link
Contributor

@kienstra kienstra May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice how this is now organized in the includes/validation/ directory.

);

$cache_key = md5( $stylesheet . serialize( $cache_impacting_options ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize
$cache_key = md5( $stylesheet . wp_json_encode( $cache_impacting_options ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice how using wp_json_encode() removes the need to ignore a phpcs violation.

$result[] = esc_html( sprintf(
/* translators: %s is count */
__( '❓ New: %s', 'amp' ),
number_format_i18n( $counts[ AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS ] )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of number_format_i18(), I didn't know about that.

// Guard against Kses from corrupting content by adding post_content after content_save_pre filter applies.
$insert_post_content = function( $post_data ) use ( $placeholder, $post_content ) {
$should_supply_post_content = (
isset( $post_data['post_content'] )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this line could also check $post_data['post_type'], instead of the isset( $post_data['post_type'] ) check 4 lines below:

isset( $post_data['post_content'], $post_data['post_type'] )

@@ -1,12 +1,12 @@
<?php
Copy link
Contributor

@kienstra kienstra May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to eventually move these tests to separate files, like test-class-amp-validation-manager.php.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, absolutely agree. We'll follow up with new PR to add/update the tests and also to polish the UI. 👍

@westonruter
Copy link
Member Author

It seems there are networking connectivity issues being experienced by GitHub or Travis that are causing jobs to fail. I'm going to go ahead and merge this even though the build isn't finishing.

@westonruter westonruter merged commit 888ac10 into develop May 29, 2018
@westonruter westonruter deleted the update/validation branch July 5, 2018 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants