Skip to content

Jetpack REST connection: Finalize #22133

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

Merged
merged 92 commits into from
Aug 18, 2025
Merged

Conversation

nbradbury
Copy link
Contributor

@nbradbury nbradbury commented Aug 14, 2025

This PR does the following:

  • Implements the Finalize step of the Jetpack REST connection flow, which activates the stats module
  • Changes the experimental feature to only be available in the Jetpack app since it's a Jetpack-only feature
  • Starts the connection flow from notifications

Note that after completing successfully and returning to My Site, you're asked for your application password again. I'm not sure why this is happening but I'll defer resolving this for a future PR (and I'm open to suggestions!).

Test A

  • Clear storage or fresh install
  • Login to a self-hosted site that doesn't have Jetpack installed
  • Enable the app password and Jetpack REST connection experimental features
  • My Site > Stats > Install Jetpack
  • Ensure the flow works as expected and completes
  • Click Done to return to My Site and notice stats are available

Test B

  • Clear storage or fresh install
  • Login to a self-hosted site that doesn't have Jetpack installed
  • Enable the app password and Jetpack REST connection experimental features
  • Switch to the Notifications tab
  • Install Jetpack
  • Ensure the flow works as expected and completes
  • Click Done to return to Notifications and notice notifications are available
  • Switch to My Site and notice stats are available

nbradbury and others added 30 commits August 12, 2025 08:03
Copy link

codecov bot commented Aug 15, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 39.43%. Comparing base (763ed78) to head (4280ce0).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines Patch % Lines
.../org/wordpress/android/fluxc/store/JetpackStore.kt 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #22133   +/-   ##
=======================================
  Coverage   39.43%   39.43%           
=======================================
  Files        2149     2149           
  Lines      101872   101872           
  Branches    15604    15604           
=======================================
  Hits        40177    40177           
  Misses      58126    58126           
  Partials     3569     3569           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…-Android into feature/jetpack-connect-finalize

Conflicts:
	WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackConnector.kt
	WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionScreen.kt
	WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModel.kt
	WordPress/src/main/res/values/strings.xml
@nbradbury nbradbury requested a review from Copilot August 18, 2025 13:13
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the final step of the Jetpack REST connection flow, enabling stats module activation upon successful connection. It also restricts the experimental feature to Jetpack-only builds and adds support for initiating the connection flow from the notifications tab.

  • Implements the finalize step that activates the stats module after Jetpack connection
  • Updates experimental feature visibility to only show in Jetpack app debug builds
  • Adds support for starting the connection flow from the notifications screen

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
JetpackStoreTest.kt Updates test mocks to use isActiveModuleEnabled method instead of string parsing
JetpackStore.kt Replaces string-based module checking with dedicated method call
strings.xml Removes "(Simulated)" from setup title and adds error message for stats activation
StatsConnectJetpackActivity.kt Adds logic to start REST connection flow when available, with Jetpack app restriction
ExperimentalFeaturesViewModel.kt Restricts Jetpack REST connection feature to Jetpack app debug builds only
NotificationsListFragment.kt Adds support for starting Jetpack REST connection from notifications tab
JetpackStatsModuleHelper.kt New helper class for activating and verifying stats module activation
JetpackRestConnectionViewModel.kt Implements finalize step, connection source tracking, and stats module activation
JetpackRestConnectionScreen.kt Adds support for Done button and stats activation error display
JetpackRestConnectionActivity.kt Adds connection source parameter handling and Done button functionality
JetpackConnector.kt Minor formatting change (trailing comma)

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@nbradbury nbradbury marked this pull request as ready for review August 18, 2025 13:21
@nbradbury nbradbury requested a review from adalpari August 18, 2025 13:21
@adalpari
Copy link
Contributor

Note that after completing successfully and returning to My Site, you're asked for your application password again. I'm not sure why this is happening but I'll defer resolving this for a future PR (and I'm open to suggestions!).

It seems like the site connection step might be saving the site without taking into account the current AP credentials. Probably those fields are null when creating the site object before passing it to the SiteStore or the SQLUtils. A potential solution is to check if the site exists and use the current credentials to build/update the object.

Screenshot 2025-08-18 at 16 16 45

@adalpari
Copy link
Contributor

Notice that if the account has more sites linked, the same happens to all of them:
Screenshot 2025-08-18 at 16 20 49

Copy link
Contributor

@adalpari adalpari left a comment

Choose a reason for hiding this comment

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

PR LGTM! The new interface and UX look neat, nice job!
I left a comment about the AP reset state, but not really connected to this PR, as it can be achieved as a separate one in the ongoing project.

@nbradbury
Copy link
Contributor Author

Notice that if the account has more sites linked, the same happens to all of them:

Hmm, I'm simply calling siteStore.fetchSites to fetch JP sites. So this problem may not be specific to this PR, but instead related to how we're fetching and saving sites.

@nbradbury nbradbury merged commit c7ba760 into trunk Aug 18, 2025
27 checks passed
@nbradbury nbradbury deleted the feature/jetpack-connect-finalize branch August 18, 2025 14:34
@adalpari
Copy link
Contributor

Notice that if the account has more sites linked, the same happens to all of them:

Hmm, I'm simply calling siteStore.fetchSites to fetch JP sites. So this problem may not be specific to this PR, but instead related to how we're fetching and saving sites.

Hmm yes. I can have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants