Add filters on taxonomy and post page #1373

kienstra · 2018-08-28T19:28:18Z

Request For Code Review

Hi @westonruter,
Could you please review this PR for error status and type filters?

Because amp_validation_error terms now has a type in the description, you might want to delete from your local environment all of the amp_invalid_url posts and amp_validation_error terms. And then run the WP-CLI validation again.

Adds status and type filters to the amp_invalid_url post (edit.php) and amp_validation_error taxonomy (edit-tags.php) pages. This modifies the existing status links from before.

Applies the new 'Errors by Type' terminology from the design.
Based on @westonruter's suggestion in Generate an error taxonomy for AMP error types #1360, this adds a type to the amp_validation_error term description.
There are now 4 error types, stored in the description: html_element_error, html_attribute_error, js_error, and css_error.

Closes #1360, #1363.

Like Weston suggested, add this to the amp_validation_error term description. This will aid in searching that taxonomy page by error type. At the moment, there are only 3 types: HTML, JS, and CSS.

westonruter · 2018-08-28T20:25:35Z

includes/sanitizers/class-amp-base-sanitizer.php

@@ -436,6 +436,11 @@ public function prepare_validation_error( array $error = array(), array $data =
 			if ( ! isset( $error['code'] ) ) {
 				$error['code'] = AMP_Validation_Error_Taxonomy::INVALID_ELEMENT_CODE;
 			}
+
+			if ( ! isset( $error['type'] ) ) {
+				$error['type'] = 'script' === $node->nodeName ? AMP_Validation_Error_Taxonomy::JS_ERROR_TYPE : AMP_Validation_Error_Taxonomy::HTML_ERROR_TYPE;


This also needs to be a js_error if it is an invalidate attribute and the attribute starts with on.

Good point. 3e9a81e sets a js_error for on* attributes:

For example, onclick. This will have JS in it, so this error code is more accurate. On Weston's suggestion in code review.

These were for an onload attribute, so update them for the latest commit.

Just realized you intend to do more on this PR before merging.

Borrowing heavily from: add_group_terms_clauses_filter(), filter the 'where' clause, to filter the taxonomy page by type.

kienstra · 2018-08-29T00:23:05Z

Beginnings Of The Query Var For Type Implemented

Borrowing heavily from add_group_terms_clauses_filter(), 3433791 adds a query var that can filter by error type on the 'AMP Validation Errors' taxonomy page.

But this still needs the filter UI.

For example, this URL will only show CSS errors:
https://loc.test/wp-admin/edit-tags.php?taxonomy=amp_validation_error&amp_validation_error_type=css_error

On clicking the 'Filter' button on the 'AMP Validation Errors' taxonomy page, edit-tags.php processes the POST request that this submits. Then, it redirects to a URL to display the page again. This filter callback looks for a value for VALIDATION_ERROR_TYPE_QUERY_VAR in the request. That means that the user filtered by error type, like 'js_error'. It then passes that value to the redirect URL as a query var, So that the taxonomy page will be filtered for that error type. @see AMP_Validation_Error_Taxonomy::add_error_type_clauses_filter() for the filtering of the 'where' clause, based on that query var.

Allows filtering by error_type, like only viewing JS errors. This UI now works for error_type, though work is probably needed for accepted status.

kienstra · 2018-08-29T21:04:39Z

Filtering By Type

This now has the beginning of a UI to filter by type:

This still doesn't use the new terminology, like 'Identified.' There wasn't much to change, as filtering by error type worked thanks to Weston.

Though these views are now applied in a <select> filter, this uses much of their infrastructure. Also, it uses their counts for each status. For example, if there are 0 accepted errors, this won't display the <option> for 'Accepted Error'.

Create a new constant, NO_FILTER_VALUE. This is for the existing value of -1. And add this to the whitelist: in_array().

kienstra · 2018-08-29T22:21:55Z

Filtering By Error Status

The UI to view errors by status is now in a <select> element ('All Statuses'), where it was before in <a> elements.

This uses the existing work to filter by status, and merely applies it to the <select> as designed in this Sketch file.

Change this to 'Errors by Type,' to match the Sketch design.

kienstra · 2018-08-29T22:45:43Z

New Terminology

This now applies the new terminology of 'Errors by Type,' compared to the previous 'AMP Validation Errors':

kienstra · 2018-08-30T14:50:08Z

Add Counts To Dropdowns

Based on @westonruter's suggestion, I'll add counts to the error status <option> elements, like the counts that were present in the links before.

There were counts earlier, when filtering by status was done by clicking an <a>. Thanks to Weston's work. Mainly copy the previous logic to add the counts.

kienstra · 2018-08-30T18:43:49Z

Error Counts Applied

caa2a26 applies the error counts, mainly copying the logic that was previously used for the <a> elements.

Before, there was only HTML_ERROR_TYPE. Based on comments in issues like 1361, this adds 2 types of errors for HTML.

kienstra · 2018-08-30T19:21:08Z

2 HTML Error Types

There are now 2 HTML error types:

The AMP validation error page also needs these filters, but it doesn't need all of render_taxonomy_filters(). So abstract it into 2 functions that can be reused.

Using the functions created earlier, output the filter <select> elements on this page. There's more work remaining, like ensuring the query vars work.

Mainly by using the method that Weston wrote: filter_posts_where_for_validation_error_status() Look for the query var of the error type. And if that's present, add to the WHERE clause.

This is at the top of the taxonomy page (Errors by Type), and links to the post page (Errors by URL).

kienstra · 2018-09-03T21:38:23Z

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

+					AND
+					$wpdb->terms.term_group = %s
+					AND
+					$wpdb->term_taxonomy.description LIKE %s


This addition to the WHERE clause is the same as before, except for the 2 lines above:

AND $wpdb->term_taxonomy.description LIKE %s

Maybe it'd be better to search the amp_invalid_url post content for the correct error_type, instead of searching the amp_validation_error term description, like it does here.

I think that's what you suggested here.

Yeah, but what you've done here seems fine as well.

Instead of an if block for most of the method, simply return if the condition is false. Then, the remaining if blocks aren't nested.

Before, that didn't take account of the filter for error type So if that filtered for js_error, the total count of error status was the same as if it were for 'All Error Types.' So look for the query var for the error type, and pass that to the WP_Query if it's valid.

…rors' This is how the text was when it was inside a link. Check for the current screen, and conditionally add the 'With'.

… redirect Before, the filter for the redirect URL did not accept -1, which is for 'All Error Types' and 'All Statuses'. This means that the URL sometimes did not redirect, and it sometimes showed the same results as before.

Make PHP DocBlocks more clear, especially when they needed to be updated.

kienstra · 2018-09-04T00:14:21Z

Filters on Post edit.php Page

There are now filters on the validation error post page:

…idation_error_status

westonruter · 2018-09-05T00:32:03Z

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

+		 * On the 'Errors by URL' page, the <option> text should be different.
+		 * For example, it should be 'With JS Errors' instead of 'JS Errors'.
+		 */
+		$possible_with_text = 'edit' === $screen_base ? __( 'With', 'amp' ) : '';


This is not able to be properly translated.

westonruter · 2018-09-05T00:54:17Z

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

+
+		} elseif ( 'edit' === $screen_base ) {
+			// The post page should have <option> text like 'With New Errors'.
+			$possible_with_text = esc_html__( 'With', 'amp' );


This is not properly translatable.

Unfortunately, such variations in language require duplicating the entire strings.

…ion when changing selected error type

Add a 'type' to all validation errors

0017edf

Like Weston suggested, add this to the amp_validation_error term description. This will aid in searching that taxonomy page by error type. At the moment, there are only 3 types: HTML, JS, and CSS.

kienstra changed the title ~~[WIP] Add a 'type' to all validation errors~~ [WIP] Allow filtering amp_validation_error taxonomy Aug 28, 2018

westonruter reviewed Aug 28, 2018

View reviewed changes

kienstra added 2 commits August 28, 2018 16:02

If an on* attribute is removed, make type js_error

3e9a81e

For example, onclick. This will have JS in it, so this error code is more accurate. On Weston's suggestion in code review.

In unit test assertions, change to js_error

0ae789d

These were for an onload attribute, so update them for the latest commit.

westonruter changed the title ~~[WIP] Allow filtering amp_validation_error taxonomy~~ Allow filtering amp_validation_error taxonomy Aug 28, 2018

westonruter previously approved these changes Aug 28, 2018

View reviewed changes

westonruter changed the title ~~Allow filtering amp_validation_error taxonomy~~ [WIP] Allow filtering amp_validation_error taxonomy Aug 28, 2018

Add a query var to filter by type

3433791

Borrowing heavily from: add_group_terms_clauses_filter(), filter the 'where' clause, to filter the taxonomy page by type.

westonruter added this to the v1.0 milestone Aug 29, 2018

kienstra added 2 commits August 29, 2018 15:06

Render the taxonomy filter for this taxonomy type

5f92cc2

Allows filtering by error_type, like only viewing JS errors. This UI now works for error_type, though work is probably needed for accepted status.

kienstra added 3 commits August 29, 2018 16:28

Add a filter <select> element for error type, like 'Accepted'

3d17734

This still doesn't use the new terminology, like 'Identified.' There wasn't much to change, as filtering by error type worked thanks to Weston.

Add the option value for 'All Statuses' to the whitelist

fe43d31

Create a new constant, NO_FILTER_VALUE. This is for the existing value of -1. And add this to the whitelist: in_array().

Use the new terminology for the error taxonomy page name

1e135d2

Change this to 'Errors by Type,' to match the Sketch design.

Address failed unit tests in Travis build

f1d4bea

kienstra mentioned this pull request Aug 29, 2018

Implement Dropdown Facet filters #1363

Closed

This was referenced Aug 30, 2018

Implement improved URL listing view #1362

Closed

Implement improved error listing view #1361

Closed

Restore error counts by status in <option>

caa2a26

There were counts earlier, when filtering by status was done by clicking an <a>. Thanks to Weston's work. Mainly copy the previous logic to add the counts.

kienstra added 2 commits August 30, 2018 14:11

Merge branch 'develop' into add/1360-taxonomy-error-type

7b937b1

Add error types for HTML (Element) and HTML (Attribute)

13d5caf

Before, there was only HTML_ERROR_TYPE. Based on comments in issues like 1361, this adds 2 types of errors for HTML.

kienstra added 2 commits August 30, 2018 21:10

Refactor logic in render_taxonomy_filters() into 2 methods

b74854c

The AMP validation error page also needs these filters, but it doesn't need all of render_taxonomy_filters(). So abstract it into 2 functions that can be reused.

Add filters on the 'Errors by URL' page

0397a52

Using the functions created earlier, output the filter <select> elements on this page. There's more work remaining, like ensuring the query vars work.

westonruter mentioned this pull request Aug 31, 2018

Source Filtering Discovery #1368

Closed

Filter for error type on Errors by URL page

228eb81

Mainly by using the method that Weston wrote: filter_posts_where_for_validation_error_status() Look for the query var of the error type. And if that's present, add to the WHERE clause.

kienstra mentioned this pull request Sep 3, 2018

Apply new error taxonomy page design #1394

Merged

Output the 'View errors by URL' link from the design

a567047

This is at the top of the taxonomy page (Errors by Type), and links to the post page (Errors by URL).

kienstra commented Sep 3, 2018

View reviewed changes

kienstra added 6 commits September 3, 2018 17:13

Simplify add_term_filter_query_var()

3bcfc03

Instead of an if block for most of the method, simply return if the condition is false. Then, the remaining if blocks aren't nested.

On the 'Errors by URL' page, use 'With New Errors' instead of 'New Er…

6c5c06d

…rors' This is how the text was when it was inside a link. Check for the current screen, and conditionally add the 'With'.

Remove extra empty line to address Travis failure

6c262f0

Improve inline documentation, address Travis error.

8443fd8

Make PHP DocBlocks more clear, especially when they needed to be updated.

kienstra changed the title ~~[WIP] Allow filtering amp_validation_error taxonomy~~ Allow filtering amp_validation_error taxonomy Sep 4, 2018

kienstra changed the title ~~Allow filtering amp_validation_error taxonomy~~ Add filters on taxonomy and post page Sep 4, 2018

kienstra requested a review from westonruter September 4, 2018 01:07

westonruter added 4 commits September 4, 2018 17:19

Skip showing status dropdown if there will be no options

a13c91e

Simplify logic conditional in render_post_filters

61c6095

Only add WHERE conditions when required in filter_posts_where_for_val…

364944e

…idation_error_status

Fix translatability of strings which vary 'with'

ea15a2e

westonruter reviewed Sep 5, 2018

View reviewed changes

westonruter added 2 commits September 4, 2018 18:24

Harden reading of input vars

2882262

Opt to output all status options even when empty to prevent de-select…

3b87595

…ion when changing selected error type

westonruter approved these changes Sep 5, 2018

View reviewed changes

westonruter merged commit 016b700 into develop Sep 5, 2018

westonruter deleted the add/1360-taxonomy-error-type branch September 5, 2018 02:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filters on taxonomy and post page #1373

Add filters on taxonomy and post page #1373

kienstra commented Aug 28, 2018 •

edited

Loading

westonruter Aug 28, 2018

kienstra Aug 28, 2018 •

edited

Loading

kienstra commented Aug 29, 2018 •

edited

Loading

kienstra commented Aug 29, 2018

kienstra commented Aug 29, 2018

kienstra commented Aug 29, 2018

kienstra commented Aug 30, 2018

kienstra commented Aug 30, 2018

kienstra commented Aug 30, 2018

kienstra Sep 3, 2018

kienstra Sep 4, 2018

westonruter Sep 5, 2018

kienstra commented Sep 4, 2018

westonruter Sep 5, 2018

westonruter Sep 5, 2018

Add filters on taxonomy and post page #1373

Add filters on taxonomy and post page #1373

Conversation

kienstra commented Aug 28, 2018 • edited Loading

westonruter Aug 28, 2018

Choose a reason for hiding this comment

kienstra Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra commented Aug 29, 2018 • edited Loading

kienstra commented Aug 29, 2018

kienstra commented Aug 29, 2018

kienstra commented Aug 29, 2018

kienstra commented Aug 30, 2018

kienstra commented Aug 30, 2018

kienstra commented Aug 30, 2018

kienstra Sep 3, 2018

Choose a reason for hiding this comment

kienstra Sep 4, 2018

Choose a reason for hiding this comment

westonruter Sep 5, 2018

Choose a reason for hiding this comment

kienstra commented Sep 4, 2018

westonruter Sep 5, 2018

Choose a reason for hiding this comment

westonruter Sep 5, 2018

Choose a reason for hiding this comment

kienstra commented Aug 28, 2018 •

edited

Loading

kienstra Aug 28, 2018 •

edited

Loading

kienstra commented Aug 29, 2018 •

edited

Loading