Skip to content

Route the remaining WP.com clients through WpComApiClientProvider - #23299

Open
adalpari wants to merge 2 commits into
trunkfrom
adalpari/route-wpcom-clients-through-provider
Open

Route the remaining WP.com clients through WpComApiClientProvider#23299
adalpari wants to merge 2 commits into
trunkfrom
adalpari/route-wpcom-clients-through-provider

Conversation

@adalpari

@adalpari adalpari commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

#TL:DR

Follow-up to #23295: the last two WP.com clients that bypassed
WpComApiClientProvider now go through it, so they get the locale provider and
the redacting error logger too.

Description

Follow-up to #23295 (wordpress-rs 0.8.0 migration). That PR added two things
to WpComApiClientProvider:

  • a WpComLanguageProvider, so WP.com responses come back localized (0.8.0 moved
    the locale query parameter off the endpoint params and onto the client), and
  • a WpRequestErrorLogger using the library's default redaction policy, replacing
    the removed WpRequestResult.toLogErrorString().

Review of that PR flagged that two callers construct WpComApiClient directly
rather than going through the provider, so they got neither:

  • DataViewViewModel (and therefore its subclasses ApplicationPasswordsViewModel,
    SubscribersViewModel, TermsViewModel)
  • AddSubscribersViewModel

Their WP.com responses were unlocalized, and their request failures were logged
without redaction.

Why they bypassed the provider

Both need the network-request tracking interceptor (the Chucker-based inspector),
and the provider's client didn't support interceptors — it built its own OkHttp
client with the shared wp-rs timeouts.

So rather than drop the interceptor, this PR gives the provider an optional
interceptors parameter that installs them on that OkHttp client:

fun getWpComApiClient(
    accessToken: String,
    interceptors: List<Interceptor> = emptyList(),
): WpComApiClient

Existing callers are unaffected (they pass nothing). The two former bypassers keep
request inspection and additionally gain the shared timeouts, the error logger and
the language provider.

The interceptor stays opt-in rather than being injected into the provider itself,
so this doesn't silently enable network-request tracking for every other WP.com
consumer (stats, domains, support).

Knock-on change

DataViewViewModel now takes WpComApiClientProvider in place of the raw
WpNetworkAvailabilityProvider it only used to build the client; its three
subclasses pass it through. Tests updated accordingly.

Testing instructions

  1. Build and static analysis.
  • ./gradlew assembleJetpackDebug succeeds
  • ./gradlew detekt passes
  1. Unit tests.
  • DataViewViewModelTest, TermsViewModelTest, SubscribersViewModelTest,
    ApplicationPasswordsViewModelTest pass
  1. Exercise the affected screens on a WP.com site: Subscribers (list, detail, add),
    Application Passwords, and Taxonomies/Terms.
  • All still load, paginate and search as before
  1. With the device language set to a non-English locale.
  • Server-rendered strings on those screens come back localized, confirming the
    client-level locale is now being sent
  1. Enable the network-request tracker (App Settings → debug tools) and use those
    screens.
  • Requests are still captured, confirming the interceptor survived the move

Related: follow-up to #23295

🤖 Generated with Claude Code

DataViewViewModel and AddSubscribersViewModel constructed WpComApiClient
directly, so they missed the locale provider and the redacting error logger
that #23295 added to WpComApiClientProvider. Their WP.com responses came
back unlocalized and their request failures were logged unredacted.

They bypassed the provider because they need the network-request tracking
interceptor, which the provider's client did not support. Give the provider
an optional interceptors parameter that installs them on the OkHttp client,
so these callers keep request inspection and additionally pick up the shared
timeouts, error logger and language provider.

DataViewViewModel now takes the provider instead of a raw
WpNetworkAvailabilityProvider, which its three subclasses pass through.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dangermattic

dangermattic commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot

wpmobilebot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

App Icon📲 You can test the changes from this Pull Request in Jetpack Android by scanning the QR code below to install the corresponding build.

App NameJetpack Android
Build TypeDebug
Versionpr23299-aa9b694
Build Number1498
Application IDcom.jetpack.android.prealpha
Commitaa9b694
Installation URL5prq1htjuaiqg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

wpmobilebot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

App Icon📲 You can test the changes from this Pull Request in WordPress Android by scanning the QR code below to install the corresponding build.

App NameWordPress Android
Build TypeDebug
Versionpr23299-aa9b694
Build Number1498
Application IDorg.wordpress.android.prealpha
Commitaa9b694
Installation URL1jb4c1btufedo
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 30.76923% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.04%. Comparing base (9fa0185) to head (aa9b694).
⚠️ Report is 3 commits behind head on trunk.

Files with missing lines Patch % Lines
.../android/ui/subscribers/AddSubscribersViewModel.kt 0.00% 5 Missing ⚠️
...droid/networking/restapi/WpComApiClientProvider.kt 50.00% 1 Missing ⚠️
...plicationpassword/ApplicationPasswordsViewModel.kt 0.00% 1 Missing ⚠️
...wordpress/android/ui/dataview/DataViewViewModel.kt 66.66% 0 Missing and 1 partial ⚠️
...ess/android/ui/subscribers/SubscribersViewModel.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #23299   +/-   ##
=======================================
  Coverage   38.04%   38.04%           
=======================================
  Files        2353     2353           
  Lines      128876   128874    -2     
  Branches    17940    17941    +1     
=======================================
+ Hits        49030    49032    +2     
+ Misses      75827    75824    -3     
+ Partials     4019     4018    -1     

☔ View full report in Codecov by Harness.
📢 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.

addSubscribers() wrapped a body that already returns Result in
runCatching, yielding Result<Result<Boolean>>. The isSuccess check in
onSubmitClick inspected the outer Result, which succeeds whenever
nothing throws, so a WpError response or a 200 with uploadId == 0 still
showed the success toast and invoked onSuccess().

Flatten with getOrElse so thrown exceptions land in the same failure
channel as the inner branches, and the existing failure toast is shown.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@adalpari
adalpari marked this pull request as ready for review September 7, 2026 09:44
@adalpari
adalpari requested a review from nbradbury September 7, 2026 09:59

@nbradbury nbradbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

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