CMM-2304: Detect Jetpack status for application-password sites - #23248
Closed
nbradbury wants to merge 5 commits into
Closed
CMM-2304: Detect Jetpack status for application-password sites#23248nbradbury wants to merge 5 commits into
nbradbury wants to merge 5 commits into
Conversation
Sites added with an application password never had their Jetpack fields populated, so the Jetpack app offered to install a plugin that was already there and the WordPress app told sites without Jetpack that they had it. The WPAPI fetch now derives isJetpackInstalled from the site's REST namespaces and reads the connection state and WP.com blog id over wordpress-rs, and the two unsolicited Jetpack-app promos are gated on whether the selected site can actually use that app.
Collaborator
Generated by 🚫 Danger |
Contributor
|
|
Contributor
|
|
The guard clauses put the function at three returns against a limit of two, so they collapse into a single condition.
Contributor
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
CMM-2304 lists MenuActivity's missing navigation branches under "Separate
bugs found while tracing" and asks for them to be split out, so revert them
here and keep this branch to Jetpack detection.
The change is unrelated to detection: those actions were silently dropped by
`else -> {}` regardless of a site's Jetpack state. It also only covers two of
the three missing branches -- ShowJetpackRemovalStaticPostersView is still
unhandled -- which is better resolved together in the follow-up.
Detecting `isJetpackConnected` for application-password sites was not enough to open Stats. The flag says the site is connected to *an* account, but the blog ID belongs to whichever account made the connection, which needn't be the one signed in here. Stats are served by WordPress.com, so the request reached the right site and was refused: `403 - user cannot view stats`. Add `WpComSiteAccessChecker`, which asks whether this account can reach a site over the WordPress.com REST API -- true for sites already accessed that way, and otherwise only when the same blog ID also arrived from `/me/sites`, since the account's site list is the record of what it can reach. `ListItemActionHandler` now requires that before routing to Stats, so an unowned site falls through to the connection flow instead of a screen that errors. `canInitiateJetpackRestConnection` takes the same signal, because its "already connected" check would otherwise reject these sites and drop them into the web-view flow; the REST flow's ConnectUser step is exactly what links the signed-in account.
StatsConnectJetpackActivity pitches installing Jetpack with no branch on whether the site has it, and only consults canInitiateJetpackRestConnection once its button is tapped. Routing an already-installed site here therefore told the user to install a plugin that was in front of them -- the symptom CMM-2304 is about. Start the connection flow directly when Jetpack is installed, and never build the pitch. That flow's install step recognises Jetpack is present and moves on to connecting the account, which is the step actually missing. Sites without Jetpack are unaffected and still get the install screen.
Contributor
Author
|
Closing this. Claude made a real mess of it and introduced some errors. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
TL/DR: Fixes CMM-2304. Sites added with an application password never had their Jetpack fields populated, which produced two visible bugs. This fills them in from the site's own REST API.
The WPAPI site fetch (
ORIGIN_WPAPI) wrotename,description,hasWooCommerceand friends but leftisJetpackInstalled,isJetpackConnectedandsiteIdat their defaults. So the Jetpack app offered to install a plugin that was already installed, and the WordPress app told the owner of a plain self-hosted site that their site has the Jetpack plugin.SiteWPAPIRestClientnow readsisJetpackInstalledfrom thejetpack/REST namespace (registered only while the plugin is active) and, for application-password sites, reads the connection state and the WP.com blog id via wordpress-rsJetpackConnectionClient.status()— the only source for either. The blog id matters because Stats reads it; detecting the flags without it would just swap "Install Jetpack" for a Stats screen that errors.Three supporting changes fall out of that.
canInitiateJetpackRestConnectionnow accepts an unknown Jetpack version — the REST namespace carries no version, andcheckMinimalVersion(null, …)returns false, which would have routed these sites to the web-view flow that can't work with an application password.fetchWPAPISite(site)carries the local id onto the returned model, orSiteSqlUtilsfalls through to aSITE_ID + URLmatch, misses, and inserts a duplicate row once a real blog id is present. And the two unsolicited Jetpack-app promos in the WordPress app (app-open overlay, My Site feature card) are gated on a newSiteModel?.canUseJetpackApp();shouldRemoveJetpackFeatures()itself is untouched, since it's the feature-removal invariant for nav visibility and the Me row.Testing instructions
Jetpack app, self-hosted site with Jetpack installed:
Jetpack app, self-hosted site without Jetpack:
WordPress app, self-hosted site without Jetpack: