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

Remove Jumpstart #12747

Merged
merged 5 commits into from
Jun 20, 2019
Merged

Remove Jumpstart #12747

merged 5 commits into from
Jun 20, 2019

Conversation

simison
Copy link
Member

@simison simison commented Jun 18, 2019

Removing everything jumpstart related.

Basically informs me that I should first PR a change which just modifies minimum to hide Jumpstart from Jetpack, and then PR separately follow-ups to clean up the code. Got convinced by @dereksmart that this is solid. (p1560894750228800-slack-jetpack-dna). If further testing reveals surprises that take more than a day to fix, we can reassess and do the simple hiding instead.

Jumpstart might normally get disabled once some module toggles are changed. (Will still need to confirm if this is the case). If so, should we keep Jumpstart for those who didn't activate any features in the checklist, @jeffgolenski?

See inline comments for some todos.

Resolves #12601

Changes proposed in this Pull Request:

  • Remove Jumpstart feature

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Removes existing feature.

Testing instructions:

  • Create a new Jetpack site and connect it
  • From Calypso checklist, click back to wp-admin without changing settings — you shouldn't see jumpstart and no errors, observe also PHP log.
  • With another site, when in Calypso, turn on some features in settings and then return to wp-admin. Still no jumpstart and no errors.
  • Try different plan tiers

Proposed changelog entry for your changes:

  • Remove feature to activate recommended features with one click

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D29600-code before merging this PR. Thank you!

@@ -3,6 +3,7 @@
/_inc/blocks
/_inc/build
/_inc/client/**/test/*.js
/_inc/jetpack-modules.models.js
Copy link
Member Author

Choose a reason for hiding this comment

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

ES5 file that was eslint-erroring a whole lotta. :-)

@jetpackbot
Copy link
Collaborator

jetpackbot commented Jun 18, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: July 2, 2019.
Scheduled code freeze: June 25, 2019

Generated by 🚫 dangerJS against 8ecdd06

if ( ! Jetpack_Options::get_option( 'jumpstart' ) ) {
Jetpack_Options::update_option( 'jumpstart', 'new_connection' );

$jetpack = Jetpack::init();
Copy link
Member Author

@simison simison Jun 18, 2019

Choose a reason for hiding this comment

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

Need to investigate what was going on here and if jumpstart option is used to somehow detect the state of Jetpack, not just state of Jumpstart? Update: probably not, says Derek

@simison simison requested a review from a team June 18, 2019 22:09
@simison simison changed the title Remove/jumpstart Remove Jumpstart Jun 18, 2019
@simison
Copy link
Member Author

simison commented Jun 18, 2019

Need to investigate what's up with this:

'recommended description' => 'Jumpstart Description',

Update: those lines can be removed, "recommended features" is synonym of jumpstart

Update: removed lines

@@ -93,7 +93,6 @@ public static function get_option_names( $type = 'compact' ) {
'identity_crisis_whitelist', // (array) An array of options, each having an array of the values whitelisted for it.
Copy link
Member Author

@simison simison Jun 18, 2019

Choose a reason for hiding this comment

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

Noting that I got heaps of phpcs errors from this file and had to use --no-verify to skip tests and commit:

FILE: packages/options/legacy/class.jetpack-options.php
----------------------------------------------------------------------
FOUND 39 ERRORS AND 10 WARNINGS AFFECTING 41 LINES
----------------------------------------------------------------------
....

@simison simison requested a review from jeffgolenski June 18, 2019 22:21
@simison simison added this to the 7.5 milestone Jun 18, 2019
@simison simison added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review This PR is ready for review. labels Jun 18, 2019
@simison
Copy link
Member Author

simison commented Jun 18, 2019

I'd like early testing/reviews on this even tho I'd still consider it also to be in progress.

sirreal
sirreal previously approved these changes Jun 19, 2019
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.

This seems to work fine and some quick digging around doesn't reveal any leftover jumpstart code, although I'm sure there's some 🙂

I do think we need some careful consideration:

  • What's enabled by default?
  • What would have been enabled by jumpstart?
  • What would be covered by the new checklist?
  • How does the following the checklist differ from triggering jumpstart?

I'd like to have a clear understanding of the above written out so we can refer to this PR and have clear expectations.

jeherve
jeherve previously approved these changes Jun 19, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

image

❤️

This looks good to me. I'd be tempted to merge it sooner rather than later, so we can test it with all future PR reviews in the next few days.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] In Progress [Status] Needs Review This PR is ready for review. labels Jun 19, 2019
@dereksmart
Copy link
Member

This tested well for me too. Would be happy to merge after resolving conflicts.

@simison
Copy link
Member Author

simison commented Jun 19, 2019

Rebased & removed lockfile changes

@simison
Copy link
Member Author

simison commented Jun 19, 2019

What's enabled by default?
What would have been enabled by jumpstart?
What would be covered by the new checklist?

Here's a list of tasks in the checklist — I'm adding ✅to those that were also enabled by Jumpstart:

  • Downtime Monitoring ✅
  • Automatic Plugin Updates
  • WordPress.com sign in ✅
  • Site Accelerator ✅
  • Lazy Load Images ✅
  • Video Hosting
  • Enhanced Search

Things that were always auto-enabled regardless:

  • Brute force login attack protection
  • Backup and Scan
  • Spam filtering

@matticbot
Copy link
Contributor

simison, Your synced wpcom patch D29600-code has been updated.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This still looks good to me. Merging.

@jeherve jeherve merged commit 9576f91 into master Jun 20, 2019
@jeherve jeherve deleted the remove/jumpstart branch June 20, 2019 10:10
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 20, 2019
@lezama
Copy link
Contributor

lezama commented Jun 20, 2019

+16 −820 ❤️

jeherve added a commit that referenced this pull request Jun 20, 2019
jeherve pushed a commit that referenced this pull request Jun 27, 2019
* Remove Jumpstart

* Add _inc/jetpack-modules.models.js to eslintignore

* Revert lockfile change

* Update build-module-headings-translations.php

* Update composer.lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Jumpstart [Pri] High Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove recommended features prompt (aka Jumpstart)
7 participants