Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jetpack Checklist: Implement Jetpack Backups task #32152

Merged
merged 13 commits into from
Apr 15, 2019

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Apr 9, 2019

Changes proposed in this Pull Request

  • Implements the Jetpack Backups task for all use cases.

Previews

Note: primary buttons in the guided tour steps are no longer primary, and there might be small textual changes as feedback is addressed in the PR.

Pre-Rewind sites install and configure backups (VaultPress) automatically:

Rewind sites require credentials, so there's a manual task for that:

Clicking the "Do it" button leads to the Security Settings page:

Clicking the "Add site credentials" button displays a confirmation section if the credentials have to be input manually:

Clicking the "Add site credentials" button displays a confirmation section if the credentials can be auto-provisioned (for example in Pressable):

After inputting credentials or auto-provisioning them successfuly, user gets a success message and a nudge to go back:

Users can use the checklist to change credentials if they want:

Users can't do anything with the VP task, it's passive:

Testing instructions

  • Checkout this branch or spin it up on calypso.live.
  • Start a new Jurassic Ninja site with the latest bleeding edge and connect it to WP.com, pick the free plan.
  • Open /plans/:site where :site is the slug of your site.
  • Buy a Premium or Professional plan.
  • Wait for the setup to finish, and verify that the Backups task is automatically setup - first displayed as "in progress", and when finished, displayed as "finished".
  • Deactivate VaultPress from wp-admin.
  • Go to VP MC and enable Rewind for that site.
  • Go to /plans/my-plan/:site where :site is the slug of the site.
  • Verify the "Backups & Scanning" task is now not done, and has a "Do it" button.
  • Click the button.
  • Verify the guided tour leads you to add credentials, as shown on the screenshots (for manual credentials entry).
  • Verify the after you add credentials, and you click the button to go back to the checklist, the Backups and Scanning task is now displayed as complete.
  • Pick a Pressable site.
  • Go to /settings/security/:site where :site is the slug of the Pressable site.
  • If you already have credentials for backups, click the green icon to edit them, and click "Revoke credentials" to remove them.
  • Go to /plans/my-plan/:site where :site is the slug of the site.
  • Verify the "Backups & Scanning" task is now not done, and has a "Do it" button.
  • Click the button.
  • Verify the guided tour leads you to add credentials, as shown on the screenshots (for auto provisioning).
  • Verify the after credentials are auto-provisioned, and you click the button to go back to the checklist, the Backups and Scanning task is now displayed as complete.

@matticbot
Copy link
Contributor

@jeffgolenski
Copy link
Member

@shaunandrews @fditrapani I'd love to get some feedback and thoughts on how we can better create a "to do" hierarchy for our customers with the checklist here. For Jetpack, some items may be "blockers" that need to be completed for customers to get 100% value out of their plans.

As noted in the video walkthrough I pinged you both on in slack, In order for Jetpack Backups and Restores to start working, we need to connect jetpack to each customer's respective server. The images @tyxla shared above, basically outline the flow right now. There's a checklist item for it, but it's not prominent and doesn't communicate to customers it needs to happen.

One thing I want to add is a more visual "urgent" list item to the security checklist that places it on a more important level than the rest of the items. Basically customers need to set this up or the backups and restores they're paying us for won't work.

I want your feedback on adding a a pattern like this and if it makes sense to do this. In the future I'd love to entertain the idea of working this into onboarding possibily, but for now, I want to make sure people can visually see it needs to happen.

jp-checklist-scanning-alert

