Validate URL if validation data does not exist while generating support data. #6718

dhaval-parekh · 2021-11-15T07:19:46Z

Summary

This PR adds changes to support request flow. With new changes, while preparing support data if validation data for the requested URL doesn't found it will validate and store the data for the URL (which previously doesn't happen)

Also, Added appropriate messages when there is no AMP related error information.

When there is no validation data.

When all validation data are stalled.

When there is no AMP error on validated URLs but there is some stalled result.

When there is no AMP error on all validated URLs.

When request URL doesn't have AMP error.

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

codecov · 2021-11-18T12:13:05Z

Codecov Report

Merging #6718 (fb5bb27) into develop (9069888) will increase coverage by 0.01%.
The diff coverage is 82.96%.

❗ Current head fb5bb27 differs from pull request most recent head edc3bd9. Consider uploading reports for the commit edc3bd9 to get more accurate results

@@              Coverage Diff              @@
##             develop    #6718      +/-   ##
=============================================
+ Coverage      77.12%   77.14%   +0.01%     
- Complexity      6553     6565      +12     
=============================================
  Files            262      262              
  Lines          20884    20929      +45     
=============================================
+ Hits           16107    16145      +38     
- Misses          4777     4784       +7

Flag	Coverage Δ
javascript	`64.56% <ø> (ø)`
php	`77.84% <82.96%> (+0.01%)`	⬆️
unit	`77.84% <82.96%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
src/Admin/SupportScreen.php	`85.13% <35.29%> (-13.28%)`	⬇️
src/Support/SupportData.php	`83.41% <88.99%> (+0.03%)`	⬆️
src/Admin/SupportLink.php	`96.29% <100.00%> (-0.07%)`	⬇️
src/Validation/URLValidationRESTController.php	`92.92% <100.00%> (+0.14%)`	⬆️
src/PairedUrl.php	`85.18% <0.00%> (+0.56%)`	⬆️
src/Support/SupportRESTController.php	`86.66% <0.00%> (+26.66%)`	⬆️

github-actions · 2021-11-18T12:13:34Z

Plugin builds for cf8d5f2 are ready 🛎️!

Download development build
Download production build

delawski

I've reviewed and tested this PR locally. Everything seems to be working as expected for me.

I left some feedback on the frontend part and briefly browsed the backend code. @bartoszgadomski would you be able to do a more thorough code review (mainly the backend part)?

assets/src/components/amp-support/notices.js

assets/src/components/amp-support/index.js

assets/src/components/amp-support/notices.js

src/Admin/SupportScreen.php

src/Support/SupportData.php

src/Admin/SupportLink.php

src/Admin/SupportScreen.php

src/Support/SupportData.php

dhaval-parekh · 2021-11-22T14:24:00Z

While testing this PR. Found a bug that when there are more than 100 amp_validated_url posts and the first 100 doesn't have any error. Support page display notices that "Site doesn't have AMP validation errors". But there are validation errors and the user can see errors in the "Validated URL" Page.

This can be fixed quickly by adding 's' => 'term_slug' in query args to fetch only amp_validated_url posts that have AMP validation errors. Or looping through all the amp_validated_url. here

However, the problem with the first approach is it might skip the posts that have a CSS budget of more than 100%.

…dated_url

Add notice in AMP Support page if there is not AMP error

delawski

Thank you for addressing the code review feedback. It looks good to me now 👍

assets/src/components/amp-support/validation-results-notice.js

bartoszgadomski

Looks good to me 👍

…support-request * 'develop' of github.com:ampproject/amp-wp: (39 commits) Harden is_needed check to include has_cap Fix another usage of Encoding Sort imports for impacted files Add noopener to external link Update to amp-toolbox 0.9.1 Open validated URLs in new window to prevent accidental loss Show URLs with validation issues to non-technical users as well Cover case when Gutenberg is not active with E2E tests Do not scan site for network activated plugins Enqueue missing `wp-components` CSS dependency Ensure timeout has been set before trying to clear it Update E2E tests Improve plugin activation post-site-scan message copy Ensure demo plugin is deactivated before running tests Update plugin activation message if validation errors are found Update E2E tests to reflect recent changes Always pass 2 arguments to `visitAdminPage()` Rename `AdminNotice` component to `AmpAdminNotice` Cleanly skip test if dependency is missing Apply notice text update suggestions from code review ...

westonruter

I'm testing this but it's not working for me. I'm getting an infinite series of HTTP requests that end in 500 errors:

error.mov

The console shows:

Error log has:

[03-Dec-2021 03:13:54 UTC] PHP Fatal error:  Uncaught Error: Call to undefined function AmpProject\AmpWP\Support\post_exists() in /app/public/content/plugins/amp/src/Support/SupportData.php:695
Stack trace:
#0 /app/public/content/plugins/amp/src/Support/SupportData.php(200): AmpProject\AmpWP\Support\SupportData->get_amp_urls()
#1 /app/public/content/plugins/amp/src/Support/SupportData.php(159): AmpProject\AmpWP\Support\SupportData->get_data()
#2 /app/public/content/plugins/amp/src/Support/SupportRESTController.php(107): AmpProject\AmpWP\Support\SupportData->send_data()
#3 /app/public/core-dev/src/wp-includes/rest-api/class-wp-rest-server.php(1141): AmpProject\AmpWP\Support\SupportRESTController->callback(Object(WP_REST_Request))
#4 /app/public/core-dev/src/wp-includes/rest-api/class-wp-rest-server.php(988): WP_REST_Server->respond_to_request(Object(WP_REST_Request), '/amp/v1/send-di...', Array, NULL)
#5 /app/public/core-dev/src/wp-includes/rest-api/class-wp-rest-server.php(414): WP_REST_Server->dispatch(Object(WP_REST_Request))
#6 /app/public/core-dev/s in /app/public/content/plugins/amp/src/Support/SupportData.php on line 695
```

