Issue 1364: Improved "Error isolation" view #1446

jacobschweitzer · 2018-09-19T15:28:39Z

Details collapsed:

Details expanded:

The design for this is very similar to the error URL index page (Errors by URL, /wp-admin/edit.php...) So use the same template from that page: edit.php.

…ror-url

westonruter · 2018-09-19T15:48:53Z

includes/validation/class-amp-invalid-url-post-type.php

-		add_filter( 'handle_bulk_actions-edit-' . self::POST_TYPE_SLUG, array( __CLASS__, 'handle_bulk_action' ), 10, 3 );
+		add_filter( 'handle_bulk_actions-edit-' . self::POST_TYPE_SLUG, array(
+			__CLASS__,
+			'handle_bulk_action',


These can be kept on the same line as in the other examples here, no?

westonruter · 2018-09-19T15:50:53Z

includes/validation/class-amp-invalid-url-post-type.php

+			$description  = json_decode( $error->description, true );
+			$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_title  = \AMP_Validation_Error_Taxonomy::get_error_title_from_code( $description['code'] );


Good. We need to do more of this. Centralizing how we convert errors into human-readable representations.

westonruter · 2018-09-19T15:51:23Z

includes/validation/class-amp-invalid-url-post-type.php

+				wp_kses_post( $output )
+			);
+
+			$error_id       = sanitize_text_field( wp_unslash( $_GET[ \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ] ) ); // WPCS: CSRF OK.


This can be sanitize_key instead of sanitize_text_field, since it is an md5 hash.

westonruter · 2018-09-19T15:52:07Z

includes/validation/class-amp-invalid-url-post-type.php

+			?>
+			<script type="text/javascript">
+				jQuery( document ).ready(function() {
+					jQuery( 'h1.wp-heading-inline' ).html( '<?php echo wp_kses_post( $heading ); ?>' );


'<?php echo wp_kses_post( $heading ); ?>' should instead be:

<?php echo wp_json_encode( $heading ); ?>

This will add the quote marks for you and escape at the same time.

westonruter · 2018-09-19T15:59:36Z

includes/validation/class-amp-invalid-url-post-type.php

+	 * 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() {


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: https://src.wordpress-develop.test/wp-admin/edit-tags.php?taxonomy=amp_validation_error&post_type=amp_invalid_url&action=amp_validation_error_accept&term_id=1292&_wpnonce=cac5f31fb4
Reject: https://src.wordpress-develop.test/wp-admin/edit-tags.php?taxonomy=amp_validation_error&post_type=amp_invalid_url&action=amp_validation_error_reject&term_id=1292&_wpnonce=8001418580

Links generated here:

https://github.com/Automattic/amp-wp/blob/808a8dabc6a879e0eff9cc4e054b86b32e62f754/includes/validation/class-amp-validation-error-taxonomy.php#L1246-L1268

And handled here:

https://github.com/Automattic/amp-wp/blob/808a8dabc6a879e0eff9cc4e054b86b32e62f754/includes/validation/class-amp-validation-error-taxonomy.php#L1508-L1546

So I suggest adding to handle_validation_error_update whatever is missing, and remove bulk_accept_reject_single_error. There may not be any changes needed because that method already redirects to the referrer.

westonruter · 2018-09-19T16:15:02Z

includes/validation/class-amp-invalid-url-post-type.php

+		 * 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.


This should be sanitize_key instead, since it is an md5 hash.

westonruter · 2018-09-19T16:16:38Z

includes/validation/class-amp-invalid-url-post-type.php

+		 */
+		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.
+			$error    = get_term_by( 'slug', $error_id, \AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG );


TODO (in #1429): Once #1429 is merged, we can use AMP_Validation_Error_Taxonomy::get_term() per 3ea0df5

westonruter · 2018-09-19T16:18:27Z

includes/validation/class-amp-invalid-url-post-type.php

+				return;
+			}
+
+			$description  = json_decode( $error->description, true );


TODO (in #1429): We should add a method to AMP_Validation_Error_Taxonomy which abstracts this away. As there is AMP_Validation_Error_Taxonomy::get_term() per 3ea0df5 we could have AMP_Validation_Error_Taxonomy::get_term_error() which returns the decoded json blob from a term ID or existing WP_Term.

westonruter · 2018-09-19T16:19:09Z

includes/validation/class-amp-validation-error-taxonomy.php

@@ -588,6 +587,7 @@ public static function add_admin_hooks() {
 		add_filter( 'posts_where', array( __CLASS__, 'filter_posts_where_for_validation_error_status' ), 10, 2 );
 		add_filter( 'handle_bulk_actions-edit-' . self::TAXONOMY_SLUG, array( __CLASS__, 'handle_validation_error_update' ), 10, 3 );
 		add_action( 'load-edit-tags.php', array( __CLASS__, 'handle_inline_edit_request' ) );
+		add_action( 'parse_query', array( __CLASS__, 'parse_term_php_query' ) );


I believe this is dead code.

westonruter · 2018-09-19T16:19:19Z

includes/validation/class-amp-validation-error-taxonomy.php

@@ -631,6 +631,10 @@ public static function add_admin_hooks() {

 		// Override the columns displayed for the validation error terms.
 		add_filter( 'manage_edit-' . self::TAXONOMY_SLUG . '_columns', function( $old_columns ) {
+			if ( 'term' === get_current_screen()->base ) {


I believe this is dead code.

westonruter · 2018-09-19T19:11:54Z

includes/validation/class-amp-invalid-url-post-type.php

+			);
+			?>
+			<script type="text/javascript">
+				jQuery( document ).ready(function() {


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 ); ?> ); } );

westonruter · 2018-09-19T20:15:23Z

@jacobschweitzer It's still failing here:

* Hide buttons entirely if sanitization is forced. * Use flexbox to lay out the buttons. * Add more information to explain what accept/reject mean.

kienstra and others added 10 commits September 4, 2018 20:14

Begin prototype for single error taxonomy term.php page

732c9b2

The design for this is very similar to the error URL index page (Errors by URL, /wp-admin/edit.php...) So use the same template from that page: edit.php.

Merge branch 'develop' into add/1364-single-error-url

c7165fa

Merge remote-tracking branch 'origin/develop' into add/1364-single-er…

b429804

…ror-url

Merge branch 'develop' into add/1364-single-error-url

2f63c01

Add notice to single error page with error info and change column names

31c26bd

Add accept + reject buttons for single error page

4ebb3d2

Fix typo

c0c4727

Add two helper methods for getting text we need

0c0d74f

Change single error page heading and re-factor

9233de0

Merge remote-tracking branch 'origin/develop' into add/1364-single-er…

76af143

…ror-url

jacobschweitzer mentioned this pull request Sep 19, 2018

Improved “Error Isolation” view #1364

Closed

6 tasks

jacobschweitzer requested a review from westonruter September 19, 2018 15:40

PHPCS fixes

0c58339

westonruter requested changes Sep 19, 2018

View reviewed changes

jacobschweitzer added 2 commits September 19, 2018 15:50

Code review updates and remove bulk update method

e0a7f34

Check post_type isset before checking value

b1df4ba

westonruter reviewed Sep 19, 2018

View reviewed changes

jacobschweitzer added 2 commits September 19, 2018 16:17

Improve jQuery code style

23ac4f8

Update check for handling inline request

88afc91

jacobschweitzer and others added 2 commits September 19, 2018 18:47

Make the checks easier to read and fix isset error

f76bd58

Hide accept/reject button if already Accepted or Rejected

5770277

* Hide buttons entirely if sanitization is forced. * Use flexbox to lay out the buttons. * Add more information to explain what accept/reject mean.

westonruter approved these changes Sep 20, 2018

View reviewed changes

westonruter merged commit 377995f into develop Sep 20, 2018

westonruter deleted the add/1364-single-error-url branch September 20, 2018 00:30

westonruter added this to the v1.0 milestone Sep 20, 2018

westonruter added the Compatability Tool label Sep 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1364: Improved "Error isolation" view #1446

Issue 1364: Improved "Error isolation" view #1446

jacobschweitzer commented Sep 19, 2018 •

edited by westonruter

Loading

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter Sep 19, 2018

westonruter commented Sep 19, 2018

Issue 1364: Improved "Error isolation" view #1446

Issue 1364: Improved "Error isolation" view #1446

Conversation

jacobschweitzer commented Sep 19, 2018 • edited by westonruter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Sep 19, 2018

jacobschweitzer commented Sep 19, 2018 •

edited by westonruter

Loading