notid/integreat
1
0
Fork 0
Code Issues Pull Requests 4 Actions Packages Projects Releases Wiki Activity

docs(sales-summaries): say concretely what each fix changes

Browse Source 
The report explained the faults and what repairing them was worth, but never
what the repair actually is. Adds a section showing the real code for each of
the five changes, with the reasoning that is not obvious from reading it.

  1  put the client in the record's name — scoped-key, five call sites
  2  find the existing record under either name before writing — existing-id,
     which is what makes the rename safe to deploy and why the totals did not
     double across 213,943 renamed records
  3  give every order its own payment record — what :keep and :clone do, and
     why the Square id must be recovered from the record's owner rather than by
     trimming a prefix, since client codes like N-30003 contain dashes
  4  add untendered tips rather than replacing the calculation, because where an
     order does have a payment the payment is the correct source
  5  credit service charges, with both branches of the vendor test explained —
     ezCater commission must be excluded, but whole eras of Square orders carry
     no vendor at all, so a test on vendor alone credits nothing

Plus the four supporting changes and why the schema install order mattered
enough to block every test in the suite.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Bryce
2026-08-15 14:42:32 -07:00
parent 011d515033
commit 3abeb575a0
1 changed files with 151 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

151
docs/2026-08-15-thirty-day-reconciliation.html
View File

