Add Ajax to Plugin and Themes Screen by ronalfy · Pull Request #61 · WordPress/wp-autoupdates

ronalfy · 2020-04-04T20:58:03Z

Resolves #65

This is a concept PR that demonstrates Ajax enabling/disabling of plugins. Would love to discuss this more and flesh it out more for themes as well if this concept is accepted.

Thanks,

Ronald Huereca

paaljoachim · 2020-04-04T21:06:19Z

Looks Good Ron..:)

ronalfy · 2020-04-04T21:07:48Z

Looks Good Ron..:)

Thanks! I'll try to attend the next meeting and bring this up.

pbiron · 2020-04-04T21:27:09Z

Related: https://wordpress.slack.com/archives/CULBN711P/p1585694448060400, #47

ronalfy · 2020-04-04T21:30:33Z

Thanks @pbiron

paaljoachim · 2020-04-04T21:34:47Z

So it sounds like it is outside the scope of this plugin.
Perhaps this is more of a WordPress trac issue to bring Ajax into the Installed plugins screen so one can activate/deactivate plugins by using Ajax.

I am guessing that Ajax is being used when initially installing and activating a plugin. (Atleast it does not jump to the top of the screen when installing/activating a plugin for the first time.)

That means on the Plugins -> Install New seems to have Ajax activated.
But that the Installed plugins screen does not.

pbiron · 2020-04-04T21:56:41Z

Thanks @pbiron

Thank you for the POC PR!

pbiron · 2020-04-04T21:59:22Z

So it sounds like it is outside the scope of this plugin.
Perhaps this is more of a WordPress trac issue to bring Ajax into the Installed plugins screen so one can activate/deactivate plugins by using Ajax.

No, it's perfectly within scope for the plugin. The implementation may change when merged into core, but that's the same with a lot of what's in the plugin.

ronalfy · 2020-04-05T01:12:05Z

@paaljoachim @pbiron updated the PR for the themes screen on multisite. I'll do another test on single-site to make sure everything behaves as expected.

One thing I noticed is we need some do_action or apply_filters when anything is updated and removed for third-party plugin compatibility such as Easy Updates Manager (200,000+ installs) so they can keep track of what's enabled/disabled. I'm imagining another PR to accomplish this.

ronalfy · 2020-04-05T01:12:41Z

Also would be good to abstract out the HTML creation so Ajax and non-Ajax use the same markup.

ronalfy · 2020-04-05T01:50:53Z

This does not work for the themes screen on single-site. I'll be happy to do another PR to get that fixed with this PR as a dependency.

Needed:

Abstraction of HTML for use in Ajax and non-Ajax settings.
Uniform set of classes to target for JS integration.
Uniform set of strings to i18n for enabled/disabled settings. I notice the themes screen on single-site has different verbiage for enabled/disabled states.

I'll be happy to discuss at the next open meeting.

Regards,

Ronald Huereca

pbiron · 2020-04-05T13:54:25Z

One thing I noticed is we need some do_action or apply_filters when anything is updated and removed for third-party plugin compatibility such as Easy Updates Manager (200,000+ installs) so they can keep track of what's enabled/disabled. I'm imagining another PR to accomplish this.

Thanx Ronald! Can you open an issue about other things we could do (e.g., other hooks we could/should add) to help and/or avoid conflicts with existing update plugins?

ronalfy · 2020-04-05T20:06:16Z

Thanx Ronald! Can you open an issue about other things we could do (e.g., other hooks we could/should add) to help and/or avoid conflicts with existing update plugins?

Done. Thank you. #63

ronalfy · 2020-04-05T20:24:52Z

@paaljoachim @pbiron I updated the PR to cover single-site themes.

This is ready for review/scrutiny :)

pedro-mendonca · 2020-04-05T21:42:32Z

Suggestion for some more consistent wording:

… dynamic.

ronalfy · 2020-04-05T21:53:29Z

Suggestion for some more consistent wording:

Yeah, we'll definitely want to tweak the wording. Thanks!

pbiron

I'm not sure why, but the colors on the Enable/Disable links in the Theme Details modal are wrong after applying this PR, see

Can you look into that?

ronalfy · 2020-04-07T19:10:34Z

@pbiron shall I keep this PR up to date with changes, or wait until the 0.6 milestone to finalize?

pbiron · 2020-04-07T19:41:23Z

@pbiron shall I keep this PR up to date with changes, or wait until the 0.6 milestone to finalize?

It's probably easier to wait, but that's up to you.

