feat: add support for abbandoned status in credits topup - #9768
Conversation
🚦 CI StatusNew build in progress, come back later! Warnings not reduced: 13132 => 13132 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (4)All Unity tests passed ✅
|
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #9768: feat: add support for abandoned status in credits topup
STEP 2 — Root-cause check: ✅ PASS
The problem is that the credits top-up flow had no awareness of the server's "abandoned" status, which occurs when a user cancels the transaction in the browser. The diff adds recognition of this status at every layer (API response → poll outcome → status type → UI controller), correctly addressing the cause.
STEP 3 — Design & integration: ✅ PASS
No new long-lived units are introduced. The change extends existing enums, adds a factory method to an existing readonly struct, and adds switch arms in existing methods. The pattern mirrors the established handling of Failed and Credited statuses. No lifecycle owners to search — no new persistent state or long-lived objects.
STEP 4 — Member audit
| New member | Consumers | Verdict |
|---|---|---|
CreditsTopUpStatus.Abandoned(CreditPack, string) |
RunTopUpAsync (1) |
Follows existing factory pattern — PASS |
PollOutcome.Abandoned |
RunTopUpAsync switch + PollOrderAsync return (2) |
PASS |
CreditsTopUpStage.Abandoned |
AcknowledgeTerminalState, ApplyStatus, MapStage (3) |
PASS |
STEP 5 — Findings
See inline comments for details. Summary:
| # | Sev | File | Finding |
|---|---|---|---|
| 1 | P1 | CreditsTopUpModalController.cs |
Missing analytics event for Abandoned in OnServiceStatusChanged — abandoned purchases are invisible to the analytics funnel |
| 2 | P2 | CreditsTopUpService.cs:20 |
Missing trailing comma on PollOutcome.Abandoned |
| 3 | P2 | CreditsTopUpServiceShould.cs |
No unit test for the abandoned status transition (compare TransitionToFailedWhenOrderFails) |
STEP 6 — Complexity: SIMPLE
Touches 4 files with ~30 lines. Straightforward enum/switch extensions. Does not modify ECS systems, async patterns, containers, or assembly definitions.
STEP 7 — QA: YES
Changes affect runtime UI behavior (the top-up modal shows a different message for abandoned transactions and enables the retry button).
STEP 8 — Non-blocking warnings
None. Main scene not modified.
Cosmetic note: The branch name and PR title use "abbandoned" (double b) — the correct spelling is "abandoned."
Security review: No security issues found.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Enum value additions and switch-case extensions in the credits top-up status flow
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: feat: add support for abandoned status in credits topup
STEP 2 — Root-cause check
PASS. The PR addresses a genuine gap: the client did not handle the "abandoned" order status returned by the server when a user cancels a Stripe checkout. The diff adds proper handling at every layer (API response constant → service poll/dispatch → domain enum/factory → controller analytics/UI). This is a cause-level fix, not a symptom workaround.
STEP 3 — Design & integration
PASS. No new long-lived units are introduced. The change is purely additive — new enum values, switch cases, and a factory method — wired into the existing CreditsTopUpService and CreditsTopUpModalController lifecycle owners.
- Pattern conformance: The
Abandonedpath mirrors the establishedCredited/Failedpatterns at every layer (API const →PollOutcome→CreditsTopUpStage→CreditsTopUpStatusfactory → controller dispatch → UI). ModalState.Failedreuse: MappingAbandoned→ModalState.Failedis correct.ModalStateis a UI-layout concern (which container to show), and both states share the same visual container (FailedContainer+FailedReasonText+RetryButton). Domain-level differentiation happens inApplyStatusvia theCreditsTopUpStageswitch.OnViewClosehandling: SinceAbandonedmaps toModalState.Failed, closing the modal callsAcknowledgeTerminalState()(notCancelTopUp()), which is correct — the server already considers the order terminal.- Retry flow: Works end-to-end:
ApplyStatusenables the retry button →OnRetryClickedguard passes (currentState == ModalState.Failed) →AcknowledgeTerminalState()resets toIdle→ user returns to pack selection. - Teardown trace: No new subscriptions, event hookups, or resources are added. All new code paths terminate in existing
SetStatus()calls.
STEP 4 — Member audit
| New member | Consumers | Verdict |
|---|---|---|
CreditsTopUpStatus.Abandoned(pack, orderId) |
1 (RunTopUpAsync line 158) |
Same pattern as GrantFailed() (also 1 consumer). Acceptable — factory methods exist for type safety and readability. |
PollOutcome.Abandoned |
2 (poll mapping line 189, dispatch line 157) | Same count as Failed. Fine. |
CreditsTopUpStage.Abandoned |
5 (service, types, controller ×3) | Consistent with Failed usage spread. Fine. |
ANALYTICS_ABANDONED |
1 (line 269) | Single-use const for analytics string. Same pattern as ANALYTICS_ERROR_GRANT_FAILED. Fine. |
No single-use-merge, absent≠false, or redundant-guard issues.
STEP 5 — Line-level review
No blocking issues (P0/P1) found. Two P2 items noted below.
[P2] Missing test coverage for the new Abandoned flow
Neither CreditsTopUpServiceShould.cs nor CreditsTopUpModalControllerShould.cs was updated. Four tests should be added to cover the new code paths:
- Service:
STATUS_ABANDONEDpoll →CreditsTopUpStage.Abandonedtransition (parallel toTransitionToFailedWhenOrderFails). Verify no balance refresh is called. - Service:
AcknowledgeTerminalStatefromAbandonedresets toIdle(parallel toResetTerminalStateToIdleOnAcknowledge). - Controller:
Abandonedstage raisesBuyCreditsFailedwith step"grant"and error code"abandoned"(parallel toRelayGrantFailureWithGrantStep). - Controller: Close while in
AbandonedcallsAcknowledgeTerminalState, notCancelTopUp(parallel toAcknowledgeWithoutCancellingWhenClosedAfterTerminalState).
Example test outlines (click to expand)
CreditsTopUpServiceShould.cs:
[Test]
public async Task TransitionToAbandonedWhenOrderIsAbandoned()
{
// Arrange
creditsApiClient.GetCheckoutOrderAsync(ORDER_ID, Arg.Any<CancellationToken>())
.Returns(Order(CreditsOrderStatusResponse.STATUS_ABANDONED));
// Act
service.StartTopUp(PACK);
await WaitForStageAsync(CreditsTopUpStage.Abandoned);
// Assert
Assert.AreEqual(ORDER_ID, service.CurrentStatus.OrderId);
Assert.AreEqual(PACK.Id, service.CurrentStatus.Pack.Id);
await creditsApiClient.DidNotReceive().GetUserCreditsAsync(
Arg.Any<string>(), Arg.Any<CancellationToken>());
}
[Test]
public async Task ResetAbandonedStateToIdleOnAcknowledge()
{
// Arrange
creditsApiClient.GetCheckoutOrderAsync(ORDER_ID, Arg.Any<CancellationToken>())
.Returns(Order(CreditsOrderStatusResponse.STATUS_ABANDONED));
service.StartTopUp(PACK);
await WaitForStageAsync(CreditsTopUpStage.Abandoned);
// Act
service.AcknowledgeTerminalState();
// Assert
Assert.AreEqual(CreditsTopUpStage.Idle, service.CurrentStatus.Stage);
Assert.IsFalse(service.IsOrderInFlight);
}CreditsTopUpModalControllerShould.cs:
[Test]
public void RelayAbandonedAsFailureWithGrantStepAndAbandonedCode()
{
// Act
RaiseStatus(CreditsTopUpStatus.Abandoned(PACK, ORDER_ID));
// Assert
Assert.AreEqual(1, failed.Count);
Assert.AreEqual("grant", failed[0].step);
Assert.AreEqual("abandoned", failed[0].errorCode);
}
[Test]
public void AcknowledgeWithoutCancellingWhenClosedAfterAbandoned()
{
// Arrange
topUpService.CurrentStatus.Returns(CreditsTopUpStatus.Abandoned(PACK, ORDER_ID));
controller.Show();
RaiseStatus(CreditsTopUpStatus.Abandoned(PACK, ORDER_ID));
// Act
controller.Close();
// Assert
topUpService.Received(1).AcknowledgeTerminalState();
topUpService.DidNotReceive().CancelTopUp();
Assert.AreEqual(0, cancelled.Count);
}[P2] Analytics classification note
The Abandoned status routes through BuyCreditsFailed with step="grant" (line 269). This is consistent with the existing pattern (post-checkout terminal states use BuyCreditsFailed), but worth confirming with the analytics consumer: "abandoned" is semantically "user chose not to pay" rather than "payment system failed." If dashboards use BuyCreditsFailed as an error signal, abandoned checkouts could inflate failure metrics. The distinct error_code="abandoned" does allow filtering — just make sure downstream dashboards are aware.
STEP 6 — Complexity
SIMPLE. Four files, ~24 lines of meaningful changes. Straightforward addition of a new enum value and its handling through existing layers. No ECS, async, container, or architectural changes.
STEP 7 — QA assessment
QA_REQUIRED: YES. The change modifies runtime UI behavior — what the user sees when a Stripe payment is abandoned. The PR description includes clear manual test steps.
STEP 8 — Non-blocking warnings
No warnings. Main.unity is not modified.
Security review
No security issues found. All new strings are hardcoded constants. No server-supplied data is interpolated into the abandoned-state UI (unlike the Failed path which can surface order.error). No new API calls, auth changes, or input handling. The trust model (client takes server status at face value) is unchanged.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Additive enum values and switch cases in the MarketplaceCredits top-up service and controller — no ECS, async, or architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by davidejensen via GitHub
|
PR #9768, run #32032427094 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [✔️ ] Backpack and wearables in world
- [ ✔️] Emotes in world and in backpack
- [ ✔️] Teleport with map/coordinates/Jump In
- [✔️ ] Chat and multiplayer
- [✔️ ] Profile card
- [✔️] Camera
Evidence:
Pull Request Description
What does this PR change?
Fix #9737
This PR adds support for the abbandoned status of a credits topup transaction. The abbandoned status happens when a user cancels the transaction in the browser.
This DOES NOT cover the case when a user closes the browser tab of the transaction, in that case we cannot do anything to detect it.
Test Instructions
Test Steps
This DOES NOT work when closing the browser page and it's normal, we cannot avoid that
Additional Testing Notes
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.