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

Plans: Introduce and hook up Jetpack product installer #31684

Merged
merged 49 commits into from
Mar 28, 2019

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Mar 22, 2019

This PR introduces a Jetpack product installer in the plans "My plan" page, where the new security checklist lives. This is aiming to replace the existing client-side Jetpack plugin setup process that happens after someone purchases a Jetpack plan.

For more context: p5XAZ9-2c7-p2

This is a big PR - I'm happy to split out some of the functionality into multiple PRs, but for the sake of testing, and given the range of this task I wrapped it as a single PR.

Changes proposed in this Pull Request

This PR introduces state, data layer and a component for installing Jetpack products, along with some UI changes in the thank you card and some behavioral changes. Uses an existing endpoint and D25853-code for the installation on the server side. A detailed list of changes is as follows:

  • Introduce action creators for:
    • Starting product install
    • Requesting and receiving current product install status
  • Introduce reducer for product install status
  • Introduce data layer for:
    • Starting product install
    • Requesting and receiving current product install status
  • Introduce several new selectors:
    • getPluginKeys selector
    • getJetpackProductInstallStatus selector
    • getJetpackProductInstallProgress selector
  • Update and cleanup tests premium plugin selector tests.
  • Introduce Jetpack product installer component that:
    • Uses D25853-code and an existing endpoint for installing, activating and configuring our product plugins
    • Handles installation and configuration process.
    • Retries when it should and can, provides insights and next actions for the user when it can't proceed further.
  • Update the thank you card in the My plan page with the new UI.
  • Point the user after checkout to My plan page with the new plan setup and the checklist.
  • Add tests for:
    • Product install status reducer
    • getPluginKeys selector
    • getJetpackProductInstallStatus selector
    • getJetpackProductInstallProgress selector

Testing instructions

  • Apply D25853-code on your WP.com sandbox.
  • Sandbox public-api.wordpress.com.
  • Do either of the following:
    • Connect a fresh site with the latest Jetpack (add &calypso_env=development to the connection URL), and buy a plan, then observe when you reach the My Plan page.
    • Use an existing connected Jetpack site with a plan, but delete OR just deactivate Akismet and VaultPress before the next step, and go to http://calypso.localhost:3000/plans/my-plan/:siteSlug?thank-you where :siteSlug is the slug of your Jetpack site.
      • You can use WP CLI's wp plugin deactivate akismet vaultpress --uninstall to deactivate and uninstall the plugins, or wp plugin deactivate akismet vaultpress to just deactivate the plugins on the Jetpack site.
  • Observe the installation progress.
  • Verify that if setup fails, it retries several times before displaying an error message. You can edit the endpoint and force vaultpress_status to be vaultpress_error to witness this.
  • Verify that if setup succeeds, you are presented with a success message.
  • Verify that after a success message, Akismet and VaultPress have been setup for your site properly.

Preview

Starting:

Progress:

More progress:

Even more progress:

Success:

Persistent error and next actions:

@tyxla tyxla added Jetpack [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Pri] Normal Schedule for the next available opportuinity. [Status] In Progress [Type] Task NUX labels Mar 22, 2019
@matticbot
Copy link
Contributor

@tyxla tyxla self-assigned this Mar 22, 2019
@tyxla
Copy link
Member Author

tyxla commented Mar 28, 2019

Thanks for the thorough reviews, @ockham and @sirreal!

One architectural question: The JetpackProductInstall component is in charge of quite a lot of network fetching/polling logic (retries, error handling etc). This was a pattern that I thought we were hoping to get away from (i.e. UI components and their state and lifecycle methods handling this kind of thing).

I thought that the data-layer had some tooling in place for some of these things, such as those retries. Did you consider those and decided against (because you were missing some functionality)?

In a similar vein, concerning those functions that are polling installation status: I vaguely remember that we have a few polling endpoints, such as theme upload (which is pre-data-layer) however. Would something like that be an alternative to the separate status polling actions?

That's a fair question, @ockham! Well, I did consider a more complex data layer implementation initially, but then after doing a couple of experiments discovered that it would be much less flexible than what the current approach suggests. With the data layer approach much of what's happening would remaing behind the scenes, while in the current approach we're more flexible and have the possibility for the UI to benefit from knowing about what's going on under the hood. Also, as Jon mentions in #31684 (review) this aligns with our current architecture already, so it's not a new concept. The data layer one would definitely be helpful for some cases (polling itself), but we'd like to do more than that, as it's visible in the implementation.

it would be nice to see this kind of plugin install flow take place outside of the render tree, but this does align with a lot of our existing architecture (query components) and it's not a blocker for me. I'm curious whether you considered other approaches, I recall deciding to use a similar approach when I last looked at this problem.

Well, this is actually nota accurate - the plugin install flow takes place outside of the render tree. Close the window, and you'll still get your plugins installed :) What we're doing here is much simpler than the previous approach you're talking about - we're only displaying UI based on the installation status, and not doing anything about the installation itself, more than just triggering it. As mentioned above, I did consider a data layer approach, but it demonstrated to be much less flexible and usable for our UI-driven use case.

I'd like to see this submitted to a thorough call for testing before release, but I don't see reason to hold this back.

Of course! This will be shipped under a feature flag (see jetpack/checklist flag), and I'm planning to post a call for testing for it, as it's a pretty big change.

@tyxla
Copy link
Member Author

tyxla commented Mar 28, 2019

@jeffgolenski @keoshi @michelleweber I've updated the copy as you all suggested:

It looks much shorter and I really dig it now. Thank you!

@tyxla
Copy link
Member Author

tyxla commented Mar 28, 2019

@ockham, @sirreal @keoshi @jeffgolenski @michelleweber thank you all for the reviews and awesome feedback!

This is ready for another code and design review. Thank you!

@tyxla tyxla removed the [Status] Needs Copy Review Add this when you'd like to get a review / feedback from the Editorial team on your PR label Mar 28, 2019
@tyxla tyxla requested review from a team, ockham, keoshi and sirreal March 28, 2019 10:57
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I did not retest, but I'm happy with the way this is looking 👍

* @param {Number} vaultpressKey VaultPress key.
* @return {Object} Action object.
*/
export const startJetpackProductInstall = ( siteId, akismetKey, vaultpressKey ) => ( {
Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree with @ockham, but I also think this is unlikely to introduce any real problems ever, so I'm not especially concerned either way 🙂

@tyxla tyxla removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 28, 2019
Copy link
Contributor

@keoshi keoshi left a comment

Choose a reason for hiding this comment

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

Looking absolutely great in my testing! Thanks again, Marin! 🛳

@keoshi keoshi 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 Mar 28, 2019
@tyxla
Copy link
Member Author

tyxla commented Mar 28, 2019

Thanks everybody! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Checklist [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. Jetpack NUX [Pri] Normal Schedule for the next available opportuinity. [Type] Task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants