First pass -- VaultPress deactivation if Rewind is running. by georgestephanis · Pull Request #8378 · Automattic/jetpack

georgestephanis · 2017-12-18T22:23:44Z

I may (probably will) need to revisit where in the $features array I'm checking to see if Rewind is enabled.

WIP.

dmsnell · 2017-12-18T22:38:07Z

class.jetpack.php

any thoughts on an early-abort-over-nested pattern, even though we can't avoid some nesting here?

if ( false !== $rewind_enabled ) { return $rewind_enabled; } jetpack_require_lib( … ) … set_transient( … ) return $rewind_enabled;

anyway no big deal just noticed this and how the "main code" was nested

Eh, I'm not huge on it either way, this is just how I'd always structured transient checks in the past. I'm not emotionally invested either direction.

dmsnell · 2017-12-18T22:50:49Z

class.jetpack.php

this is the topic of the conversation 😄

I think that you are checking the right values here, but we have overlap in terms between the Rewind system and Jetpack itself, so this is the content of the review

this code is checking if a site has "anything to do with Rewind" including that it could have Rewind activated but it's missing the necessary plan.

this does not verify that Rewind is actively backing up and protecting a site, which is probably okay for the purposes we are wanting - know whether or not to prompt to deactivate VaultPress (because if it's missing the plan then VaultPress is probably also missing the plan and if it should be backing up but isn't it's likely due to some external problem)

@dmsnell what do you think about setting a transient/option with the response from the /rewind endpoint that we're using for the UI?

https://github.com/Automattic/jetpack/blob/master/_inc/lib/class.core-rest-api-endpoints.php#L656-L672

We could also have it return a different string 'available' or 'enabled' based on its status and test against those.

I don't care which, but we need to decide the conditions for it appearing.

So, in EITHER case we don't want VaultPress active, because they should be using rewind. So the original code is GTG on that front

Yeah, the /rewind endpoint we're getting in that link above includes a response that we're using to determine it. I think it's the way to go.

array( 'reason' => 'host_not_supported', 'state' => 'active', // Can also be 'available' or 'unavailable' 'downloads' => array(), 'last_updated' => 1513366050 );

'state' => 'active', // Can also be 'available' or 'unavailable'

^^^ we will want to make sure we stick to the actively confusing definitions because our terms overlap with different meanings - p9rlnk-1r-p2

what do you think about setting a transient/option with the response

it's probably about time we look at latency and performance issues and address it appropriately. I doubt putting it in the API call is better than putting it in get_status() and trusted_get_status() but I don't know if you are talking about a transient on the Jetpack side

I may (probably will) need to revisit where in the `$features` array I'm checking to see if Rewind is enabled.

…ining status of Rewind

georgestephanis · 2017-12-20T16:31:44Z

3rd-party/vaultpress.php

-		'plugin' => $plugin_file,
-	);
-	$deactivate_url = wp_nonce_url( add_query_arg( $query_args, admin_url( 'plugins.php' ) ), "deactivate-plugin_{$plugin_file}" );
+	deactivate_plugins( $plugin_file );


"Murder most foul…" -- Shakespeare, Hamlet, Act 1 Scene V

georgestephanis · 2017-12-20T16:35:01Z

There was 1 failure:

WP_Test_Jetpack_Sync_Queue::test_queue_lag

Failed asserting that 23 matches expected 6.

/tmp/wordpress-latest/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-queue.php:61

Restarting job now -- could have just been cranky?

dereksmart

Made some changes, works well for me

georgestephanis · 2017-12-20T17:22:17Z

Are we okay with it only being deactivated off of admin_init() ? I think that's good as it avoids race conditions for busy sites (the deactivation wouldn't happen on front-end visits), and the deactivation only happening on admin_notices also avoids anything triggering on login pages.

dereksmart · 2017-12-20T17:26:07Z

I think that's good

@georgestephanis I agree!

* First pass -- VaultPress deactivation if Rewind is running. I may (probably will) need to revisit where in the `$features` array I'm checking to see if Rewind is enabled. * Add the file to the included list for 3rd-party * Retool to new method to check for Rewind availability. * Update is_rewind_enabled() method to read the correct data for determining status of Rewind * VaultPress Murder * faulty logic

thingalon · 2017-12-20T23:52:04Z

3rd-party/vaultpress.php

+	deactivate_plugins( $plugin_file );
+	?>
+	<div class="notice notice-success">
+		<h2 style="margin-bottom: 0.25em;"><?php _e( 'Jetpack is now handling your backups.', 'jetpack' ); ?></h2>


The headline "Jetpack is now handling your backups" is not necessarily true if Rewind is available but inactive.

It sounds like (in the other thread) there is consensus that we want VaultPress disabled if Rewind is available, even if isn't active, so I don't want to tell users we're handling their backups if it's possible we're not. :)

This message should only show if Rewind is actually configured, but we will confirm that in testing leading up to our wide launch.

georgestephanis added Vault [Status] In Progress labels Dec 18, 2017

georgestephanis self-assigned this Dec 18, 2017

georgestephanis requested review from dmsnell and thingalon December 18, 2017 22:23

georgestephanis requested a review from a team as a code owner December 18, 2017 22:23

dmsnell reviewed Dec 18, 2017

View reviewed changes

georgestephanis added 3 commits December 20, 2017 10:12

First pass -- VaultPress deactivation if Rewind is running.

6d269fb

I may (probably will) need to revisit where in the `$features` array I'm checking to see if Rewind is enabled.

Add the file to the included list for 3rd-party

c5d69b6

Retool to new method to check for Rewind availability.

16f64f3

dereksmart force-pushed the add/vaultpress-deactivation-if-rewind branch from 829399e to 16f64f3 Compare December 20, 2017 15:12

dereksmart and others added 2 commits December 20, 2017 10:36

Update is_rewind_enabled() method to read the correct data for determ…

c4a5181

…ining status of Rewind

VaultPress Murder

4f40f8e

georgestephanis commented Dec 20, 2017

View reviewed changes

faulty logic

5e49213

dereksmart added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Dec 20, 2017

dereksmart approved these changes Dec 20, 2017

View reviewed changes

dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Dec 20, 2017

dereksmart added this to the vp-rewind-pressable milestone Dec 20, 2017

dereksmart merged commit e6d331b into master Dec 20, 2017

dereksmart deleted the add/vaultpress-deactivation-if-rewind branch December 20, 2017 17:32

thingalon reviewed Dec 20, 2017

View reviewed changes

eliorivero added the [Feature] Activity Log label Dec 22, 2017

oskosk modified the milestones: vp-rewind-pressable, 5.7 Dec 22, 2017

jeherve added [Status] Needs Changelog and removed [Status] Needs Changelog labels Jan 11, 2018

This was referenced Jan 22, 2018

Allow rendering of JITM's on Jetpack admin page #8555

Merged

Deactivate VP only if Rewind is active #8581

Merged

kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020

matticbot added the Touches WP.com Files label Oct 13, 2020

jeherve added the [Feature] Backups label Oct 29, 2020

Conversation

georgestephanis commented Dec 18, 2017

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!

Choose a reason for hiding this comment

Uh oh!

dereksmart Dec 19, 2017 • 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!

georgestephanis commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dereksmart left a comment

Choose a reason for hiding this comment

Uh oh!

georgestephanis commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dereksmart commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

dereksmart Dec 19, 2017 •

edited

Loading

georgestephanis commented Dec 20, 2017 •

edited

Loading

georgestephanis commented Dec 20, 2017 •

edited

Loading