[Woo POS] Report the refund calculation flow in analytics - #16419
[Woo POS] Report the refund calculation flow in analytics#16419samiuelson wants to merge 5 commits into
Conversation
The server-computed create and the classic create emitted identical events, and the route-availability fallback emitted nothing, so a rollout of woo_pos_server_refunds could not be measured: no adoption figure, no way to compare failure rates between the flows, and no telemetry to attribute a "totals look wrong" report to either. Add a refund_flow property (local / server_computed) to the refund processing events, carry the store's REST error code on failures so deterministic rejections separate from transport errors, and emit a new event when a preview probe finds the route missing, carrying the store's WooCommerce version so a misjudged version gate is visible. The reported flow is decided by the same predicate the submission path branches on, extracted as isServerComputedRefundConfirmed(), so it cannot drift from the path actually taken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Generated by 🚫 Danger |
There was a problem hiding this comment.
AI Code Review - No issues found. The changes look good.
The PR adds analytics instrumentation to the POS refund flow (a refund_flow local/server-computed dimension, the api_error_code on failures, and a new refund_server_flow_unavailable fallback event). Verified: error.apiErrorCode resolves correctly (error = result.error), AnalyticsTracker.KEY_API_ERROR_CODE exists, the raw "woocommerce_version" key matches existing events in the same file, and isServerComputedRefundConfirmed() / refundFlowFor(request) cannot drift because forComputedCreate returns a non-null list on the server branch. Unit tests cover both the local and server-computed flows plus the API-error-code failure path.
PR housekeeping
- Applied repo-specific POS analytics guidance from
AGENTS.md(WooPos* events useWooPosAnalyticsTracker/WooPosAnalyticsEvent, plainViewModel).
Automatic review · claude-opus-4-8 · Workflow run
How to reply to a finding
Reply on this review (or inline at the line the finding refers to) with one of:
@claude addressed- I made the change. Bot verifies against the next diff before marking resolved.@claude rejected: <reason>- Will not fix; reason gets quoted on the next review.@claude not-applicable- Finding does not apply (wrong file, already covered elsewhere, etc.).
The bot honours these on the next review pass.
|
|
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Heads-up on overlap with #16338, which is approved and will likely merge first. Both PRs add an Git will still flag it as a conflict in |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverses the previous commit, which stripped the Given/When/Then markers from the new tests. The intent was the opposite: production code carries no comments here, tests keep their structure markers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

WOOMOB-3750
Description
The server-computed create and the classic create emit identical analytics, and the route-availability fallback emits nothing at all. During the
woo_pos_server_refundsrollout that leaves us blind: no measure of server-flow adoption, no way to compare failure rates between the two flows, and no telemetry to attribute a "the totals look wrong" report to either one.Mirrors the iOS change in woocommerce-ios#17716; the property name and values match so the funnels stay comparable across platforms.
Three additions:
refund_flowon the refund processing events.RefundProcessingStarted,RefundProcessingSuccessandRefundProcessingFailedbecome data classes carryingrefund_flow(local/server_computed), with the values in a newWooPosAnalyticsEventConstant.RefundFlowenum. The value comes fromisServerComputedRefundConfirmed(), extracted frombuildSubmissionRequestso the reported flow is decided by the same predicate the submission path branches on and cannot drift from the path actually taken. Success and failure derive it fromrequest.serverLineItems, the documented discriminator on the request.api_error_codeonRefundProcessingFailed.WooPosRefundSubmissionState.Failuregains anapiErrorCodefield, populated fromWooError.apiErrorCodewhere the backend refund result is handled. The message alone cannot do this — it is localized to the store and varies by wording, so deterministic server rejections were indistinguishable from transport failures. The property is omitted rather than sent empty when there is no code.refund_server_flow_unavailable. Emitted fromWooPosRefundPreviewwhere the availability cache is marked unavailable, carrying the store'swoocommerce_version(already in scope viaflow.wooVersion). The version is what tells us whether the 11.1.0 gate is behaving or a store is genuinely too old. Because the cache short-circuits the resolver on later refunds, this counts stores that fell back rather than refunds.RefundFlowStartedis deliberately left alone. It fires on the item-selection step, before the preview probe runs, so only eligibility is known there — a store that later 404s would reportserver_computedat start andlocalafterwards. Reporting a value that can be contradicted later is worse than reporting none, and the three processing events already cover the comparison we need. Worth revisiting if a start-to-outcome funnel split turns out to be needed.Test Steps
./gradlew :WooCommerce:testVanillaDebugUnitTest --tests "com.woocommerce.android.ui.woopos.orders.details.refund.*"— 181 tests pass, including four new ones:server_computedon the server-computed path,refund_flowislocalon the classic path (the three pre-existing assertions, updated),To check on device with
woo_pos_server_refundsenabled: issue a POS refund on a WooCommerce 11.1.0+ store and confirmrefund_processing_startedcarriesrefund_flow: server_computed; repeat on a pre-11.1.0 store and confirmrefund_flow: localplus onerefund_server_flow_unavailablefor that store.Note on detekt: the per-module
./gradlew :WooCommerce:detekttask fails locally with "Run failed with 1 invalid config property" on trunk as well as here, so it is not a usable local check../gradlew detektAll— the task CI runs — works, and did flag a realSpacingBetweenDeclarationsWithCommentsviolation in this branch, now fixed.Images/gif
N/A — no UI changes.
RELEASE-NOTES.txtif necessary.