From e9970bd41a59af0f59f4971f9272e088e9d0a144 Mon Sep 17 00:00:00 2001 From: Bryce Date: Fri, 14 Aug 2026 13:55:38 -0700 Subject: [PATCH] fix(reports): collapse accounts sharing a numeric code to one row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A client's bank account and the financial account it posts to carry the same numeric code. Reports keyed their detail rows on [code, name], so the pair rendered as two rows — one labelled for the bank account, one for the financial account. The amount was duplicated too, not just the label: the row's figure is filtered by code alone, so both rows printed the whole code's total. The group subtotal counts the code once, so a section's visible rows stopped footing to their own subtotal. Penelope's Coffee and Tea overshot the 2000 Accounts Payable subtotal by $251,751.80 this way. Resolve it in two places: - build-account-lookup now maps every account at a shared code down to the bank account's name, per client. Where a client has two bank accounts on one code, lowest :bank-account/sort-order wins, then lowest :db/id, so the label is stable across runs. - used-accounts now keys rows on the code alone. Across a multi-client report the clients can still disagree, since only some of them have a bank account at the code; a bank-sourced name wins there, which the new :bank_account_name? flag carries through from the lookup. Rows are code-keyed now, so detail-rows decides whether to print a figure by asking whether the client has data at the code rather than under the winning name — otherwise a client reaching a code under a name another client won would blank out. Balance sheet, profit and loss and cash flows all route through used-accounts and are all fixed. The GraphQL and cljs balance sheets pick up the unified name through build-account-lookup. A sweep of all 146 clients with bank accounts finds duplicate rows on 8 of them before this change and none after, with every section total unchanged. --- src/clj/auto_ap/ledger.clj | 62 +++++++-- src/clj/auto_ap/ssr/ledger/balance_sheet.clj | 1 + src/clj/auto_ap/ssr/ledger/cash_flows.clj | 1 + .../auto_ap/ssr/ledger/profit_and_loss.clj | 1 + src/cljc/auto_ap/ledger/reports.cljc | 35 ++++- test/clj/auto_ap/ledger/reports_test.clj | 129 ++++++++++++++++++ test/clj/auto_ap/ledger_test.clj | 92 ++++++++++++- 7 files changed, 298 insertions(+), 23 deletions(-) create mode 100644 test/clj/auto_ap/ledger/reports_test.clj diff --git a/src/clj/auto_ap/ledger.clj b/src/clj/auto_ap/ledger.clj index 7737c100..83952261 100644 --- a/src/clj/auto_ap/ledger.clj +++ b/src/clj/auto_ap/ledger.clj @@ -315,21 +315,55 @@ [[(:db/id a) (:db/id (:account-client-override/client o))] (:account-client-override/name o)]) (:account/client-overrides a)))) - (into {}))] + (into {})) + + ;; A client's bank account and the financial account it posts to share a + ;; numeric code. Reports key their rows off that code, so the two would + ;; otherwise render as two rows for the same account. Resolve every + ;; account at a shared code down to the bank account's name. Where a + ;; client has two bank accounts on one code (a data-entry error, but it + ;; happens) the lowest :bank-account/sort-order wins, then the lowest + ;; :db/id, so the name is stable across runs. + bank-name-by-code (->> (dc/q {:find ['(pull ?b [:db/id :bank-account/name + :bank-account/numeric-code + :bank-account/sort-order])] + :in ['$ '?c] + :where ['[?c :client/bank-accounts ?b]]} + (dc/db conn) + client-id) + (map first) + (filter (every-pred :bank-account/numeric-code :bank-account/name)) + (sort-by (juxt #(or (:bank-account/sort-order %) Long/MAX_VALUE) + :db/id)) + (reduce (fn [m b] + (let [code (:bank-account/numeric-code b)] + (cond-> m + (not (contains? m code)) + (assoc code (:bank-account/name b))))) + {}))] (fn [a] - {:name (or (:bank-account/name (bank-accounts a)) - (overrides-by-client [a client-id]) - (:account/name (accounts a))) - :account_type (or (:db/ident (:account/type (accounts a))) - ({:bank-account-type/check :account-type/asset - :bank-account-type/cash :account-type/asset - :bank-account-type/credit :account-type/liability} - (:db/ident (:bank-account/type (bank-accounts a)))) - :account-type/asset ;; DEFAULT TO ASSET, for things like unknown - ) - :numeric_code (or (:account/numeric-code (accounts a)) - (:bank-account/numeric-code (bank-accounts a))) - :client_id client-id}))) + (let [account (accounts a) + bank-account (bank-accounts a) + numeric-code (or (:account/numeric-code account) + (:bank-account/numeric-code bank-account)) + bank-name (or (bank-name-by-code numeric-code) + (:bank-account/name bank-account))] + {:name (or bank-name + (overrides-by-client [a client-id]) + (:account/name account)) + ;; Whether :name above came from a bank account. Reports that pool + ;; several clients into one row per code use this to pick the label, + ;; since only some of those clients may have a bank account at the code. + :bank_account_name? (some? bank-name) + :account_type (or (:db/ident (:account/type account)) + ({:bank-account-type/check :account-type/asset + :bank-account-type/cash :account-type/asset + :bank-account-type/credit :account-type/liability} + (:db/ident (:bank-account/type bank-account))) + :account-type/asset ;; DEFAULT TO ASSET, for things like unknown + ) + :numeric_code numeric-code + :client_id client-id})))) (defn find-mismatch-index [] (reduce + 0 diff --git a/src/clj/auto_ap/ssr/ledger/balance_sheet.clj b/src/clj/auto_ap/ssr/ledger/balance_sheet.clj index 4236cd40..287ae2b2 100644 --- a/src/clj/auto_ap/ssr/ledger/balance_sheet.clj +++ b/src/clj/auto_ap/ssr/ledger/balance_sheet.clj @@ -89,6 +89,7 @@ :account-type (:account_type account) :numeric-code (:numeric_code account) :name (:name account) + :bank-account-name? (:bank_account_name? account) :period (coerce/to-date d)})) args (assoc (:query-params request) :periods (map coerce/to-date (filter identity date))) diff --git a/src/clj/auto_ap/ssr/ledger/cash_flows.clj b/src/clj/auto_ap/ssr/ledger/cash_flows.clj index ec5d9306..2a6d6249 100644 --- a/src/clj/auto_ap/ssr/ledger/cash_flows.clj +++ b/src/clj/auto_ap/ssr/ledger/cash_flows.clj @@ -88,6 +88,7 @@ :account-type (:account_type account) :numeric-code (:numeric_code account) :name (:name account) + :bank-account-name? (:bank_account_name? account) :period {:start (coerce/to-date (:start p)) :end (coerce/to-date (:end p))}})) args (assoc (:form-params request) :periods (map (fn [d] {:start (coerce/to-date (:start d)) :end (coerce/to-date (:end d))}) periods)) diff --git a/src/clj/auto_ap/ssr/ledger/profit_and_loss.clj b/src/clj/auto_ap/ssr/ledger/profit_and_loss.clj index 1093418f..fd84aff0 100644 --- a/src/clj/auto_ap/ssr/ledger/profit_and_loss.clj +++ b/src/clj/auto_ap/ssr/ledger/profit_and_loss.clj @@ -101,6 +101,7 @@ :account-type (:account_type account) :numeric-code (:numeric_code account) :name (:name account) + :bank-account-name? (:bank_account_name? account) :sample sample :period {:start (coerce/to-date (:start p)) :end (coerce/to-date (:end p))}})) diff --git a/src/cljc/auto_ap/ledger/reports.cljc b/src/cljc/auto_ap/ledger/reports.cljc index 7e138156..c811ea80 100644 --- a/src/cljc/auto_ap/ledger/reports.cljc +++ b/src/cljc/auto_ap/ledger/reports.cljc @@ -267,12 +267,33 @@ account)) accounts)))) -(defn used-accounts [pnl-datas] +(defn used-accounts + "One entry per numeric code in play, with the name to label its row. + + Rows are keyed on the code alone, never on [code name]: the amount a row + reports is the total for its code, so two names at one code would print the + same figure twice and the section would stop footing to its own subtotal. + + `build-account-lookup` has already resolved a client's shared codes down to + the bank account's name, so the name here is normally unanimous. It can + still differ across a multi-client report, where only some of the clients + have a bank account at the code. A bank account's name wins there; failing + that the most common name does, ties broken alphabetically so the report is + stable across runs." + [pnl-datas] (->> pnl-datas (mapcat :data) - (map #(select-keys % [:numeric-code :name])) - (set) + (map #(select-keys % [:numeric-code :name :bank-account-name?])) + (group-by :numeric-code) + (map (fn [[numeric-code entries]] + {:numeric-code numeric-code + :name (->> (or (seq (filter :bank-account-name? entries)) + entries) + (map :name) + frequencies + (sort-by (juxt (comp - val) key)) + ffirst)})) (sort-by :numeric-code))) (defn subtotal-by-column-row [pnl-datas title & [cell-args]] @@ -477,11 +498,11 @@ (map (fn [p] (let [pnl-data (-> p (filter-numeric-code numeric-code numeric-code)) - this-name-exists? (->> (:data p) - (filter (comp #{name} :name)) - seq)] + ;; Code, not name: the row is keyed on the code, and a + ;; client can reach it under a name another client won. + this-code-exists? (seq (:data pnl-data))] (merge - (if this-name-exists? + (if this-code-exists? {:format :dollar :filters (:filters pnl-data) :value (aggregate-accounts pnl-data)} diff --git a/test/clj/auto_ap/ledger/reports_test.clj b/test/clj/auto_ap/ledger/reports_test.clj new file mode 100644 index 00000000..7669a99b --- /dev/null +++ b/test/clj/auto_ap/ledger/reports_test.clj @@ -0,0 +1,129 @@ +(ns auto-ap.ledger.reports-test + (:require + [auto-ap.ledger.reports :as sut] + [clojure.test :refer [deftest is testing]])) + +;; A client's bank account and the financial account it posts to share a +;; numeric code. Reports key their rows off that code, so before the fix the +;; pair rendered as two rows carrying the same code-level total, and the +;; section stopped footing to its own subtotal. + +(def ^:private period #inst "2026-08-14") + +(defn- account + [numeric-code name amount] + {:client-id 1 + :location "M" + :numeric-code numeric-code + :name name + :amount amount + :debits 0.0 + :credits amount + :count 1 + :account-type :account-type/liability + :period period}) + +(defn- bank-account + "Same as `account`, but its name came from a :bank-account rather than the + chart of accounts — which is what earns it the row label." + [numeric-code name amount] + (assoc (account numeric-code name amount) :bank-account-name? true)) + +(defn- pnl-data + [data] + (sut/->PNLData {:periods [period]} data {1 "CLIENT"})) + +(defn- labels + [rows] + (map (comp :value first) rows)) + +(deftest used-accounts-collapses-a-shared-numeric-code + (testing "two names on one code yield a single entry, named for the bank account" + (is (= [{:numeric-code 21010 :name "Capital One CC - 3196"}] + (sut/used-accounts + [{:data [(bank-account 21010 "Capital One CC - 3196" -100.0) + (account 21010 "Accounts Payable 10" -150.0)]}])))) + + (testing "the bank account wins even when the financial name is more common" + (is (= [{:numeric-code 21010 :name "Capital One CC - 3196"}] + (sut/used-accounts + [{:data [(bank-account 21010 "Capital One CC - 3196" -100.0)]} + {:data [(account 21010 "Accounts Payable 10" -150.0)]} + {:data [(account 21010 "Accounts Payable 10" -150.0)]}])))) + + (testing "distinct codes are left alone, ordered by code" + (is (= [{:numeric-code 21009 :name "Due to Grand Ventures"} + {:numeric-code 21010 :name "Capital One CC - 3196"}] + (sut/used-accounts + [{:data [(bank-account 21010 "Capital One CC - 3196" -100.0) + (account 21009 "Due to Grand Ventures" 25.0)]}])))) + + (testing "with no bank account in play the most common name wins" + (is (= [{:numeric-code 21010 :name "Accounts Payable 10"}] + (sut/used-accounts + [{:data [(account 21010 "Due to Sandwich Monkey" -100.0)]} + {:data [(account 21010 "Accounts Payable 10" -150.0)]} + {:data [(account 21010 "Accounts Payable 10" -150.0)]}])))) + + (testing "an even split breaks alphabetically so runs are reproducible" + (is (= [{:numeric-code 21010 :name "Accounts Payable 10"}] + (sut/used-accounts + [{:data [(account 21010 "Due to Sandwich Monkey" -100.0)]} + {:data [(account 21010 "Accounts Payable 10" -150.0)]}]))) + (is (= [{:numeric-code 21010 :name "Amex - 41001"}] + (sut/used-accounts + [{:data [(bank-account 21010 "BofA CC - 8779" -100.0)]} + {:data [(bank-account 21010 "Amex - 41001" -150.0)]}])) + "two bank accounts still resolve to one, deterministically"))) + +(deftest balance-sheet-renders-one-row-per-shared-code + (let [report (sut/summarize-balance-sheet + (pnl-data [(bank-account 21010 "Capital One CC - 3196" -100.0) + (account 21010 "Accounts Payable 10" -150.0) + (account 21009 "Due to Grand Ventures" 25.0)])) + rows (:rows report)] + + (testing "the shared code appears once, under the bank account's name" + (is (= 1 (count (filter #(= "Capital One CC - 3196:21010" %) (labels rows))))) + (is (not (some #{"Accounts Payable 10:21010"} (labels rows))))) + + (testing "its row carries the total for the code, not one account's share" + (is (= -250.0 + (->> rows + (filter #(= "Capital One CC - 3196:21010" (:value (first %)))) + first + second + :value)))) + + (testing "the detail rows foot to the section subtotal" + (let [detail (->> rows + (filter #(re-find #":\d+$" (str (:value (first %))))) + (map (comp :value second))) + subtotal (->> rows + (filter #(= "Liabilities" (:value (first %)))) + (keep (comp :value second)) + last)] + (is (= 2 (count detail)) + "three accounts across two codes collapse to two rows") + (is (= -225.0 (reduce + 0.0 detail) subtotal) + "before the fix 21010 printed twice and the rows overshot the subtotal"))))) + +(deftest balance-sheet-keeps-a-clients-figure-when-another-client-named-the-code + (testing "a column is blank only when that client has nothing at the code" + (let [rows (:rows (sut/summarize-balance-sheet + (sut/->PNLData + {:periods [period]} + [(assoc (bank-account 21010 "Capital One CC - 3196" -100.0) :client-id 1) + (assoc (account 21010 "Accounts Payable 10" -150.0) :client-id 2) + (assoc (account 21009 "Due to Grand Ventures" 25.0) :client-id 1)] + {1 "ONE" 2 "TWO"}))) + shared (->> rows + (filter #(re-find #":21010$" (str (:value (first %))))) + first) + absent (->> rows + (filter #(re-find #":21009$" (str (:value (first %))))) + first)] + (is (= [-100.0 -150.0] (map :value (drop-last (rest shared)))) + "client TWO reaches 21010 under a name client ONE won, and still shows its balance") + (is (= [25.0 ""] (map :value (drop-last (rest absent)))) + "client TWO has nothing at 21009 and stays blank")))) diff --git a/test/clj/auto_ap/ledger_test.clj b/test/clj/auto_ap/ledger_test.clj index 12b0099a..ce978a58 100644 --- a/test/clj/auto_ap/ledger_test.clj +++ b/test/clj/auto_ap/ledger_test.clj @@ -1,10 +1,98 @@ (ns auto-ap.ledger-test (:require - [auto-ap.integration.util :refer [wrap-setup]] - [clojure.test :as t])) + [auto-ap.datomic :refer [conn]] + [auto-ap.integration.util :refer [test-account test-bank-account test-client + wrap-setup]] + [auto-ap.ledger :refer [build-account-lookup]] + [clojure.test :as t] + [datomic.api :as dc])) (t/use-fixtures :each wrap-setup) +(t/deftest build-account-lookup-prefers-the-bank-account-name + (t/testing "a bank account and the financial account sharing its code resolve to one name" + (let [{:strs [client bank financial]} + (:tempids @(dc/transact conn + [(test-account :db/id "financial" + :account/name "Accounts Payable 10" + :account/numeric-code 21010 + :account/type :account-type/liability) + (test-client :db/id "client" + :client/bank-accounts + [(test-bank-account :db/id "bank" + :bank-account/name "Capital One CC - 3196" + :bank-account/numeric-code 21010 + :bank-account/type :bank-account-type/credit)])])) + lookup (build-account-lookup client)] + + (t/is (= "Capital One CC - 3196" (:name (lookup bank)))) + (t/is (= "Capital One CC - 3196" (:name (lookup financial))) + "the financial account takes the bank account's name, so reports render one row") + + (t/testing "and both are flagged as bank-named for downstream report labelling" + (t/is (true? (:bank_account_name? (lookup bank)))) + (t/is (true? (:bank_account_name? (lookup financial))))) + + (t/testing "without changing the code or the account type" + (t/is (= 21010 (:numeric_code (lookup bank)))) + (t/is (= 21010 (:numeric_code (lookup financial)))) + (t/is (= :account-type/liability (:account_type (lookup financial)))))))) + +(t/deftest build-account-lookup-leaves-unshared-codes-alone + (t/testing "an account with no bank account at its code keeps its own name" + (let [{:strs [client financial]} + (:tempids @(dc/transact conn + [(test-account :db/id "financial" + :account/name "Sales Taxes Payable" + :account/numeric-code 23000 + :account/type :account-type/liability) + (test-client :db/id "client" + :client/bank-accounts + [(test-bank-account :db/id "bank" + :bank-account/name "Capital One CC - 3196" + :bank-account/numeric-code 21010 + :bank-account/type :bank-account-type/credit)])])) + lookup (build-account-lookup client)] + (t/is (= "Sales Taxes Payable" (:name (lookup financial)))) + (t/is (false? (:bank_account_name? (lookup financial)))))) + + (t/testing "another client's bank account never renames this client's accounts" + (let [{:strs [mine financial]} + (:tempids @(dc/transact conn + [(test-account :db/id "financial" + :account/name "Accounts Payable 10" + :account/numeric-code 21010 + :account/type :account-type/liability) + (test-client :db/id "mine") + (test-client :db/id "theirs" + :client/bank-accounts + [(test-bank-account :db/id "bank" + :bank-account/name "Capital One CC - 3196" + :bank-account/numeric-code 21010 + :bank-account/type :bank-account-type/credit)])])) + lookup (build-account-lookup mine)] + (t/is (= "Accounts Payable 10" (:name (lookup financial)))) + (t/is (false? (:bank_account_name? (lookup financial))))))) + +(t/deftest build-account-lookup-breaks-ties-between-bank-accounts + (t/testing "two bank accounts on one code resolve deterministically by sort-order" + (let [{:strs [client first-bank second-bank]} + (:tempids @(dc/transact conn + [(test-client :db/id "client" + :client/bank-accounts + [(test-bank-account :db/id "second-bank" + :bank-account/name "US Bank 2974" + :bank-account/numeric-code 13101 + :bank-account/sort-order 3) + (test-bank-account :db/id "first-bank" + :bank-account/name "Fremont Gyro HB Main 8576" + :bank-account/numeric-code 13101 + :bank-account/sort-order 0)])])) + lookup (build-account-lookup client)] + (t/is (= "Fremont Gyro HB Main 8576" + (:name (lookup first-bank)) + (:name (lookup second-bank))))))) + (t/deftest entity-change->ledger #_(t/testing "Should code an expected deposit" (let [{:strs [ed ccp receipts-split client]}