Sidebar upsell: show only on connected sites#47967
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 1 file.
|
keoshi
left a comment
There was a problem hiding this comment.
Confirmed:
- The button is not visible when the site is not connected
- The button shows up when it's connected
- When disconnecting the user account (site-only connection) the button is visible. Clicking it correctly takes users to the checkout. However, checkout fails because the user is not connected to the site. Unrelated and not blocking, though we could show the button only when site+user are connected.
27c82e5 to
5d908bc
Compare
|
Nice one! I'll add that and I also like your method for tests more. Looks cleaner. |
This reverts commit 27c82e5.
6e35adb to
66199db
Compare
There was a problem hiding this comment.
Pull request overview
Updates the admin-ui package’s “Upgrade Jetpack” sidebar upsell so it only appears when the site is usable for upgrades (i.e., connected and not in offline mode), aligning the UI with actual upgrade capability on self‑hosted installs.
Changes:
- Gate the upsell behind offline-mode detection and “site + current user connected” checks.
- Add PHPUnit coverage for “site not connected” and “offline mode” hiding behavior.
- Add a changelog fragment describing the behavior change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| projects/packages/admin-ui/src/class-admin-menu.php | Adds offline-mode + connection gating to the upgrade submenu and its style enqueue, with an injectable connection manager for tests. |
| projects/packages/admin-ui/tests/php/bootstrap.php | Ensures connection-related classes are available during the test bootstrap. |
| projects/packages/admin-ui/tests/php/Admin_Menu_Test.php | Extends tests to assert the upsell is hidden when offline or when the site isn’t connected. |
| projects/packages/admin-ui/changelog/update-upsell-sidebar-hide-when-not-connected | Adds a changelog entry for the upsell visibility change. |
| if ( ! class_exists( 'Jetpack_Options' ) ) { | ||
| require_once __DIR__ . '/../../../connection/legacy/class-jetpack-options.php'; | ||
| } | ||
|
|
||
| if ( ! class_exists( '\Automattic\Jetpack\Connection\Manager' ) ) { | ||
| require_once __DIR__ . '/../../../connection/src/class-manager.php'; | ||
| } | ||
|
|
There was a problem hiding this comment.
The test bootstrap is hard-coding monorepo-relative paths into require_once for the connection package. This bypasses the Composer dev dependency (automattic/jetpack-connection) and can break when this package is used/tested outside the monorepo (e.g., mirrored repo / different directory layout). Prefer relying on Composer autoload (or resolving the dependency via vendor/) rather than ../../../connection/... paths.
| if ( ! class_exists( 'Jetpack_Options' ) ) { | |
| require_once __DIR__ . '/../../../connection/legacy/class-jetpack-options.php'; | |
| } | |
| if ( ! class_exists( '\Automattic\Jetpack\Connection\Manager' ) ) { | |
| require_once __DIR__ . '/../../../connection/src/class-manager.php'; | |
| } | |
| if ( ! class_exists( 'Jetpack_Options' ) || ! class_exists( '\Automattic\Jetpack\Connection\Manager' ) ) { | |
| throw new RuntimeException( | |
| 'Missing test dependency: automattic/jetpack-connection must be installed and autoloaded via Composer.' | |
| ); | |
| } |
| private static function is_site_and_user_connected() { | ||
| $connection_manager = self::$connection_manager; | ||
| if ( ! $connection_manager && class_exists( '\Automattic\Jetpack\Connection\Manager' ) ) { | ||
| $connection_manager = new \Automattic\Jetpack\Connection\Manager(); |
There was a problem hiding this comment.
is_site_and_user_connected() instantiates a new \Automattic\Jetpack\Connection\Manager every time it runs when no dependency is injected. Since should_show_upgrade_menu() is called from multiple hooks in the same request (menu registration and enqueue), this can create multiple Manager instances unnecessarily. Consider caching the created Manager in self::$connection_manager after instantiation so subsequent calls reuse it.
| $connection_manager = new \Automattic\Jetpack\Connection\Manager(); | |
| $connection_manager = new \Automattic\Jetpack\Connection\Manager(); | |
| self::$connection_manager = $connection_manager; |
| public function test_upgrade_menu_item_hidden_when_site_not_connected() { | ||
| wp_set_current_user( self::$admin_user_id ); | ||
| $connection = $this->getMockBuilder( 'Automattic\Jetpack\Connection\Manager' ) | ||
| ->disableOriginalConstructor() | ||
| ->getMock(); | ||
| $connection->expects( $this->atLeastOnce() ) | ||
| ->method( 'is_connected' ) | ||
| ->willReturn( false ); | ||
| Admin_Menu::set_connection_manager( $connection ); | ||
|
|
||
| Admin_Menu::init(); | ||
| do_action( 'admin_menu' ); | ||
|
|
||
| $this->assertUpgradeMenuItemAbsent(); | ||
| } |
There was a problem hiding this comment.
New behavior requires both site and user to be connected (is_user_connected), but the tests only cover the site-not-connected case and offline mode. Add coverage for the case where the site is connected but the current user is not (for both menu item registration and stylesheet enqueue) to ensure the new gating logic is enforced.
| Significance: minor | ||
| Type: changed | ||
|
|
||
| Sidebar upsell: hide when site is not connected to Jetpack.com. |
There was a problem hiding this comment.
The changelog entry text is inconsistent with the behavior introduced in this PR: it mentions “Jetpack.com” (the connection is to WordPress.com) and doesn't mention the offline-mode suppression. Please update the entry text to accurately reflect the conditions under which the sidebar upsell is hidden.
| Sidebar upsell: hide when site is not connected to Jetpack.com. | |
| Hide the sidebar upsell when the site is not connected to WordPress.com or is in offline mode. |
…cted test, fix changelog Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jeherve
left a comment
There was a problem hiding this comment.
This looks good, should be good to merge.
Not a blocker, more of a question. We typically do not add class_exists and is_callable checks for those methods in other packages in the codebase, mostly because the package is required by the consuming package so we know those are available. Do we really need those here?
Fixes JETPACK-1518
Closes #47993
Follow-up to #47418
Proposed changes
Before

After
Other information
Related product discussion/links
Does this pull request change what data or activity we track or use?
Testing instructions
is_free_plan()return true alwaysadd_filter( 'jetpack_offline_mode', '__return_true' );