Skip to content
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.

Sign up for GitHub

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

Prevent ACK state of non-modified validation errors from being lost; update validation error icons #4382

Merged
merged 51 commits into from
May 4, 2020
Merged

Prevent ACK state of non-modified validation errors from being lost; update validation error icons #4382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
4860ea1
Only update the status of the validation error if it changes status (…
pierlon Mar 13, 2020
51ce770
Ensure bitwise comparison is done between integers
pierlon Mar 13, 2020
5188d56
Merge branch 'develop' into fix/4725-ack-validation-errors
pierlon Apr 10, 2020
06bd910
Use dashicons for icons in admin bar
pierlon Apr 11, 2020
4dbd3e5
Use dashicons for "CSS Usage" menu item
pierlon Apr 12, 2020
f93c9a8
Replace accepted & rejected icons within Compatibility Tool
pierlon Apr 12, 2020
3557314
Replace icons used in stylesheet summary
pierlon Apr 12, 2020
9c90d82
Add 'Approved' column to allow acknowledgement of validation errors
pierlon Apr 13, 2020
5c4eb8d
Change validation error status color & row state on update of inputs
pierlon Apr 13, 2020
06aba49
Fix tests
pierlon Apr 13, 2020
26973ae
Add dashicons as a dependency for amp-icons
pierlon Apr 13, 2020
67b2a3c
Fix lint errors
pierlon Apr 13, 2020
a2f7a90
Rebrand 'new errors' to 'unapproved errors'
pierlon Apr 13, 2020
e49d1ea
Add warning of unapproved invalid markup to metabox
pierlon Apr 13, 2020
f257126
Add actions to approve/disapprove errors on error index page
pierlon Apr 14, 2020
e4fd56c
Fix lint errors
pierlon Apr 14, 2020
45d21bd
Fix phpstan error
pierlon Apr 14, 2020
aaac827
Count rejected errors as well
pierlon Apr 14, 2020
2e0e4f4
Merge branch 'develop' into fix/4725-ack-validation-errors
pierlon Apr 23, 2020
b1d7a9f
Fix tests
pierlon Apr 23, 2020
64e47f3
Apply suggestions from Alain
pierlon Apr 24, 2020
f4595ca
Fix lint errors
pierlon Apr 25, 2020
c615be8
Centralize icon markup
pierlon Apr 25, 2020
8769996
Do not set color for link icon in admin bar
pierlon Apr 25, 2020
ff8e065
Update description for invalid disapproved markup
pierlon Apr 25, 2020
04eacac
Instantiate Icon via named constructors
pierlon Apr 25, 2020
bd2dbea
Fix non-static call
pierlon Apr 25, 2020
fc6aa7f
Fix test
pierlon Apr 25, 2020
d1bbb4e
Prevent unneeded whitespace when building attributes string
pierlon Apr 25, 2020
2ade4b2
Change "Approved" terminology to "Reviewed"
pierlon Apr 29, 2020
93b08b2
Only allow 'Delete' and 'Details' actions for error index screen
pierlon Apr 30, 2020
0730b75
Show notice on single validation error page to delete error
pierlon Apr 30, 2020
b85a2e9
Test delete row action
pierlon Apr 30, 2020
335e863
Add 'admin-bar' as dependency to bypass tree-shaking of CSS
pierlon May 1, 2020
49dc805
Merge branch 'develop' into fix/4725-ack-validation-errors
pierlon May 2, 2020
145bb49
Prevent refresh if review checkbox is unsaved
pierlon May 3, 2020
9f4de59
Move error status from heading to error details
pierlon May 3, 2020
b27a961
Increase margin between AMP icon and text
pierlon May 3, 2020
33f19be
Increase font size for icon
pierlon May 3, 2020
34e3e80
Action review from Weston
pierlon May 3, 2020
5346025
Compare against 0 instead of type casting to bool
pierlon May 3, 2020
f571c17
Inject error status into details when adding admin notice
pierlon May 3, 2020
4cee087
Fix test
pierlon May 3, 2020
740a7ef
Escape status text
pierlon May 3, 2020
53c7a49
Fix tests failing for WP trunk
pierlon May 3, 2020
07b411e
Remove obsolete Fonts class; use Icon::warning for CronBasedBackgroun…
westonruter May 3, 2020
c35663d
Reject old accept/reject variable name
westonruter May 3, 2020
0f22a81
Simply construction of escaped term delete button
westonruter May 3, 2020
ec71188
Use HTML syntax for empty tags
westonruter May 3, 2020
c4cb9ab
Ensure amp-icons are enqueued on the specific screens that need them
westonruter May 4, 2020
566aa7e
Fix tests after change in PWA v0.5-alpha
westonruter May 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 44 additions & 45 deletions includes/validation/class-amp-validated-url-post-type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,51 +735,48 @@ public static function store_validation_errors( $validation_errors, $url, $args
$term_data = AMP_Validation_Error_Taxonomy::prepare_validation_error_taxonomy_term( $data );
$term_slug = $term_data['slug'];

if ( ! isset( $terms[ $term_slug ] ) ) {
schlessera marked this conversation as resolved.
Show resolved Hide resolved

// Not using WP_Term_Query since more likely individual terms are cached and wp_insert_term() will itself look at this cache anyway.
$term = AMP_Validation_Error_Taxonomy::get_term( $term_slug );
if ( ! ( $term instanceof WP_Term ) ) {
/*
* The default term_group is 0 so that is AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS.
* If sanitization auto-acceptance is enabled, then the term_group will be updated below.
*/
$r = wp_insert_term( $term_slug, AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG, wp_slash( $term_data ) );
if ( is_wp_error( $r ) ) {
continue;
}
$term_id = $r['term_id'];
update_term_meta( $term_id, 'created_date_gmt', current_time( 'mysql', true ) );

/*
* When sanitization is forced by filter, make sure the term is created with the filtered status.
* For some reason, the wp_insert_term() function doesn't work with the term_group being passed in.
*/
$sanitization = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $data );
if ( 'with_filter' === $sanitization['forced'] ) {
$term_data['term_group'] = $sanitization['status'];
wp_update_term(
$term_id,
AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG,
[
'term_group' => $sanitization['status'],
]
);
} elseif ( AMP_Validation_Manager::is_sanitization_auto_accepted( $data ) ) {
$term_data['term_group'] = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS;
wp_update_term(
$term_id,
AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG,
[
'term_group' => $term_data['term_group'],
]
);
}

$term = get_term( $term_id );
// Not using WP_Term_Query since more likely individual terms are cached and wp_insert_term() will itself look at this cache anyway.
pierlon marked this conversation as resolved.
Show resolved Hide resolved
$term = AMP_Validation_Error_Taxonomy::get_term( $term_slug );
if ( ! ( $term instanceof WP_Term ) ) {
/*
* The default term_group is 0 so that is AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS.
* If sanitization auto-acceptance is enabled, then the term_group will be updated below.
*/
$r = wp_insert_term( $term_slug, AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG, wp_slash( $term_data ) );
if ( is_wp_error( $r ) ) {
continue;
}
$term_id = $r['term_id'];
update_term_meta( $term_id, 'created_date_gmt', current_time( 'mysql', true ) );

/*
* When sanitization is forced by filter, make sure the term is created with the filtered status.
* For some reason, the wp_insert_term() function doesn't work with the term_group being passed in.
*/
$sanitization = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $data );
if ( 'with_filter' === $sanitization['forced'] ) {
$term_data['term_group'] = $sanitization['status'];
wp_update_term(
$term_id,
AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG,
[
'term_group' => $sanitization['status'],
]
);
} elseif ( AMP_Validation_Manager::is_sanitization_auto_accepted( $data ) ) {
$term_data['term_group'] = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS;
wp_update_term(
$term_id,
AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG,
[
'term_group' => $term_data['term_group'],
]
);
}
$terms[ $term_slug ] = $term;

$term = get_term( $term_id );
}
$terms[ $term_slug ] = $term;

$stored_validation_errors[] = compact( 'term_slug', 'data' );
}
Expand Down Expand Up @@ -1673,7 +1670,10 @@ public static function handle_validation_error_status_update() {
continue;
}
$term_group = AMP_Validation_Error_Taxonomy::sanitize_term_status( $status );
if ( null !== $term_group && $term_group !== $term->term_group ) {

$status_changed = ( $term->term_group | AMP_Validation_Error_Taxonomy::ACKNOWLEDGED_VALIDATION_ERROR_BIT_MASK ) !== $term_group;
pierlon marked this conversation as resolved.
Show resolved Hide resolved

if ( null !== $term_group && $status_changed && $term_group !== $term->term_group ) {
$updated_count++;
wp_update_term( $term->term_id, AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG, compact( 'term_group' ) );
}
Expand All @@ -1690,7 +1690,6 @@ public static function handle_validation_error_status_update() {
// Re-check the post after the validation status change.
if ( $updated_count > 0 ) {
$validation_results = self::recheck_post( $post->ID );
// @todo For WP_Error case, see <https://github.com/ampproject/amp-wp/issues/1166>.
if ( ! is_wp_error( $validation_results ) ) {
$args[ self::REMAINING_ERRORS ] = count(
array_filter(
Expand Down
33 changes: 26 additions & 7 deletions tests/php/validation/test-class-amp-validated-url-post-type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1068,22 +1068,39 @@ public function test_handle_validation_error_status_update() {
$_REQUEST[ AMP_Validated_URL_Post_Type::UPDATE_POST_TERM_STATUS_ACTION . '_nonce' ] = wp_create_nonce( AMP_Validated_URL_Post_Type::UPDATE_POST_TERM_STATUS_ACTION );
AMP_Validated_URL_Post_Type::handle_validation_error_status_update(); // No-op since no post.

$error = [ 'code' => 'foo' ];
$errors = [
[ 'code' => 'foo' ],
[ 'code' => 'bar' ],
];

// Mark error with code 'bar' as being accepted.
add_filter(
'amp_validation_error_sanitized',
static function ( $sanitized, $error ) {
return isset( $error['code'] ) && 'bar' === $error['code'] ? true : null;
},
10,
2
);

$invalid_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors(
[ $error ],
$errors,
home_url( '/' )
);
add_filter(
'pre_http_request',
static function() use ( $error ) {
static function() use ( $errors ) {
return [
'body' => wp_json_encode(
[
'results' => [
[
'sanitized' => false,
'error' => $error,
'error' => $errors[0],
],
[
'sanitized' => true,
'error' => $errors[1],
],
],
]
Expand All @@ -1094,10 +1111,11 @@ static function() use ( $error ) {

$post = get_post( $invalid_url_post_id );

$errors = AMP_Validated_URL_Post_Type::get_invalid_url_validation_errors( $invalid_url_post_id );
$validation_errors = AMP_Validated_URL_Post_Type::get_invalid_url_validation_errors( $invalid_url_post_id );

$_POST[ AMP_Validation_Manager::VALIDATION_ERROR_TERM_STATUS_QUERY_VAR ] = [
$errors[0]['term']->slug => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS,
$validation_errors[0]['term']->slug => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS,
$validation_errors[1]['term']->slug => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS,
];

add_filter(
Expand All @@ -1116,7 +1134,8 @@ static function( $url, $status ) {
}
$this->assertInstanceOf( 'Exception', $exception );
$this->assertEquals( 302, $exception->getCode() );
$this->assertStringEndsWith( 'action=edit&amp_taxonomy_terms_updated=1&amp_remaining_errors=0', $exception->getMessage() );
// No validation error statuses should be updated.
$this->assertStringEndsWith( 'action=edit&amp_taxonomy_terms_updated=0', $exception->getMessage() );
}

/**
Expand Down