Skip to content

OUT-3815 | OUT-3819: collapse qb_product_sync to one row per product, drop price columns#259

Merged
SandipBajracharya merged 2 commits into
OUT-3816from
OUT-3815
Jun 5, 2026
Merged

OUT-3815 | OUT-3819: collapse qb_product_sync to one row per product, drop price columns#259
SandipBajracharya merged 2 commits into
OUT-3816from
OUT-3815

Conversation

@SandipBajracharya

Copy link
Copy Markdown
Collaborator

Summary

Stacked on OUT-3816. Combines two tickets plus a transaction-safety fix.

  • OUT-3815: add partial unique index uq_qb_product_sync_product_active on (portal_id, product_id) WHERE deleted_at IS NULL.
  • OUT-3819: drop the now-vestigial price_id, unit_price, copilot_unit_price columns and remove the legacy unitPrice read in webhookProductUpdated.
  • Both schema changes are folded into one migration (20260603102427).
  • Hardening (product.service.ts):
    • onConflictDoNothing (scoped to the partial index via where) on the initial-save batch insert, so a re-fired/concurrent save no-ops instead of 500-ing on the new unique constraint.
    • wrap both db.transaction callbacks in try/finally so unsetTransaction() always restores the this.db singleton, even on a throw or early return.

⚠️ Deploy ordering

The one-time dedup SQL lives in the gitignored supabase/snippets/cleanup.sql and must be hand-run against prod before this deploys. build.sh auto-runs drizzle-kit migrate, and the non-concurrent CREATE UNIQUE INDEX aborts if duplicate live rows still exist. The script has a dry-run SELECT followed by the soft-delete UPDATE (keeps the earliest live row per (portal_id, product_id)).

