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

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

Merged
merged 30 commits into from Sep 21, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Member

westonruter commented Sep 14, 2018

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.

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

westonruter added some commits Sep 13, 2018

Update term list table styling for validation errors
* 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…
…ate VALIDATION_ERROR_NEW_ACCEPTED_STATUS

@westonruter westonruter force-pushed the update/validation-statuses branch from 3e94b3a to de7b04b Sep 15, 2018

westonruter added some commits Sep 16, 2018

Merge branch 'develop' of https://github.com/Automattic/amp-wp into u…
…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
* 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 westonruter force-pushed the update/validation-statuses branch from 3d13afd to 4375a40 Sep 16, 2018

westonruter added some commits Sep 17, 2018

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

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

@westonruter westonruter requested a review from amedina Sep 18, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 18, 2018

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

westonruter added some commits Sep 18, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 20, 2018

@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):

image

westonruter added some commits Sep 20, 2018

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

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

.row-actions .amp_validation_error_reject > a {
color: #a00;
}

.notice.accept-reject-error {
display: flex;
}

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

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.

singel-url

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

errors-icons-inline

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

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

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

Applied in 276c5a5

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

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

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

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

This worked well:

reset-site-validation

no-invalid-urls

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

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

Great idea to abstract this into a method.

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

@kienstra

kienstra Sep 21, 2018

Collaborator

This looks good:

notice-reject-bulk-action

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

@kienstra

kienstra Sep 21, 2018

Collaborator

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

after-clear-empty

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

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

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

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

Fixed with cd20106

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 21, 2018

Errors Appear As Expected

Paired Mode, 'Automatically accept sanitization' unchecked:

paired-new-rejected

Paired Mode, 'Automatically accept sanitization' checked:

paired-auto

Native Mode:
native-mode

@kienstra

kienstra approved these changes Sep 21, 2018 edited

Approved

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

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

kienstra added some commits Sep 21, 2018

Make the 'Status' column wider, to fit the icon and <select>
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
This was in a DocBlock, so it didn't have any effect.
But this removes the <<<<HEAD.

@westonruter westonruter merged commit d97f353 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 update/validation-statuses branch Sep 21, 2018

@kienstra kienstra referenced this pull request Sep 25, 2018

Closed

Implement improved URL listing view #1362

1 of 19 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment