diff --git a/.gitignore b/.gitignore index 871187c7..3275a7ff 100644 --- a/.gitignore +++ b/.gitignore @@ -44,5 +44,4 @@ next-env.d.ts .trigger # local decision notes (not published) -/docs /supabase/snippets \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index c7dee4e4..1c268965 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -70,9 +70,9 @@ Services extend `BaseService` (`src/app/api/core/services/base.service.ts`), whi - `this.db` — the **module-level Drizzle singleton** from `src/db/index.ts` (`DBClient.getInstance()`); `casing: 'snake_case'`. - `this.user` — the authenticated `User` for the request. -- `setTransaction(tx)` / `unsetTransaction()` — swap `this.db` for a transaction handle inside a `db.transaction(...)` callback, then restore. +- `setTransaction(tx)` / `unsetTransaction()` — swap `this.db` between the pool and a transaction handle. **Use `withTransaction(fn, services?)` rather than pairing these by hand.** -**Pitfall (known, see `memory/project_unsetTransaction_bug.md`):** `unsetTransaction()` is sometimes called inside the transaction callback or skipped on error paths — across `BaseService` subclasses this leaves the singleton pointed at a closed tx. When introducing or modifying transactional code, audit that `setTransaction` / `unsetTransaction` are paired in `try/finally` and that nested service calls share the tx handle. +**Transactions (full reasoning in `docs/baseservice-transactions.md`).** `this.db` is a mutable field so a service's whole method flow can join a tx without threading `tx` through every signature. `withTransaction(fn, services?)` runs `fn` in a transaction and restores `this` plus any passed `services` in a `finally` that _wraps_ it, so a throw or failed commit can't leave a service on a closed handle. **Footgun:** each `BaseService` has its own `this.db`, so a nested service (`this.syncLogService`, a `new TokenService`, …) that writes inside `fn` MUST be in the `services` array — TypeScript does **not** catch an omission, and an unbound service's writes silently run outside the tx (this was the pre-OUT-4081 `checkAndSuspendAccount` bug). The DB singleton is also why test helpers (`test/helpers/seed.ts`, `test/helpers/testDb.ts`) import `@/db` directly — see `docs/why-test-helpers-use-the-app-db-singleton.md`. Don't introduce a separate test-only Drizzle client; tests must read what the app writes. @@ -88,7 +88,7 @@ The DB singleton is also why test helpers (`test/helpers/seed.ts`, `test/helpers ### Token refresh -QBO access tokens expire in ~1h, refresh tokens in ~100 days. `src/utils/intuitAPI.ts` sends authenticated requests; `src/utils/tokenRefresh.ts` (`getValidQbTokens`) refreshes when stale. The `vercel.json` cron `/api/quickbooks/refresh-tokens` runs daily at 06:00 UTC to keep refresh tokens warm. There's a known silent-401 bug — expired tokens cause `null` returns from `getFetchWithHeader/postFetchWithHeaders`; the planned fix is auto-refresh inside those helpers (design at `docs/intuit-api-token-refresh.md`, summary in `memory/project_intuit_api_token_refresh.md`). +QBO access tokens expire in ~1h, refresh tokens in ~100 days. `src/utils/intuitAPI.ts` sends authenticated requests; `src/utils/tokenRefresh.ts` (`getValidQbTokens`) refreshes when stale. The `vercel.json` cron `/api/quickbooks/refresh-tokens` runs daily at 06:00 UTC to keep refresh tokens warm. There's a known silent-401 bug — expired tokens cause `null` returns from `getFetchWithHeader/postFetchWithHeaders`; the planned fix is auto-refresh inside those helpers (summary in `memory/project_intuit_api_token_refresh.md`). ### Background work @@ -112,11 +112,11 @@ Every `WHERE` clause that touches a portal-scoped table needs `portalId = this.u ## Testing - Two Vitest **projects** in `vitest.config.ts` — `unit` (mock-heavy, isolated) and `integration` (real Postgres via testcontainers). Run order is enforced via `sequence.groupOrder` (unit=0, integration=1). -- Integration project is configured **`pool: 'forks'` + `fileParallelism: false` + `isolate: false`** so all integration tests share one Postgres container _and_ one app DB connection. Don't change these without reading `docs/vitest-gotchas.md` and `docs/why-test-helpers-use-the-app-db-singleton.md`. +- Integration project is configured **`pool: 'forks'` + `fileParallelism: false` + `isolate: false`** so all integration tests share one Postgres container _and_ one app DB connection. Don't change these without reading `docs/why-test-helpers-use-the-app-db-singleton.md`. - `.env.test` is loaded by `test/integration/globalSetup.ts` with `override: true` so a developer's local `.env` can't leak into tests. `DATABASE_URL` is intentionally **not** in `.env.test` — globalSetup sets it from the container's URI before any worker imports `src/config`. -- Module mocks for integration are in `test/integration/setup.ts` — `@/utils/copilotAPI`, `@/utils/intuitAPI`, and `@sentry/nextjs` must be mocked with **explicit factories** (and Intuit/Copilot mock implementations must use `function`, not `=>`, because the code does `new IntuitAPI(...)`). See `docs/vitest-gotchas.md` items 1–3. +- Module mocks for integration are in `test/integration/setup.ts` — `@/utils/copilotAPI`, `@/utils/intuitAPI`, and `@sentry/nextjs` must be mocked with **explicit factories** (and Intuit/Copilot mock implementations must use `function`, not `=>`, because the code does `new IntuitAPI(...)`). - Test helpers in `test/helpers/`: `seed.ts` (`seedHealthyPortal`, `TEST_PORTAL_ID`, etc.), `webhook.ts` (`postWebhook` via `next-test-api-route-handler`), `testDb.ts` (`truncateAllTestTables`). -- Test-data philosophy in `docs/test-data-dos-and-donts.md`: static fixtures for the thing under test, factories with explicit overrides for single-dimension variants, **no faker** in fixtures or assertions. +- Test-data philosophy: static fixtures for the thing under test, factories with explicit overrides for single-dimension variants, **no faker** in fixtures or assertions. ## Path aliases @@ -132,15 +132,12 @@ Configured in `tsconfig.json` and propagated to Vitest via `vite-tsconfig-paths` - Prettier: single quotes, no semis, trailing comma all (`.prettierrc`). - ESLint: `next/core-web-vitals` + TypeScript; `prefer-const` and `no-var` are errors; unused-var underscore prefix is exempt; `@typescript-eslint/no-explicit-any` is disabled (the codebase uses `any` deliberately at framework boundaries). - Tailwind v4 + `copilot-design-system`. UI surface is small (settings dashboard + OAuth callback) — most work happens in the API/service layer. -- The `docs/` folder is **gitignored** (per `.gitignore`) and used for local decision notes — design docs, post-mortems, comparison tables. Save non-trivial tradeoff discussions there rather than in code comments or commit messages. +- `docs/` holds **committed** decision docs (design notes, post-mortems, comparison tables); `private-local-docs/` is gitignored for local-only notes. Save non-trivial tradeoff discussions in `docs/` rather than in code comments or commit messages. ## Things to read before non-trivial changes -- `docs/testcontainers-vs-local-supabase.md` — why integration tests use testcontainers, not the local Supabase stack. - `docs/why-test-helpers-use-the-app-db-singleton.md` — why test helpers import `@/db` and what would break if you opened a separate client. -- `docs/vitest-gotchas.md` — the five real traps already hit in this project. -- `docs/test-data-dos-and-donts.md` — the test-data rules. -- `docs/intuit-api-token-refresh.md` — design for the silent-401 fix. +- `docs/baseservice-transactions.md` — why `this.db` is mutable, how to use `withTransaction`, and the nested-service footgun. ## What this repo doesn't have diff --git a/docs/baseservice-transactions.md b/docs/baseservice-transactions.md new file mode 100644 index 00000000..3c9bc07b --- /dev/null +++ b/docs/baseservice-transactions.md @@ -0,0 +1,54 @@ +# BaseService transactions: why `this.db` is mutable, and how to use `withTransaction` + +## The pattern + +`BaseService` exposes `this.db` (the Drizzle client) and two methods that swap it: + +```ts +setTransaction(tx) // this.db = tx — point this service at a transaction +unsetTransaction() // this.db = pool — restore the shared pool +``` + +Every service method reads `this.db`, so swapping it changes which connection all of that service's queries run on. + +## Why it's built this way + +Two constraints force it: + +1. **Drizzle only hands you the tx handle inside the callback.** `db.transaction(async (tx) => …)` gives you `tx` only within that closure. But a service operation is usually a _flow_ of methods (`getOne`, `createQBProduct`, `logSync`, …), each of which reads `this.db`. Threading `tx` through every method signature — and through every nested service they call — would be invasive. +2. **Services are constructed synchronously.** `new InvoiceService(user)` runs in a normal constructor, and the SDK/DB handles are already in place. There's no async-factory seam to inject a tx at construction time. + +So instead of passing `tx` everywhere, `setTransaction(tx)` temporarily repoints `this.db` at the tx; every method the service calls transparently joins the transaction; `unsetTransaction()` restores the pool afterward. + +## Use `withTransaction`, not the raw pair + +Pairing `setTransaction`/`unsetTransaction` by hand is error-prone (the unset can be skipped on a throw, or placed inside the callback so it runs before the commit). Use the helper on `BaseService`: + +```ts +await this.withTransaction(async () => { + await this.updateQBInvoice(...) + await this.syncLogService.updateOrCreateQBSyncLog(...) +}, [this.syncLogService]) +``` + +It runs `fn` inside `db.transaction(...)`, binds `this` **and every service in the array** to the tx, and restores all of them in a `finally` that **wraps** the transaction — so a throw or a failed commit can never leave a service pointing at a closed handle. (Introduced in OUT-4081.) + +## The sharp edge — the part that bites + +**Each `BaseService` instance has its own independent `this.db`.** A nested service is _not_ in the transaction just because the outer service started one. If a `fn` writes through `this.syncLogService` or a freshly-`new`'d `TokenService`, that instance must be in the `services` array — otherwise its writes run on the **pool, outside the transaction**, silently. + +**TypeScript does not catch this.** The `services` array is completely decoupled from what `fn` actually calls, and a bound vs. unbound `this.db` have the identical type. So this compiles clean and is wrong at runtime: + +```ts +await this.withTransaction(async () => { + await this.syncLogService.updateQBSyncLog(...) // compiles fine… +}, []) // …but syncLogService was never bound → write escapes the tx +``` + +This is not hypothetical: `sync.service#checkAndSuspendAccount` did exactly this before OUT-4081 — the suspend-account and delete-logs writes lived inside `db.transaction(...)` but their services were never bound, so the two writes weren't atomic despite looking like they were. + +**Rule when writing transactional code:** trace every DB call inside `fn` to the service instance it runs on, and confirm that instance is `this` or is in the `services` array. + +## Why not make it type-safe? + +The only way to get the compiler to enforce completeness is to stop mutating `this.db` and thread `tx` explicitly — e.g. `tx.insert(...)` directly, or DB methods that take a required `tx` parameter. That removes the footgun entirely (you physically cannot write without the handle in hand), but it's a large refactor touching every transactional method and the nested methods they call. It's deliberately deferred; `withTransaction` is the pragmatic middle ground that fixes the _pairing_ bug now while leaving the _completeness_ gap documented. See the OUT-4081 PR and `memory/project_unsetTransaction_bug.md`. diff --git a/docs/why-test-helpers-use-the-app-db-singleton.md b/docs/why-test-helpers-use-the-app-db-singleton.md new file mode 100644 index 00000000..182ae9c6 --- /dev/null +++ b/docs/why-test-helpers-use-the-app-db-singleton.md @@ -0,0 +1,61 @@ +# Why `truncateAllTestTables` (and other test helpers) import the real app `db` + +**Date:** 2026-04-13 +**Context:** Reviewing the integration test scaffolding for `price.created`, the question came up: why does `test/helpers/testDb.ts` import `@/db` — isn't that the production-style module-level singleton? Shouldn't tests use a dedicated test client? + +## Short answer + +An integration test's contract is: **"the app's code runs against a real Postgres, and we verify observable outcomes in that Postgres."** That requires the test and the app to talk to the _same_ database through the _same_ client. Using a separate test-only DB client would silently decouple "what the test wipes / seeds / asserts on" from "what the app writes to", which defeats the purpose. + +## Data flow on one test run + +``` +globalSetup.ts → starts Postgres container, sets process.env.DATABASE_URL + ↓ +test file imports @/db → DBClient.getInstance() constructs once, + reads DATABASE_URL = container URL, + opens a postgres-js connection pool + ↓ +beforeEach → truncateAllTestTables() uses db → wipes the container's tables + seedHealthyPortal() uses db → inserts into the container + ↓ +POST /api/quickbooks/webhook + ↓ controller → service code also imports @/db (SAME singleton) + → reads the seed, writes qb_product_sync, writes qb_sync_logs + ↓ +test assertions: db.select(...) → SAME singleton, SAME container, + reads back what the app wrote +``` + +If `truncateAllTestTables` used a _different_ Drizzle client, we'd get a subtle bug: truncate wipes DB "A", app writes to DB "B", assertions check DB "A" and see nothing. Tests pass when they shouldn't, or fail for mysterious reasons. + +## Why the singleton isn't a risk here + +Normally, a module-level singleton pointing at a DB is something to be nervous about in tests — it implies "shared state across everything, hard to override." Two things make it fine in this setup: + +1. **Only one DB is reachable from this process.** `DATABASE_URL` is set by `globalSetup.ts` to the testcontainer's URI _before_ any `src/` code is imported by the test worker. The singleton can't bind to anything else. No production DB is reachable. +2. **Tests run sequentially.** `vitest.config.ts` sets `fileParallelism: false` and `isolate: false` for the integration project, so no two tests mutate the shared DB concurrently. Truncate + seed in `beforeEach` is enough to guarantee a clean slate. + +## Alternatives we considered and rejected + +| Approach | Why not | +| -------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `truncateAllTestTables` uses its own `drizzle(postgres(url))` client | Two connection pools to the same DB — works, but pointless; also re-raises the "was URL set in time?" ordering question | +| Truncate via raw `psql` in a shell script before Vitest starts | Runs once, not per-test; doesn't reset between tests | +| `BEGIN`/`ROLLBACK` wrapping each test instead of truncate | Cleaner in theory, but **doesn't work for this codebase** — `BaseService.setTransaction()` opens its own transaction, and Postgres doesn't allow nested real transactions (only savepoints). Rolling back at the test level would abort the app's transaction mid-flight and mask real bugs. | +| A separate test-only Drizzle client with its own schema imports | Adds a second pool for no benefit; more chances for the two to drift (casing, schema refs, pooling behavior) | + +The simplest correct answer is: **use the same `db` the app uses**. That's what integration testing means. + +## Future consideration: DI refactor + +This pattern works _because_ `@/db` is a singleton. If the codebase ever moves to dependency injection (the 3–5 week refactor noted in the testing-strategy memory), these helpers would change shape — they'd accept a `db` argument rather than importing it. At that point `truncateAllTestTables(db)` becomes natural and the "why is it importing app code" question goes away. + +Until then, the singleton is a constraint we work _with_, not against. + +## TL;DR for future-me + +- Tests and app must share the DB client. Do **not** open a second Drizzle/postgres-js client just for the test. +- The singleton is safe because `DATABASE_URL` is fixed to the container before any `src/` import loads. +- Keep integration tests sequential (`fileParallelism: false`, `isolate: false`) as long as they share a container. +- If you're tempted to switch to `BEGIN/ROLLBACK` for isolation: check that no code under test opens its own transaction first. In this codebase, `BaseService.setTransaction()` does — so truncate is the right tool. diff --git a/src/app/api/core/services/base.service.ts b/src/app/api/core/services/base.service.ts index 16c40293..cc84839f 100644 --- a/src/app/api/core/services/base.service.ts +++ b/src/app/api/core/services/base.service.ts @@ -24,4 +24,24 @@ export class BaseService { unsetTransaction() { this.db = db } + + /** + * Runs `fn` in a transaction with this service (and any passed `services`) + * bound to the tx, restored in a `finally` that wraps the transaction so none + * is left on a closed handle. + */ + protected async withTransaction( + fn: (tx: PostgresJsDatabase) => Promise, + services: BaseService[] = [], + ): Promise { + const bound = [this, ...services] + try { + return await this.db.transaction(async (tx) => { + bound.forEach((service) => service.setTransaction(tx)) + return await fn(tx) + }) + } finally { + bound.forEach((service) => service.unsetTransaction()) + } + } } diff --git a/src/app/api/quickbooks/invoice/invoice.service.ts b/src/app/api/quickbooks/invoice/invoice.service.ts index 13d985f9..72aaea6c 100644 --- a/src/app/api/quickbooks/invoice/invoice.service.ts +++ b/src/app/api/quickbooks/invoice/invoice.service.ts @@ -1108,39 +1108,32 @@ export class InvoiceService extends BaseService { console.info( 'InvoiceService#handleInvoiceDeleted | Invoice absent from QBO. Soft-deleting logs, marking local mapping as DELETED, and recording pre-soft-deleted DELETED event.', ) - try { - await this.db.transaction(async (tx) => { - this.setTransaction(tx) - this.syncLogService.setTransaction(tx) - const now = new Date() - await this.syncLogService.softDeleteLogsByCopilotId( - payload.id, - EntityType.INVOICE, - now, + await this.withTransaction(async () => { + const now = new Date() + await this.syncLogService.softDeleteLogsByCopilotId( + payload.id, + EntityType.INVOICE, + now, + ) + if (syncedInvoice) { + await this.updateQBInvoice( + { status: InvoiceStatus.DELETED }, + eq(QBInvoiceSync.id, syncedInvoice.id), + ['id'], ) - if (syncedInvoice) { - await this.updateQBInvoice( - { status: InvoiceStatus.DELETED }, - eq(QBInvoiceSync.id, syncedInvoice.id), - ['id'], - ) - } - await this.syncLogService.updateOrCreateQBSyncLog({ - portalId: this.user.workspaceId, - entityType: EntityType.INVOICE, - eventType: EventType.DELETED, - status: LogStatus.SUCCESS, - copilotId: payload.id, - invoiceNumber: payload.number, - amount: payload.total ? payload.total.toFixed(2) : undefined, - syncAt: now, - deletedAt: now, - }) + } + await this.syncLogService.updateOrCreateQBSyncLog({ + portalId: this.user.workspaceId, + entityType: EntityType.INVOICE, + eventType: EventType.DELETED, + status: LogStatus.SUCCESS, + copilotId: payload.id, + invoiceNumber: payload.number, + amount: payload.total ? payload.total.toFixed(2) : undefined, + syncAt: now, + deletedAt: now, }) - } finally { - this.unsetTransaction() - this.syncLogService.unsetTransaction() - } + }, [this.syncLogService]) return } diff --git a/src/app/api/quickbooks/product/product.service.ts b/src/app/api/quickbooks/product/product.service.ts index cf191e9b..ac65fbf7 100644 --- a/src/app/api/quickbooks/product/product.service.ts +++ b/src/app/api/quickbooks/product/product.service.ts @@ -215,53 +215,47 @@ export class ProductService extends BaseService { 'initialProductSettingMap', ]) - return await this.db.transaction(async (tx) => { - this.setTransaction(tx) - try { - if (!setting?.initialProductSettingMap) { - const formattedPayload = mappingItems.map((item) => { - return { - ...item, - portalId: this.user.workspaceId, - } + return await this.withTransaction(async () => { + if (!setting?.initialProductSettingMap) { + const formattedPayload = mappingItems.map((item) => { + return { + ...item, + portalId: this.user.workspaceId, + } + }) + // Skip products already saved so a repeated save doesn't error. + await this.db + .insert(QBProductSync) + .values(formattedPayload) + .onConflictDoNothing({ + target: [QBProductSync.portalId, QBProductSync.productId], + where: isNull(QBProductSync.deletedAt), }) - // Skip products already saved so a repeated save doesn't error. - await this.db - .insert(QBProductSync) - .values(formattedPayload) - .onConflictDoNothing({ - target: [QBProductSync.portalId, QBProductSync.productId], - where: isNull(QBProductSync.deletedAt), - }) - return await this.getAll() - } + return await this.getAll() + } - if (changedItemReference.length > 0) { - await Promise.all( - changedItemReference?.map(async (item) => { - const payload = { - portalId: this.user.workspaceId, - productId: item.id, - name: item.isExcluded ? null : item.qbItem?.name, - description: item.isExcluded ? null : item.description, - qbItemId: item.isExcluded ? null : item.qbItem?.id, - qbSyncToken: item.isExcluded ? null : item.qbItem?.syncToken, - copilotName: item.name, - isExcluded: item.isExcluded, - } - const conditions = and( - eq(QBProductSync.portalId, this.user.workspaceId), - eq(QBProductSync.productId, item.id), - ) as WhereClause - await this.updateOrCreateQBProduct(payload, conditions) - }), - ) + if (changedItemReference.length > 0) { + // Sequential: these writes share one tx connection (no concurrent queries). + for (const item of changedItemReference) { + const payload = { + portalId: this.user.workspaceId, + productId: item.id, + name: item.isExcluded ? null : item.qbItem?.name, + description: item.isExcluded ? null : item.description, + qbItemId: item.isExcluded ? null : item.qbItem?.id, + qbSyncToken: item.isExcluded ? null : item.qbItem?.syncToken, + copilotName: item.name, + isExcluded: item.isExcluded, + } + const conditions = and( + eq(QBProductSync.portalId, this.user.workspaceId), + eq(QBProductSync.productId, item.id), + ) as WhereClause + await this.updateOrCreateQBProduct(payload, conditions) } - - return await this.getAll() - } finally { - this.unsetTransaction() } + + return await this.getAll() }) } @@ -488,77 +482,71 @@ export class ProductService extends BaseService { }) const intuitApi = new IntuitAPI(qbTokenInfo) - await this.db.transaction(async (tx) => { - this.setTransaction(tx) - try { - const mappedProduct = await this.getOne( - // 01. if this product is already mapped to a QB item, do nothing. - and( - eq(QBProductSync.portalId, this.user.workspaceId), - eq(QBProductSync.productId, productResource.id), - ) as WhereClause, - ['id'], - ) + await this.withTransaction(async () => { + const mappedProduct = await this.getOne( + // 01. if this product is already mapped to a QB item, do nothing. + and( + eq(QBProductSync.portalId, this.user.workspaceId), + eq(QBProductSync.productId, productResource.id), + ) as WhereClause, + ['id'], + ) - addSyncBreadcrumb('Product mapping check', { - alreadyMapped: !!mappedProduct, - }) - if (mappedProduct) { - console.info('Product already mapped to a QB item; skipping') - return - } + addSyncBreadcrumb('Product mapping check', { + alreadyMapped: !!mappedProduct, + }) + if (mappedProduct) { + console.info('Product already mapped to a QB item; skipping') + return + } - const qbItemName = truncateForQB( - replaceSpecialCharsForQB(productResource.name), - ) - const productDescription = convert(productResource.description) - - // check if item with name exists in QBO - let qbItem = await intuitApi.getAnItem(qbItemName, undefined, true) - - if (!qbItem) { - const tokenService = new TokenService(this.user) - const incomeAccountRef = - await tokenService.checkAndUpdateAccountStatus( - AccountTypeObj.Income, - qbTokenInfo.intuitRealmId, - intuitApi, - qbTokenInfo.incomeAccountRef, - ) - // create item in QB. No price at product.created time — invoice lines - // carry their own UnitPrice. - qbItem = await this.createItemInQB( - { - productName: z.string().parse(qbItemName), - incomeAccRefVal: z.string().parse(incomeAccountRef), - productDescription, - }, - intuitApi, - ) - } + const qbItemName = truncateForQB( + replaceSpecialCharsForQB(productResource.name), + ) + const productDescription = convert(productResource.description) - // map product to the QB item - await this.createQBProduct({ - portalId: this.user.workspaceId, - productId: productResource.id, - qbItemId: qbItem.Id, - qbSyncToken: qbItem.SyncToken, - name: qbItemName, - copilotName: productResource.name, - description: productDescription, - }) + // check if item with name exists in QBO + let qbItem = await intuitApi.getAnItem(qbItemName, undefined, true) - console.info( - 'WebhookService#webhookProductCreated | Product created in QB', + if (!qbItem) { + const tokenService = new TokenService(this.user) + const incomeAccountRef = await tokenService.checkAndUpdateAccountStatus( + AccountTypeObj.Income, + qbTokenInfo.intuitRealmId, + intuitApi, + qbTokenInfo.incomeAccountRef, + ) + // create item in QB. No price at product.created time — invoice lines + // carry their own UnitPrice. + qbItem = await this.createItemInQB( + { + productName: z.string().parse(qbItemName), + incomeAccRefVal: z.string().parse(incomeAccountRef), + productDescription, + }, + intuitApi, ) - await this.logSync(productResource.id, qbItem.Id, EventType.CREATED, { - productName: productResource.name, - qbItemName: qbItem.Name, - }) - } finally { - this.unsetTransaction() } - }) + + // map product to the QB item + await this.createQBProduct({ + portalId: this.user.workspaceId, + productId: productResource.id, + qbItemId: qbItem.Id, + qbSyncToken: qbItem.SyncToken, + name: qbItemName, + copilotName: productResource.name, + description: productDescription, + }) + + console.info( + 'WebhookService#webhookProductCreated | Product created in QB', + ) + await this.logSync(productResource.id, qbItem.Id, EventType.CREATED, { + productName: productResource.name, + qbItemName: qbItem.Name, + }) + }, [this.syncLogService]) } async queryItemsFromQB(qbTokenInfo: IntuitAPITokensType, limit: number) { diff --git a/src/app/api/quickbooks/sync/sync.service.ts b/src/app/api/quickbooks/sync/sync.service.ts index 9e6129c9..e6a64418 100644 --- a/src/app/api/quickbooks/sync/sync.service.ts +++ b/src/app/api/quickbooks/sync/sync.service.ts @@ -577,29 +577,22 @@ export class SyncService extends BaseService { // TODO: notify IU about the account suspension - await this.db.transaction(async (tx) => { - this.setTransaction(tx) - const tokenService = new TokenService(this.user) - const suspendAccount = tokenService.updateQBPortalConnection( - { - isSuspended: true, - }, + const tokenService = new TokenService(this.user) + // Sequential: both writes share one tx connection (no concurrent queries). + await this.withTransaction(async () => { + await tokenService.updateQBPortalConnection( + { isSuspended: true }, eq(QBPortalConnection.portalId, this.user.workspaceId), ['id'], ) - - const deleteLogs = this.syncLogService.updateQBSyncLog( - { - deletedAt: new Date(), - }, + await this.syncLogService.updateQBSyncLog( + { deletedAt: new Date() }, and( eq(QBSyncLog.portalId, this.user.workspaceId), eq(QBSyncLog.status, LogStatus.FAILED), ) as WhereClause, ) - await Promise.all([suspendAccount, deleteLogs]) - this.unsetTransaction() - }) + }, [tokenService, this.syncLogService]) CustomLogger.info({ message: `SyncService#checkAndSuspendAccount | Suspended the account. Portal Id: ${this.user.workspaceId}`, diff --git a/test/unit/core/baseService.withTransaction.test.ts b/test/unit/core/baseService.withTransaction.test.ts new file mode 100644 index 00000000..a611d093 --- /dev/null +++ b/test/unit/core/baseService.withTransaction.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +// Hoisted so the vi.mock factory (also hoisted) can reference these safely. +const { singletonDb, FAKE_TX } = vi.hoisted(() => { + const FAKE_TX = { __tx: true } + return { + FAKE_TX, + // Runs the callback with FAKE_TX, re-throwing on error like drizzle. + singletonDb: { + transaction: vi.fn(async (cb: (tx: unknown) => Promise) => + cb(FAKE_TX), + ), + }, + } +}) +vi.mock('@/db', () => ({ db: singletonDb, client: {} })) + +import { BaseService } from '@/app/api/core/services/base.service' +import User from '@/app/api/core/models/User.model' + +// Subclass to reach the protected members under test. +class TestService extends BaseService { + get currentDb() { + return this.db + } + run(fn: () => Promise, services: BaseService[] = []) { + return this.withTransaction(() => fn(), services) + } +} + +const makeService = () => new TestService({} as User) + +describe('BaseService.withTransaction', () => { + beforeEach(() => vi.clearAllMocks()) + + it('points this.db at the tx during fn and restores it after success', async () => { + const service = makeService() + expect(service.currentDb).toBe(singletonDb) + + let dbDuringFn: unknown + await service.run(async () => { + dbDuringFn = service.currentDb + }) + + expect(dbDuringFn).toBe(FAKE_TX) + expect(service.currentDb).toBe(singletonDb) // restored + }) + + it('also binds and restores extra services passed in', async () => { + const service = makeService() + const nested = makeService() + + let nestedDbDuringFn: unknown + await service.run(async () => { + nestedDbDuringFn = nested.currentDb + }, [nested]) + + expect(nestedDbDuringFn).toBe(FAKE_TX) // nested joined the tx + expect(nested.currentDb).toBe(singletonDb) // restored + }) + + it('restores every bound service even when fn throws', async () => { + const service = makeService() + const nested = makeService() + + await expect( + service.run(async () => { + throw new Error('boom') + }, [nested]), + ).rejects.toThrow('boom') + + // The finally wraps the transaction, so neither is left on a dead handle. + expect(service.currentDb).toBe(singletonDb) + expect(nested.currentDb).toBe(singletonDb) + }) + + it('returns the value produced inside the transaction', async () => { + const service = makeService() + const result = await service.run(async () => 'done') + expect(result).toBe('done') + }) +})