Update validation model with auto-accepted (instead of forcibly sanitized) and statuses for new-accepted/new-rejected #1429

westonruter · 2018-09-14T05:32:58Z

We currently have 3 term statuses:

new = 0
accepted = 1
rejected = 2

So what we talked about was combining new with both accepted and rejected to then have 4:

new rejected = 0 = 0b00
new accepted = 1 = 0b01
ack accepted = 3 = 0b11
ack rejected = 2 = 0b10

In this arrangement, we have a nice feature: a bitmask of 0b10 will match all acknowledged (not-new) validation errors, whereas a bitmask of 0b01 will match all accepted validation errors (regardless of whether new or acknowledged).

New-accepted and new-rejected validation errors are styled similar to unmoderated comments.
The force-sanitization option is replaced with an auto-accept sanitization option. Validation errors can still be rejected after the fact.
Validation errors will be marked as new-accepted if the auto-accept sanitization checkbox is checked, or the user has native mode enabled.
The preview button is restored on the Invalid URL screen in native mode.
When a validation error is explicitly marked as rejected in native mode, then the amp attribute is removed from the html element.
Replace trashing Invalid URLs with permanent deletion.
Add wp amp reset-site-validation command to purge site of validation data.
Stop hiding empty validation error terms and add an “Clear Empty” button to remove those with no URLs associated with them.

Todo

Figure out why validation errors get marked as new even though previously they were accepted, when re-validating a trashed invalid URL.
Add a button that allows the user to delete all terms with 0 count. This explicit request to delete 0-count terms could then be preceded by a call to wp_update_term_count_now() for each term to make sure it is actually 0 before we go ahead and delete them.
Make sure updating invalid URL post won't modify new validation errors if no selection is made.
Update filters so that “Accepted” matches both new and acknowledged, and the same for “Rejected” (which should be linked to from auto-accept checkbox label). New can match both new-accepted and new-rejected still. (To explore at another time, see update/error-status-dropdown-options branch)
~~Delete taxonomy terms entirely when an invalid URL post is deleted. When the count goes to zero.~~

* Add unapproved comment styling for new validation rows. * Show new accepted/rejected. * Let reject link be red and accept link be green.

…ate VALIDATION_ERROR_NEW_ACCEPTED_STATUS

…pdate/validation-statuses # Conflicts: # assets/src/amp-validation-error-detail-toggle.js # tests/validation/test-class-amp-invalid-url-post-type.php

* Show notice when there are rejected validation errors which are new or acknowledged. * Update messages based on simplified logic for serving non-AMP native with errors.

…pdate/validation-statuses

…rors See #1413 Also fixes eslint error

…pdate/validation-statuses # Conflicts: # assets/src/amp-validation-error-detail-toggle.js # tests/validation/test-class-amp-invalid-url-post-type.php

…-duplicate logic

westonruter · 2018-09-18T06:30:48Z

@amedina This is ready for initial testing according to the revised debugging workflow. PTAL and check if it aligns with your expectations.

…data

…uplication

westonruter · 2018-09-20T06:55:09Z

@amedina There is now a “Clear Empty” button that appears when there are validation errors with no associated URLs (i.e. they have all been forgotten):

…pdate/validation-statuses

kienstra · 2018-09-21T14:49:52Z

assets/css/amp-validation-error-taxonomy.css

+.row-actions .amp_validation_error_reject > a {
+	color: #a00;
+}
+
 .notice.accept-reject-error {
 	display: flex;
 }


In a different CSS file, what do you think about changing this width to 20%:

https://github.com/Automattic/amp-wp/blob/e67234d86375cbc3454416e84365219eab1d12df/assets/css/amp-validation-single-error-url.css#L118-L120

The 'status' icon sometimes appears above the <select> when the <select> is wider. Though this happens for all of them at slightly more narrow screen widths.

This width: 20% seems to work well in my testing so far:

This styling issue isn't caused by this PR, it's just more apparent when the <select> has longer text, like 'New Rejected' instead of 'Rejected'.

Applied in 276c5a5

kienstra · 2018-09-21T15:07:53Z

includes/class-amp-cli.php