Test plan

  • tsc --noEmit clean
  • yarn lint:check 0 errors
  • yarn test — 286/286 pass (new migration applies cleanly in the testcontainers DB; unique index doesn't collide with seeds)

🤖 Generated with Claude Code

… price columns

OUT-3815: add partial unique index uq_qb_product_sync_product_active on
(portal_id, product_id) WHERE deleted_at IS NULL.

OUT-3819: drop the vestigial price_id, unit_price, copilot_unit_price columns
and remove the legacy unitPrice read in webhookProductUpdated. Both schema
changes are folded into one migration (20260603102427). The one-time dedup SQL
is hand-run against prod before deploy (gitignored snippet).

Hardening in product.service.ts:
- guard the initial-save batch insert in handleProductMap with
  onConflictDoNothing scoped to the partial index, so a re-fired/concurrent
  save no-ops instead of 500-ing.
- wrap both db.transaction callbacks in try/finally so unsetTransaction()
  always restores the db singleton, even on a throw or early return.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Jun 4, 2026

Copy link
Copy Markdown

OUT-3815

OUT-3819

@vercel

vercel Bot commented Jun 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
quickbooks-sync Building Building Jun 4, 2026 7:38am
quickbooks-sync (dev) Ready Ready Preview, Comment Jun 4, 2026 7:38am

Request Review

@SandipBajracharya SandipBajracharya changed the title feat(OUT-3815): collapse qb_product_sync to one row per product, drop price columns OUT-3815: collapse qb_product_sync to one row per product, drop price columns Jun 4, 2026
@greptile-apps

greptile-apps Bot commented Jun 4, 2026

Copy link
Copy Markdown

Greptile Summary

This PR enforces one active row per product in qb_product_sync (partial unique index on (portal_id, product_id) WHERE deleted_at IS NULL), drops the vestigial price_id, unit_price, and copilot_unit_price columns, and hardens the two transaction callbacks in product.service.ts with try/finally so unsetTransaction() is always restored.

  • Schema / migration: new partial unique index created in a single migration alongside the three DROP COLUMN statements; Drizzle schema and snapshot updated to match.
  • Service hardening: handleProductMap now calls getAll() after the onConflictDoNothing insert (fixing the previous RETURNING [] concern), and both db.transaction callbacks gain try/finally so the this.db singleton is restored on early return or exception.
  • Test coverage: new integration test (reFiredInitialSave) fires the initial-save endpoint twice and asserts the second call returns the live mapping row rather than an empty array, with no duplicate row created.

Confidence Score: 5/5

Safe to merge — all schema changes are additive or purely drop vestigial columns, service logic is straightforward, and the new integration test directly exercises the conflict path.

Both transaction callbacks are correctly hardened with try/finally, the onConflictDoNothing + getAll() approach properly handles re-fired saves without leaking an empty response, and the migration cleanly removes three unused columns alongside the new partial unique index. The new test locks in the fixed behavior end-to-end.

No files require special attention. The deploy-ordering note (run cleanup.sql before migrating) is documented in the PR description and is the only operational gate.

Important Files Changed

Filename Overview
src/app/api/quickbooks/product/product.service.ts Removes returningFields from handleProductMap, replaces bare RETURNING with getAll() after onConflictDoNothing, wraps both transaction callbacks in try/finally so unsetTransaction() is guaranteed even on early return or throw, and drops all unitPrice references.
src/db/migrations/20260603102427_collapse_qb_product_sync_one_row_drop_price_columns.sql Creates partial unique index uq_qb_product_sync_product_active on (portal_id, product_id) WHERE deleted_at IS NULL, then drops the three vestigial price columns.
src/db/schema/qbProductSync.ts Drops priceId, unitPrice, copilotUnitPrice columns and adds the partial unique index declaration.
test/integration/quickbooks/productMap/reFiredInitialSave.test.ts New integration test covering the onConflictDoNothing + getAll() fix end-to-end.
src/hook/useSettings.ts Removes unitPrice: null assignments from product-mapping payloads.
test/helpers/seed.ts Removes unitPrice from base product sync seed.
test/integration/quickbooks/invoiceCreated/lineAmountUnitPrice.test.ts Removes stale unitPrice comment from seed call.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[handleProductMap] --> B[setTransaction tx]
    B --> C{initialProductSettingMap?}
    C -->|false - initial save| D[insert mappingItems + portalId\nonConflictDoNothing on partial index]
    D --> E[return getAll]
    C -->|true - subsequent save| F{changedItemReference length > 0?}
    F -->|yes| G[Promise.all: updateOrCreateQBProduct per item]
    G --> H[return getAll]
    F -->|no| H
    E --> FIN[finally: unsetTransaction]
    H --> FIN
    FIN --> I[transaction committed]
Loading

Reviews (2): Last reviewed commit: "fix(OUT-3815): return live mapping from ..." | Re-trigger Greptile

Comment on lines +234 to +246
const query = this.db
.insert(QBProductSync)
.values(formattedPayload)
.onConflictDoNothing({
target: [QBProductSync.portalId, QBProductSync.productId],
where: isNull(QBProductSync.deletedAt),
})
const products = returningFields?.length
? await query.returning(
buildReturningFields(QBProductSync, returningFields),
)
: await query.returning()
return products

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 onConflictDoNothing + RETURNING returns [] on a conflict, which the API sends to the client

PostgreSQL's RETURNING clause only yields rows that were actually inserted; rows skipped by ON CONFLICT DO NOTHING are silently omitted. On a re-fired or concurrent initial save every row will conflict, so products will be []. The controller wraps this in NextResponse.json(products), meaning the frontend receives an empty array and may treat it as the authoritative, empty product list — overwriting whatever state it already had. Consider using onConflictDoUpdate (no-op set) so that RETURNING still yields the surviving rows, or fall through to this.getAll() when the returned array is empty.

@@ -0,0 +1,4 @@
CREATE UNIQUE INDEX "uq_qb_product_sync_product_active" ON "qb_product_sync" USING btree ("portal_id","product_id") WHERE "qb_product_sync"."deleted_at" is null;--> statement-breakpoint

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Non-concurrent unique index creation takes an ACCESS EXCLUSIVE lock

CREATE UNIQUE INDEX (without CONCURRENTLY) acquires an ACCESS EXCLUSIVE lock on qb_product_sync for the entire duration of the index build, blocking all reads and writes. On a busy production table this will cause visible downtime. CREATE UNIQUE INDEX CONCURRENTLY builds the index without the table-level lock, but it cannot run inside a transaction block — if drizzle-kit migrate wraps statements in a transaction, the CONCURRENTLY keyword must be handled via a raw migration outside that transaction.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're managing all migrations via drizzle-kit. Besides, the records in the table is quiet low (2k) so this might not have severe impact.

The initial-save insert uses onConflictDoNothing, so RETURNING omitted rows
skipped on a re-fired/concurrent save and the client could receive []. Always
return getAll() instead; drop the now-unused returningFields param. Add a
regression test covering a repeated initial save.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@SandipBajracharya

Copy link
Copy Markdown
Collaborator Author

@greptileai

@SandipBajracharya SandipBajracharya changed the title OUT-3815: collapse qb_product_sync to one row per product, drop price columns OUT-3815 | OUT-3819: collapse qb_product_sync to one row per product, drop price columns Jun 4, 2026

@priosshrsth priosshrsth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@SandipBajracharya SandipBajracharya merged commit 72901b1 into OUT-3816 Jun 5, 2026
5 checks passed
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