Update support request flow. #6627

dhaval-parekh · 2021-09-29T06:22:26Z

Summary

Amends #6147

Related: #5939

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

github-actions · 2021-09-29T06:25:28Z

Plugin builds for 4fe6689 are ready 🛎️!

Download development build
Download production build

pierlon · 2021-09-30T05:51:20Z

tests/features/support-request.feature

@@ -0,0 +1,27 @@
+Feature: AMP Support Request


It would be great to also test data for a validated URL is in the output as well.

To test validated URLs we need to mock those. (Like done previouslly ) But it's not the case here.

pierlon · 2021-09-30T05:53:57Z

(Sorry pushed to the wrong branch in #6627 (comment)).

There's a few more issues in #6147 to fix (which is why #5939 was re-opened), so this PR shouldn't close the issue just yet.

dhaval-parekh · 2021-10-07T09:39:41Z

This PR also addresses feedbacks from PR #6147.

src/Admin/SupportLink.php

pierlon · 2021-10-14T06:25:08Z

src/Admin/SupportScreen.php

@@ -170,8 +173,8 @@ public function enqueue_assets( $hook_suffix ) {
 			];
 		}

-		$this->support_data->set_args( $args );
-		$data = $this->support_data->get_data();
+		$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );


After taking a closer look, I think instantiating SupportData via the injector makes things overly complicated. Directly creating a SupportData object should be simple enough here, and therefore there would be no need for Injector.

Suggested change

$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );

$support_data = new SupportData( $args );

Another alternative:

Suggested change

$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );

$support_data = $this->injector->make( SupportData::class, compact( 'args' ) );

pierlon · 2021-10-14T06:25:59Z

src/Admin/SupportScreen.php

+		$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );
+		$data         = $support_data->get_data();


Alternatively, to get the data in one line:

Suggested change

$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );

$data = $support_data->get_data();

$data = ( new SupportData( $args ) )->get_data();

pierlon · 2021-10-14T06:28:00Z

src/Support/SupportCliCommand.php

