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 single error url page #1418

Merged
merged 93 commits into from Sep 21, 2018

Conversation

Projects
None yet
5 participants
@kienstra
Collaborator

kienstra commented Sep 10, 2018

current-state-url

Closes #1365.

kienstra added some commits Sep 6, 2018

Begin prototype for single error URL post.php page
This is really more like an edit-tags.php page,
like the 'Errors by Type' page.
In that it has a WP_Terms_List_Table of
validation error terms.
There are several filters needed for this,
including for parse_term_query.
Refactor simple anonymous function into method with PHPUnit test
Create add_single_post_columns(),
and a corresponding unit test.
Create method get_terms_per_page()
Previously, this was simply an anonymous function.
Also, add a simple PHPUnit test for it.
Add a unit test for parse_post_php_term_query()
Also, make other changes,
like adding test_ before the method name of a test.
Remove print_validation_errors_meta_box(), though might borrow some l…
…ogic

Because this now uses WP_Terms_List_Table,
that meta box is probably not needed.
Move template list table into method
Before, that method simply required that template.
Make some more changes to the code,
including copying more from edit-tags.
Also, modify get_terms_per_page()
so that it only affects the post.php page.
Add unit test for render_single_url_list_table
Before, this wasn't tested at all.
Cover several cases,
including the conditional being false,
and the user not having permissions.
* are encoded.
* @return string $link The filtered link.
*/
public static function add_taxonomy_to_edit_link( $link, $post_id, $context ) {

This comment has been minimized.

@kienstra

kienstra Sep 10, 2018

Collaborator

Hi @westonruter, what do you think of this approach?

This changes the URL of this single URL page from:
https://example.test/wp-admin/post.php?post=4416&action=edit
to:
https://example.test/wp-admin/post.php?post=4416&action=edit&taxonomy=amp_validation_error

This is because the WP_Terms_List_Table that displays the error terms needs access to the taxonomy:

$taxonomy = $this->screen->taxonomy;

The best way I could find so far to make this taxonomy available is by adding it as a query var to that URL, like how it's available in the query var of the taxonomy page (#1361):

https://loc.test/wp-admin/edit-tags.php?taxonomy=amp_validation_error&post_type=amp_invalid_url

It looks like WP_Screen gets that taxonomy from the query var, and then it becomes available in WP_Terms_List_Table.

This comment has been minimized.

@westonruter

westonruter Sep 11, 2018

Member

See note above about using AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG directly. You could just do:

$_REQUEST['taxonomy'] = AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG;

Just do this before $wp_list_table = _get_list_table( 'WP_Terms_List_Table' );.

Reorder methods, add example URLs to DocBlock
The order of the methods should reflect the order in the add_action() and add_filter() calls.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 10, 2018

Question About Overall Approach

Hi @westonruter,
Hope you had a great weekend.

When you have a chance, could you please look at this and let me know what you think of the overall approach? Especially in #1418 (comment)

As you can see, this is WIP. But your feedback would really help to make sure this is on the right path.

Thanks, Weston!

kienstra added some commits Sep 10, 2018

Output the error type in that column
Now that the new column exists,
output the type by using a helper method.
Allow filtering the single error page by type
Much of the logic was already there.
But add a filter to pass the query var
from the POST request to the redirect.
Add the <tr> with 'Showing 4 of x validation errors' at the top of th…
…e list table

Using the script that Weston began,
add a method for outputting this text.
It still needs to have the text translated,
and to use dynamic numbers.
Use dynamic values and translated text for the 'Showing errors' text
This moves some of the logic from
enqueue_edit_post_screen_scripts()
into
add_edit_post_inline_script().

Because the dynamic value for the script
depends on the WP_Terms_List_Table,
it's now called inside
render_single_url_list_table().
If there are no errors to show, do not display this message
This isn't common,
but it's possible to filter for 'JS Errors', for example,
and have no errors display.
In that case, there's no need for the message.
Allow searching for errors
Mainly use add_to_post_redirect_url(),
looking for a $_POST['s'] value.
If it's not empty, pass it to the redirect URL.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 11, 2018

Current Display

Though there's still much work remaining, here are the latest changes:

single-url-page

Minor DocBlock improvements
Make some clarifications and corrections,
like changing the number 4 to 'maximum'.
*
* @var int
*/
const MAX_TERMS_ON_SINGLE_PAGE = 4;

This comment has been minimized.

@westonruter

westonruter Sep 11, 2018

Member

Shouldn't this be infinite? On an amp_invalid_url post edit screen, there should be no limit to the number of validation errors shown.

This comment has been minimized.

@kienstra

kienstra Sep 11, 2018

Collaborator

Sure, 834b141#diff-57518db0811ee918fee17d27459ed4ecR1320 uses PHP_INT_MAX as the maximum number of errors that can display.

) );
if ( isset( $_REQUEST['s'] ) && strlen( $_REQUEST['s'] ) ) { // WPCS: CSRF OK.

This comment has been minimized.

@westonruter

westonruter Sep 11, 2018

Member

This condition should not be done in PHP. The logic would need to be done in JS because JS needs to be used for the filtering.

This comment has been minimized.

@kienstra

kienstra Sep 11, 2018

Collaborator

Sure, 834b141#diff-57518db0811ee918fee17d27459ed4ecL1278 removes this PHP handling of search term(s).

$data = array(
'l10n' => array(
'unsaved_changes' => __( 'You have unsaved changes. Are you sure you want to leave?', 'amp' ),
),
);
if ( self::$total_errors_for_url ) {

This comment has been minimized.

@westonruter

westonruter Sep 11, 2018

Member

I don't think this condition here is right. The filtering of the list of validation errors needs to be done completely with JavaScript, as otherwise the changes that a user may make to a validation error status will be lost whenever changing the filters.

So all of the validation errors should be shown, and then when filters are interacted with it should be JavaScript that removes/hides rows that do not relate to the filtered selection. And at this point the row with this message would be dynamically added with JS.

This comment has been minimized.

@kienstra

kienstra Sep 12, 2018

Collaborator

Thanks, these commits apply the filtering with JavaScript, and the message only shows after filtering.

This .gif shows that.

return;
}
$url = self::get_url_from_post( $post );
$taxonomy = sanitize_key( $_GET['taxonomy'] ); // WPCS: CSRF OK.

This comment has been minimized.

@westonruter

westonruter Sep 11, 2018

Member

I don't think you need to populate this in the $_GET variable. You can just use AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG directly.

This comment has been minimized.

@kienstra

kienstra Sep 11, 2018

Collaborator

Thanks, good idea.

If it's alright, aef6e05#diff-57518db0811ee918fee17d27459ed4ecR1345 sets the $_REQUEST['taxonomy'] on an admin_init callback, as it looks like set_current_screen() runs before AMP_Invalid_URL_Post_Type::render_single_url_list_table().

* are encoded.
* @return string $link The filtered link.
*/
public static function add_taxonomy_to_edit_link( $link, $post_id, $context ) {

This comment has been minimized.

@westonruter

westonruter Sep 11, 2018

Member

See note above about using AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG directly. You could just do:

$_REQUEST['taxonomy'] = AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG;

Just do this before $wp_list_table = _get_list_table( 'WP_Terms_List_Table' );.

* @param int $post_id The post ID.
* @return int $url The filtered redirect URL.
*/
public static function add_to_post_redirect_url( $url, $post_id ) {

This comment has been minimized.

@westonruter

westonruter Sep 11, 2018

Member

As noted above, all of this needs to be done in JS instead.

This comment has been minimized.

@kienstra

kienstra Sep 11, 2018

Collaborator

Thanks, c4ed164 removes this PHP logic. I'll reimplement it in JS.

This comment has been minimized.

@kienstra

kienstra Sep 12, 2018

Collaborator

These commits allow filtering, using JavaScript. I still need to implement the search box with JS.

kienstra added some commits Sep 11, 2018

Instead of adding the taxonomy as a query var, add it to $_REQUEST
As Weston suggested.
It would be ideal to do this in render_single_url_list_table(),
but set_current_screen() looks to run before that, and that needs access to the $_REQUEST['taxonomy'].
Correct failed unit test, updating with latest change
render_single_url_list_table() now does not
require the $_GET['taxonomy'] be set.
Remove PHP logic for displaying search term(s), change number of post…
…s to PHP_INT_MAX

As Weston mentioned, the 'Search Errors' box
needs to be implemented in JS.
So this removes the PHP logic.
Also, it changes the max number of errors
that can display on a single page from 4 to PHP_INT_MAX.
Remove PHP logic to support filtering and searching
As Weston mentioned, this should be done in JS,
so remove this method and its PHPUnit test.
Filter the errors by type, using JavaScript
Weston suggested this,
so that filtering doesn't reload the page
and remove any changes in accepted status.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 11, 2018

Filtering By Error Type With JS

filtering-by-js

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 12, 2018

Text Formatting Applied

The text in the 'Error' and 'Details' columns is formatted per the design:

text-formatted-designs

Match text formatting of the desings for the 'Error' and 'Details' co…
…lumns

In the 'Error' column, use ucfirst() to capitalize the error,
and use str_replace() to convert underscores to spaces.
In the 'Details' column, output the 'parent_name',
if it exists.

kienstra and others added some commits Sep 20, 2018

Change 'Reject' icon to red
Per a recent discussion,
change this circle icon to red, from blue.
Only display the 'Accept' and 'Reject' bulk action buttons if a check…
…box is checked

These only apply if a box is checked.
On unchecking the last checked box,
hide these again.
@todo: fix styling issue with the error type filter <select>.
Fix styling issue in 'Accept' and 'Reject' buttons, remove dead code
render_taxonomy_filters() isn't needed anymore
So remove it, and its add_action() call.
Also, move inline styling to the stylesheet.
Rename CSS file to reflect that it applies to the whole page
Now that I've added non-details styling,
this removes 'details' from the slug.
On clicking 'Show all', still show the notice with the count
Like 'Showing 22 of 22 validation errors'

This will show that the errors are now all displaying.
Prevent showing the 'Invalid URL' page header before it's overwritten…
… in JS

Before, the header of 'Invalid URL' displayed first,
and then a JS file changed it.
Now, this will be empty until the JS file applies it.
Update Accept/Reject buttons to update select dropdown, not submit form.
* Remove unnecessary nested posts-filter form.
* Use let/const instead of var.
When all of the errors are displaying, hide the notice
This 'Showing x of y' errors
notice won't apply if all are showing.
So on selecting 'All Error Types',
or clicking 'Show all',
this notice will be hidden.
Because the show all button might be newly created, do a new query fo…
…r it

conditionallyCreateShowAllButton() runs right before
a block that might remove its 'hidden' class.
So instead of using the stored showAllButton,
run a new query for it.
Fix presentation of toggle button in narrower screen resolutions
* Remove incorrect attribute escaping of translation exported to JS.
* Restore amp-validation-error-detail-toggle to webpack config.
Don't show the 'Reject' and 'Accept' action links on the single URL p…
…age.

They will only show in the error taxonomy index page,
but no on this single URL page.

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

@westonruter westonruter merged commit e67234d into develop Sep 21, 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/1365-single-error-url branch Sep 21, 2018

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