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

Add filters on taxonomy and post page #1373

Merged
merged 30 commits into from Sep 5, 2018

Conversation

Projects
None yet
2 participants
@kienstra
Copy link
Collaborator

kienstra commented Aug 28, 2018

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.

filters-on-both-pages

  • Applies the new 'Errors by Type' terminology from the design.
  • Based on @westonruter's suggestion in #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.

Add a 'type' to all validation errors
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 kienstra changed the title [WIP] Add a 'type' to all validation errors [WIP] Allow filtering amp_validation_error taxonomy Aug 28, 2018

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

@westonruter

westonruter Aug 28, 2018

Member

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

This comment has been minimized.

@kienstra

kienstra Aug 28, 2018

Collaborator

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

type-js-error

kienstra added some commits Aug 28, 2018

If an on* attribute is removed, make type js_error
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
These were for an onload attribute,
so update them for the latest commit.

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

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

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

Add a query var to filter by type
Borrowing heavily from:
add_group_terms_clauses_filter(),
filter the 'where' clause,
to filter the taxonomy page by type.
@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Aug 29, 2018

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.

validation-error-taxonomy

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

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

kienstra added some commits Aug 29, 2018

Filter the term redirect location, to possibly add a query var
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.
Render the taxonomy filter for this taxonomy type
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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Aug 29, 2018

Filtering By Type

This now has the beginning of a UI to filter by type:
trimmed-gif-validation-g

kienstra added some commits Aug 29, 2018

Add a filter <select> element for error type, like 'Accepted'
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.
Remove views for filtering taxonomy, but reuse their counts
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'.
Add the option value for 'All Statuses' to the whitelist
Create a new constant, NO_FILTER_VALUE.
This is for the existing value of -1.
And add this to the whitelist: in_array().
@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Aug 29, 2018

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.

error-status-g

Use the new terminology for the error taxonomy page name
Change this to 'Errors by Type,'
to match the Sketch design.
@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Aug 29, 2018

New Terminology

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

new-terminology

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Aug 30, 2018

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.

add-error-counts

Restore error counts by status in <option>
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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Aug 30, 2018

Error Counts Applied

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

count-each-status

kienstra added some commits Aug 30, 2018

Add error types for HTML (Element) and HTML (Attribute)
Before, there was only HTML_ERROR_TYPE.
Based on comments in issues like 1361,
this adds 2 types of errors for HTML.
@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Aug 30, 2018

2 HTML Error Types

There are now 2 HTML error types:

2-error-types-html

kienstra added some commits Aug 31, 2018

Refactor logic in render_taxonomy_filters() into 2 methods
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
Using the functions created earlier,
output the filter <select> elements on this page.
There's more work remaining,
like ensuring the query vars work.
Filter for error type on Errors by URL page
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.
Output the 'View errors by URL' link from the design
This is at the top of the taxonomy page (Errors by Type),
and links to the post page (Errors by URL).
AND
$wpdb->terms.term_group = %s
AND
$wpdb->term_taxonomy.description LIKE %s

This comment has been minimized.

@kienstra

kienstra Sep 3, 2018

Collaborator

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

AND
$wpdb->term_taxonomy.description LIKE %s

This comment has been minimized.

@kienstra

kienstra Sep 4, 2018

Collaborator

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.

This comment has been minimized.

@westonruter

westonruter Sep 5, 2018

Member

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

kienstra added some commits Sep 3, 2018

Simplify add_term_filter_query_var()
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.
Make the error status counts accurate on the Errors by URL page
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.
On the 'Errors by URL' page, use 'With New Errors' instead of 'New Er…
…rors'

This is how the text was when it was inside a link.
Check for the current screen,
and conditionally add the 'With'.
Fix an issue where filtering by 'All Error Types' wasn't reflected in…
… 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.
Improve inline documentation, address Travis error.
Make PHP DocBlocks more clear,
especially when they needed to be updated.
@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 4, 2018

Filters on Post edit.php Page

There are now filters on the validation error post page:
filters-on-post-page

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

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

@kienstra kienstra requested a review from westonruter Sep 4, 2018

westonruter added some commits Sep 5, 2018

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

@westonruter

westonruter Sep 5, 2018

Member

This is not able to be properly translated.

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

This comment has been minimized.

@westonruter

westonruter Sep 5, 2018

Member

This is not properly translatable.

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

westonruter added some commits Sep 5, 2018

@westonruter westonruter merged commit 016b700 into develop Sep 5, 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/1360-taxonomy-error-type branch Sep 5, 2018

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