@@ -106,8 +111,8 @@ public function send_diagnostic( /** @noinspection PhpUnusedParameterInspection
 			'is_synthetic' => $is_synthetic,
 		];

-		$this->support_data->set_args( $args );
-		$data = $this->support_data->get_data();
+		$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );


Same suggestion as above, creating the SupportData object should be simple enough.

Suggested change

$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );

$support_data = new SupportData( $args );

src/Support/SupportData.php

pierlon · 2021-10-14T07:25:33Z

src/Support/SupportData.php

@@ -476,11 +450,7 @@ public static function normalize_error( $error_data ) {

 		$error_data['text'] = ( ! empty( $error_data['text'] ) ) ? trim( $error_data['text'] ) : '';

-		$error_data = wp_json_encode( $error_data );
 		$error_data = static::remove_domain( $error_data );


Any reason for overloading the remove_domain() method and not just iterating over the error data and calling remove_domain() on each value?

Suggested change

$error_data = static::remove_domain( $error_data );

foreach ( $error_data as $key => $value ) {

$error_data[ $key ] = self::remove_domain( $value );

}

Internally self::remove_domain() will do the same thing if the provided value is an array. I kept that part in self::remove_domain() because $error_data can be multi-dimension array.

Ah I see. Not a fan of overloading remove_domain() and returning multiple data types however as that increases code complexity.

pierlon · 2021-10-14T07:28:36Z

src/Support/SupportRESTController.php

@@ -101,10 +103,10 @@ public function callback( WP_REST_Request $request ) {
 		$request_args = $request->get_param( 'args' );
 		$request_args = ( ! empty( $request_args ) && is_array( $request_args ) ) ? $request_args : [];

-		$this->support_data->set_args( $request_args );
-		$support_response = $this->support_data->send_data();
+		$support_data     = $this->injector->make( SupportData::class, [ 'args' => $request_args ] );


Same suggestion as above, creating the SupportData object should be simple enough.

Suggested change

$support_data = $this->injector->make( SupportData::class, [ 'args' => $request_args ] );

$support_data = new SupportData( $args );

Originally it was $support_data = new SupportData( $args );. I have used an injector after suggestions from the previous PR.

Hmm yea, but unless I'm missing something there's no benefit in creating the SupportData via the injector as its not a service or anything. cc @westonruter.

Another example of using the injector to create a class can be found in PairedRouting:

amp-wp/src/PairedRouting.php

Lines 252 to 263 in 002f88e

$structure_class = apply_filters( 'amp_custom_paired_url_structure', null );

if ( ! $structure_class || ! is_subclass_of( $structure_class, PairedUrlStructure::class ) ) {

$structure_slug = AMP_Options_Manager::get_option( Option::PAIRED_URL_STRUCTURE );

if ( array_key_exists( $structure_slug, self::PAIRED_URL_STRUCTURES ) ) {

$structure_class = self::PAIRED_URL_STRUCTURES[ $structure_slug ];

} else {

$structure_class = QueryVarUrlStructure::class;

}

}

$this->paired_url_structure = $this->injector->make( $structure_class );

While in practice it makes no difference, in theory the injector is better because this could reference an interface instead of an actual class, and the bindings defined in AmpWpPlugin could automatically route the class to instantiate. This could make it easier to test with a mock, as I understand.

Also, by using make the injector is able to re-use instances which are listed among the shared instances in AmpWpPlugin, which isn't possible otherwise.

So yeah, I understand from dependency injection that classes should generally not be instantiated directly.

cc @schlessera

Thanks for the explanation @westonruter. In this scenario we wouldn't be benefiting from using the injector, since the class being instantiated does not require any sort of dependency resolution and so would not make use of the bindings or shared instances provided by AmpWpPlugin. However, I do understand the benefits of using the injector with regards to testing especially, so after giving it more thought I'm OK with the current implementation.

westonruter · 2021-10-07T23:33:52Z

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

@@ -89,7 +89,7 @@ export function AMPSupport( props ) {
 					{
 						__html: sprintf(
 							/* translators: %s is the URL to create a new support topic */
-							__( 'In order to best assist you, please submit the following information to our private database. Once you have done so, copy the the resulting support ID and mention it in a <a href="%s" rel="noreferrer" target="_blank">support forum topic</a>. You do not have to submit data to get support, but our team will be able to help you more effectively if you do so.', 'amp' ),
+							__( 'In order to best assist you, please click the Send Data button below to send the following site information to our private database. Once you have done so, copy the the resulting Support UUID in the blue box that appears and include the ID in a new <a href="%s" rel="noreferrer" target="_blank">support forum topic</a>. You do not have to submit data to get support, but our team will be able to help you more effectively if you do so.', 'amp' ),


Suggested change

__( 'In order to best assist you, please click the Send Data button below to send the following site information to our private database. Once you have done so, copy the the resulting Support UUID in the blue box that appears and include the ID in a new <a href="%s" rel="noreferrer" target="_blank">support forum topic</a>. You do not have to submit data to get support, but our team will be able to help you more effectively if you do so.', 'amp' ),

__( 'In order to best assist you, please tap the Send Data button below to send the following site information to our private database. Once you have done so, copy the the resulting Support UUID in the blue box that appears and include the ID in a new <a href="%s" rel="noreferrer" target="_blank">support forum topic</a>. You do not have to submit data to get support, but our team will be able to help you more effectively if you do so.', 'amp' ),

westonruter · 2021-10-14T17:11:24Z

src/Support/SupportRESTController.php

@@ -101,10 +103,10 @@ public function callback( WP_REST_Request $request ) {
 		$request_args = $request->get_param( 'args' );
 		$request_args = ( ! empty( $request_args ) && is_array( $request_args ) ) ? $request_args : [];

-		$this->support_data->set_args( $request_args );
-		$support_response = $this->support_data->send_data();
+		$support_data     = $this->injector->make( SupportData::class, [ 'args' => $request_args ] );


Another example of using the injector to create a class can be found in PairedRouting:

amp-wp/src/PairedRouting.php

Lines 252 to 263 in 002f88e

$structure_class = apply_filters( 'amp_custom_paired_url_structure', null );

if ( ! $structure_class || ! is_subclass_of( $structure_class, PairedUrlStructure::class ) ) {

$structure_slug = AMP_Options_Manager::get_option( Option::PAIRED_URL_STRUCTURE );

if ( array_key_exists( $structure_slug, self::PAIRED_URL_STRUCTURES ) ) {

$structure_class = self::PAIRED_URL_STRUCTURES[ $structure_slug ];

} else {

$structure_class = QueryVarUrlStructure::class;

}

}

$this->paired_url_structure = $this->injector->make( $structure_class );

While in practice it makes no difference, in theory the injector is better because this could reference an interface instead of an actual class, and the bindings defined in AmpWpPlugin could automatically route the class to instantiate. This could make it easier to test with a mock, as I understand.

Also, by using make the injector is able to re-use instances which are listed among the shared instances in AmpWpPlugin, which isn't possible otherwise.

So yeah, I understand from dependency injection that classes should generally not be instantiated directly.

cc @schlessera

pierlon

This looks good, passing onto @westonruter for a final review.

pierlon · 2021-10-14T19:11:10Z

src/Support/SupportData.php

@@ -476,11 +450,7 @@ public static function normalize_error( $error_data ) {

 		$error_data['text'] = ( ! empty( $error_data['text'] ) ) ? trim( $error_data['text'] ) : '';

-		$error_data = wp_json_encode( $error_data );
 		$error_data = static::remove_domain( $error_data );


Ah I see. Not a fan of overloading remove_domain() and returning multiple data types however as that increases code complexity.

codecov · 2021-10-15T07:59:24Z

Codecov Report

Merging #6627 (a0fb445) into develop (4547bfb) will increase coverage by 0.53%.
The diff coverage is 74.41%.

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

@@              Coverage Diff              @@
##             develop    #6627      +/-   ##
=============================================
+ Coverage      76.63%   77.16%   +0.53%     
- Complexity      6318     6412      +94     
=============================================
  Files            248      250       +2     
  Lines          19812    20077     +265     
=============================================
+ Hits           15182    15493     +311     
+ Misses          4630     4584      -46

Flag	Coverage Δ
javascript	`79.06% <ø> (ø)`
php	`77.08% <74.41%> (+0.56%)`	⬆️
unit	`77.08% <74.41%> (+0.56%)`	⬆️

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

Impacted Files	Coverage Δ
src/Support/SupportCliCommand.php	`40.00% <0.00%> (-35.87%)`	⬇️
src/Support/SupportRESTController.php	`60.00% <60.00%> (ø)`
src/Support/SupportData.php	`78.83% <72.72%> (+1.87%)`	⬆️
...s/validation/class-amp-validated-url-post-type.php	`66.15% <100.00%> (ø)`
src/Admin/SupportLink.php	`96.36% <100.00%> (ø)`
src/Admin/SupportScreen.php	`98.41% <100.00%> (ø)`
src/RemoteRequest/CachedResponse.php	`0.00% <0.00%> (-77.78%)`	⬇️
src/RemoteRequest/CachedRemoteGetRequest.php	`52.17% <0.00%> (-6.53%)`	⬇️
...es/sanitizers/class-amp-allowed-tags-generated.php	`57.14% <0.00%> (-3.58%)`	⬇️
... and 18 more

…rces with deleted themes/plugins

westonruter

I've addressed some of my PR feedback with commits.

westonruter · 2021-10-18T19:16:09Z

src/Admin/SupportLink.php

@@ -42,15 +51,16 @@ public function register() {
 		// Add support link to Admin Bar.
 		add_action( 'admin_bar_menu', [ $this, 'admin_bar_menu' ], 105 );

-		// Add support link to meta box.
-		add_filter( 'amp_validated_url_status_actions', [ $this, 'amp_validated_url_status_actions' ], 10, 2 );
+		if ( is_admin() ) {


Not strictly required since outside the admin these actions won't do anything, but it doesn't hurt.

westonruter · 2021-10-18T19:19:50Z

src/Admin/SupportScreen.php

@@ -170,8 +173,8 @@ public function enqueue_assets( $hook_suffix ) {
 			];
 		}

-		$this->support_data->set_args( $args );
-		$data = $this->support_data->get_data();
+		$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );


Another alternative:

Suggested change

$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );

$support_data = $this->injector->make( SupportData::class, compact( 'args' ) );

westonruter · 2021-10-18T19:21:56Z

src/Support/SupportCliCommand.php

+	 * @codeCoverageIgnore
+	 *


Why ignore coverage?

Ah, because now it is covered in the feature test.

westonruter · 2021-10-18T19:33:10Z

src/Support/SupportData.php

+		if ( is_resource( $file ) ) {
+			while ( ! feof( $file ) ) {
+				$line       = fgets( $file );
+				$lines[]    = $line;
+				$line_count = count( $lines );
+				if ( $line_count > $no_of_lines ) {
+					array_shift( $lines );
+				}
 			}
+
+			fclose( $file );


While this looks like it will do the job, it is still reading through the entire file. It is using less memory now, which is good, but it could be more optimal by only seeking through the file starting from the end. For example: https://stackoverflow.com/a/15017711/93579

This would still be good to do.

src/Support/SupportData.php

tests/php/src/Support/SupportDataTest.php

dhaval-parekh added 2 commits September 27, 2021 12:29

Update message on support page

be2444b

Add feature test case for WP cli command of amp support send-diagnostic

363d8dc

dhaval-parekh requested review from delawski and pierlon September 29, 2021 06:43

dhaval-parekh changed the title ~~Support request flow.~~ Add test cases for support request CLI. Sep 29, 2021

dhaval-parekh mentioned this pull request Sep 29, 2021

Add support request flow #5939

Closed

pierlon force-pushed the feature/5939-support-request-flow branch from 44b3b42 to 363d8dc Compare September 30, 2021 05:47

pierlon reviewed Sep 30, 2021

View reviewed changes

maitreyie-chavan added this to the v2.2 milestone Oct 1, 2021

maitreyie-chavan linked an issue Oct 3, 2021 that may be closed by this pull request

Add support request flow #5939

Closed

dhaval-parekh self-assigned this Oct 5, 2021

dhaval-parekh added 3 commits October 6, 2021 17:57

Apply suggestion from PR

92c070b

Use WP_Query to fetch data instead of direct query

9dbbe20

Update the usage of injector

066d519

dhaval-parekh mentioned this pull request Oct 7, 2021

Adds streamlined support request flow #6147

Merged

3 tasks

Update logic to get error log

b1290d9

dhaval-parekh requested a review from pierlon October 7, 2021 09:39

dhaval-parekh added 4 commits October 7, 2021 15:16

Fix fatal error in WP 4.9

281e97b

Merge branch 'develop' into feature/5939-support-request-flow

5eb0c91

Fix unit test cases

7c7bbfe

Update unit test case

9b0db99

pierlon mentioned this pull request Oct 7, 2021

Address last review for PR #6147 #6630

Closed

2 tasks

dhaval-parekh added 5 commits October 7, 2021 21:55

Apply suggestions from PR and update unit test cases

dc270fd

Add more unit test cases for SupportData()

d5b4c45

Merge branch 'develop' into feature/5939-support-request-flow

51ebd95

Add delayed in SupportLink class and update the unit test cases

d84f3ca

Use class injector instead of fetching from Service class

889a607

pierlon suggested changes Oct 14, 2021

View reviewed changes

Apply suggestions from PR

39e51e6

dhaval-parekh requested a review from pierlon October 14, 2021 16:07

dhaval-parekh changed the title ~~Add test cases for support request CLI.~~ Update support request flow. Oct 14, 2021

westonruter reviewed Oct 14, 2021

View reviewed changes

pierlon approved these changes Oct 14, 2021

View reviewed changes

Update message in AMP Support page

a0fb445

westonruter added 9 commits October 18, 2021 12:53

Ensure data is array before sending it through array_map

da95491

Use imported WP_Query

5004206

Add since tag and sort imports

9d722ae

Use class name constant

3106278

Use closure to call normalize_plugin_info

669355d

Omit parent_theme field if child theme not active

ffc2990

Only obtain theme versions for parent theme and child theme

ee7e4e4

Use wp_get_theme() to obtain theme version on demand; account for sou…

0e3bb40

…rces with deleted themes/plugins

Fix logic in get_theme_info to return parent and child theme both

855f48e

westonruter approved these changes Oct 18, 2021

View reviewed changes

westonruter enabled auto-merge October 18, 2021 20:46

Skip test_get_theme_info when no active theme (in WP 4.9)

4fe6689

westonruter merged commit 65cdda6 into develop Oct 18, 2021

westonruter deleted the feature/5939-support-request-flow branch October 18, 2021 20:56

dhaval-parekh mentioned this pull request Oct 19, 2021

Optimize reading tail of error log #6652

Merged

2 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update support request flow. #6627

Update support request flow. #6627

dhaval-parekh commented Sep 29, 2021 •

edited by pierlon

Loading

github-actions bot commented Sep 29, 2021 •

edited

Loading

pierlon Sep 30, 2021

dhaval-parekh Oct 7, 2021

pierlon commented Sep 30, 2021 •

edited

Loading

dhaval-parekh commented Oct 7, 2021

pierlon Oct 14, 2021

westonruter Oct 18, 2021

pierlon Oct 14, 2021

pierlon Oct 14, 2021

pierlon Oct 14, 2021

dhaval-parekh Oct 14, 2021

pierlon Oct 14, 2021

pierlon Oct 14, 2021

dhaval-parekh Oct 14, 2021

pierlon Oct 14, 2021

westonruter Oct 14, 2021

pierlon Oct 14, 2021

westonruter Oct 7, 2021

westonruter Oct 14, 2021

pierlon left a comment

pierlon Oct 14, 2021

codecov bot commented Oct 15, 2021 •

edited

Loading

westonruter left a comment •

edited

Loading

westonruter Oct 18, 2021

westonruter Oct 18, 2021

westonruter Oct 18, 2021

westonruter Oct 18, 2021

westonruter Oct 18, 2021

westonruter Oct 18, 2021

	$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );
	$support_data = new SupportData( $args );

		$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );
		$data = $support_data->get_data();

	$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );
	$data = $support_data->get_data();
	$data = ( new SupportData( $args ) )->get_data();

