Issue 1364: Improved "Error isolation" view #1446
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,13 @@ class AMP_Invalid_URL_Post_Type { | |
| */ | ||
| const VALIDATION_ERRORS_META_BOX = 'amp_validation_errors'; | ||
|
|
||
| /** | ||
| * The name of the action which allows bulk accepting or rejecting of an error across multiple URLs. | ||
| * | ||
| * @var string | ||
| */ | ||
| const BULK_ACCEPT_REJECT_ACTION = 'amp_bulk_accept_reject'; | ||
|
|
||
| /** | ||
| * Registers the post type to store URLs with validation errors. | ||
| * | ||
|
|
@@ -143,34 +150,43 @@ public static function add_admin_hooks() { | |
| add_filter( 'manage_' . self::POST_TYPE_SLUG . '_posts_columns', array( __CLASS__, 'add_post_columns' ) ); | ||
| add_action( 'manage_posts_custom_column', array( __CLASS__, 'output_custom_column' ), 10, 2 ); | ||
| add_filter( 'bulk_actions-edit-' . self::POST_TYPE_SLUG, array( __CLASS__, 'filter_bulk_actions' ), 10, 2 ); | ||
| add_filter( 'handle_bulk_actions-edit-' . self::POST_TYPE_SLUG, array( | ||
| __CLASS__, | ||
| 'handle_bulk_action', | ||
| ), 10, 3 ); | ||
| add_action( 'admin_notices', array( __CLASS__, 'print_admin_notice' ) ); | ||
| add_action( 'admin_action_' . self::VALIDATE_ACTION, array( __CLASS__, 'handle_validate_request' ) ); | ||
| add_action( 'post_action_' . self::UPDATE_POST_TERM_STATUS_ACTION, array( | ||
| __CLASS__, | ||
| 'handle_validation_error_status_update', | ||
| ) ); | ||
| add_action( 'admin_menu', array( __CLASS__, 'add_admin_menu_new_invalid_url_count' ) ); | ||
| add_filter( 'post_row_actions', array( __CLASS__, 'filter_post_row_actions' ), 10, 2 ); | ||
| add_filter( sprintf( 'views_edit-%s', self::POST_TYPE_SLUG ), array( __CLASS__, 'filter_table_views' ) ); | ||
| add_filter( 'bulk_post_updated_messages', array( __CLASS__, 'filter_bulk_post_updated_messages' ), 10, 2 ); | ||
|
|
||
| // Hide irrelevant "published" label in the invalid URL post list. | ||
| add_filter( 'post_date_column_status', function ( $status, $post ) { | ||
| if ( AMP_Invalid_URL_Post_Type::POST_TYPE_SLUG === get_post_type( $post ) ) { | ||
| $status = ''; | ||
| } | ||
|
|
||
| return $status; | ||
| }, 10, 2 ); | ||
|
|
||
| // Prevent query vars from persisting after redirect. | ||
| add_filter( 'removable_query_args', function ( $query_vars ) { | ||
| $query_vars[] = 'amp_actioned'; | ||
| $query_vars[] = 'amp_taxonomy_terms_updated'; | ||
| $query_vars[] = AMP_Invalid_URL_Post_Type::REMAINING_ERRORS; | ||
| $query_vars[] = 'amp_urls_tested'; | ||
| $query_vars[] = 'amp_validate_error'; | ||
|
|
||
| return $query_vars; | ||
| } ); | ||
|
|
||
| add_action( 'admin_init', array( __CLASS__, 'bulk_accept_reject_single_error' ) ); | ||
| } | ||
| /** | ||
| * Enqueue style. | ||
| */ | ||
|
|
@@ -717,6 +733,13 @@ public static function add_post_columns( $columns ) { | |
| $columns['date'] = esc_html__( 'Last Checked', 'amp' ); | ||
| } | ||
|
|
||
| if ( ! empty( $_GET[ \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ] ) ) { // WPCS: CSRF OK. | ||
| unset( $columns['error_status'], $columns[ AMP_Validation_Error_Taxonomy::REMOVED_ELEMENTS ], $columns[ AMP_Validation_Error_Taxonomy::REMOVED_ATTRIBUTES ] ); | ||
| $columns[ AMP_Validation_Error_Taxonomy::SOURCES_INVALID_OUTPUT ] = esc_html__( 'Sources', 'amp' ); | ||
| $columns['date'] = esc_html__( 'Last Checked', 'amp' ); | ||
| $columns['title'] = esc_html__( 'URL', 'amp' ); | ||
| } | ||
|
|
||
| return $columns; | ||
| } | ||
|
|
||
|
|
@@ -1026,6 +1049,89 @@ public static function print_admin_notice() { | |
| wp_kses_post( $message ) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Adds notices to the single error page. | ||
| * 1. Notice with detailed error information in an expanding box. | ||
| * 2. Notice with accept and reject buttons. | ||
| */ | ||
| if ( ! empty( $_GET[ \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ] ) && isset( $_GET['post_type'] ) && self::POST_TYPE_SLUG === $_GET['post_type'] ) { // WPCS: CSRF OK. | ||
| $error_id = sanitize_text_field( wp_unslash( $_GET[ \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ] ) ); // WPCS: CSRF OK. | ||
|
There was a problem hiding this comment. This should be |
||
| $error = get_term_by( 'slug', $error_id, \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ); | ||
|
|
||
| if ( ! $error ) { | ||
| return; | ||
| } | ||
|
|
||
| $description = json_decode( $error->description, true ); | ||
|
There was a problem hiding this comment. |
||
| $sanitization = \AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $description ); | ||
| $status_text = \AMP_Validation_Error_Taxonomy::get_status_text_with_icon( $sanitization['term_status'], $sanitization['forced'] ); | ||
| $error_code = isset( $description['code'] ) ? $description['code'] : 'error'; | ||
| $error_title = \AMP_Validation_Error_Taxonomy::get_error_title_from_code( $error_code ); | ||
|
|
||
| $output = ''; | ||
| foreach ( $description as $desc_name => $desc_info ) { | ||
| if ( ! is_array( $desc_info ) ) { | ||
| $output .= sprintf( '<li><span class="details-attributes__title">%s:</span><br/><span class="details-attributes__value">%s</span></li>', $desc_name, $desc_info ); | ||
| } else { | ||
| $output .= sprintf( '<ul class="secondary-details-array"><span class="details-attributes__title">%s:</span><br/>', $desc_name ); | ||
| foreach ( $desc_info as $sec_desc_info_name => $sec_desc_info ) { | ||
| $output .= sprintf( '<li><span class="details-attributes__attr">%s:</span><br/><span class="details-attributes__value">%s</span></li>', $sec_desc_info_name, $sec_desc_info ); | ||
| } | ||
| $output .= '</ul>'; | ||
| } | ||
| } | ||
|
|
||
| printf( | ||
| '<div class="notice"><details class="single-error-detail"><summary class="single-error-detail-summary"><strong>%s</strong></summary><ul>%s</ul></details></div>', | ||
| esc_html( $error_title ), | ||
| wp_kses_post( $output ) | ||
| ); | ||
|
|
||
| $base_url = admin_url( | ||
| add_query_arg( | ||
| array( | ||
| 'post_type' => self::POST_TYPE_SLUG, | ||
| \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG => $error_id, | ||
| ), | ||
| 'edit.php' | ||
| ) | ||
| ); | ||
| $accept_all_url = add_query_arg( | ||
| array( | ||
| self::BULK_ACCEPT_REJECT_ACTION => \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPT_ACTION, | ||
| ), | ||
| $base_url | ||
| ); | ||
| $reject_all_url = add_query_arg( | ||
|
There was a problem hiding this comment. As per below, I believe this should be using the same logic that generates the row actions for the terms: https://github.com/Automattic/amp-wp/blob/808a8dabc6a879e0eff9cc4e054b86b32e62f754/includes/validation/class-amp-validation-error-taxonomy.php#L1246-L1268 |
||
| array( | ||
| self::BULK_ACCEPT_REJECT_ACTION => \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECT_ACTION, | ||
| ), | ||
| $base_url | ||
| ); | ||
|
|
||
| printf( | ||
| '<div class="notice accept-reject-error"><p>%s</p><a class="button" href="%s">%s</a><a class="button button-primary" href="%s">%s</a></div>', | ||
| esc_html__( 'Accept or Reject this error for all shown use cases', 'amp' ), | ||
| esc_url( $reject_all_url ), | ||
| esc_html__( 'Reject', 'amp' ), | ||
| esc_url( $accept_all_url ), | ||
| esc_html__( 'Accept', 'amp' ) | ||
| ); | ||
|
|
||
| $heading = sprintf( | ||
| '%s: <code>%s</code>%s', | ||
| esc_html( $error_title ), | ||
| esc_html( $description['node_name'] ), | ||
| wp_kses_post( $status_text ) | ||
| ); | ||
| ?> | ||
| <script type="text/javascript"> | ||
| jQuery( document ).ready(function() { | ||
|
There was a problem hiding this comment. Not a big deal, but there is a more idiomatic way to write this: jQuery( function( $ ) {
$( 'h1.wp-heading-inline' ).html( <?php echo wp_json_encode( $heading ); ?> );
} ); |
||
| jQuery( 'h1.wp-heading-inline' ).html( '<?php echo wp_kses_post( $heading ); ?>' ); | ||
|
There was a problem hiding this comment.
This will add the quote marks for you and escape at the same time. |
||
| }); | ||
| </script> | ||
| <?php | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1922,4 +2028,37 @@ public static function filter_bulk_post_updated_messages( $messages, $bulk_count | |
|
|
||
| return $messages; | ||
| } | ||
|
|
||
| /** | ||
| * Bulk Accept Reject Single Error | ||
| * This adds functionality to the single error page where there are two buttons which allow accepting or rejecting an error. | ||
| */ | ||
| public static function bulk_accept_reject_single_error() { | ||
|
There was a problem hiding this comment. I don't think we need this method as we can re-use another. There is already an action for accepting or rejecting a term. For example, the row actions for a term:
Accept: Links generated here: And handled here: So I suggest adding to |
||
| if ( ! empty( $_GET[ \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ] ) && isset( $_GET['post_type'] ) && self::POST_TYPE_SLUG === $_GET['post_type'] && isset( $_GET[ self::BULK_ACCEPT_REJECT_ACTION ] ) ) { // WPCS: CSRF OK. | ||
|
|
||
| $type = sanitize_text_field( wp_unslash( $_GET[ self::BULK_ACCEPT_REJECT_ACTION ] ) ); // WPCS: CSRF OK. | ||
| if ( \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPT_ACTION !== $type && \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECT_ACTION !== $type ) { | ||
| return; | ||
| } | ||
|
|
||
| $error_id = sanitize_text_field( wp_unslash( $_GET[ \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ] ) ); // WPCS: CSRF OK. | ||
| $term = get_term_by( 'slug', $error_id, \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ); | ||
| if ( ! $term ) { | ||
| return; | ||
| } | ||
|
|
||
| $redirect_to = add_query_arg( | ||
| array( | ||
| 'amp_validation_error' => $error_id, | ||
| 'post_type' => self::POST_TYPE_SLUG, | ||
| ), | ||
| admin_url( 'edit.php' ) | ||
| ); | ||
| $redirect_url = \AMP_Validation_Error_Taxonomy::handle_validation_error_update( $redirect_to, $type, array( $term->term_id ) ); | ||
| if ( $redirect_url !== $redirect_to ) { | ||
| wp_safe_redirect( $redirect_url ); | ||
| exit; | ||
| } | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be kept on the same line as in the other examples here, no?