Also, with regard to adding $hook === 'site-themes.php' to the conditional, see #69 which I just opened...so definitely wait to make that change.

ronalfy · 2020-04-16T22:52:21Z

@pbiron this has been updated with the latest 0.5.1 changes.

pbiron

On quick inspection things seem to work great. I'll have to go over the code in detail later.

The other thing I notice is that without the dashicon animating while processing is happening it's unclear to a user that anything is happening :-( Yes, the wording changes from Enable auto-updates to Enabling auto-updates but that change is so minor I didn't really see it the first couple of times.

I don't really know what to do about that. Maybe one thing would be to change the text to Enabling auto-updates ... (append ellipsis). Even better would be if the ellipsis could be animated; that is, draw one full-stop, then two, then 3, then back to 1, then 2, then 3, etc. I have no idea whether that would be possible.

Also see the 3 comments attached to specific lines (e.g., the version arg when enqueueing the JS, and about the blur() in the on.click handlers.

audrasjb · 2020-04-20T20:32:55Z

Thanks all and particularly @ronalfy for all the great work on this pull request.
Currently testing it, and all looks fine on my side for the moment…
I have some accessibility concerns, but I think it’d be easier to handle them once the current workaround is merged.

TimothyBJacobs · 2020-04-20T22:48:51Z

+	$nonce = sanitize_text_field( $_POST['nonce'] );
+	$type  = sanitize_text_field( $_POST['type'] );
+	$asset = sanitize_text_field( urldecode( $_POST['asset'] ) );
+	if ( ! wp_verify_nonce(


For consistency with other WordPress Ajax functions, like wp_ajax_update_plugin it might be worth using check_ajax_referer instead.

@TimothyBJacobs Changed with bbc30fd

ronalfy · 2020-04-20T22:49:16Z

@TimothyBJacobs @audrasjb @pbiron ready for re-review. Made the CSS changes and removes the filter_inputs. Thanks for allowing me to be apart of this.

audrasjb

I think this pull request will need a security audit before merge 🙂

audrasjb · 2020-04-20T22:52:35Z

+ * Disable auto updates via Ajax.
+ */
+function wp_autoupdates_disable_auto_updates() {
+	$nonce = sanitize_text_field( $_POST['nonce'] );


I’m a bit worried about using sanitize_text_field as it's only sanitization. Shouldn't we also escape unwanted HTML bits?

Pinging @whyisjake as WordPress security team maintainer for further consideration.

Yeah, great idea.

Coding standards want me to use wp_unslash and then a sanitization before even touching the variable. But I'll let the security team come up with the suggestions.

whyisjake

There are a few nits here, overall, looks good.

whyisjake · 2020-04-20T23:10:24Z

+
+					wp_send_json_success(
+						array(
+							'enabled_count'  => '(' . $enabled_count . ')',


Can you add an absint here?

Suggested change

'enabled_count' => '(' . $enabled_count . ')',

'enabled_count' => '(' . absint( $enabled_count ) . ')',

whyisjake · 2020-04-20T23:12:49Z

+
+			wp_send_json_success(
+				array(
+					'enabled_count'  => '(' . $enabled_count . ')',


Let's add an absint here too.

Suggested change

'enabled_count' => '(' . $enabled_count . ')',

'enabled_count' => '(' . absint( $enabled_count ) . ')',

whyisjake · 2020-04-20T23:14:44Z

+				array(
+					'enabled_count'  => '(' . $enabled_count . ')',
+					'disabled_count' => '(' . absint( count( $all_plugins ) - $enabled_count ) . ')',
+					'return_html'    => $return_html,


Wondering if we should add another esc_ function here. I can see the parts above look great, but...

whyisjake · 2020-04-20T23:15:24Z

+				array(
+					'enabled_count'  => '(' . $enabled_count . ')',
+					'disabled_count' => '(' . absint( count( $all_themes ) - $enabled_count ) . ')',
+					'return_html'    => $return_html,


Same comment here as above about thinking about late escaping.

@whyisjake is wp_kses_post sufficient for late escaping of the HTML, or do you want to be stricter than that?

See af2480b please.

pbiron

see https://github.com/WordPress/wp-autoupdates/pull/61/files#r411760174

pbiron

there are some WPCS related things that should be addressed at some point, but don't think those need to be done before this is merged. So, once the others sign off, I think it's good to go.

whyisjake · 2020-04-21T16:20:04Z

💥

Ronald Huereca added 3 commits April 2, 2020 08:59

Adding beginnings of Ajax to auto-update options.

b0ba4ab

Adding Ajax disable.

6bd8a3c

Adding state for Enabling/Disabling.

e20237f

Enabling Ajax for themes enabling/disabling.

82603be

ronalfy changed the title ~~Concept: Add Ajax to Plugins Screen~~ Concept: Add Ajax to Plugin and Themes Screen Apr 5, 2020

ronalfy marked this pull request as ready for review April 5, 2020 01:50

This was referenced Apr 5, 2020

HTML Abstraction #64

Closed

Allow Ajax for Enabling Auto-Updates on Plugins/Themes #65

Closed

Allowing auto-updates themes single-site.

401168a

ronalfy changed the title ~~Concept: Add Ajax to Plugin and Themes Screen~~ Add Ajax to Plugin and Themes Screen Apr 5, 2020

Changing to theme overlay since the JS can't be captured because it's…

cb54fe9

… dynamic.

pbiron suggested changes Apr 7, 2020

View reviewed changes

Comment thread functions.php Outdated

Merge branch 'master' into add/ajax-plugins

8e57087

Ronald Huereca added 2 commits April 16, 2020 17:50

Adding necessary class wrappers for JS events.

97c62fb

Removing spin reference.

6af450b

pbiron suggested changes Apr 16, 2020

View reviewed changes

Comment thread functions.php Outdated

Comment thread js/wp-autoupdates.js

Ronald Huereca added 4 commits April 16, 2020 20:00

Changing version name to constant.

7da442f

Adding back in animation for Ajax.

306b380

Changing verbiage.

8ff6f2b

Adding back in animation and blur/focus on success.

6cd3256

ronalfy requested a review from pbiron April 17, 2020 12:38

audrasjb self-requested a review April 20, 2020 20:30

audrasjb added the enhancement New feature or request label Apr 20, 2020

audrasjb added this to the 0.6.0 milestone Apr 20, 2020

TimothyBJacobs reviewed Apr 20, 2020

View reviewed changes

Comment thread functions.php Outdated

Ronald Huereca added 2 commits April 20, 2020 17:30

Removing unneeded CSS.

860f00f

Changing from filter_input to sanitize_text_field.

9b352f1

TimothyBJacobs reviewed Apr 20, 2020

View reviewed changes

audrasjb reviewed Apr 20, 2020

View reviewed changes

Switching to check_ajax_referer for nonce verification.

bbc30fd

whyisjake suggested changes Apr 20, 2020

View reviewed changes

pbiron suggested changes Apr 20, 2020

View reviewed changes

Ronald Huereca added 2 commits April 20, 2020 18:39

Adding late escaping per security recommendations.

af2480b

Avoiding enqueuing in non-network-admin plugins/themes screen.

ca4f685

ronalfy requested review from pbiron and whyisjake April 20, 2020 23:50

pbiron approved these changes Apr 21, 2020

View reviewed changes

whyisjake approved these changes Apr 21, 2020

View reviewed changes

audrasjb merged commit 597a5bb into WordPress:master Apr 21, 2020

	'enabled_count' => '(' . $enabled_count . ')',
	'enabled_count' => '(' . absint( $enabled_count ) . ')',

Conversation

ronalfy commented Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paaljoachim commented Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronalfy commented Apr 4, 2020

Uh oh!

pbiron commented Apr 4, 2020

Uh oh!

ronalfy commented Apr 4, 2020

Uh oh!

paaljoachim commented Apr 4, 2020

Uh oh!

pbiron commented Apr 4, 2020

Uh oh!

pbiron commented Apr 4, 2020

Uh oh!

ronalfy commented Apr 5, 2020

Uh oh!

ronalfy commented Apr 5, 2020

Uh oh!

ronalfy commented Apr 5, 2020

Uh oh!

pbiron commented Apr 5, 2020

Uh oh!

ronalfy commented Apr 5, 2020

Uh oh!

ronalfy commented Apr 5, 2020

Uh oh!

pedro-mendonca commented Apr 5, 2020

Uh oh!

ronalfy commented Apr 5, 2020

Uh oh!

pbiron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ronalfy commented Apr 7, 2020

Uh oh!

pbiron commented Apr 7, 2020

Uh oh!

ronalfy commented Apr 16, 2020

Uh oh!

pbiron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

audrasjb commented Apr 20, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronalfy commented Apr 20, 2020

Uh oh!

audrasjb 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!

whyisjake 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ronalfy commented Apr 4, 2020 •

edited

Loading

paaljoachim commented Apr 4, 2020 •

edited

Loading