Skip to content

Calypsoify: Fix Calypsoify so that it removes core menus correctly#12460

Merged
jeherve merged 1 commit intomasterfrom
fix/calypsoify-menus
May 24, 2019
Merged

Calypsoify: Fix Calypsoify so that it removes core menus correctly#12460
jeherve merged 1 commit intomasterfrom
fix/calypsoify-menus

Conversation

@apeatling
Copy link
Copy Markdown
Contributor

@apeatling apeatling commented May 23, 2019

Fix Calypsoify so that it removes core menus when looking at the plugins section of WP-Admin.

Fixes #12459

Before fix (core admin menus show):

Screen Shot 2019-05-23 at 3 11 26 PM

After fix (core admin menus hidden, only plugin menus show):

Screen Shot 2019-05-23 at 3 10 55 PM

Changes proposed in this Pull Request:

  • The admin_init hook is called after the admin_menu hook, so menus were never going to be removed correctly in the previous setup. I moved this out alongside the admin_menu hook call.

Testing instructions:

  • Go to Calypso, and select the "Plugins" menu for a connected Jetpack site with this change.
  • Confirm that you do not see the core menus.
  • Go to /wp-admin/ and confirm that Calypsoify is disabled, and you see all core menus again.

Proposed changelog entry for your changes:

  • Fixed an issue with core menus displaying incorrectly when using Calypso.

@apeatling
Fix Calypsoify so that it removes core menus when looking at the plug…
263d2e1 
…ins section of WP-Admin. The admin_init hook is called after the admin_menu hook, so menus were never going to be removed correctly in the previous setup.
@apeatling apeatling requested review from a team and mdawaffe May 23, 2019 22:14
@apeatling apeatling self-assigned this May 23, 2019
@apeatling apeatling requested a review from davemart-in May 23, 2019 22:16
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented May 23, 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: June 4, 2019.
Scheduled code freeze: May 28, 2019

Generated by 🚫 dangerJS against 263d2e1

@mdawaffe
Copy link
Copy Markdown
Member

mdawaffe commented May 23, 2019

  • Go to Calypso, and select the "Plugins" menu for a connected Jetpack site with this change.
  • Confirm that you do not see the core menus.
  • Go to /wp-admin/ and confirm that Calypsoify is disabled, and you see all core menus again.

This PR works as you describe. I did those test steps and saw what you described as expected.

I don't see as many menus as you show in your after screenshot, though. I'm not sure if that's because of different settings on my site or if there's a subtle bug with something. (My test site is also slightly broken, so… :))

Screen Shot 2019-05-23 at 4 17 01 PM

@mdawaffe
Copy link
Copy Markdown
Member

mdawaffe commented May 23, 2019

Broken in #11651. I'll retest that bug with this PR.

mdawaffe
mdawaffe approved these changes May 23, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

Looks good to me. I checked the other hooks that got moved as part of #11651, and they all get fired after admin_init, so they should be fine.

@mdawaffe
Copy link
Copy Markdown
Member

mdawaffe commented May 23, 2019

Broken in #11651. I'll retest that bug with this PR.

✅: This PR does not cause a regression for #11651.

LGTM.

@kraftbj kraftbj added this to the 7.4 milestone May 24, 2019
@kraftbj kraftbj added [Pri] High Bug When a feature is broken and / or not performing as intended [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels May 24, 2019
kraftbj
kraftbj approved these changes May 24, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

The steps to reproduce didn't move me from Calypso to Calypsoify, but manually triggering it via /wp-admin/plugins.php?calypsoify=1 confirmed the fix for me. The issue with not moving out of Calypso isn't something related to this PR, so approving this without regard for that.

@jeherve jeherve merged commit e174201 into master May 24, 2019
@jeherve jeherve deleted the fix/calypsoify-menus branch May 24, 2019 07:55
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 24, 2019
davemart-in
davemart-in reviewed May 24, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@davemart-in davemart-in left a comment

Choose a reason for hiding this comment

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

Looks like this merged, but LGTM! Thanks!

@apeatling
Copy link
Copy Markdown
Contributor Author

apeatling commented May 24, 2019

Thanks for testing!

jeherve added a commit that referenced this pull request May 24, 2019
@jeherve
Changelog: add #12460
6c4d552
jeherve added a commit that referenced this pull request May 27, 2019
@kraftbj @jeherve
7.4 Changelog (#12304)
0fb4436 
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davemart-in davemart-in davemart-in left review comments

@kraftbj kraftbj kraftbj approved these changes

@mdawaffe mdawaffe mdawaffe approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@apeatling apeatling

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Calypsoify [Pri] High

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

Calypsoify: remove_core_menus() is not working

7 participants

@apeatling @jetpackbot @mdawaffe @kraftbj @jeherve @davemart-in @matticbot