+	 * @param array $assoc_args Associative args.
+	 * @throws Exception If an error happens.
+	 */
+	public function reset_site_validation( $args, $assoc_args ) {


Great idea, this is something I've had to do manually with longer WP-CLI commands to delete the error posts and terms.

This worked well:

kienstra · 2018-09-21T15:25:42Z

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

+	 * @param int[] $groups Term groups.
+	 * @return string SQL.
+	 */
+	public static function prepare_term_group_in_sql( $groups ) {


Great idea to abstract this into a method.

kienstra · 2018-09-21T15:42:35Z

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

+					/* translators: %s is number of errors rejected */
+					_n(
+						'Rejected %s error. It will continue to block related URLs from being served as AMP.',
+						'Rejected %s errors. They will continue to block related URLs from being served as AMP.',


This looks good:

kienstra · 2018-09-21T15:49:51Z

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

-		if ( $message ) {
-			printf( '<div class="notice notice-success is-dismissible"><p>%s</p></div>', esc_html( $message ) );
+		// Show success message for clearing empty terms.
+		if ( isset( $_GET[ self::VALIDATION_ERRORS_CLEARED_QUERY_VAR ] ) ) { // WPCS: csrf ok.


This looks good. After 'forgetting' some errors and clicking 'Clear empty,' this appeared:

kienstra · 2018-09-21T16:01:25Z

tests/validation/test-class-amp-validation-error-taxonomy.php

@@ -513,6 +705,38 @@ public function test_add_error_type_clauses_filter() {
 		$this->assertEquals( $initial_clauses, apply_filters( $tested_filter, $initial_clauses, $taxonomies ) );
 	}

+	/**
+<<<<<<< HEAD


It looks like this is still here from resolving the (many) merge conflicts.

Fixed with cd20106

kienstra · 2018-09-21T16:46:02Z

Errors Appear As Expected

Paired Mode, 'Automatically accept sanitization' unchecked:

Paired Mode, 'Automatically accept sanitization' checked:

Native Mode:

kienstra

Approved

Hi @westonruter,
This looks really good, and the errors appeared as expected.

There are some minor points here, one related to styling.

There was an issue in the width of this before, but it's more apparent with longer text like 'New Accepted'.

This was in a DocBlock, so it didn't have any effect. But this removes the <<<<HEAD.

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

westonruter added 5 commits September 14, 2018 17:09

Rename force-sanitization to auto-accept-sanitization

fe33e95

Split new status into new-accepted and new-rejected

aed7380

Update options related to auto-sanitization

8f6a26e

Update term list table styling for validation errors

58d03c9

* Add unapproved comment styling for new validation rows. * Show new accepted/rejected. * Let reject link be red and accept link be green.

Eliminate forced sanitization with special with_option case; incorpor…

de7b04b

…ate VALIDATION_ERROR_NEW_ACCEPTED_STATUS

westonruter force-pushed the update/validation-statuses branch from 3e94b3a to de7b04b Compare September 15, 2018 05:02

westonruter added 3 commits September 15, 2018 21:13

Add support for querying multiple term statuses at a time; use for New

a3d442d

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

c2193fb

…pdate/validation-statuses # Conflicts: # assets/src/amp-validation-error-detail-toggle.js # tests/validation/test-class-amp-invalid-url-post-type.php

Update display of edit post screen notices for AMP validation errors

4375a40

* Show notice when there are rejected validation errors which are new or acknowledged. * Update messages based on simplified logic for serving non-AMP native with errors.

westonruter force-pushed the update/validation-statuses branch from 3d13afd to 4375a40 Compare September 16, 2018 19:08

westonruter added 8 commits September 16, 2018 20:08

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

5bd1ea7

…pdate/validation-statuses

Partially revert de7b04b to restore omission of forcibly-sanitized er…

1ed50f2

…rors See #1413 Also fixes eslint error

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

b057e46

…pdate/validation-statuses # Conflicts: # assets/src/amp-validation-error-detail-toggle.js # tests/validation/test-class-amp-invalid-url-post-type.php

WIP

e8e550e

Introduce AMP_Validation_Error_Taxonomy::sanitize_term_status() to de…

06e3d4d

…-duplicate logic

Refactor duplicated SQL prepare for IN condition

c1e0658

Remove amp attribute from html element on non-AMP documents

e5274ac

Show preview button on amp_invalid_url screen in native mode

d334f2a

westonruter requested a review from amedina September 18, 2018 06:29

westonruter added 6 commits September 17, 2018 23:38

Add link to validation errors screen for bulk acceptance

0d6781e

Fix PHP 5.2 error with call to protected method in closure

9493858

Add wp amp reset-site-validation command to purge site of validation …

70e9cbf

…data

Eliminate trashing for invalid URL posts

85d1641

Add AMP_Validation_Error_Taxonomy::get_term() method to reduce code d…

3ea0df5

…uplication

Discontinue hiding validation errors that have no URL counts

b5fc798

This was referenced Sep 19, 2018

Issue 1364: Improved "Error isolation" view #1446

Merged

Add single error url page #1418

Merged

Add ability to clear empty validation error terms

827503b

westonruter added 5 commits September 20, 2018 00:01

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

f8f7db0

…pdate/validation-statuses

Update status constants which were missed in merge conflict resolution

e8c4e8a

Make single-error-detail open by default

273a00b

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

a272ce6

…pdate/validation-statuses

Show accepted/rejected in new status option and disable it

945e763

westonruter changed the title ~~[WIP] Update validation model with auto-accepted (instead of forcibly sanitized) and statuses for new-accepted/new-rejected~~ Update validation model with auto-accepted (instead of forcibly sanitized) and statuses for new-accepted/new-rejected Sep 21, 2018

westonruter requested a review from kienstra September 21, 2018 06:16

kienstra reviewed Sep 21, 2018

View reviewed changes

kienstra approved these changes Sep 21, 2018

View reviewed changes

kienstra added 2 commits September 21, 2018 14:42

Make the 'Status' column wider, to fit the icon and <select>

276c5a5

There was an issue in the width of this before, but it's more apparent with longer text like 'New Accepted'.

Remove a (trivial) artifiact from a merge conflict

cd20106

This was in a DocBlock, so it didn't have any effect. But this removes the <<<<HEAD.

westonruter added the Compatability Tool label Sep 21, 2018

westonruter merged commit d97f353 into develop Sep 21, 2018

westonruter deleted the update/validation-statuses branch September 21, 2018 19:04

This was referenced Sep 23, 2018

Periodically delete amp_validation_error terms which no longer appear on the site #1415

Closed

Allow previewing changes to validation error statuses in native mode or paired with auto-sanitization #1422

Closed

kienstra mentioned this pull request Sep 25, 2018

Implement improved URL listing view #1362

Closed

19 tasks

westonruter mentioned this pull request Oct 3, 2018

Fix detection for whether AMP is available to allow new accepted errors #1485

Merged

kienstra mentioned this pull request Oct 3, 2018

Address an issue in the Gutenberg notice when a block has an error #1484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update validation model with auto-accepted (instead of forcibly sanitized) and statuses for new-accepted/new-rejected #1429

Update validation model with auto-accepted (instead of forcibly sanitized) and statuses for new-accepted/new-rejected #1429

westonruter commented Sep 14, 2018 •

edited

Loading

westonruter commented Sep 18, 2018

westonruter commented Sep 20, 2018

kienstra Sep 21, 2018 •

edited

Loading

kienstra Sep 21, 2018 •

edited

Loading

kienstra Sep 21, 2018

kienstra Sep 21, 2018

kienstra Sep 21, 2018

kienstra Sep 21, 2018

kienstra Sep 21, 2018

kienstra Sep 21, 2018

kienstra Sep 21, 2018

kienstra Sep 21, 2018

kienstra commented Sep 21, 2018

kienstra left a comment •

edited

Loading

Update validation model with auto-accepted (instead of forcibly sanitized) and statuses for new-accepted/new-rejected #1429

Update validation model with auto-accepted (instead of forcibly sanitized) and statuses for new-accepted/new-rejected #1429

Conversation

westonruter commented Sep 14, 2018 • edited Loading

Todo

westonruter commented Sep 18, 2018

westonruter commented Sep 20, 2018

kienstra Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra Sep 21, 2018 • edited Loading

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 commented Sep 21, 2018

kienstra left a comment • edited Loading

Choose a reason for hiding this comment

westonruter commented Sep 14, 2018 •

edited

Loading

kienstra Sep 21, 2018 •

edited

Loading

kienstra Sep 21, 2018 •

edited

Loading

kienstra left a comment •

edited

Loading