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

Issue 1364: Improved "Error isolation" view #1446

Merged
merged 17 commits into from Sep 20, 2018

Conversation

Projects
None yet
3 participants
@jacobschweitzer
Collaborator

jacobschweitzer commented Sep 19, 2018

Fixes #1364.

Details collapsed:

image

Details expanded:

image

@jacobschweitzer jacobschweitzer referenced this pull request Sep 19, 2018

Closed

Improved “Error Isolation” view #1364

5 of 6 tasks complete

@jacobschweitzer jacobschweitzer requested a review from westonruter Sep 19, 2018

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',

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

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

$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'] );

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

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

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

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

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

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

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

'<?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.

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

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

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:

image

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.

* 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 comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

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

*/
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 );

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

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

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

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

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.

@@ -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' ) );

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

I believe this is dead code.

@@ -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 ) {

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

I believe this is dead code.

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

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

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

This comment has been minimized.

Member

westonruter commented Sep 19, 2018

@jacobschweitzer It's still failing here:

image

jacobschweitzer and others added some commits Sep 19, 2018

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

@westonruter westonruter merged commit 377995f into develop Sep 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/1364-single-error-url branch Sep 20, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment