Add plugin check ajax check request by vishalkakadiya · Pull Request #91 · WordPress/plugin-check

vishalkakadiya · 2023-02-27T17:23:08Z

Add plugin check ajax check request when user clicks on Check it! button.

Addresses the acceptance criteria in Create AJAX function and triggers and handling of progress indicator for the user #34

Closes #34

vishalkakadiya · 2023-02-28T07:23:49Z

@joemcgill I have addressed your feedback on this PR and left one comment. You can review it now, thanks!

joemcgill

Thanks for the updates 👍🏻

jjgrainger

@vishalkakadiya looks good, just left some minor changes to the action/method name based on discussions in #84

jjgrainger · 2023-03-03T16:57:08Z

Hi @felixarntz I've made some minor changes to the method/action names. Please can you review and merge if approved. Thanks!

felixarntz

@jjgrainger @vishalkakadiya Left some feedback here. Overall this looks solid, but there are a few things to clean up.

felixarntz · 2023-03-03T19:29:01Z

+			}
+		)
+		.catch(
+			( error ) => { console.log( error ); }


Nit-pick: Better to use console.error.

Also, per my other comment, we should check if a response was still returned from the server. If yes, we can extract the first error message from it and display it (response.data[ 0 ].message). This needs to be optional though, as an error could also occur just because e.g. there is no network connection, in which case there won't be a response.

jjgrainger · 2023-03-06T14:00:51Z

Thanks @felixarntz I've addressed the changes you've requested. This is ready for another review.

felixarntz

@jjgrainger A few small follow up things, most importantly about the error handling. I'm just unsure whether the approach you have in JS works, so I want to make sure you've tested this.

felixarntz

@jjgrainger The error logging is still not working as expected. You now added another check to ensure the expected success data is present, but at the same time got rid of the (also needed) check that logs the error message if there is one.

To clarify, we need to cover 4 scenarios here:

Request successful and data.data.message is present.
Request seemingly successful and data.data.message is not present (least likely to ever happen in reality).
Request error and data.data[ 0 ].message is present (which means our AJAX endpoint returned an error).
Request error and data.data[ 0 ].message is not present (which means something out of our control failed, e.g. the user is offline).

felixarntz · 2023-03-07T16:18:12Z

+				if ( ! data.success && ( ! data.data || ! data.data.message ) ) {
+					throw new Error( 'Response contains no data' );
+				}
+
+				console.log( data.data.message );


Doesn't this need to use data.data[ 0 ].message? You had that here before which I thought was right, but now you got rid of the [ 0 ].

I tested this and it is indeed not working. You need to also still include a condition that covers the error case. If the 403 error from the AJAX action is returned, the data.data[ 0 ].message in the error response should be logged via console.error. That is not the case right now.

Thanks @felixarntz

This final console log here is the final success message and all previous errors are handled. Going over the scenarios here:

Request successful and data.data.message is present - This is this final console log.

Request seemingly successful and data.data.message is not present (least likely to ever happen in reality). - This is the error thrown above 'Response contains no data', however I can see how this doesn't catch if success is true but the message is not set. I can update this.

Request error and data.data[ 0 ].message is present (which means our AJAX endpoint returned an error) - This is now captured earlier in the response when checking response.ok. This will be set to false for any error status including the 403 we return.

Request error and data.data[ 0 ].message is not present (which means something out of our control failed, e.g. the user is offline). - I don't think we can handle the logic with code here, if something like a network failure happens, the response is not handled and a generic console error is logged.

For point 2, I can make the updates here to capture when success is true but data/message is undefined.

For point 3, I can include an additional check as the point in the logic to use the message we return instead of the generic status message?

Let me know if you have any questions.

Thanks @felixarntz I've updated the code based on your feedback with comments to highlight the different scenarios/errors that are captured here.

mukeshpanchal27

@jjgrainger The checked Ajax request run if we don't select any plugin in dropdown can we restrict it so it will only run when the option has value?

jjgrainger · 2023-03-10T15:08:27Z

@jjgrainger The checked Ajax request run if we don't select any plugin in dropdown can we restrict it so it will only run when the option has value?

Thanks @mukeshpanchal27 I agree with the idea here, but I think this is something to address later as we have additional functionality to build for the WP Admin and may want to tackle validation later as part of Milestone 1 Review updates.

felixarntz

@jjgrainger One last problem with the error logging still remains. I still don't see our error message logged. Did you test this? Did your browser maybe behave differently than mine?

felixarntz · 2023-03-10T20:14:59Z

+				if ( ! response.ok ) {
+					throw new Error( `${response.status}: ${response.statusText}` );
+				}


This here is what throws a wrench in things. You already error out due to the bad response code, which makes it impossible to even reach the code below where we expose the more specific JSON error that is present.

I think by removing this our error logging works correctly. Requests that come back with a bad response are automatically logged in the console anyway, so no need to separately log the status code in our logic.

felixarntz · 2023-03-10T20:17:10Z

+		)
+		.then(
+			( data ) => {
+				if ( ! data.success ) {


Before checking this, we should check whether data even is an object, particularly when removing the above response.ok check. If the response is empty, then this line here would cause a JS error.

Suggested change

if ( ! data.success ) {

if ( ! data ) {

throw new Error( 'Response is empty or does not contain valid JSON' );

}

if ( ! data.success ) {

felixarntz · 2023-03-10T20:18:44Z

 				</select>

-				<input type="submit" value="<?php esc_attr_e( 'Check it!', 'plugin-check' ); ?>" />
+				<input type="submit" value="<?php esc_attr_e( 'Check it!', 'plugin-check' ); ?>" id="plugin-check__submit" class="button" />


Maybe make it a primary button, since it is the primary action on the page. We'll revise design later, so this is a nit-pick at this point, but a good practice.

Suggested change

<input type="submit" value="<?php esc_attr_e( 'Check it!', 'plugin-check' ); ?>" id="plugin-check__submit" class="button" />

<input type="submit" value="<?php esc_attr_e( 'Check it!', 'plugin-check' ); ?>" id="plugin-check__submit" class="button button-primary" />

joemcgill · 2023-03-10T21:48:43Z

+			'plugin-check-admin',
+			'const PLUGIN_CHECK = ' . json_encode(
+				array(
+					'ajaxUrl' => admin_url( 'admin-ajax.php' ),


Nitpick, but I think sending this value is unnecessary since ajaxurl is already set as a global in the admin.

jjgrainger · 2023-03-13T17:13:09Z

Thanks @felixarntz I've addressed the feedback here and this is ready for review

felixarntz

Thanks @jjgrainger, LGTM!

vishalkakadiya added 2 commits February 27, 2023 15:29

Working on ajax fetch api

1a0bee1

Add plugin check ajax check request

6e3698b

vishalkakadiya mentioned this pull request Feb 27, 2023

Create AJAX function and triggers and handling of progress indicator for the user #34

Closed

vishalkakadiya added 2 commits February 27, 2023 22:55

Fix phplint issue

54bc9db

Fix phpunit test

ce20901

vishalkakadiya requested review from felixarntz and jjgrainger February 27, 2023 18:16

joemcgill requested changes Feb 27, 2023

View reviewed changes

Comment thread includes/Admin/Admin_AJAX.php Outdated

Comment thread includes/Admin/Admin_Page.php Outdated

Comment thread templates/admin-page.php Outdated

Comment thread templates/admin-page.php Outdated

Address feedback

d79db4e

vishalkakadiya added 2 commits February 28, 2023 13:01

Merge trunk and resolve conflicts

2ab6968

Fix phpunit test

8f86b45

joemcgill approved these changes Feb 28, 2023

View reviewed changes

jjgrainger requested changes Mar 3, 2023

View reviewed changes

Comment thread includes/Admin/Admin_AJAX.php Outdated

Comment thread includes/Admin/Admin_AJAX.php Outdated

Comment thread assets/js/plugin-check-admin.js Outdated

Comment thread tests/Admin/Admin_AJAX_Tests.php Outdated

jjgrainger added 3 commits March 3, 2023 15:49

update action and method names to plural

556e18d

Merge branch 'trunk' into feature/add-ajax-check-trigger

49857e4

fix typo

f08f33e

jjgrainger approved these changes Mar 3, 2023

View reviewed changes

felixarntz requested changes Mar 3, 2023

View reviewed changes

jjgrainger added 5 commits March 6, 2023 11:40

use constant for nonce key

aaf5847

use WP_PLUGIN_CHECK_VERSION for asset version

9799130

return page hook for the admin page

d4e4042

update AJAX response and form element ids

9a737bd

Merge branch 'trunk' into feature/add-ajax-check-trigger

af87e52

jjgrainger requested a review from felixarntz March 6, 2023 14:00

felixarntz reviewed Mar 6, 2023

View reviewed changes

update AJAX response error handling

640b101

jjgrainger requested a review from felixarntz March 7, 2023 15:04

felixarntz reviewed Mar 7, 2023

View reviewed changes

update response error checking

0fa60eb

jjgrainger requested a review from felixarntz March 8, 2023 17:28

mukeshpanchal27 reviewed Mar 10, 2023

View reviewed changes

Comment thread templates/admin-page.php Outdated

Comment thread includes/Admin/Admin_AJAX.php Outdated

add button class for correct styles

1d28427

jjgrainger requested a review from mukeshpanchal27 March 10, 2023 15:08

felixarntz reviewed Mar 10, 2023

View reviewed changes

joemcgill reviewed Mar 10, 2023

View reviewed changes

jjgrainger added 4 commits March 13, 2023 12:13

update admin page elements

59e9724

update error handling

0672b99

Merge branch 'trunk' into feature/add-ajax-check-trigger

8e5261d

fix lint issues

33900ed

jjgrainger requested a review from felixarntz March 13, 2023 17:12

felixarntz approved these changes Mar 13, 2023

View reviewed changes

felixarntz merged commit 08df399 into trunk Mar 13, 2023

felixarntz deleted the feature/add-ajax-check-trigger branch March 13, 2023 17:57

	<input type="submit" value="<?php esc_attr_e( 'Check it!', 'plugin-check' ); ?>" id="plugin-check__submit" class="button" />
	<input type="submit" value="<?php esc_attr_e( 'Check it!', 'plugin-check' ); ?>" id="plugin-check__submit" class="button button-primary" />

Conversation

vishalkakadiya commented Feb 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vishalkakadiya commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joemcgill left a comment

Choose a reason for hiding this comment

Uh oh!

jjgrainger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjgrainger commented Mar 3, 2023

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjgrainger commented Mar 6, 2023

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jjgrainger commented Mar 10, 2023

Uh oh!

felixarntz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjgrainger commented Mar 13, 2023

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

vishalkakadiya commented Feb 28, 2023 •

edited

Loading

felixarntz left a comment •

edited

Loading