Skip to content

Include sources for validation error formatted data #5835

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

Merged
merged 5 commits into from
Feb 2, 2021

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Feb 1, 2021

Summary

Fixes #5825

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@pierlon pierlon added this to the v2.0.11 milestone Feb 1, 2021
@pierlon pierlon requested a review from westonruter February 1, 2021 21:03
@pierlon pierlon self-assigned this Feb 1, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2021

Plugin builds for 1fa8938 are ready 🛎️!

@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #5835 (f5efb19) into develop (e05b910) will increase coverage by 0.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #5835      +/-   ##
=============================================
+ Coverage      75.13%   75.14%   +0.01%     
- Complexity      5647     5655       +8     
=============================================
  Files            210      210              
  Lines          16961    16981      +20     
=============================================
+ Hits           12743    12760      +17     
- Misses          4218     4221       +3     
Flag Coverage Δ Complexity Δ
javascript 75.93% <ø> (ø) 0.00 <ø> (ø)
php 75.11% <88.88%> (+0.01%) 0.00 <11.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...validation/class-amp-validation-error-taxonomy.php 50.30% <88.00%> (+0.40%) 541.00 <11.00> (+8.00)
...s/validation/class-amp-validated-url-post-type.php 66.08% <100.00%> (+0.04%) 447.00 <0.00> (ø)

if ( isset( $_GET['post'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
$validation_errors = AMP_Validated_URL_Post_Type::get_invalid_url_validation_errors( (int) $_GET['post'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotValidated
foreach ( $validation_errors as $error ) {
if ( isset( $error['data']['sources'], $error['term']->term_id ) && $error['term']->term_id === $term->term_id ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize here that it's possible that the sources may not be specifically for this term. Specifically, it's possible for an error to occur multiple times on a URL, and each time the source may different. That does complicate things here, however, since the {{$taxonomy}}_row_actions filter doesn't pass any index. Perhaps that could be captured by by setting an index to 0 before rendering the table?

Then with each invocation of filter_tag_row_actions the index could be incremented. The index can then be provided from filter_tag_row_actions to get_error_details_json in addition to the $term (which would actually be redundant here), since the error could then be obtained in get_error_details_json here via $validation_errors[ $index ].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter I think I have it resolved in 6af38a9.

@pierlon pierlon requested a review from westonruter February 2, 2021 02:51
@westonruter
Copy link
Member

Rebasing to include #5837 along with a couple additional commits.

@westonruter westonruter force-pushed the fix/5825-json-validation-error-sources branch from 6af38a9 to 1fa8938 Compare February 2, 2021 07:03
@westonruter westonruter merged commit 89c1def into develop Feb 2, 2021
@westonruter westonruter deleted the fix/5825-json-validation-error-sources branch February 2, 2021 19:04
@westonruter westonruter modified the milestones: v2.0.11, v2.1 Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validation errors JSON data copied to clipboard lacks sources
2 participants