From 436d8d023f5c8f4d05d8633af082afb533816546 Mon Sep 17 00:00:00 2001 From: Jaromil Date: Wed, 20 May 2026 16:29:28 +0200 Subject: [PATCH] feat: render readable server-side timesheet diff --- src/agiladmin/view_timesheet.clj | 206 +++++++++++++++++++++---- src/agiladmin/webpage.clj | 1 - test/agiladmin/view_timesheet_test.clj | 50 +++++- test/e2e/timesheet-upload.spec.js | 2 + 4 files changed, 230 insertions(+), 29 deletions(-) diff --git a/src/agiladmin/view_timesheet.clj b/src/agiladmin/view_timesheet.clj index 8e18040..bd8b00d 100644 --- a/src/agiladmin/view_timesheet.clj +++ b/src/agiladmin/view_timesheet.clj @@ -161,31 +161,181 @@ (concat fields [[:input {:type "submit" :value label :class class-name}]])))) -(defn textual-diff [left right] - [:div {:class "grid gap-4 lg:grid-cols-2"} - [:div {:class "rounded-box border border-base-300 bg-base-100 p-4 shadow-sm"} - [:pre (str "\n" (with-out-str (print left)))]] - [:div {:class "rounded-box border border-base-300 bg-base-100 p-4 shadow-sm"} - [:pre {:id "display"}] - [:script - (str "\n" - "function dodiff() {\n" - "var left = `" (with-out-str (print left)) "`;\n" - "var right = `" (with-out-str (print right)) "`;\n" - "var color = '', span = null;\n" - "var diff = JsDiff.diffLines(left, right);\n" - "var display = document.getElementById('display')\n" - "var fragment = document.createDocumentFragment();\n" - "diff.forEach(function(part){\n - color = part.added ? 'green' : part.removed ? 'red' : 'darkgrey';\n - span = document.createElement('span');\n - span.style.color = color;\n - span.appendChild(document.createTextNode(part.value));\n - fragment.appendChild(span);\n -});\n -display.appendChild(fragment);\n -}\n -window.onload = dodiff;\n")]]]) +(def ^:private diff-row-key-cols [:month :project :task :tag]) + +(defn- diff-row-key + [row] + (zipmap diff-row-key-cols (mapv #(get row %) diff-row-key-cols))) + +(defn- index-diff-rows + [rows] + (reduce (fn [idx row] + (assoc idx (diff-row-key row) row)) + {} + rows)) + +(defn- parse-number + [value] + (cond + (number? value) (double value) + (string? value) (try + (Double/parseDouble value) + (catch Exception _ nil)) + :else nil)) + +(defn- compare-hours-row + [old-row new-row] + (let [old-hours (get old-row :hours) + new-hours (get new-row :hours) + old-num (parse-number old-hours) + new-num (parse-number new-hours)] + (cond + (and old-row new-row (= old-hours new-hours)) + {:status :unchanged + :old old-row + :new new-row + :old-hours old-hours + :new-hours new-hours + :delta 0.0} + + (and old-row new-row) + {:status :changed + :old old-row + :new new-row + :old-hours old-hours + :new-hours new-hours + :delta (when (and (some? old-num) (some? new-num)) + (- new-num old-num))} + + new-row + {:status :added + :old nil + :new new-row + :old-hours nil + :new-hours (get new-row :hours) + :delta new-num} + + :else + {:status :removed + :old old-row + :new nil + :old-hours (get old-row :hours) + :new-hours nil + :delta (when (some? old-num) + (- old-num))}))) + +(defn- status-label + [status] + (case status + :added "Added" + :removed "Removed" + :changed "Changed" + :unchanged "Unchanged" + "Unknown")) + +(defn- status-badge-class + [status] + (case status + :added "badge badge-success" + :removed "badge badge-error" + :changed "badge badge-warning" + :unchanged "badge badge-neutral" + "badge")) + +(defn- status-row-class + [status] + (case status + :added "bg-success/10" + :removed "bg-error/10" + :changed "bg-warning/10" + :unchanged "opacity-70" + "")) + +(defn- format-hours + [value] + (if (nil? value) + "-" + (str value))) + +(defn- format-delta + [value] + (cond + (nil? value) "-" + (pos? value) (format "+%.2f" value) + :else (format "%.2f" value))) + +(defn- timesheet-diff-model + [old-hours new-hours] + (let [old-rows (tab/rows old-hours) + new-rows (tab/rows new-hours) + old-index (index-diff-rows old-rows) + new-index (index-diff-rows new-rows) + keys-in-order (->> (concat (keys old-index) (keys new-index)) + distinct + (sort-by #(mapv (fn [col] (str (get % col ""))) + diff-row-key-cols))) + rows (mapv (fn [k] + (assoc (compare-hours-row (get old-index k) (get new-index k)) + :key k)) + keys-in-order) + status-counts (merge {:added 0 :removed 0 :changed 0 :unchanged 0} + (frequencies (map :status rows))) + total-delta (reduce (fn [acc row] + (+ acc (double (or (:delta row) 0.0)))) + 0.0 + rows)] + {:rows rows + :summary (assoc status-counts :total-delta total-delta)})) + +(defn- summary-card + [title value class-name] + [:div {:class (str "rounded-box border border-base-300 bg-base-100 p-3 shadow-sm " class-name)} + [:div {:class "text-xs uppercase tracking-wide text-base-content/60"} title] + [:div {:class "text-xl font-semibold"} value]]) + +(defn- timesheet-diff + [old-hours new-hours] + (let [{:keys [rows summary]} (timesheet-diff-model old-hours new-hours) + rows-to-show (filterv #(not= :unchanged (:status %)) rows) + has-visible-rows (seq rows-to-show)] + [:div {:class "space-y-4"} + [:div {:class "grid gap-3 sm:grid-cols-2 xl:grid-cols-5"} + (summary-card "Added" (:added summary) "bg-success/10") + (summary-card "Removed" (:removed summary) "bg-error/10") + (summary-card "Changed" (:changed summary) "bg-warning/10") + (summary-card "Unchanged" (:unchanged summary) "bg-base-200/40") + (summary-card "Total hour delta" (format-delta (:total-delta summary)) "bg-info/10")] + [:div {:class "flex flex-wrap gap-3 text-sm"} + [:span {:class "badge badge-success"} "Added"] + [:span {:class "badge badge-error"} "Removed"] + [:span {:class "badge badge-warning"} "Changed"] + [:span {:class "badge badge-neutral"} "Unchanged"]] + (if has-visible-rows + [:div {:class "overflow-x-auto"} + [:table {:class "table table-zebra w-full"} + [:thead + [:tr + [:th "Status"] + [:th "Month"] + [:th "Project"] + [:th "Task"] + [:th "Tag"] + [:th {:class "text-right"} "Old hours"] + [:th {:class "text-right"} "New hours"] + [:th {:class "text-right"} "Delta"]]] + [:tbody + (for [{:keys [status key old-hours new-hours delta]} rows-to-show] + [:tr {:class (status-row-class status)} + [:td [:span {:class (status-badge-class status)} (status-label status)]] + [:td (or (:month key) "-")] + [:td (or (:project key) "-")] + [:td (or (:task key) "-")] + [:td (or (:tag key) "-")] + [:td {:class "text-right"} (format-hours old-hours)] + [:td {:class "text-right"} (format-hours new-hours)] + [:td {:class "text-right font-medium"} (format-delta delta)]])]]] + [:div {:class "alert alert-info shadow-sm" :role "alert"} + "No differences found between the archived timesheet and this upload."])])) (defn upload-form [config] @@ -271,7 +421,9 @@ window.onload = dodiff;\n")]]]) [{:id "diff" :title "Differences" :content [:div {:class "space-y-4"} - [:h2 {:class "text-2xl font-semibold"} "Differences: old (to the left) and new (to the right)"] + [:h2 {:class "text-2xl font-semibold"} "Timesheet changes"] + [:p {:class "text-base-content/70"} + "Compare the archived timesheet with the uploaded file before submitting."] (if (.exists (io/file (str (conf/q config [:agiladmin :budgets :path]) @@ -282,7 +434,7 @@ window.onload = dodiff;\n")]]]) (str (conf/q config [:agiladmin :budgets :path]) (fs/base-name filename))) old-hours (map-timesheets [old-ts])] - (textual-diff old-hours hours) + (timesheet-diff old-hours hours) (f/when-failed [e] (web/render-error (log/spy :error ["Error parsing old timesheet: " e])))) diff --git a/src/agiladmin/webpage.clj b/src/agiladmin/webpage.clj index 1b20123..9ee1cdd 100644 --- a/src/agiladmin/webpage.clj +++ b/src/agiladmin/webpage.clj @@ -429,7 +429,6 @@ "?v=" version/current)) (page/include-js (asset-path config "/static/js/highlight.pack.js")) - (page/include-js (asset-path config "/static/js/diff.js")) (page/include-js (asset-path config "/static/js/jsondiffpatch.min.js")) (page/include-js (asset-path config "/static/js/jsondiffpatch-formatters.min.js")) (page/include-js (asset-path config "/static/js/diff_match_patch_uncompressed.js")) diff --git a/test/agiladmin/view_timesheet_test.clj b/test/agiladmin/view_timesheet_test.clj index c97421e..628dbe3 100644 --- a/test/agiladmin/view_timesheet_test.clj +++ b/test/agiladmin/view_timesheet_test.clj @@ -1,5 +1,6 @@ (ns agiladmin.view-timesheet-test (:require [agiladmin.view-timesheet :as view-timesheet] + [agiladmin.tabular :as tab] [clojure.java.io :as io] [hiccup.core :as hiccup] [failjure.core] @@ -23,6 +24,53 @@ html => (contains "action=\"/admin/timesheets/upload\"") html => (contains "hx-post=\"/admin/timesheets/upload\""))) +(fact "Timesheet diff model tracks added removed changed and unchanged rows" + (let [old-hours (tab/dataset + [{:month "2026-1" :project "ALPHA" :task "A" :tag "" :hours 10} + {:month "2026-2" :project "ALPHA" :task "B" :tag "" :hours 5} + {:month "2026-3" :project "BETA" :task "C" :tag "VOL" :hours 4}]) + new-hours (tab/dataset + [{:month "2026-1" :project "ALPHA" :task "A" :tag "" :hours 12} + {:month "2026-3" :project "BETA" :task "C" :tag "VOL" :hours 4} + {:month "2026-4" :project "GAMMA" :task "D" :tag "" :hours 8}]) + model (#'agiladmin.view-timesheet/timesheet-diff-model old-hours new-hours)] + (get-in model [:summary :changed]) => 1 + (get-in model [:summary :unchanged]) => 1 + (get-in model [:summary :removed]) => 1 + (get-in model [:summary :added]) => 1 + (get-in model [:summary :total-delta]) => 5.0 + (->> (:rows model) + (map (juxt :status :old-hours :new-hours)) + vec) + => [[:changed 10 12] + [:removed 5 nil] + [:unchanged 4 4] + [:added nil 8]])) + +(fact "Timesheet diff render shows summary and side-by-side columns" + (let [old-hours (tab/dataset + [{:month "2026-1" :project "ALPHA" :task "A" :tag "" :hours 10} + {:month "2026-2" :project "ALPHA" :task "B" :tag "" :hours 5}]) + new-hours (tab/dataset + [{:month "2026-1" :project "ALPHA" :task "A" :tag "" :hours 12} + {:month "2026-3" :project "GAMMA" :task "C" :tag "" :hours 8}]) + html (hiccup/html (#'agiladmin.view-timesheet/timesheet-diff old-hours new-hours))] + html => (contains "Added") + html => (contains "Removed") + html => (contains "Changed") + html => (contains "Old hours") + html => (contains "New hours") + html => (contains "Delta") + html => (contains "2026-1") + html => (contains "2026-2") + html => (contains "2026-3"))) + +(fact "Timesheet diff render shows an explicit message when there are no differences" + (let [hours (tab/dataset + [{:month "2026-1" :project "ALPHA" :task "A" :tag "" :hours 10}]) + html (hiccup/html (#'agiladmin.view-timesheet/timesheet-diff hours hours))] + html => (contains "No differences found between the archived timesheet and this upload."))) + (fact "Timesheet upload rejects files above the default size limit" (let [response (view-timesheet/upload {:params {:file {:size 500001 @@ -238,7 +286,7 @@ :role "admin"})] (:body response) => (contains "Uploaded: 2016_timesheet_Luca-Pacioli.xlsx") (:body response) => (contains "Contents of the new timesheet") - (:body response) => (contains "Differences: old (to the left) and new (to the right)") + (:body response) => (contains "Timesheet changes") (:body response) => (contains "This is a new timesheet, no historical information available to compare") (:body response) =not=> (contains "Error parsing timesheet")) (finally diff --git a/test/e2e/timesheet-upload.spec.js b/test/e2e/timesheet-upload.spec.js index 272150e..0e186f3 100644 --- a/test/e2e/timesheet-upload.spec.js +++ b/test/e2e/timesheet-upload.spec.js @@ -19,6 +19,8 @@ test("admin can login and upload a real timesheet", async ({ page }) => { await uploadTimesheet(page, state.fixtures.admin); await expect(page.getByText("Uploaded: 2016_timesheet_Luca-Pacioli.xlsx")).toBeVisible(); + await expect(page.getByRole("heading", { name: "Timesheet changes" })).toBeVisible(); + await expect(page.getByText("Differences: old (to the left) and new (to the right)")).toHaveCount(0); const uploadedTempPath = await page.locator('input[name="tempfile"]').inputValue(); await expect(uploadedTempPath).toBeTruthy(); await expect(sha256File(uploadedTempPath)).resolves.toBe(await sha256File(state.fixtures.admin));