@@ -161,6 +161,157 @@
</div>
</section>
<section>
<h2>What the fixes actually are</h2>
<div class="measure">
<p>Five changes. The first three stop two clients from sharing a record; the last two record
money that was being collected but not booked. Each is small — the difficulty was knowing
which line to change, not writing it.</p>
</div>
<h3>1 · Put the client in the record's name</h3>
<div class="measure">
<p>Every imported record has an identifier the importer uses to decide "have I seen this
before?". Sales orders already included the client; refunds, card payments, payouts and
cash-drawer shifts did not, which is precisely why two clients could land on one record.</p>
</div>
<div class="scroll">
<pre><span class="dim">;; before — the bare Square id, identical for both clients</span>
(str "square/refund/" (:id r)) <span class="dim">;; square/refund/NOkQOTIiJULWN6…</span>
<span class="dim">;; after</span>
(scoped-key "square/refund/" client location (:id r))
<span class="dim">;; square/refund/NGCD-CD-NOkQOTIiJULWN6…</span>
(defn scoped-key [prefix client location id]
(str prefix (:client/code client) "-"
(:square-location/client-location location) "-" id))</pre>
</div>
<div class="measure">
<p>Applied at five places in the Square importer: order payments, refunds, payouts (twice —
the record itself and the lookup that finds it) and cash-drawer shifts. ezCater orders
already did this and needed no change.</p>
</div>
<h3>2 · Find the existing record before writing, under either name</h3>
<div class="measure">
<p>This is the one that makes the change safe to deploy. The identifiers are unique keys, so
the importer relies on "same id, same record". Rename them and the next import matches
nothing — and would quietly create a <em>second</em> copy of every refund and payment in the
system, leaving the originals orphaned. So the importer looks up the record explicitly,
new name first, old name second, and writes to whichever it finds.</p>
</div>
<div class="scroll">
<pre>(defn existing-id [db attr prefix client location id]
(when id
(or (dc/entid db [attr (scoped-key prefix client location id)]) <span class="dim">;; new scheme</span>
(dc/entid db [attr (str prefix id)])))) <span class="dim">;; legacy scheme</span></pre>
</div>
<div class="measure">
<p>The result is pinned as the record's id on the way in, so the write lands on the existing
row regardless of which name it currently carries. The proof this worked: after re-naming
213,943 records, the totals for refunds, payouts and shifts were <strong>identical before and
after</strong>. Had the fallback been missing they would have doubled.</p>
</div>
<h3>3 · Give every order its own payment record</h3>
<div class="measure">
<p>Renaming stops <em>new</em> collisions but does not undo old ones: a payment already shared
by two orders is still one row with two owners. The migration walks each order's payments and,
where another order has already claimed one, makes that order its own copy with the same
amounts and points the order at the copy.</p>
</div>
<div class="scroll">
<pre><span class="dim">;; for each order, for each of its payments:</span>
:keep <span class="dim">→</span> first order to claim it; rename in place
:clone <span class="dim">→</span> copy type, total, tip, tax, date, processor, note, receipt link
set the copy's client and location to this order's
retract this order's link to the shared payment
link it to the copy instead</pre>
</div>
<div class="measure">
<p>On the restored data that was <strong>189,167 renamed and 77,599 copied</strong>, and payments
owned by two orders went from 11,469 to zero. The record count rose by exactly 77,599 — the
number of copies it reported making, which is the check that it created what it meant to and
nothing else.</p>
<p>One subtlety worth recording, because it bit us: the Square id has to be recovered from the
record's current owner rather than by trimming a fixed prefix. Client codes contain dashes —
<span class="mono">N-30003</span> — so a pattern cannot tell where the client name ends and the
Square id begins. Getting this wrong scoped some records twice and doubled their tender.</p>
</div>
<h3>4 · Count tips that were handed back</h3>
<div class="measure">
<p>Tips were summed by walking from the order to its payments. A refund-only order has no
payment attached, so its negative tip was invisible. The fix adds those tips rather than
replacing the calculation.</p>
</div>
<div class="scroll">
<pre><span class="dim">;; before</span>
:ledger-mapped/amount (tendered-tip c date)
<span class="dim">;; after</span>
:ledger-mapped/amount (+ (tendered-tip c date)
(untendered-tip c date))
<span class="dim">;; untendered-tip — tips on orders with no payment attached</span>
[?e :sales-order/tip ?tip]
(not [?e :sales-order/charges])</pre>
</div>
<div class="measure">
<p><strong>Adding rather than replacing is deliberate.</strong> Where an order does have a
payment, the payment is the correct source: real orders exist whose payment carries a tip the
order does not — an auto-gratuity recorded as a service charge, or a wallet tip missing from
the order totals. Reading the order instead would have dropped those. Three tests hold this
in place: the refund case must change, and the tendered and ordinary cases must not.</p>
</div>
<h3>5 · Credit Square service charges, both signs</h3>
<div class="measure">
<p>Nothing read the service-charge field at all. A new line credits it, for Square orders only
and for negative amounts as well as positive.</p>
</div>
<div class="scroll">
<pre>[?e :sales-order/service-charge ?service-charge]
(or-join [?e]
[?e :sales-order/vendor :vendor/ccp-square]
(and (not [?e :sales-order/vendor])
[?e :sales-order/external-id ?external-id]
[(clojure.string/starts-with? ?external-id "square/order/")]))</pre>
</div>
<div class="measure">
<p><strong>Why the vendor test has two branches.</strong> ezCater service charges are commission
the platform deducts from the restaurant, not money the diner hands over, so crediting them
would make a day worse rather than better — hence the Square-only condition. But whole eras of
Square orders carry no vendor field at all, and a test on vendor alone would silently credit
nothing. The second branch falls back to the order's own identifier.</p>
<p><strong>Why negatives matter.</strong> A returned catering fee arrives as a negative service
charge and is already deducted from the day's returns; dropping negatives would lose the
reversal. The line sits behind a per-client switch, off by default, so it can be turned on a
few restaurants at a time.</p>
</div>
<h3>Supporting changes</h3>
<div class="scroll">
<table>
<thead><tr><th>Change</th><th>Why</th></tr></thead>
<tbody>
<tr><td>Log each day's imbalance and its suspect lines</td><td>an out-of-balance day was only visible by opening the screen; now it can be queried</td></tr>
<tr><td>Stop the dirty-summary scan at the client boundary</td><td>it read every later client's summaries too — 1,321 ms to 5.6 ms per client</td></tr>
<tr><td>Split the recompute driver into a per-client function</td><td>lets a backfill spread clients across threads instead of grinding one at a time</td></tr>
<tr><td>Install schema attributes before the tuples that compose them</td><td>the test suite could not build an empty database at all, so no test could run</td></tr>
</tbody>
</table>
</div>
<div class="measure">
<p>That last one is worth a sentence for engineers: <code>transact-schema</code> installed
schema.edn then cloud-migration-schema.edn, but a composite tuple in the first file is built
from an attribute in the second. Datomic will not create a tuple before its members exist, so
every test fixture died in setup. It is very likely why sales summaries had no tests before
this work.</p>
</div>
</section>
<section>
<h2>What each fix is worth</h2>
<div class="measure">