In the case of an error, it shouldn't automatically try to re-send. It should prompt the user to push the submit button again.

assets/src/components/amp-support/validation-results-notice.js

src/Admin/SupportScreen.php

src/Support/SupportData.php

src/Admin/SupportLink.php

src/Admin/SupportScreen.php

src/Support/SupportData.php

Use AMP_Validated_URL_Post_Type::get_invalid_url_post() instead of post_exists()

dhaval-parekh · 2021-12-03T16:33:35Z

In the case of an error, it shouldn't automatically try to re-send. It should prompt the user to push the submit button again.

I have updated it. And now in case of error, it will show an error message to the user.

Regarding error. While checking further found that post_exists() is available in admin requests and WP CLI but not in REST endpoint which was causing the issue during sending a support request.

Now instead of using post_exists() now using AMP_Validated_URL_Post_Type::get_invalid_url_post() that will resolve the issue.

…support-request * 'develop' of github.com:ampproject/amp-wp: Clear timeout correctly Simplify the E2E test Simplify logic for ensuring consistent units Propagate more styles AMP-valid dimensions to AMP element Add tests for deriving width/height from styles Propagate width/height styles as width/height attributes when setting layout

westonruter · 2021-12-03T19:39:43Z

src/Support/SupportRESTController.php

+		register_rest_route(
+			$this->namespace,
+			'/get-diagnostic',
+			[
+				[
+					'methods'             => WP_REST_Server::READABLE,
+					'args'                => [],
+					'permission_callback' => [ $this, 'permission_callback' ],
+					'callback'            => [ $this, 'get_item' ],
+				],
+			]
+		);
+


This endpoint isn't being used anywhere, and it's not validating the args. So I think it should be removed.

Partially revert "Update function docs." This reverts commit 262371d.

westonruter · 2021-12-03T20:04:21Z

I'm still getting an error when submitting this, but this time it seems to be an issue with the insights server. It's responding with:

{
    "status": "fail",
    "data": [
        {
            "message": "required validation failed on plugins.0.version",
            "field": "plugins.0.version",
            "validation": "required"
        },
        {
            "message": "required validation failed on plugins.5.version",
            "field": "plugins.5.version",
            "validation": "required"
        },
        {
            "message": "required validation failed on plugins.6.version",
            "field": "plugins.6.version",
            "validation": "required"
        }
    ]
}