(Marin, don't implement this just yet. Just want some feedback on this first. Thanks! )

cc @seear

@tyxla tyxla force-pushed the update/jetpack-checklist-vp-rewind branch from 0ad1409 to d2dc252 Compare April 11, 2019 09:03
@fditrapani
Copy link
Contributor

Thanks for the ping @jeffgolenski. There is some work happening on the navigator that can help you (p9Jlb4-Nh-p2). The work doesn't look 100% solidified but I think it's heading in a good direction that we'll want to impement on our checklist.

image

The thing to note is more emphasis on the task you're working on and less emphasis on the upcoming tasks. The current checklist on WP.com doesn't quite emphasize the current task but we did remove lots of the noise around the upcoming tasks.

image

Something else to note about the screenshot above is there are some screens with multiple primary buttons. We need to make sure that pink is applied to the most important action. In a couple of instances it brings urgency to returning to the checklist vs completing the action.

image

@tyxla tyxla force-pushed the update/jetpack-checklist-vp-rewind branch from 09cb290 to f0e9f22 Compare April 11, 2019 14:35
@tyxla
Copy link
Member Author

tyxla commented Apr 11, 2019

Thanks @fditrapani - I've addressed your feedback on leaving a single primary button, none of the steps in this tour will have primary buttons, as we want to remove the main focus from getting back from the checklist:

@fditrapani
Copy link
Contributor

Awesome. Thanks @tyxla.

@tyxla
Copy link
Member Author

tyxla commented Apr 11, 2019

This is ready for code and design review.

@tyxla tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Apr 11, 2019
@tyxla
Copy link
Member Author

tyxla commented Apr 12, 2019

I think this is a pre-existing Guided Tours bug, but the point is in the wrong place:

Yeah, that can be encountered in some screen sizes indeed. As you and @fditrapani already confirmed, it's a pre-existing known bug with tours. Would be nice to get it fixed, but it's beyond this PR, I guess.

The first time I completed the flow and returned (by clicking the guided tour complete), the task did not reflect completion:

Yeah, I'm getting reports for that for other tasks too, including from @seear below. Opened an issue to track that, as it's unrelated with this PR: #32248

@tyxla
Copy link
Member Author

tyxla commented Apr 12, 2019

Anything we can do to highlight the importance of the credentials adding step, as per Jeff's comment would be welcome.

I'm happy to follow up in another PR with this. As @jeffgolenski requested, I haven't proceeded here, as it's not yet decided whether this will be the way to highlight important tasks.

Also, anything we could do to reduce clicks between this checklist and the actual credentials adding screen would be good. After clicking Do it!, there is another click on the appropriate item in Settings and yet another click for some terms-of-service.

cc @jeffgolenski about this 😉 this will need some thinking and mockups.

I saw a similar problem to Jon, where the checklist item didn't get marked as done until I navigated away from the checklist and back again.

Good point, opened an issue for that: #32248

@tyxla
Copy link
Member Author

tyxla commented Apr 12, 2019

Seems to me that feedback has either been addressed, or moved to other issues for cases where it's unrelated to the PR.

Could I please get another code review and ✅ @simison @sirreal @seear? Thanks!

@jeffgolenski
Copy link
Member

jeffgolenski commented Apr 12, 2019

Finally avoided jetpack manage problems and was able to test. Works wonderfully. I even downgraded my plan to free and re-upgraded my plan to professional and the checklist kept up. Nice!

As mentioned above. design mods coming soon for future PRs

Copy link
Member

@jeffgolenski jeffgolenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design review approved.

@jeffgolenski jeffgolenski removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Apr 12, 2019
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait for the setup to finish, and verify that the Backups task is automatically setup - first displayed as "in progress", and when finished, displayed as "finished".

This indeed happened. 👍

Screenshot 2019-04-12 at 17 57 08

Screenshot 2019-04-12 at 17 57 12

Then I did the monitor step and got back to checklist, and found that the checklist item was again "loading" and monitor was unfinished:

Screenshot 2019-04-12 at 17 57 30

Screenshot 2019-04-12 at 17 58 14

@tyxla
Copy link
Member Author

tyxla commented Apr 12, 2019

@simison I think that's the same one that Jon and Steve encountered - I've opened #32248 for it and will be tackling it separately.

@simison
Copy link
Member

simison commented Apr 12, 2019

Is it also expected that if I refresh this page, I get disconnection problem and spinners just keep spinning:

image

Site's wp-admin loads fine via URL. I'm not sandboxed.

@tyxla
Copy link
Member Author

tyxla commented Apr 12, 2019

Is it also expected that if I refresh this page, I get disconnection problem and spinners just keep spinning:

@jeffgolenski reported the same thing. It seems like an unrelated connection problem that I haven't been able to diagnose, but I asked in the Crew channel. FWIW, reconnecting fixes it, so it's unrelated with this PR.

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-connected the site, activated rewind..

Checklist doesn't infinite-spin anymore and I got to press "do it" for backups and scan.

I end up here:

Screenshot 2019-04-12 at 18 13 58

Where I'm pretty confused what to press obviously. 😅 I choose to press big plus although other things on the page look much more like buttons (even primary pink ones) than the plus icon.

I then get this:

Screenshot 2019-04-12 at 18 14 21

I now have 4 buttons to choose from 😵 I press "ok" because I know that'll bring me onwards, all other 3 buttons seem like backwards (maybe?).

I fill in credentials and get a lot of green ✅ icons scattered across my screen:

Screenshot 2019-04-12 at 18 18 17

I press "let's do it" and get one final ✅

image

...and with that, Marin, I give one for you too: ✅

Ship it! 😄

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good!

Testing experience otherwise was a bit rough but I'd expect visual issues raised up here be ironed out in follow ups. :-)

@tyxla
Copy link
Member Author

tyxla commented Apr 12, 2019

Thank you for the thorough testing and review 🙌 Will ship on Monday and continue iterating on making it less rough 😅

@tyxla tyxla added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 12, 2019
@tyxla
Copy link
Member Author

tyxla commented Apr 12, 2019

And I've opened #32249 to start thinking about how we can simplify and enhance the backups task to have less buttons and clicks and make it generally less rough 😅

@simison
Copy link
Member

simison commented Apr 12, 2019

Also noting that once I cancel monitor and remove SSH credentials, checklist is back to this again as expected:

image

✅ ✅ 👍 🌮

@tyxla tyxla merged commit 114c684 into master Apr 15, 2019
@tyxla tyxla deleted the update/jetpack-checklist-vp-rewind branch April 15, 2019 08:16
@sgomes
Copy link
Contributor

sgomes commented Apr 15, 2019

@tyxla : I just saw this PR when looking at the commit log and wanted to point out that you didn't add the new tour to layout/guided-tours/config.js. This means that it'll be ignored by the guided tours selector (client/state/ui/guided-tours/selectors) and may not work properly if e.g. it ever gets a when condition.

@tyxla
Copy link
Member Author

tyxla commented Apr 15, 2019

Thanks for that @sgomes 👍 I'll make sure to add it soon.

@tyxla
Copy link
Member Author

tyxla commented Apr 15, 2019

#32274 registers the new guided tour properly, thanks again @sgomes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants