Skip to content

OUT-3625: suffix DisplayName on QB Vendor/Employee collision - #233

Merged
SandipBajracharya merged 3 commits into
masterfrom
OUT-3625
Apr 22, 2026
Merged

OUT-3625: suffix DisplayName on QB Vendor/Employee collision#233
SandipBajracharya merged 3 commits into
masterfrom
OUT-3625

Conversation

@SandipBajracharya

@SandipBajracharya SandipBajracharya commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes QB customer creation failing with error 6240 when the Copilot client's DisplayName collides with an existing QB Vendor or Employee (QBO enforces DisplayName uniqueness across all three entity types).
  • Adds a pre-check via a new IntuitAPI.getNameCollisionEntity(displayName) that queries Vendor then Employee; if a collision is found, appends (Customer) suffix to the payload DisplayName before calling createCustomer.
  • Separate refactor commit removes 8 unreachable Fault-check branches that sat after customQuery calls (dead because _customQuery already throws on Fault and returns res.QueryResponse, which has no Fault field).

Notes

  • Inactive Vendors/Employees are intentionally excluded from the collision check — QBO auto-suffixes their DisplayName with (deleted) when deactivated, freeing the original name.
  • _getNameCollisionEntity short-circuits after finding a Vendor match (skips the Employee query).
  • Mapping table's displayName column continues to store the Copilot-side original (not the suffixed QB name); the column's semantic is preserved and a comment now documents the intentional divergence.
  • Critical-path tests were not added (no test infrastructure in this repo yet per OUT-3625 memory; separate initiative).

Test plan

  • In QB sandbox: create a Vendor named Acme Corp, then trigger a Copilot invoice webhook for a client whose display name resolves to Acme Corp → verify a Customer named Acme Corp (Customer) is created in QB without error 6240.
  • In QB sandbox: deactivate a Vendor named Beta Inc (QB renames it to Beta Inc (deleted)), trigger webhook for a client Beta Inc → verify a Customer named Beta Inc is created (no suffix, since the inactive vendor no longer holds the name).
  • Happy path regression: trigger webhook for a recipient whose display name does not collide with any Vendor/Employee → verify customer is created with the unsuffixed DisplayName and only 2 QB API calls were made (Vendor query + Employee query, or fewer if short-circuit).
  • Verify existing findOrCreateCustomer behaviors unchanged (email-based short-circuit, CompanyName mismatch clearing).
  • Refactor regression: trigger a query that would fault (e.g., invalid realm) → verify error surfaces with IntuitAPIErrorMessage prefix so isIntuitError detection in src/utils/error.ts still classifies it correctly.

Testing Criteria

handling of duplicate name across Customer, Employee and Vendor

https://www.loom.com/share/14cdacf0e4ed4918ac5b9454ac918eb5

Case when two companies have same name

https://www.loom.com/share/11c319b68ec54fee8b7c8be75ba57581

🤖 Generated with Claude Code

SandipBajracharya and others added 2 commits April 21, 2026 20:07
QB enforces DisplayName uniqueness across Customer, Vendor, and Employee.
Customer creation was failing with error 6240 when a Copilot client's
DisplayName collided with an existing QB Vendor/Employee. Pre-check the
collision and append " (Customer)" suffix to disambiguate.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_customQuery already throws an APIError when the QB response contains a
Fault, and returns res.QueryResponse (which has no Fault field). The
post-call Fault checks in read methods that wrap customQuery were
therefore unreachable. Remove them from the 8 affected methods.

Mutation methods (createInvoice, createCustomer, etc.) are unchanged —
they call postFetchWithHeaders directly, which does not throw on Fault,
so their Fault checks are load-bearing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@linear-code

linear-code Bot commented Apr 21, 2026

Copy link
Copy Markdown

@vercel

vercel Bot commented Apr 21, 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 Apr 22, 2026 7:13am
quickbooks-sync (dev) Ready Ready Preview, Comment Apr 22, 2026 7:13am

Request Review

@SandipBajracharya SandipBajracharya changed the title fix(OUT-3625): suffix DisplayName on QB Vendor/Employee collision OUT-3625: suffix DisplayName on QB Vendor/Employee collision Apr 21, 2026
@greptile-apps

greptile-apps Bot commented Apr 21, 2026

Copy link
Copy Markdown

Greptile Summary

Adds a pre-flight Vendor/Employee collision check in findOrCreateCustomer so that when a Copilot client's DisplayName already exists in QBO as a Vendor or Employee (which would trigger error 6240), it is re-created with a (Customer) suffix. A companion refactor removes 8 now-confirmed-dead Fault-check branches that sat after customQuery calls; _customQuery already throws on Fault, making those branches unreachable.

Confidence Score: 4/5

Safe to merge; the two P2 findings are edge cases that don't affect the common path.

The primary fix is correct and well-reasoned. Two P2 findings remain: a .trim() discrepancy between the query and the payload (cosmetic in practice), and the absence of a second-pass collision check for the suffixed name itself (very unlikely but could still yield 6240 in a contrived scenario). Neither blocks production use.

src/utils/intuitAPI.ts — review the two P2 comments in _getNameCollisionEntity.

Important Files Changed

Filename Overview
src/utils/intuitAPI.ts Adds _getNameCollisionEntity (Vendor → Employee short-circuit, wrapped with retry); removes 8 dead Fault-check branches that were unreachable because _customQuery already throws on Fault.
src/app/api/quickbooks/customer/customer.service.ts Inserts pre-flight collision check before createCustomer; applies (Customer) suffix when a Vendor/Employee collision is found; stores original Copilot displayName in mapping table (correct, now documented).
src/utils/string.ts Adds getNameAsCustomer — appends (Customer) suffix, idempotent (no-op if already present). Simple, correct utility function.

Sequence Diagram

sequenceDiagram
    participant CS as CustomerService
    participant IA as IntuitAPI
    participant QBO as QuickBooks Online

    CS->>IA: getCustomerByEmail(email) OR getACustomer(displayName)
    IA->>QBO: SELECT Customer WHERE ...
    QBO-->>IA: null (not found)
    IA-->>CS: null

    CS->>IA: getNameCollisionEntity(sanitizedDisplayName)
    IA->>QBO: SELECT Id FROM Vendor WHERE DisplayName = ?
    QBO-->>IA: Vendor match OR empty

    alt Vendor collision found
        IA-->>CS: { type: Vendor, id: ... }
        CS->>CS: finalDisplayName = sanitizedDisplayName + (Customer)
    else No vendor collision
        IA->>QBO: SELECT Id FROM Employee WHERE DisplayName = ?
        QBO-->>IA: Employee match OR empty
        alt Employee collision found
            IA-->>CS: { type: Employee, id: ... }
            CS->>CS: finalDisplayName = sanitizedDisplayName + (Customer)
        else No collision
            IA-->>CS: null
            CS->>CS: finalDisplayName = sanitizedDisplayName
        end
    end

    CS->>IA: createCustomer({ DisplayName: finalDisplayName, ... })
    IA->>QBO: POST /customer
    QBO-->>IA: Created customer
    IA-->>CS: Customer record

    CS->>CS: createQBCustomer mapping (stores original Copilot displayName)
Loading

Reviews (1): Last reviewed commit: "refactor: remove dead Fault checks after..." | Re-trigger Greptile

Comment thread src/utils/intuitAPI.ts Outdated
Comment on lines +326 to +337
const vendorQuery = `SELECT Id FROM Vendor WHERE DisplayName = '${sanitizedDisplayName}' maxresults 1`
const vendorRes = await this.customQuery(vendorQuery)

if (vendorRes?.Vendor?.length) {
return { type: 'Vendor', id: vendorRes.Vendor[0].Id }
}

const employeeQuery = `SELECT Id FROM Employee WHERE DisplayName = '${sanitizedDisplayName}' maxresults 1`
const employeeRes = await this.customQuery(employeeQuery)

if (employeeRes?.Employee?.length) {
return { type: 'Employee', id: employeeRes.Employee[0].Id }

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 Suffixed name not re-checked for collision

After finding a collision and computing finalDisplayName = "X (Customer)", the code never verifies that this new name itself is free of Vendor/Employee collisions. If a Vendor (or Employee) named "X (Customer)" already exists, the createCustomer call will still fail with error 6240 — the same error the fix is trying to prevent.

The fix handles the common case well, but a second-pass query for the suffixed name would make it fully resilient. Consider adding a similar check for the (Customer) name before returning, or at minimum documenting the unhandled case.

Comment thread src/utils/intuitAPI.ts Outdated
async _getNameCollisionEntity(
displayName: string,
): Promise<{ type: 'Vendor' | 'Employee'; id: string } | null> {
const sanitizedDisplayName = escapeForQBQuery(displayName.trim())

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 .trim() diverges from the payload name

displayName arrives here already processed by replaceSpecialCharsForQB, which preserves spaces (space is in the allowed-character list). The .trim() call creates a mismatch: the collision query checks the trimmed form while finalDisplayName — and the eventual QB DisplayName payload — retains any leading/trailing spaces from the sanitized name.

In practice display names are unlikely to carry stray spaces, but aligning the trim with the call site (or trimming before replaceSpecialCharsForQB in customer.service.ts) would eliminate the inconsistency.

Suggested change
const sanitizedDisplayName = escapeForQBQuery(displayName.trim())
const sanitizedDisplayName = escapeForQBQuery(displayName)

Comment thread src/app/api/quickbooks/customer/customer.service.ts Outdated
…re create

The previous pre-check only looked at Vendor and Employee, so two Copilot
clients with identical display names sharing a QB Vendor would still
collide: the first would create "<name> (Customer)" and the second would
try the same name and fail with QB error 6200.

Replace the collision check with a resolver that builds a candidate
sequence (base, "(Customer)", "(Customer) 2", ..., up to 20) and queries
Customer, Vendor, and Employee in parallel with DisplayName IN (...),
returning the first candidate not in the used set. Case-insensitive
match because QBO DisplayName equality is case-insensitive.

Not wrapped in wrapWithRetry: the inner customQuery calls already retry
on 429, and re-wrapping would amplify rate-limit bursts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@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. I think istead of appending number to suffixed name, we could do it in displayName. But it looks good to me as it is as well.

@SandipBajracharya
SandipBajracharya merged commit 787fcb9 into master Apr 22, 2026
4 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