Add single error url page #1418

kienstra · 2018-09-10T17:16:13Z

Implements the single URL page, like https://example.test/wp-admin/post.php?post=4414&action=edit
Begins to apply the design from Improved “single error” by URL view #1365.
As you can see, this is just the beginning of the work here. Many changes are needed:

Closes #1365.

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.

Create add_single_post_columns(), and a corresponding unit test.

Previously, this was simply an anonymous function. Also, add a simple PHPUnit test for it.

Also, make other changes, like adding test_ before the method name of a test.

…ogic Because this now uses WP_Terms_List_Table, that meta box is probably not needed.

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.

Before, this wasn't tested at all. Cover several cases, including the conditional being false, and the user not having permissions.

kienstra · 2018-09-10T17:29:28Z

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

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


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.

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

The order of the methods should reflect the order in the add_action() and add_filter() calls.

kienstra · 2018-09-10T18:45:22Z

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!

Now that the new column exists, output the type by using a helper method.

Much of the logic was already there. But add a filter to pass the query var from the POST request to the redirect.

…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.

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().

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.

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 · 2018-09-11T02:00:49Z

Current Display

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

Make some clarifications and corrections, like changing the number 4 to 'maximum'.

westonruter · 2018-09-11T03:52:58Z

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

+	 *
+	 * @var int
+	 */
+	const MAX_TERMS_ON_SINGLE_PAGE = 4;


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.

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

westonruter · 2018-09-11T03:53:52Z

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

 		) );

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


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.

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

westonruter · 2018-09-11T03:58:23Z

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

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


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.

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

This .gif shows that.

westonruter · 2018-09-11T03:59:58Z

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

 			return;
 		}

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


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

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().

westonruter · 2018-09-11T04:04:14Z

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

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


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

westonruter · 2018-09-11T04:04:53Z

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

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


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

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

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

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

render_single_url_list_table() now does not require the $_GET['taxonomy'] be set.

…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.

As Weston mentioned, this should be done in JS, so remove this method and its PHPUnit test.

Weston suggested this, so that filtering doesn't reload the page and remove any changes in accepted status.

kienstra · 2018-09-11T23:58:38Z

Filtering By Error Type With JS

kienstra · 2018-09-12T01:15:24Z

Text Formatting Applied

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

…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.

…Types' As Alberto mentioned, this reflects the actual state.

Per a recent discussion, change this circle icon to red, from blue.

@todo

…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>.

render_taxonomy_filters() isn't needed anymore So remove it, and its add_action() call. Also, move inline styling to the stylesheet.

Now that I've added non-details styling, this removes 'details' from the slug.

Like 'Showing 22 of 22 validation errors' This will show that the errors are now all displaying.

… 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.

* Remove unnecessary nested posts-filter form. * Use let/const instead of var.

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.

…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.

* Remove incorrect attribute escaping of translation exported to JS. * Restore amp-validation-error-detail-toggle to webpack config.

…dd/1365-single-error-url

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

kienstra added 9 commits September 5, 2018 21:03

Begin prototype for single error URL post.php page

e616a08

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.

Output a search box, though it will need to move above the 'Status' box

f809c3d

Refactor simple anonymous function into method with PHPUnit test

469d3d2

Create add_single_post_columns(), and a corresponding unit test.

Create method get_terms_per_page()

93b4f26

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

d91786d

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…

96141eb

…ogic Because this now uses WP_Terms_List_Table, that meta box is probably not needed.

Move template list table into method

7d501f0

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

500a6f5

Before, this wasn't tested at all. Cover several cases, including the conditional being false, and the user not having permissions.

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

9446f68

kienstra commented Sep 10, 2018

View reviewed changes

Reorder methods, add example URLs to DocBlock

37e29f6

The order of the methods should reflect the order in the add_action() and add_filter() calls.

kienstra added 6 commits September 10, 2018 14:26

Output the error type in that column

9adc5d4

Now that the new column exists, output the type by using a helper method.

Allow filtering the single error page by type

67955cb

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…

b8d8459

…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.

If there are no errors to show, do not display this message

2473214

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

838c393

Mainly use add_to_post_redirect_url(), looking for a $_POST['s'] value. If it's not empty, pass it to the redirect URL.