-		$error_data = static::remove_domain( $error_data );
+		foreach ( $error_data as $key => $value ) {
+			$error_data[ $key ] = self::remove_domain( $value );
+		}

	$structure_class = apply_filters( 'amp_custom_paired_url_structure', null );

	if ( ! $structure_class \|\| ! is_subclass_of( $structure_class, PairedUrlStructure::class ) ) {
	$structure_slug = AMP_Options_Manager::get_option( Option::PAIRED_URL_STRUCTURE );
	if ( array_key_exists( $structure_slug, self::PAIRED_URL_STRUCTURES ) ) {
	$structure_class = self::PAIRED_URL_STRUCTURES[ $structure_slug ];
	} else {
	$structure_class = QueryVarUrlStructure::class;
	}
	}

	$this->paired_url_structure = $this->injector->make( $structure_class );

Update support request flow. #6627

Update support request flow. #6627

Conversation

dhaval-parekh commented Sep 29, 2021 • edited by pierlon Loading

Summary

Checklist

github-actions bot commented Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon commented Sep 30, 2021 • edited Loading

dhaval-parekh commented Oct 7, 2021

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

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

pierlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 15, 2021 • edited Loading

Codecov Report

westonruter left a comment • 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 Sep 29, 2021 •

edited by pierlon

Loading

github-actions bot commented Sep 29, 2021 •

edited

Loading

pierlon commented Sep 30, 2021 •

edited

Loading

codecov bot commented Oct 15, 2021 •

edited

Loading

westonruter left a comment •

edited

Loading