Skip to content

Remove legacy "My Jetpack" code#6856

Merged
dereksmart merged 3 commits intomasterfrom
remove/my-jetpack
Apr 7, 2017
Merged

Remove legacy "My Jetpack" code#6856
dereksmart merged 3 commits intomasterfrom
remove/my-jetpack

Conversation

@dereksmart
Copy link
Copy Markdown
Contributor

@dereksmart dereksmart commented Mar 31, 2017

None of this is used anymore with no plans on reinstating it. The endpoint to change primary users still lives in wpcom, and will be trivial to add back if we ever want to re-add the functionality later.

To test:

  • Build, click and do everything. Make sure nothing looks broken and you get no errors.
  • I'm mostly concerned about the css removed, but I didn't find any usage of it in my searches.

@dereksmart dereksmart added [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Type] Janitorial labels Mar 31, 2017
jeherve
jeherve reviewed Apr 3, 2017
View reviewed changes
_inc/lib/admin-pages/class.jetpack-my-jetpack-page.php
@@ -1,187 +0,0 @@
<?php
Copy link
Copy Markdown
Member

@jeherve jeherve Apr 3, 2017

Choose a reason for hiding this comment

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

In the past we've sometimes avoided to delete PHP files, and instead left them as blank to avoid fatals on some sites upon update. See 8c3790d for example, or 46bacdd. Should we do the same here?

Copy link
Copy Markdown
Contributor Author

@dereksmart dereksmart Apr 3, 2017

Choose a reason for hiding this comment

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

I think problems before came because of cached PHP filed that were requiring the removed php files. We haven't required or included this one for many releases, so it should be ok.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Needs Review This PR is ready for review. and removed [Status] Needs Review This PR is ready for review. [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Apr 3, 2017
@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented Apr 3, 2017

It's good. Found no other references to the files being deleted.

@eliorivero eliorivero 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 Apr 3, 2017
@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Apr 4, 2017 •
edited
Loading

Looks good, I love the removed lines count! I have removed one import line to avoid SCSS build errors:

Error in plugin 'sass'
Message:
    scss/jetpack-admin.scss
Error: File to import not found or unreadable: pages/connection-settings
       Parent style sheet: /home/zinigor/workspace/jetpack/scss/jetpack-admin.scss
        on line 1 of scss/jetpack-admin.scss

Otherwise LGTM once Travis finishes.
Edit: also tested with opcache enabled to make sure we're not removing a file that's currently required somewhere.

@dereksmart
Copy link
Copy Markdown
Contributor Author

dereksmart commented Apr 7, 2017

also tested with opcache enabled to make sure we're not removing a file that's currently required somewhere.

Nice.

@dereksmart dereksmart merged commit 63740d5 into master Apr 7, 2017
@dereksmart dereksmart deleted the remove/my-jetpack branch April 7, 2017 19:37
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 7, 2017
jeherve added a commit that referenced this pull request Apr 24, 2017
@jeherve
Changelog: add #6856
74c4b7b
eliorivero pushed a commit that referenced this pull request Apr 25, 2017
@jeherve @eliorivero
Changelog: 4.9 release. (#6949)
07307cb 
* Changelog: initial commit for 4.9 release.

* Changelog: add #6929

* Changelog: move old changelogs to changelog.txt

* Readme: restore deleted release post link.

The post is now live.

* Changelog: add #6853

* Changelog: add #6856

* Changelog: add #6857

* Changelog: add #6884

* Changelog: add #6885

* Changelog: add #6892

* Changelog: add #6894

* Changelog: add #6898

* Changelog: add #6899

* Changelog: add #6900

* Changelog: add #6909

* Changelog: add #6927

* Changelog: add #6947

* Chagelog: add #6958

* Changelog: add #6961

* Changelog: add #6963

* Changelog: add #6965

* Changelog: add #6986

* Changelog: add #7000

* Changelog: add #7013

* Changelog: add #7015

* Changelog: add #7019

* Changelog: add #7028

* Changelog: add #6998

* Changelog: add #6999

* Changelog: add #7044

* Changelog: add #6881

* Changelog: add #6922

* Changelog: add #6940

* Changelog: add #6962

* Changelog: add #6942

* Changelog: add #6959

* Changelog: add #7018

* Changelog: add #6948

* Changelog: add #6657

* Changelog: add #7030

* Changelog: add #7048

* Changelog: add #7031

* Changelog: add #6990

* Changelog: add #6957

* Changelog: add #7027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve left review comments

+1 more reviewer

@eliorivero eliorivero eliorivero approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Type] Janitorial

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dereksmart @eliorivero @zinigor @jeherve