Minor DocBlock improvements

6324b34

Make some clarifications and corrections, like changing the number 4 to 'maximum'.

westonruter reviewed Sep 11, 2018

View reviewed changes

kienstra added 6 commits September 11, 2018 15:34

Instead of adding the taxonomy as a query var, add it to $_REQUEST

aef6e05

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

9aaffdf

render_single_url_list_table() now does not require the $_GET['taxonomy'] be set.

Remove PHP logic to support filtering and searching

c4ed164

As Weston mentioned, this should be done in JS, so remove this method and its PHPUnit test.

Filter the errors by type, using JavaScript

4a1e7f0

Weston suggested this, so that filtering doesn't reload the page and remove any changes in accepted status.

Address Travis error by removing empty line

a552168

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

3fe3ffa

…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 24 commits September 20, 2018 13:50

On clicking 'Show all,' change the error type <select> to 'All Error …

03cd2a8

…Types' As Alberto mentioned, this reflects the actual state.

Change 'Reject' icon to red

756b884

Per a recent discussion, change this circle icon to red, from blue.

Only display the 'Accept' and 'Reject' bulk action buttons if a check…

9074772

…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

5965fee

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

731f313

Now that I've added non-details styling, this removes 'details' from the slug.

Fix undefined index notices related to queried_object meta missing

f2b62e8

On clicking 'Show all', still show the notice with the count

8e427d7

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…

9db69ee

… 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.

b6695c3

* Remove unnecessary nested posts-filter form. * Use let/const instead of var.

When all of the errors are displaying, hide the notice

75f128f

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…

505a42e

…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

2ba31f9

* Remove incorrect attribute escaping of translation exported to JS. * Restore amp-validation-error-detail-toggle to webpack config.

Commit changes in order to merge remote

818259e

Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…

5f15604

…dd/1365-single-error-url

Single error view - add toggle all button for sources column

aa21e14

Merge remote and resolve conflicts

b28875c

Merge develop into add/1365-single-error-url

5b7c2e0

Single error view - remove debug statement

39cb830

Validation - tooltip fixes

f0668f7

Validation - PHPCS fixes

da5169b

Validation - PHPCS fixes

3dce980

Validation PHPUnit - restore removed line in test

8d603f4

Don't show the 'Reject' and 'Accept' action links on the single URL p…

d5b3a42

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

Set document title in addition to the heading

f0677bf

westonruter approved these changes Sep 21, 2018

View reviewed changes

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

westonruter added the Compatability Tool label Sep 21, 2018

westonruter merged commit e67234d into develop Sep 21, 2018

westonruter deleted the add/1365-single-error-url branch September 21, 2018 05:09

kienstra mentioned this pull request Sep 22, 2018

On single error URL page, integrate sources in expanded 'Error' section with 'Sources' column #1450

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add single error url page #1418

Add single error url page #1418

kienstra commented Sep 10, 2018 •

edited

Loading

kienstra Sep 10, 2018 •

edited

Loading

westonruter Sep 11, 2018

kienstra commented Sep 10, 2018

kienstra commented Sep 11, 2018

westonruter Sep 11, 2018

kienstra Sep 11, 2018

westonruter Sep 11, 2018

kienstra Sep 11, 2018

westonruter Sep 11, 2018

kienstra Sep 12, 2018

westonruter Sep 11, 2018

kienstra Sep 11, 2018

westonruter Sep 11, 2018

westonruter Sep 11, 2018

kienstra Sep 11, 2018

kienstra Sep 12, 2018 •

edited

Loading

kienstra commented Sep 11, 2018 •

edited

Loading

kienstra commented Sep 12, 2018

		) );

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

Add single error url page #1418

Add single error url page #1418

Conversation

kienstra commented Sep 10, 2018 • edited Loading

kienstra Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Sep 10, 2018

kienstra commented Sep 11, 2018

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

kienstra Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra commented Sep 11, 2018 • edited Loading

kienstra commented Sep 12, 2018

kienstra commented Sep 10, 2018 •

edited

Loading

kienstra Sep 10, 2018 •

edited

Loading

kienstra Sep 12, 2018 •

edited

Loading

kienstra commented Sep 11, 2018 •

edited

Loading