-
Notifications
You must be signed in to change notification settings - Fork 19
fix: remove dead code and stale TODOs in transaction purchase routes #233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,3 +46,4 @@ bv-pages/ | |
|
|
||
| # Local backup files | ||
| prisma/backups/ | ||
| .zed/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { redirect } from '@remix-run/node' | ||
| import { loader } from '../dashboard.purchase.verify' | ||
| import { TRANSACTION_STATUS } from '~/models/enum' | ||
| import { requireUser } from '~/services/auth.server' | ||
| import { getFirstTransaction } from '~/models/transaction' | ||
|
|
||
| vi.mock('~/services/auth.server', () => ({ | ||
| requireUser: vi.fn(), | ||
| })) | ||
|
|
||
| vi.mock('~/models/transaction', () => ({ | ||
| getFirstTransaction: vi.fn(), | ||
| })) | ||
|
|
||
| function buildRequest(url = 'http://example.com/dashboard/purchase/verify') { | ||
| return new Request(url, { method: 'GET' }) | ||
| } | ||
|
|
||
| const fakeUser = { id: 'user-1', email: 'test@example.com' } | ||
|
|
||
| describe('dashboard.purchase.verify loader', () => { | ||
| beforeEach(() => { | ||
| vi.mocked(requireUser).mockResolvedValue(fakeUser as never) | ||
| }) | ||
|
|
||
| it('redirects to /dashboard/purchase when no transaction exists', async () => { | ||
| vi.mocked(getFirstTransaction).mockResolvedValue(null) | ||
|
|
||
| const response = await loader({ | ||
| request: buildRequest(), | ||
| params: {}, | ||
| context: {}, | ||
| }) | ||
|
|
||
| expect(response).toEqual(redirect('/dashboard/purchase')) | ||
| }) | ||
|
|
||
| it('redirects to /dashboard/purchase/completed when transaction is VERIFIED', async () => { | ||
| vi.mocked(getFirstTransaction).mockResolvedValue({ | ||
| id: 'tx-1', | ||
| status: TRANSACTION_STATUS.VERIFIED, | ||
| } as never) | ||
|
|
||
| const response = await loader({ | ||
| request: buildRequest(), | ||
| params: {}, | ||
| context: {}, | ||
| }) | ||
|
|
||
| expect(response).toEqual(redirect('/dashboard/purchase/completed')) | ||
| }) | ||
|
|
||
| it('returns transaction and user when transaction is not verified', async () => { | ||
| const fakeTransaction = { | ||
| id: 'tx-1', | ||
| status: TRANSACTION_STATUS.SUBMITTED, | ||
| } | ||
| vi.mocked(getFirstTransaction).mockResolvedValue(fakeTransaction as never) | ||
|
|
||
| const response = await loader({ | ||
| request: buildRequest(), | ||
| params: {}, | ||
| context: {}, | ||
| }) | ||
|
|
||
| expect(response).toEqual({ transaction: fakeTransaction, user: fakeUser }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,19 +34,11 @@ export default function Verify() { | |
| useLoaderData<{ transaction: Transaction; user: User }>() | ||
| return ( | ||
| <> | ||
| {/* TODO: Render action buttons conditionally */} | ||
| <TransactionDetails transaction={transaction} user={user}> | ||
| <SecondaryButtonLink | ||
| to="/dashboard/purchase/confirm" | ||
| disabled={transaction.status === TRANSACTION_STATUS.VERIFIED} | ||
| > | ||
| <SecondaryButtonLink to="/dashboard/purchase/confirm"> | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The dead-code reasoning checks out here, but after removing the UI-level guard this route now depends entirely on the loader redirect above to keep
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validated the invariant here: |
||
| Ubah Detail Transaksi | ||
| </SecondaryButtonLink> | ||
| <PrimaryButtonLink | ||
| to={transaction.id} | ||
| replace | ||
| disabled={transaction.status === TRANSACTION_STATUS.VERIFIED} | ||
| > | ||
| <PrimaryButtonLink to={transaction.id} replace> | ||
| Verifikasi Pembelian | ||
| </PrimaryButtonLink> | ||
| </TransactionDetails> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice regression case. Covering the
VERIFIEDredirect in the loader makes the dead-code removal indashboard.purchase.verifymuch safer against future regressions.