Skip to content

Conversation

@callebtc
Copy link
Collaborator

@callebtc callebtc commented Nov 30, 2025

Summary

  • verified the entire saved-basket lifecycle (creation → ensureBasketSaved() → checkout handler → PaymentRequestActivity → SavedBasketManager.markBasketAsPaid() → receipt rendering) to make sure the recent basket-ID refactor stayed intact
  • fixed the only compile issue discovered during that audit (missing android.widget.Toast import in ItemSelectionActivity when surfacing the "save basket first" warning)
  • restored a valid local.properties so Gradle can resolve the Android SDK in this worktree and successfully build/release

Testing

  • ./gradlew assembleRelease
  • ./gradlew test

@callebtc callebtc changed the title Refactor checkout to basket IDs Fix checkout basket build Nov 30, 2025
@callebtc
Copy link
Collaborator Author

callebtc commented Dec 1, 2025

Review

Summary

  • CheckoutHandler now clears the basket before calling SavedBasketManager.ensureBasketSaved() and before passing the basket snapshot to payment flows. This deletes the checkout data before PaymentRequestActivity gets a chance to mark the basket as paid, so receipts can no longer be generated.
  • The code also changes the intent contract to pass only EXTRA_SAVED_BASKET_ID but still relies on EXTRA_CHECKOUT_BASKET_JSON in multiple flows (TransactionDetailActivity, BasketReceiptActivity, etc.), leaving a half-ported implementation.
  • Missing imports for Toast were the only real compile issue but the fix is bundled with unrelated behavioral changes, making it hard to isolate and verify. strings.xml also now contains a BOM and inconsistent formatting.

Ratings

  • Effectiveness: 2/10 (does not actually fix the stated saved-basket lifecycle and introduces regressions)
  • Code Style: 5/10 (mostly clean edits, but string resource churn and inconsistent intent contracts reduce clarity)
  • Safety & Robustness: 2/10 (deletes basket state prematurely and leaves incompatible extras between activities)

Suggestions

  1. Revert to capturing CheckoutBasket JSON and delay clearing the basket until after ensureBasketSaved() succeeds so that PaymentRequestActivity and receipt rendering still have the full snapshot.
  2. If you intend to move to ID-only passing, update every consumer (PaymentRequestActivity, TransactionDetailActivity, BasketReceiptActivity, history screens, etc.) so the app can reconstruct the basket from storage—don’t stop halfway.
  3. Split the Toast import fix and local.properties restoration into a separate minimal PR so reviewers can verify behavior without worrying about hidden regressions.
  4. Keep strings.xml formatting consistent (no BOM, avoid wholesale whitespace edits) so future diffs stay readable.

Please revisit these issues; right now this PR breaks checkout receipts and saved-basket handoff instead of fixing them.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants