fix: harden transaction status updates and expand email/e2e coverage#228
fix: harden transaction status updates and expand email/e2e coverage#228zainfathoni merged 8 commits intomainfrom
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-019c7de1-5a6d-74ab-91f4-55ea234b4c45 Co-authored-by: Amp <amp@ampcode.com>
…s called The existing test had a TODO comment noting that it should assert emailProvider.sendEmail was called but only verified no exceptions were thrown. Now uses vi.spyOn to verify: - emailProvider.sendEmail is called exactly once - called with the correct recipient address - called with an html body that includes the magic link URL email-provider.server.ts already short-circuits in test mode (returns Promise.resolve without hitting Mailgun), so no mocking of HTTP is needed.
… 11) Playwright's fill() is supposed to focus the element before typing, but on WebKit/Safari (used for the iPhone 11 test project), the synthetic onFocus event is not always reliably dispatched in CI's headless environment. The Field component tracks 'interacted' state (set on onFocus or onChange) to decide whether to show validation errors after blur. If onFocus doesn't fire, 'interacted' stays false and the error message never renders, causing the E2E assertion to fail with 'element(s) not found'. Fix: add an explicit .click() before .fill() in both validation tests. A click is guaranteed to trigger focus events in all browsers, ensuring 'interacted' is set to true before we blur and check for the error. Applied to both phone-number and name validation tests for consistency.
zainfathoni
left a comment
zainfathoni
left a comment
There was a problem hiding this comment.
Thanks for the cleanup and for addressing the email test TODO + WebKit flakiness note. I found one correctness/security gap that should be fixed before merge: the action still trusts any string status from form data and casts it to TransactionStatus, so crafted requests can still transition VERIFIED -> SUBMITTED (or other unintended states). That bypasses the new business rule intent and can leave subscription state inconsistent.
What looks good:
- Email test now verifies provider handoff with concrete argument checks.
- WebKit click-before-fill adjustment is a reasonable stabilization step for focus-triggered validation.
Please add strict runtime validation for status (allow only VERIFIED/REJECTED in this action) and expand tests to cover tampered status payloads and forbidden transitions at the action level.
There was a problem hiding this comment.
Pull request overview
This PR improves reliability and correctness across tests and transaction status handling by (1) strengthening an email unit test to assert provider invocation, (2) hardening the dashboard transaction reject flow so verified transactions can’t be rejected, and (3) making a couple of E2E interactions more reliable on WebKit.
Changes:
- Add a Vitest spy assertion to ensure
emailProvider.sendEmailis invoked with expected payload. - Introduce
canUpdateTransactionStatusand apply it in the transaction status action + disable reject UI for verified transactions. - Refine
TRANSACTION_STATUStyping (as const+ derivedTransactionStatus) and add unit tests for the new status-transition helper.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/profile.spec.ts | Adds click() before filling fields to improve WebKit focus/validation reliability. |
| app/utils/transaction-status.ts | Adds a centralized rule for allowed transaction status transitions. |
| app/utils/tests/transaction-status.test.ts | Unit tests for canUpdateTransactionStatus. |
| app/services/tests/email.server.test.ts | Adds spy assertions to verify provider handoff occurs. |
| app/routes/dashboard.transactions.$transactionId.tsx | Disables “reject” action in UI when transaction is VERIFIED (or already REJECTED). |
| app/routes/dashboard.transactions.$transactionId.$action.tsx | Enforces “VERIFIED can’t become REJECTED” server-side via the new helper. |
| app/models/enum.ts | Makes TRANSACTION_STATUS literal-typed and derives TransactionStatus from it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Amp-Thread-ID: https://ampcode.com/threads/T-019ca093-14b6-767d-af4e-6fa737f1254f Co-authored-by: Amp <amp@ampcode.com>
zainfathoni
left a comment
zainfathoni
left a comment
There was a problem hiding this comment.
ISSUES FOUND
Thanks for the improvements around transaction status validation and added coverage. I reviewed all changed files and ran checks locally (npm test, npm run lint, npm run type-check).
Blocking issue:
npm run type-checkcurrently fails due to the new action test usingresponse.status/response.headerswithout narrowing from the broadActionFunctionreturn type. This makes the PR non-mergeable in its current form in a type-checked pipeline.
What looks good:
- Server-side allowlist for transaction status and transition guard reduce payload tampering risk.
- UI now disables reject action for already verified transactions, matching backend rules.
sendEmailtest TODO is now fulfilled with concrete assertions.- E2E focus handling adjustments improve WebKit stability.
Please fix the type error in the test and I can re-review quickly.
Amp-Thread-ID: https://ampcode.com/threads/T-019ca09b-2eda-7357-9a28-71f3648d5c02 Co-authored-by: Amp <amp@ampcode.com>
zainfathoni
left a comment
zainfathoni
left a comment
There was a problem hiding this comment.
ISSUES FOUND
I reviewed all changed files in this PR (transaction status validation/UI guard, related unit tests, email test assertion update, and E2E profile tweaks). Most changes improve correctness and coverage, but there is one merge-readiness issue:
- The transition check is currently vulnerable to a TOCTOU race. The code validates
existingTransaction.statusand then performs an unconditionalupdateTransactionStatusbyid. Two concurrent moderator actions can still produce a forbidden VERIFIED -> REJECTED end state despite the guard. Please make the transition enforcement atomic at the DB write boundary (for example, conditional update whereid+ allowed current statuses, then verify affected row count).
After this is fixed, the rest of the PR looks good from security/correctness/type/test-quality perspective.
Amp-Thread-ID: https://ampcode.com/threads/T-019ca09f-32dc-76ab-b793-4096a1f407fe Co-authored-by: Amp <amp@ampcode.com>
zainfathoni
left a comment
zainfathoni
left a comment
There was a problem hiding this comment.
NO ISSUES FOUND
I reviewed all changed files in this PR (models, routes, utils, unit tests, and E2E spec updates) with focus on correctness, security, TypeScript safety, and test quality.
What I validated:
- Status transition hardening now enforces server-side allowlisting and blocks forbidden VERIFIED -> REJECTED transitions, including against tampered payloads.
updateTransactionStatususes conditionalwherefiltering with handledP2025to convert race-condition misses into a controlled null result, which the route now maps to HTTP 409.- UI behavior now aligns with backend rules by disabling reject action for already VERIFIED transactions.
- New tests cover payload tampering rejection, forbidden transition rejection, and allowed transition success; utility-level transition rules are also covered.
- Email test TODO is fully addressed with explicit spy assertions on invocation and key message fields.
- E2E profile updates improve WebKit reliability without weakening assertions.
Verification run locally on this branch:
npm testpassed (116/116)npm run type-checkpassednpm run lintshows only pre-existing Playwright warnings (no errors).
Merge-readiness: looks good from a code quality and safety standpoint.
zainfathoni
changed the title
Summary
emailProvider.sendEmailis called with the expected payload.canUpdateTransactionStatushelper409 Conflicton concurrent update conflictsVERIFIEDandREJECTEDtransactions.Why
This PR now covers both testing improvements and transaction update hardening (backend + UI), plus E2E stability fixes. The updated scope clarifies all shipped changes so reviewers can evaluate the full impact.