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 validation model with auto-accepted (instead of forcibly sanitized) and statuses for new-accepted/new-rejected #1429

Merged
merged 30 commits into from
Sep 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
fe33e95
Rename force-sanitization to auto-accept-sanitization
westonruter Sep 13, 2018
aed7380
Split new status into new-accepted and new-rejected
westonruter Sep 14, 2018
8f6a26e
Update options related to auto-sanitization
westonruter Sep 14, 2018
58d03c9
Update term list table styling for validation errors
westonruter Sep 15, 2018
de7b04b
Eliminate forced sanitization with special with_option case; incorpor…
westonruter Sep 15, 2018
a3d442d
Add support for querying multiple term statuses at a time; use for New
westonruter Sep 16, 2018
c2193fb
Merge branch 'develop' of https://github.com/Automattic/amp-wp into u…
westonruter Sep 16, 2018
4375a40
Update display of edit post screen notices for AMP validation errors
westonruter Sep 16, 2018
5bd1ea7
Merge branch 'develop' of https://github.com/Automattic/amp-wp into u…
westonruter Sep 17, 2018
1ed50f2
Partially revert de7b04b to restore omission of forcibly-sanitized er…
westonruter Sep 17, 2018
b057e46
Merge branch 'develop' of https://github.com/Automattic/amp-wp into u…
westonruter Sep 17, 2018
e8e550e
WIP
westonruter Sep 17, 2018
06e3d4d
Introduce AMP_Validation_Error_Taxonomy::sanitize_term_status() to de…
westonruter Sep 18, 2018
c1e0658
Refactor duplicated SQL prepare for IN condition
westonruter Sep 18, 2018
e5274ac
Remove amp attribute from html element on non-AMP documents
westonruter Sep 18, 2018
d334f2a
Show preview button on amp_invalid_url screen in native mode
westonruter Sep 18, 2018
0d6781e
Add link to validation errors screen for bulk acceptance
westonruter Sep 18, 2018
9493858
Fix PHP 5.2 error with call to protected method in closure
westonruter Sep 18, 2018
70e9cbf
Add wp amp reset-site-validation command to purge site of validation …
westonruter Sep 18, 2018
85d1641
Eliminate trashing for invalid URL posts
westonruter Sep 18, 2018
3ea0df5
Add AMP_Validation_Error_Taxonomy::get_term() method to reduce code d…
westonruter Sep 19, 2018
b5fc798
Discontinue hiding validation errors that have no URL counts
westonruter Sep 19, 2018
827503b
Add ability to clear empty validation error terms
westonruter Sep 20, 2018
f8f7db0
Merge branch 'develop' of https://github.com/Automattic/amp-wp into u…
westonruter Sep 20, 2018
e8c4e8a
Update status constants which were missed in merge conflict resolution
westonruter Sep 20, 2018
273a00b
Make single-error-detail open by default
westonruter Sep 20, 2018
a272ce6
Merge branch 'develop' of https://github.com/Automattic/amp-wp into u…
westonruter Sep 21, 2018
945e763
Show accepted/rejected in new status option and disable it
westonruter Sep 21, 2018
276c5a5
Make the 'Status' column wider, to fit the icon and <select>
kienstra Sep 21, 2018
cd20106
Remove a (trivial) artifiact from a merge conflict
kienstra Sep 21, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions assets/css/amp-validation-error-taxonomy.css
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,27 @@ details[open] .details-attributes__summary::after, details.single-error-detail[o
margin-left: 10px;
}

body.taxonomy-amp_validation_error .wp-list-table .new th,
body.taxonomy-amp_validation_error .wp-list-table .new td {
background-color: #fef7f1;
}

body.taxonomy-amp_validation_error .wp-list-table .new th.check-column {
border-left: 4px solid #d54e21;
}

body.taxonomy-amp_validation_error .wp-list-table .new th.check-column input {
margin-left: 4px;
}

.row-actions .amp_validation_error_accept > a {
color: #006505;
}

.row-actions .amp_validation_error_reject > a {
color: #a00;
}

.notice.accept-reject-error {
display: flex;
}
Copy link
Contributor

@kienstra kienstra Sep 21, 2018

Choose a reason for hiding this comment

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

In a different CSS file, what do you think about changing this width to 20%:

https://github.com/Automattic/amp-wp/blob/e67234d86375cbc3454416e84365219eab1d12df/assets/css/amp-validation-single-error-url.css#L118-L120

The 'status' icon sometimes appears above the <select> when the <select> is wider. Though this happens for all of them at slightly more narrow screen widths.

singel-url

This width: 20% seems to work well in my testing so far:

errors-icons-inline

Copy link
Contributor

@kienstra kienstra Sep 21, 2018

Choose a reason for hiding this comment

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

This styling issue isn't caused by this PR, it's just more apparent when the <select> has longer text, like 'New Rejected' instead of 'Rejected'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in 276c5a5

Expand Down
2 changes: 1 addition & 1 deletion assets/css/amp-validation-single-error-url.css
Original file line number Diff line number Diff line change
Expand Up @@ -116,5 +116,5 @@ table.striped > tbody > tr.even {
}

.wp-list-table th.column-status {
width: 15%;
width: 20%;
}
25 changes: 14 additions & 11 deletions assets/js/amp-block-validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ var ampBlockValidation = ( function() { // eslint-disable-line no-unused-vars
* @return {void}
*/
handleValidationErrorsStateChange: function handleValidationErrorsStateChange() {
var currentPost, validationErrors, blockValidationErrors, noticeElement, noticeMessage, blockErrorCount, ampValidity, hasActuallyUnacceptedError;
var currentPost, validationErrors, blockValidationErrors, noticeElement, noticeMessage, blockErrorCount, ampValidity;

if ( ! module.isAMPEnabled() ) {
if ( ! module.lastStates.noticesAreReset ) {
Expand All @@ -161,15 +161,18 @@ var ampBlockValidation = ( function() { // eslint-disable-line no-unused-vars
return;
}

hasActuallyUnacceptedError = false;
currentPost = wp.data.select( 'core/editor' ).getCurrentPost();
ampValidity = currentPost[ module.data.ampValidityRestField ] || {};

// Show all validation errors which have not been explicitly acknowledged as accepted.
validationErrors = _.map(
_.filter( ampValidity.results, function( result ) {
if ( result.status !== 1 /* ACCEPTED */ ) {
hasActuallyUnacceptedError = true;
}
return result.term_status !== 1; // ACCEPTED
// @todo Show VALIDATION_ERROR_ACK_REJECTED_STATUS differently since moderated?
return (
0 /* \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS */ === result.status ||
1 /* \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS */ === result.status ||
2 /* \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_REJECTED_STATUS */ === result.status // eslint-disable-line no-magic-numbers
);
} ),
function( result ) {
return result.error;
Expand All @@ -192,8 +195,8 @@ var ampBlockValidation = ( function() { // eslint-disable-line no-unused-vars

noticeMessage = wp.i18n.sprintf(
wp.i18n._n(
'There is %s issue from AMP validation.',
'There are %s issues from AMP validation.',
'There is %s issue from AMP validation which needs review.',
'There are %s issues from AMP validation which need review.',
validationErrors.length,
'amp'
),
Expand Down Expand Up @@ -239,10 +242,10 @@ var ampBlockValidation = ( function() { // eslint-disable-line no-unused-vars
}

noticeMessage += ' ';
if ( hasActuallyUnacceptedError && ! module.data.isCanonical ) {
noticeMessage += wp.i18n.__( 'Non-accepted validation errors prevent AMP from being served, and the user will be redirected to the non-AMP version.', 'amp' );
if ( module.data.isCanonical ) {
noticeMessage += wp.i18n.__( 'Non-accepted validation errors prevent AMP from being served.', 'amp' );
} else {
noticeMessage += wp.i18n.__( 'The invalid markup will be automatically sanitized to ensure a valid AMP response is served.', 'amp' );
noticeMessage += wp.i18n.__( 'Non-accepted validation errors prevent AMP from being served, and the user will be redirected to the non-AMP version.', 'amp' );
}

noticeElement = wp.element.createElement( 'p', {}, [
Expand Down
4 changes: 2 additions & 2 deletions assets/js/amp-invalid-url-post-edit-screen.js
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ var ampInvalidUrlPostEditScreen = ( function() { // eslint-disable-line no-unuse
acceptButton.addEventListener( 'click', function() {
Array.prototype.forEach.call( document.querySelectorAll( 'select.amp-validation-error-status' ), function( select ) {
if ( select.closest( 'tr' ).querySelector( '.check-column input[type=checkbox]' ).checked ) {
select.value = 1;
select.value = '3';
component.updateSelectIcon( select );
component.addBeforeUnloadPrompt();
}
Expand All @@ -357,7 +357,7 @@ var ampInvalidUrlPostEditScreen = ( function() { // eslint-disable-line no-unuse
rejectButton.addEventListener( 'click', function() {
Array.prototype.forEach.call( document.querySelectorAll( 'select.amp-validation-error-status' ), function( select ) {
if ( select.closest( 'tr' ).querySelector( '.check-column input[type=checkbox]' ).checked ) {
select.value = 2; // @todo Update to 3 when merging with <https://github.com/Automattic/amp-wp/pull/1429>.
select.value = '2';
component.updateSelectIcon( select );
component.addBeforeUnloadPrompt();
}
Expand Down
19 changes: 19 additions & 0 deletions assets/src/amp-validation-error-detail-toggle.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,26 @@ function addToggleListener() {
} );
}

/**
* Adds classes to the rows for the amp_validation_error term list table.
*
* This is needed because \WP_Terms_List_Table::single_row() does not allow for additional
* attributes to be added to the <tr> element.
*/
function addTermListTableRowClasses() {
const rows = [ ...document.querySelectorAll( '#the-list > tr' ) ];
rows.forEach( ( row ) => {
const statusText = row.querySelector( '.column-status > .status-text' );
if ( statusText ) {
row.classList.toggle( 'new', statusText.classList.contains( 'new' ) );
row.classList.toggle( 'accepted', statusText.classList.contains( 'accepted' ) );
row.classList.toggle( 'rejected', statusText.classList.contains( 'rejected' ) );
}
} );
}

domReady( () => {
addToggleButtons();
addToggleListener();
addTermListTableRowClasses();
} );
65 changes: 64 additions & 1 deletion includes/class-amp-cli.php
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,69 @@ public function validate_site( $args, $assoc_args ) {
WP_CLI::line( sprintf( __( 'For more details, please see: %s', 'amp' ), $url_more_details ) );
}

/**
* Reset all validation data on a site.
*
* This deletes all amp_invalid_url posts and all amp_validation_error terms.
*
* ## OPTIONS
*
* [--yes]
* : Proceed to empty the site validation data without a confirmation prompt.
*
* ## EXAMPLES
*
* wp amp reset-site-validation --yes
*
* @subcommand reset-site-validation
* @param array $args Positional args. Unused.
* @param array $assoc_args Associative args.
* @throws Exception If an error happens.
*/
public function reset_site_validation( $args, $assoc_args ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great idea, this is something I've had to do manually with longer WP-CLI commands to delete the error posts and terms.

Copy link
Contributor

Choose a reason for hiding this comment

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

This worked well:

reset-site-validation

no-invalid-urls

unset( $args );
global $wpdb;
WP_CLI::confirm( 'Are you sure you want to empty all amp_invalid_url posts and amp_validation_error taxonomy terms?', $assoc_args );

// Delete all posts.
$count = $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(*) FROM $wpdb->posts WHERE post_type = %s", AMP_Invalid_URL_Post_Type::POST_TYPE_SLUG ) );
$query = $wpdb->prepare( "SELECT ID FROM $wpdb->posts WHERE post_type = %s", AMP_Invalid_URL_Post_Type::POST_TYPE_SLUG );
$posts = new WP_CLI\Iterators\Query( $query, 10000 );

$progress = WP_CLI\Utils\make_progress_bar(
/* translators: %d is the number of posts */
sprintf( __( 'Deleting %d amp_invalid_url posts...', 'amp' ), $count ),
$count
);
while ( $posts->valid() ) {
$post_id = $posts->current()->ID;
$posts->next();
wp_delete_post( $post_id, true );
$progress->tick();
}
$progress->finish();

// Delete all terms. Note that many terms should get deleted when their post counts go to zero above.
$count = $wpdb->get_var( $wpdb->prepare( "SELECT COUNT( * ) FROM $wpdb->term_taxonomy WHERE taxonomy = %s", AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ) );
$query = $wpdb->prepare( "SELECT term_id FROM $wpdb->term_taxonomy WHERE taxonomy = %s", AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG );
$terms = new WP_CLI\Iterators\Query( $query, 10000 );

$progress = WP_CLI\Utils\make_progress_bar(
/* translators: %d is the number of terms */
sprintf( __( 'Deleting %d amp_taxonomy_error terms...', 'amp' ), $count ),
$count
);
while ( $terms->valid() ) {
$term_id = $terms->current()->term_id;
$terms->next();
wp_delete_term( $term_id, AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG );
$progress->tick();
}
$progress->finish();

WP_CLI::success( 'All AMP validation data has been removed.' );
}

/**
* Gets the total number of URLs to validate.
*
Expand Down Expand Up @@ -586,7 +649,7 @@ public static function validate_and_store_url( $url, $type ) {
$validation_errors,
function( $error ) {
$validation_status = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $error );
return AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS !== $validation_status['term_status'];
return AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS !== $validation_status['term_status'];
}
) );

Expand Down
12 changes: 8 additions & 4 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -1697,11 +1697,15 @@ public static function prepare_response( $response, $args = array() ) {

self::ensure_required_markup( $dom, array_keys( $amp_scripts ) );

if ( ! AMP_Validation_Manager::should_validate_response() && $blocking_error_count > 0 && ! self::is_customize_preview_iframe() ) {

// Note the canonical check will not currently ever be met because dirty AMP is not yet supported; all validation errors will forcibly be sanitized.
if ( $blocking_error_count > 0 && ! AMP_Validation_Manager::should_validate_response() ) {
/*
* In native AMP, strip html@amp attribute to prevent GSC from complaining about a validation error
* already surfaced inside of WordPress. This is intended to not serve dirty AMP, but rather a
* non-AMP document (intentionally not valid AMP) that contains the AMP runtime and AMP components.
*/
if ( amp_is_canonical() ) {
$dom->documentElement->removeAttribute( 'amp' );
$dom->documentElement->removeAttribute( '⚡️' );

/*
* Make sure that document.write() is disabled to prevent dynamically-added content (such as added
Expand All @@ -1712,7 +1716,7 @@ public static function prepare_response( $response, $args = array() ) {
$script->appendChild( $dom->createTextNode( 'document.addEventListener( "DOMContentLoaded", function() { document.write = function( text ) { throw new Error( "[AMP-WP] Prevented document.write() call with: " + text ); }; } );' ) );
$head->appendChild( $script );
}
} else {
} elseif ( ! self::is_customize_preview_iframe() ) {
$response = esc_html__( 'Redirecting to non-AMP version.', 'amp' );

if ( $cache_response ) {
Expand Down
24 changes: 12 additions & 12 deletions includes/options/class-amp-options-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ class AMP_Options_Manager {
* @var array
*/
protected static $defaults = array(
'theme_support' => 'disabled',
'supported_post_types' => array( 'post' ),
'analytics' => array(),
'force_sanitization' => true,
'accept_tree_shaking' => true,
'disable_admin_bar' => false,
'all_templates_supported' => true,
'supported_templates' => array( 'is_singular' ),
'enable_response_caching' => true,
'theme_support' => 'disabled',
'supported_post_types' => array( 'post' ),
'analytics' => array(),
'auto_accept_sanitization' => true,
'accept_tree_shaking' => true,
'disable_admin_bar' => false,
'all_templates_supported' => true,
'supported_templates' => array( 'is_singular' ),
'enable_response_caching' => true,
);

/**
Expand Down Expand Up @@ -126,9 +126,9 @@ public static function validate_options( $new_options ) {
$options['theme_support'] = $new_options['theme_support'];
}

$options['force_sanitization'] = ! empty( $new_options['force_sanitization'] );
$options['accept_tree_shaking'] = ! empty( $new_options['accept_tree_shaking'] );
$options['disable_admin_bar'] = ! empty( $new_options['disable_admin_bar'] );
$options['auto_accept_sanitization'] = ! empty( $new_options['auto_accept_sanitization'] );
$options['accept_tree_shaking'] = ! empty( $new_options['accept_tree_shaking'] );
$options['disable_admin_bar'] = ! empty( $new_options['disable_admin_bar'] );

// Validate post type support.
$options['supported_post_types'] = array();
Expand Down
39 changes: 28 additions & 11 deletions includes/options/class-amp-options-menu.php
Original file line number Diff line number Diff line change
Expand Up @@ -257,20 +257,37 @@ public function render_validation_handling() {
<div class="notice notice-info notice-alt inline">
<p><?php esc_html_e( 'Your install is configured via a theme or plugin to automatically sanitize any AMP validation error that is encountered.', 'amp' ); ?></p>
</div>
<input type="hidden" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[force_sanitization]' ); ?>" value="<?php echo AMP_Options_Manager::get_option( 'force_sanitization' ) ? 'on' : ''; ?>">
<input type="hidden" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[auto_accept_sanitization]' ); ?>" value="<?php echo AMP_Options_Manager::get_option( 'auto_accept_sanitization' ) ? 'on' : ''; ?>">
<?php else : ?>
<div class="amp-force-sanitize-canonical notice notice-info notice-alt inline">
<p><?php esc_html_e( 'All validation errors are forcibly accepted when in native mode.', 'amp' ); ?></p>
<div class="amp-auto-accept-sanitize-canonical notice notice-info notice-alt inline">
<p><?php esc_html_e( 'All new validation errors are automatically accepted when in native mode.', 'amp' ); ?></p>
</div>
<div class="amp-force-sanitize">
<div class="amp-auto-accept-sanitize">
<p>
<label for="force_sanitization">
<input id="force_sanitization" type="checkbox" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[force_sanitization]' ); ?>" <?php checked( AMP_Options_Manager::get_option( 'force_sanitization' ) ); ?>>
<?php esc_html_e( 'Automatically accept sanitization for any AMP validation error that is encountered.', 'amp' ); ?>
<label for="auto_accept_sanitization">
<input id="auto_accept_sanitization" type="checkbox" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[auto_accept_sanitization]' ); ?>" <?php checked( AMP_Options_Manager::get_option( 'auto_accept_sanitization' ) ); ?>>
<?php esc_html_e( 'Automatically accept sanitization for any newly encountered AMP validation errors.', 'amp' ); ?>
</label>
</p>
<p class="description">
<?php esc_html_e( 'This will ensure your responses are always valid AMP but some important content may get stripped out (e.g. scripts).', 'amp' ); ?>
<?php
echo wp_kses_post(
sprintf(
/* translators: %s is URL to validation errors screen */
__( 'Existing validation errors which you have already rejected will not be modified (you may want to consider <a href="%s">bulk-accepting them</a>).', 'amp' ),
esc_url(
add_query_arg(
array(
'taxonomy' => AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG,
'post_type' => AMP_Invalid_URL_Post_Type::POST_TYPE_SLUG,
),
admin_url( 'edit-tags.php' )
)
)
)
)
?>
</p>
</div>
<?php endif; ?>
Expand Down Expand Up @@ -302,20 +319,20 @@ public function render_validation_handling() {
};

var updateTreeShakingHiddenClass = function() {
var checkbox = $( '#force_sanitization' );
var checkbox = $( '#auto_accept_sanitization' );
$( '.amp-tree-shaking' ).toggleClass( 'hidden', checkbox.prop( 'checked' ) && 'native' !== getThemeSupportMode() );
};

var updateHiddenClasses = function() {
var themeSupportMode = getThemeSupportMode();
$( '.amp-force-sanitize' ).toggleClass( 'hidden', 'native' === themeSupportMode );
$( '.amp-auto-accept-sanitize' ).toggleClass( 'hidden', 'native' === themeSupportMode );
$( '.amp-validation-field' ).toggleClass( 'hidden', 'disabled' === themeSupportMode );
$( '.amp-force-sanitize-canonical' ).toggleClass( 'hidden', 'native' !== themeSupportMode );
$( '.amp-auto-accept-sanitize-canonical' ).toggleClass( 'hidden', 'native' !== themeSupportMode );
updateTreeShakingHiddenClass();
};

$( 'input[type=radio][name="amp-options[theme_support]"]' ).change( updateHiddenClasses );
$( '#force_sanitization' ).change( updateTreeShakingHiddenClass );
$( '#auto_accept_sanitization' ).change( updateTreeShakingHiddenClass );

updateHiddenClasses();
})( jQuery );
Expand Down
Loading