It is because I have active plugins which do not have a version. This should not be a required field.

Filed as https://github.com/rtCamp/px-wp-insights/issues/186.

…ontent

westonruter · 2021-12-03T20:16:38Z

src/Admin/SupportScreen.php

+			'posts_per_page' => 1,
+			'post_status'    => 'publish',
+			'meta_key'       => AMP_Validated_URL_Post_Type::VALIDATED_ENVIRONMENT_POST_META_KEY,
+			'meta_value'     => maybe_serialize( AMP_Validated_URL_Post_Type::get_validated_environment() ), // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_value


This is a clever way to query for non-stale results.

westonruter · 2021-12-03T20:35:16Z

src/Support/SupportData.php

 		}

+		$amp_invalid_url = [
+			'url'                   => $amp_validated_post->post_title,


This and other instances in the file could use \AMP_Validated_URL_Post_Type::get_url_from_post().

Updated in new PR #6764

westonruter · 2021-12-03T20:40:43Z

src/Support/SupportData.php

+				'post_status'    => 'publish',
+				'meta_key'       => AMP_Validated_URL_Post_Type::VALIDATED_ENVIRONMENT_POST_META_KEY,
+				'meta_value'     => maybe_serialize( AMP_Validated_URL_Post_Type::get_validated_environment() ), // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_value
+				's'              => 'term_slug',


Why search for term_slug? Just something that exists in the post_content?

Added term_slug as a search query string to make sure any data we fetch has an error and is not empty.
term_slug will always be available in post content for amp_invalidated_url posts that have errors.

westonruter · 2021-12-03T20:43:57Z

src/Support/SupportData.php

+		if ( ! empty( $source['sources'] ) && is_array( $source['sources'] ) ) {
+			foreach ( $source['sources'] as $index => $inner_source ) {
+				$source['sources'][ $index ] = self::normalize_error_source( $inner_source );
+			}
+
+			$source['sources'] = array_values( array_filter( $source['sources'] ) );
+		}


This was added in 9c8ef66 to add normalization of nested sources?

Yes, I have added to normalize nested sources.

dhaval-parekh requested review from delawski and milindmore22 and removed request for delawski and milindmore22 November 15, 2021 11:07

dhaval-parekh force-pushed the bug/6707-support-request branch from 71d2bdf to fb5bb27 Compare November 18, 2021 10:50

dhaval-parekh requested review from milindmore22 and delawski November 18, 2021 12:05

dhaval-parekh marked this pull request as ready for review November 18, 2021 12:05

delawski requested changes Nov 18, 2021

View reviewed changes

delawski requested a review from bartoszgadomski November 18, 2021 20:18

delawski mentioned this pull request Nov 18, 2021

Validated URL data may not be included with Support request #6707

Closed

bartoszgadomski suggested changes Nov 22, 2021

View reviewed changes

dhaval-parekh added 10 commits November 24, 2021 16:59

Pass the front-end URL to support page instead of post id of amp_vali…

da2c78b

…dated_url

Validate URL while sending support data if data not exist for URL

5e2477f

Update unit test cases

72773d5

Refactor SupportData service.

9c8ef66

Add notice in AMP Support page if there is not AMP error

Update unit test cases

22e7958

Update AMP support notice component

7c9ff5a

Validate URL before passing to SupportData service

8f55b75

Apply suggestion from PR

f64fd23

Update prop types

e4d95fd

Fetch only amp_validated_url post that have error on support page

aab5aee

dhaval-parekh force-pushed the bug/6707-support-request branch from 05c4e1c to aab5aee Compare November 25, 2021 13:10

delawski reviewed Nov 26, 2021

View reviewed changes

assets/src/components/amp-support/validation-results-notice.js Outdated Show resolved Hide resolved

delawski approved these changes Nov 26, 2021

View reviewed changes

dhaval-parekh added 2 commits November 29, 2021 16:05

Use plural nouns in variable name

c98d9be

Fix typo in Sandboxinf level drawer

8740be4

dhaval-parekh requested a review from bartoszgadomski November 29, 2021 14:14

bartoszgadomski approved these changes Nov 30, 2021

View reviewed changes

dhaval-parekh added 2 commits December 1, 2021 19:08

Merge branch 'develop' into bug/6707-support-request

f8c245e

Merge branch 'develop' into bug/6707-support-request

9448e98

westonruter added this to the v2.2 milestone Dec 2, 2021

westonruter requested changes Dec 3, 2021

View reviewed changes

dhaval-parekh added 5 commits December 3, 2021 20:03

Update function docs.

262371d

Use AMP_Validated_URL_Post_Type::get_invalid_url_post() instead of post_exists()

Add test case for SupportScreen::get_amp_validated_post_counts()

575dba3

Prevent multiple support request in case of error

3b11805

Apply suggestion from PR

853143f

Sanitize value of query param before using

fbd5537

dhaval-parekh requested a review from westonruter December 3, 2021 17:05

westonruter reviewed Dec 3, 2021

View reviewed changes

westonruter added 2 commits December 3, 2021 11:41

Further reuse amp_validated_post variable

afdcec3

Remove unused REST API endpoint

0c4b7eb

Partially revert "Update function docs." This reverts commit 262371d.

westonruter added 2 commits December 3, 2021 12:08

Translate error message when failing to send to insights server

3a5f4aa

Improve phpdoc comments and guard against JSON parse error for post_c…

906b244

…ontent

westonruter approved these changes Dec 3, 2021

View reviewed changes

westonruter enabled auto-merge December 3, 2021 21:00

Fix test after message update in 3a5f4aa

cf8d5f2

westonruter merged commit eacd6cf into develop Dec 3, 2021

westonruter deleted the bug/6707-support-request branch December 3, 2021 21:10

dhaval-parekh mentioned this pull request Dec 6, 2021

Use AMP_Validated_URL_Post_Type::get_url_from_post() instead of post_title property #6764

Merged

2 tasks

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate URL if validation data does not exist while generating support data. #6718

Validate URL if validation data does not exist while generating support data. #6718

dhaval-parekh commented Nov 15, 2021 •

edited

Loading

codecov bot commented Nov 18, 2021 •

edited

Loading

github-actions bot commented Nov 18, 2021 •

edited

Loading

delawski left a comment

dhaval-parekh commented Nov 22, 2021

delawski left a comment

bartoszgadomski left a comment

westonruter left a comment •

edited

Loading

dhaval-parekh commented Dec 3, 2021

westonruter Dec 3, 2021

westonruter commented Dec 3, 2021 •

edited

Loading

westonruter Dec 3, 2021

westonruter Dec 3, 2021

dhaval-parekh Dec 6, 2021

westonruter Dec 3, 2021

dhaval-parekh Dec 4, 2021

westonruter Dec 3, 2021

dhaval-parekh Dec 6, 2021

Validate URL if validation data does not exist while generating support data. #6718

Validate URL if validation data does not exist while generating support data. #6718

Conversation

dhaval-parekh commented Nov 15, 2021 • edited Loading

Summary

Checklist

codecov bot commented Nov 18, 2021 • edited Loading

Codecov Report

github-actions bot commented Nov 18, 2021 • edited Loading

delawski left a comment

Choose a reason for hiding this comment

dhaval-parekh commented Nov 22, 2021

delawski left a comment

Choose a reason for hiding this comment

bartoszgadomski left a comment

Choose a reason for hiding this comment

westonruter left a comment • edited Loading

Choose a reason for hiding this comment

dhaval-parekh commented Dec 3, 2021

Choose a reason for hiding this comment

westonruter commented Dec 3, 2021 • 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

dhaval-parekh commented Nov 15, 2021 •

edited

Loading

codecov bot commented Nov 18, 2021 •

edited

Loading

github-actions bot commented Nov 18, 2021 •

edited

Loading

westonruter left a comment •

edited

Loading

westonruter commented Dec 3, 2021 •

edited

Loading