Repository navigation
feat(quickbooks): disambiguate payment-id confirmation and surface unposted DDB receipts - #239
Merged
Merged
Conversation
…posted DDB receipts
Two production findings from the October 15 reconciliation:
- The approval prompt interpolated the PaymentRefNum as the value to
enter, but the field validates against the internal QBO Payment ID.
The UI now labels both identifiers explicitly ("QBO Payment ID" /
"Payment reference") and the instruction names the required value.
Backend validation is unchanged — PaymentRefNum can never substitute.
- A $5 pre-flight website charge predating the sync feature had no QBO
Sales Receipt yet the payout ledger read as fully reconciled. DDB
lines now load their durable sync record: anything other than
`synced` (or no record) marks the line ddb_receipt_unverified and
raises a ddb_receipt_unposted exception naming the payment and the
expected SalesReceipt. The executor's existing ddb_gross_unposted
refusal remains the apply-time gate.
- Marker-adopt now verifies the found receipt's DepositToAccountRef
against the expected mapped account — a receipt parked in the wrong
account lands needs_attention (wrong_deposit_account) instead of
being counted as synced.
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideThe PR makes QBO payment approvals unambiguous, surfaces missing or incomplete DDB Sales Receipt posting in payout reconciliation through durable sync-record checks, and fails closed when marker-based receipt recovery finds a receipt in the wrong deposit account, without adding new QBO or Stripe writes. Sequence diagram for DDB receipt verification during payout reconciliationsequenceDiagram
participant P as Stripe payout processor
participant F as Firebase sync records
participant L as Payout ledger
participant A as Admin
P->>F: loadDdbReceiptStatuses()
F-->>P: status per resolved payment
P->>L: buildPayoutLedger(ddbReceiptStatus)
alt status is synced
L-->>A: DDB line verified
else missing, pending, failed, or needs_attention
L-->>A: ddb_receipt_unposted exception
end
Sequence diagram for marker-adopt deposit account validationsequenceDiagram
participant W as QBO sync worker
participant Q as QuickBooks
participant H as Human operator
W->>Q: findQboSalesReceiptForMarker()
Q-->>W: marker receipt with deposit account
alt expected deposit account
W-->>H: synced existing receipt
else wrong deposit account
W-->>H: needs_attention wrong_deposit_account
Note over W,Q: No replacement receipt is created
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 11 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely summarizes the primary changes: payment-ID confirmation and unposted DDB receipt detection. Description check The description provides detailed change context, root cause, scope, test results, deployment impact, and documentation updates. It does not reproduce the template headings or include a Closes issue r… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 11 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/qbo-sync.ts" line_range="572-574" />
<code_context>
+ // the gross is NOT in the expected account: fail closed for a
+ // human rather than marking the sync complete or posting a
+ // duplicate receipt.
+ if (
+ existing.depositToAccountId &&
+ existing.depositToAccountId !== depositAccountId
+ ) {
+ return attention(
</code_context>
<issue_to_address>
**Unverified receipts appear posted**
When the marker lookup returns a receipt without a usable `DepositToAccountRef`, `runSyncWrite` skips the account mismatch check and marks the adopted receipt `synced`, so the sync record reports it posted to the mapped account without proving its destination.
Treat a missing or unparseable deposit account as unverified and require human review instead of returning `synced`.
</issue_to_address>
### Comment 2
<location path="lib/stripe-payouts-common.ts" line_range="966" />
<code_context>
+ // than letting the line read as complete.
+ if (input.ddbReceiptStatus) {
+ const receipt = input.ddbReceiptStatus.get(ddbHit);
+ if (receipt !== "synced") {
+ evidence.push("ddb_receipt_unverified");
+ exceptions.push({
</code_context>
<issue_to_address>
**Moved receipts remain verified**
When a synced receipt is moved out of the mapped account, deleted, or the expected account mapping changes, `buildPayoutLedger` trusts the durable `synced` status, while `verifyDdbGrossReceipts` checks only status and entity ID; `processQboSyncRecord` also returns `already_synced` without rechecking QuickBooks, so the receipt still appears valid and aggregate transfers can execute.
Recheck the receipt's current existence and deposit account in QuickBooks before treating it as verified in the ledger or allowing aggregate transfers.
</issue_to_address>
### Comment 3
<location path="lib/qbo-protocol.ts" line_range="739" />
<code_context>
);
}
- return `select Id, PrivateNote, TxnDate from SalesReceipt where CustomerRef = '${safe}' and TxnDate = '${txnDate}' orderby TxnDate desc startposition ${startPosition} maxresults ${maxResults}`;
+ return `select Id, PrivateNote, TxnDate, DepositToAccountRef from SalesReceipt where CustomerRef = '${safe}' and TxnDate = '${txnDate}' orderby TxnDate desc startposition ${startPosition} maxresults ${maxResults}`;
}
</code_context>
<issue_to_address>
**Marked receipts can be duplicated**
When a manually created, marker-carrying receipt has a different customer or transaction date from the sync record's configured customer and payment `paidAt` date, `qboSalesReceiptCorrelationQuery` searches only the configured customer and exact transaction date, so `findQboSalesReceiptForMarker` misses a marked receipt outside either filter and `runSyncWrite` proceeds to create another receipt. This contradicts the recovery guidance that any receipt carrying the marker will be adopted.
Either make marker recovery find the payment marker independently of customer and date, or document and enforce those required receipt fields before claiming it will be adopted.
Also at `docs/operations/quickbooks.md:459-460`.
</issue_to_address>
### Comment 4
<location path="lib/stripe-payouts.ts" line_range="318-321" />
<code_context>
+ .get();
+ for (const doc of snap.docs) {
+ const paymentId = idToPayment.get(doc.id);
+ const status = doc.data().status;
+ if (paymentId && typeof status === "string") {
+ statuses.set(paymentId, status);
+ }
+ }
+ }
</code_context>
<issue_to_address>
**Receipts without IDs appear verified**
When a sync record has status `synced` but its `qboEntityId` is missing or empty, `loadDdbReceiptStatuses` records the status as `synced` without checking the entity ID, so `buildPayoutLedger` suppresses the unposted exception and reviewers see an unverified gross as confirmed.
Only report a receipt as synced in the status map when its sync record also has a nonempty `qboEntityId`.
Also at `lib/stripe-payouts-common.ts:966`.
</issue_to_address>
### Comment 5
<location path="lib/stripe-payouts-common.ts" line_range="968-973" />
<code_context>
+ const receipt = input.ddbReceiptStatus.get(ddbHit);
+ if (receipt !== "synced") {
+ evidence.push("ddb_receipt_unverified");
+ exceptions.push({
+ code: "ddb_receipt_unposted",
+ ref: txn.txnId,
+ detail: `DDB payment ${ddbHit} has no confirmed QBO Sales Receipt (sync status: ${receipt ?? "no record"}) — the gross sale is not proven posted to Stripe clearing. Expected document: SalesReceipt. Check the QBO sync panel for the durable sync record.`,
+ amountMinor: txn.grossMinor,
+ });
+ }
+ }
</code_context>
<issue_to_address>
**Receipt warnings are dropped**
When a payout has more than 100 exceptions and a DDB receipt warning falls after the first 100, `processStripePayoutLedger` persists only the first 100 exceptions, dropping later `ddb_receipt_unposted` warnings so the payout view omits affected receipts.
Ensure DDB receipt warnings are retained when persisting the ledger, rather than dropping them at the 100-exception limit.
</issue_to_address>
### Comment 6
<location path="lib/stripe-payouts-common.ts" line_range="964-966" />
<code_context>
+ // that balances arithmetically while the receipt is missing or
+ // unfinished is NOT fully reconciled; surface it loudly rather
+ // than letting the line read as complete.
+ if (input.ddbReceiptStatus) {
+ const receipt = input.ddbReceiptStatus.get(ddbHit);
+ if (receipt !== "synced") {
+ evidence.push("ddb_receipt_unverified");
+ exceptions.push({
</code_context>
<issue_to_address>
**Resolved warnings remain visible**
When a payout ledger is built while its DDB sync record is unfinished and that record later reaches `synced`, the ledger keeps the earlier `ddb_receipt_unposted` exception, and later sweeps skip the unchanged payout once its ledger is up to date; operators continue seeing a stale exception until they manually reprocess the payout.
Invalidate or rebuild the payout ledger when the associated DDB sync record changes to `synced`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 6 findings to address first, and a marker receipt found in the wrong deposit account now leaves its sync record needing human attention, and a DDB payout line without a confirmed receipt is recorded as an exception. If either check is wrong, it can delay reconciliation, but the affected records and payout ledger can be repaired or recomputed; reverting alone may not clear the stored state.
Blocking findings: lib/qbo-sync.ts:574, lib/stripe-payouts-common.ts:966, lib/qbo-protocol.ts:739, lib/stripe-payouts.ts:321, lib/stripe-payouts-common.ts:973, and 1 more
- Marker adoption: a receipt whose DepositToAccountRef cannot be read is unproven, not verified — now fails closed to needs_attention instead of silently syncing. - Ledger receipt status: a sync record marked `synced` with no qboEntityId is unfinished evidence — the line now raises ddb_receipt_unposted with `synced_no_entity` rather than reading as confirmed. - Docs: correct the marker-adopt wording — the correlation lookup is bounded to the mapped generic customer and the payment's posted date. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
7 tasks
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two findings from the October 15 production reconciliation, each fixed narrowly:
move_payment_to_clearingapproval prompt interpolated the human-readablePaymentRefNum(e.g.1626166) as the value to enter, while the field validates against the internal QBO Payment ID (e.g.1813). The prompt now labels both identifiers explicitly (QBO Payment ID vs Payment reference) and reads "Enter QBO Payment ID 42 to confirm this payment." Backend validation is untouched —confirmQboPaymentIdstill must equal the proposal's boundqboRefs.paymentId; the reference can never substitute.$5.00pre-flight charge (predating the Sales-Receipt sync feature, outside the sweep lookback) had no QBO Sales Receipt yet the payout ledger read as reconciled. The ledger now fetches each resolved DDB payment's durable sync record: anything other thansynced— including no record — marks the lineddb_receipt_unverifiedand raises addb_receipt_unpostedexception naming the payment id and expected document. The executor's existingddb_gross_unpostedrefusal stays the apply-time gate; this adds the observability layer.DepositToAccountRefagainst the expected mapped account. A marker-carrying receipt deposited to the wrong account parksneeds_attention(wrong_deposit_account) for a human instead of being counted as synced — no blind replacement is ever created.Root cause (October 4 $5.00)
Confirmed mechanism, not a webhook failure: the charge predates the Sales-Receipt sync feature, so no durable
qboSyncRecordsrecord was ever created; the sweep's bounded lookback (72 h default, 30-day cap) deliberately does not backfill it — see Backfill bounds indocs/operations/quickbooks.md. The ledger then compounded it: addblane proved which charge it was but nothing proved the gross sale had posted. Detection now flows through the new exception; recovery was correctly done manually (the marker-adopt path would have adopted a receipt carryingddb:<paymentId>).What did not change
$5receipt is untouched; historical manual-only processing preserved.Test plan
npx tsc --noEmit— cleannpm run lint— pass (pre-existing jsx-ast-utils warning)npm test— 1252 pass (+9: ddb posted/unposted/pending/error receipt statuses, sync-record fetch wiring, wrong-account adoption, canonicalizer shape)npm run test:rules— 49 passnpm run build— cleannpx playwright test smoke-tests/admin-quickbooks.spec.ts— 18/18 incl. new test: labels distinguish ID vs reference, ref value cannot confirm, internal ID unlocks Approvenpm run check:md-links/check:react-versions— passGenerated with Devin
Summary by Sourcery
Harden QuickBooks payout reconciliation by distinguishing payment identifiers, surfacing unverified DDB receipts, and validating adopted receipts against the expected deposit account.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit