Merge pull request 'fix(reports): collapse accounts sharing a numeric code to one row' (#16) from integreat-fix-report into staging

Reviewed-on: #16

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

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)))

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))

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))}}))
 

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)}

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"))))

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]}