Merge pull request #6765 from logseq/fix/sort-by-filter

Fix sort-by filter for simple queries

src/main/frontend/components/query_table.cljs

@@ -2,6 +2,7 @@
   (:require [frontend.components.svg :as svg]
             [frontend.date :as date]
             [frontend.db :as db]
+            [frontend.db.query-dsl :as query-dsl]
             [frontend.handler.common :as common-handler]
             [frontend.handler.editor :as editor-handler]
             [frontend.state :as state]
@@ -13,38 +14,9 @@
             [rum.core :as rum]
             [frontend.modules.outliner.tree :as tree]))
 
-;; TODO: extract to table utils
-(defn- sort-result-by
-  [by-item desc? result]
-  (let [comp (if desc? > <)]
-    (sort-by by-item comp result)))
-
-(rum/defc sortable-title
-  [title key by-item desc? block-id]
-  [:th.whitespace-nowrap
-   [:a {:on-click (fn []
-                    (reset! by-item key)
-                    (swap! desc? not)
-                    (when block-id
-                      (when key
-                        (editor-handler/set-block-property! block-id :query-sort-by (name key)))
-                      (editor-handler/set-block-property! block-id :query-sort-desc @desc?)))}
-    [:div.flex.items-center
-     [:span.mr-1 title]
-     (when (= @by-item key)
-       [:span
-        (if @desc? (svg/caret-down) (svg/caret-up))])]]])
-
-(defn get-keys
-  [result page?]
-  (let [keys (->> (distinct (mapcat keys (map :block/properties result)))
-                  (remove (property/built-in-properties))
-                  (remove #{:template}))
-        keys (if page? (cons :page keys) (cons :block keys))
-        keys (if page? (distinct (concat keys [:created-at :updated-at])) keys)]
-    keys))
-
-(defn attach-clock-property
+;; Util fns
+;; ========
+(defn- attach-clock-property
   [result]
   (let [ks [:block/properties :clock-time]
         result (map (fn [b]
@@ -55,8 +27,8 @@
       (map #(medley/dissoc-in % ks) result)
       result)))
 
-(defn- sort-by-fn [sort-by-item item]
-  (case sort-by-item
+(defn- sort-by-fn [sort-by-column item]
+  (case sort-by-column
     :created-at
     (:block/created-at item)
     :updated-at
@@ -65,72 +37,106 @@
     (:block/content item)
     :page
     (:block/name item)
-    (get-in item [:block/properties sort-by-item])))
+    (get-in item [:block/properties sort-by-column])))
+
+(defn- sort-result [result {:keys [sort-by-column sort-desc?]}]
+  (if (some? sort-by-column)
+    (let [comp (if sort-desc? > <)]
+      (sort-by (fn [item]
+                 (block/normalize-block (sort-by-fn sort-by-column item)))
+               comp
+               result))
+    result))
+
+(defn- get-sort-state
+  "Return current sort direction and column being sorted, respectively
+  :sort-desc? and :sort-by-column. :sort-by-column is nil if no sorting is to be
+  done"
+  [current-block]
+  (let [p-desc? (get-in current-block [:block/properties :query-sort-desc])
+        desc? (if (some? p-desc?) p-desc? true)
+        p-sort-by (keyword (get-in current-block [:block/properties :query-sort-by]))
+        sort-by-column (or (some-> p-sort-by keyword)
+                         (if (query-dsl/query-contains-filter? (:block/content current-block) "sort-by")
+                           nil
+                           :updated-at))]
+    {:sort-desc? desc?
+     :sort-by-column sort-by-column}))
+
+;; Components
+;; ==========
+(rum/defc sortable-title
+  [title column {:keys [sort-by-column sort-desc?]} block-id]
+  [:th.whitespace-nowrap
+   [:a {:on-click (fn []
+                    (editor-handler/set-block-property! block-id :query-sort-by (name column))
+                    (editor-handler/set-block-property! block-id :query-sort-desc (not sort-desc?)))}
+    [:div.flex.items-center
+     [:span.mr-1 title]
+     (when (= sort-by-column column)
+       [:span
+        (if sort-desc? (svg/caret-down) (svg/caret-up))])]]])
+
+(defn get-keys
+  "Get keys for a query table result, which are the columns in a table"
+  [result page?]
+  (let [keys (->> (distinct (mapcat keys (map :block/properties result)))
+                  (remove (property/built-in-properties))
+                  (remove #{:template}))
+        keys (if page? (cons :page keys) (cons :block keys))
+        keys (if page? (distinct (concat keys [:created-at :updated-at])) keys)]
+    keys))
 
-(defn- desc?
-  [*desc? p-desc?]
-  (cond
-    (some? @*desc?)
-    @*desc?
-    (some? p-desc?)
-    p-desc?
-    :else
-    true))
+(defn- get-columns [current-block result {:keys [page?]}]
+  (let [query-properties (some-> (get-in current-block [:block/properties :query-properties] "")
+                                 (common-handler/safe-read-string "Parsing query properties failed"))
+        columns (if (seq query-properties)
+                  query-properties
+                  (get-keys result page?))
+        included-columns #{:created-at :updated-at}]
+    (distinct
+     (if (some included-columns columns)
+       (concat (remove included-columns columns)
+               (filter included-columns columns)
+               included-columns)
+       columns))))
 
+;; Table rows are called items
 (rum/defcs result-table < rum/reactive
-  (rum/local nil ::sort-by-item)
-  (rum/local nil ::desc?)
   (rum/local false ::select?)
   [state config current-block result {:keys [page?]} map-inline page-cp ->elem inline-text]
   (when current-block
     (let [result (tree/filter-top-level-blocks result)
-          p-sort-by (keyword (get-in current-block [:block/properties :query-sort-by]))
-          p-desc? (get-in current-block [:block/properties :query-sort-desc])
           select? (get state ::select?)
-          *sort-by-item (get state ::sort-by-item)
-          *desc? (get state ::desc?)
-          sort-by-item (or @*sort-by-item (some-> p-sort-by keyword) :updated-at)
           ;; remove templates
           result (remove (fn [b] (some? (get-in b [:block/properties :template]))) result)
           result (if page? result (attach-clock-property result))
           clock-time-total (when-not page?
                              (->> (map #(get-in % [:block/properties :clock-time] 0) result)
                                   (apply +)))
-          query-properties (some-> (get-in current-block [:block/properties :query-properties] "")
-                                   (common-handler/safe-read-string "Parsing query properties failed"))
-          keys (if (seq query-properties)
-                 query-properties
-                 (get-keys result page?))
-          included-keys #{:created-at :updated-at}
-          keys (distinct
-                (if (some included-keys keys)
-                  (concat (remove included-keys keys)
-                          (filter included-keys keys)
-                          included-keys)
-                  keys))
-          desc? (desc? *desc? p-desc?)
-          result (sort-result-by (fn [item]
-                                   (block/normalize-block (sort-by-fn sort-by-item item)))
-                                 desc?
-                                 result)]
+          columns (get-columns current-block result {:page? page?})
+          ;; Sort state needs to be in sync between final result and sortable title
+          ;; as user needs to know if there result is sorted
+          sort-state (get-sort-state current-block)
+          result' (sort-result result sort-state)]
       [:div.overflow-x-auto {:on-mouse-down (fn [e] (.stopPropagation e))
                              :style {:width "100%"}
                              :class (when-not page? "query-table")}
        [:table.table-auto
         [:thead
          [:tr.cursor
-          (for [key keys]
-            (let [key-name (if (and (= key :clock-time) (integer? clock-time-total))
+          (for [column columns]
+            (let [title (if (and (= column :clock-time) (integer? clock-time-total))
                              (util/format "clock-time(total: %s)" (clock/minutes->days:hours:minutes
                                                                    clock-time-total))
-                             (name key))]
-              (sortable-title key-name key *sort-by-item *desc? (:block/uuid current-block))))]]
+                             (name column))]
+              (sortable-title title column sort-state (:block/uuid current-block))))]]
         [:tbody
-         (for [item result]
+         (for [item result']
            (let [format (:block/format item)]
              [:tr.cursor
-              (for [key keys]
-                (let [value (case key
+              (for [column columns]
+                (let [value (case column
                               :page
                               [:string (or (:block/original-name item)
                                            (:block/name item))]
@@ -154,7 +160,7 @@
                               [:string (when-let [updated-at (:block/updated-at item)]
                                          (date/int->local-time-2 updated-at))]
 
-                              [:string (get-in item [:block/properties key])])]
+                              [:string (get-in item [:block/properties column])])]
                   [:td.whitespace-nowrap {:on-mouse-down (fn [] (reset! select? false))
                                           :on-mouse-move (fn [] (reset! select? true))
                                           :on-mouse-up (fn []

src/main/frontend/db/query_dsl.cljs

@@ -38,11 +38,7 @@
 ;; project (block, TBD)
 
 ;; Sort by (field, asc/desc):
-
-;; created_at
-;; last_modified_at
-
-;; (sort-by last_modified_at asc)
+;; (sort-by created-at asc)
 
 ;; (between -7d +7d)
 
@@ -151,13 +147,16 @@
 
       :else
       (->> clauses
-           (map (fn [result]
-                  (if (list? result)
-                    result
-                    (let [result (if (vector? (ffirst result))
-                                   (apply concat result)
-                                   result)]
-                      (cons 'and (seq result))))))
+           (mapcat (fn [result]
+                     (cond
+                       ;; rule like (task ?b #{"NOW"})
+                       (list? result)
+                       [result]
+                       ;; datalog clause like [[?b :block/uuid]]
+                       (vector? result)
+                       result
+                       :else
+                       [(cons 'and (seq result))])))
            (apply list fe)))
 
     :else
@@ -320,33 +319,24 @@
   (when-let [num (second e)]
     (when (integer? num)
       (reset! sample num)
-      {:query [['?p :block/uuid]]})))
+      ;; blank b/c this post-process filter doesn't effect query
+      {})))
 
 (defn- build-sort-by
   [e sort-by_]
-  (let [[k order] (rest e)
-             order (if (and order (contains? #{:asc :desc}
-                                             (keyword (string/lower-case (name order)))))
-                     (keyword (string/lower-case (name order)))
-                     :desc)
-             k (-> (string/lower-case (name k))
-                   (string/replace "_" "-"))
-             get-value (cond
-                         (= k "created-at")
-                         :block/created-at
-
-                         (= k "updated-at")
-                         :block/updated-at
-
-                         :else
-                         #(get-in % [:block/properties k]))
-             comp (if (= order :desc) >= <=)]
-         (reset! sort-by_
-                 (fn [result]
-                   (->> result
-                        flatten
-                        (sort-by get-value comp))))
-         nil))
+  (let [[k order*] (map keyword (rest e))
+        order (if (contains? #{:asc :desc} order*)
+                order*
+                :desc)
+        comp (if (= order :desc) >= <=)]
+    (reset! sort-by_
+            (fn sort-results [result]
+              ;; first because there is one binding result in query-wrapper
+              (sort-by #(-> % first (get-in [:block/properties k]))
+                       comp
+                       result)))
+    ;; blank b/c this post-process filter doesn't effect query
+    {}))
 
 (defn- build-page
   [e]
@@ -392,7 +382,7 @@ Some bindings in this fn:
          page-ref? (page-ref/page-ref? e)]
      (when (or (and page-ref?
                     (not (contains? #{'page-property 'page-tags} (:current-filter env))))
-               (contains? #{'between 'property 'todo 'task 'priority 'sort-by 'page} fe)
+               (contains? #{'between 'property 'todo 'task 'priority 'page} fe)
                (and (not page-ref?) (string? e)))
        (reset! blocks? true))
      (cond
@@ -587,6 +577,10 @@ Some bindings in this fn:
                             (when sort-by
                               {:transform-fn sort-by})))))))
 
+(defn query-contains-filter?
+  [query filter-name]
+  (string/includes? query (str "(" filter-name)))
+
 (comment
   ;; {{query (and (page-property foo bar) [[hello]])}}
 

src/test/frontend/db/query_dsl_test.cljs

@@ -235,10 +235,10 @@ prop-d:: nada"}])
 
   (is (= 1
          (count (dsl-query "(and (task todo) (sample 1))")))
-      "Correctly limits results")
-  (is (= 2
-         (count (dsl-query "(and (task todo) (sample blarg))")))
-      "Non-integer arg is ignored"))
+      "Correctly limits block results")
+  (is (= 1
+         (count (dsl-query "(and (page-property foo) (sample 1))")))
+      "Correctly limits page results"))
 
 (deftest priority-queries
   (load-test-files [{:file/path "pages/page1.md"
@@ -447,76 +447,30 @@ tags: other
          (map :block/content
               (dsl-query "(and [[Parent page]] (not [[Child page]]))")))))
 
-(defn- load-test-files-with-timestamps
-  []
-  (let [files [{:file/path "journals/2020_12_26.md"
-                :file/content "---
-title: Dec 26th, 2020
----
-- DONE 26-b1
+(deftest between-queries
+  (load-test-files [{:file/path "journals/2020_12_26.md"
+                     :file/content "- DONE 26-b1
 created-at:: 1608968448113
-last-modified-at:: 1608968448113
 - LATER 26-b2-modified-later
 created-at:: 1608968448114
-last-modified-at:: 1608968448120
 - DONE 26-b3
 created-at:: 1608968448115
-last-modified-at:: 1608968448115
-"}
-               {:file/path "journals/2020_12_27.md"
-                :file/content "---
-title: Dec 27th, 2020
----
-- NOW b1
-created-at:: 1609052958714
-last-modified-at:: 1609052958714
-- LATER b2-modified-later
-created-at:: 1609052959376
-last-modified-at:: 1609052974285
-- b3
-created-at:: 1609052959954
-last-modified-at:: 1609052959954
-- b4
-created-at:: 1609052961569
-last-modified-at:: 1609052961569
-- b5
-created-at:: 1609052963089
-last-modified-at:: 1609052963089"}
-               {:file/path "journals/2020_12_28.md"
-                :file/content "---
-title: Dec 28th, 2020
----
-- 28-b1
-created-at:: 1609084800000
-last-modified-at:: 1609084800000
-- 28-b2-modified-later
-created-at:: 1609084800001
-last-modified-at:: 1609084800020
-- 28-b3
-created-at:: 1609084800002
-last-modified-at:: 1609084800002"}]]
-    (load-test-files files)))
-
-(deftest between-queries
-  (load-test-files-with-timestamps)
+- 26-b4
+created-at:: 1608968448116
+"}])
 
   (are [x y] (= (count (dsl-query x)) y)
        "(and (task now later done) (between [[Dec 26th, 2020]] tomorrow))"
-       5
+       3
 
        ;; between with journal pages
-       "(and (task now later done) (between [[Dec 27th, 2020]] [[Dec 28th, 2020]]))"
-       2
+       "(and (task now later done) (between [[Dec 26th, 2020]] [[Dec 27th, 2020]]))"
+       3
 
        ;; ;; between with created-at
        ;; "(and (task now later done) (between created-at [[Dec 26th, 2020]] tomorrow))"
-       ;; 5
-
-       ;; ;; between with last-modified-at
-       ;; "(and (task now later done) (between last-modified-at [[Dec 26th, 2020]] tomorrow))"
-       ;; 5
-       )
-  )
+       ;; 3
+       ))
 
 (deftest custom-query-test
   (load-test-files [{:file/path "pages/page1.md"
@@ -533,63 +487,63 @@ last-modified-at:: 1609084800002"}]]
          (map :block/content (custom-query {:query (list 'and '(task later) "b")})))
       "Query with rule that can't be derived from the form itself"))
 
-#_(deftest sort-by-queries
-    (load-test-files-with-timestamps)
-    ;; (testing "sort-by (created-at defaults to desc)"
-    ;;   (db/clear-query-state!)
-    ;;   (let [result (->> (dsl-query "(and (task now later done)
-    ;;                              (sort-by created-at))")
-    ;;                     (map #(get-in % [:block/properties "created-at"])))]
-    ;;     (is (= result
-    ;;            '(1609052959376 1609052958714 1608968448115 1608968448114 1608968448113)))))
-
-    ;; (testing "sort-by (created-at desc)"
-    ;;   (db/clear-query-state!)
-    ;;   (let [result (->> (dsl-query "(and (todo now later done)
-    ;;                              (sort-by created-at desc))")
-    ;;                     (map #(get-in % [:block/properties "created-at"])))]
-    ;;     (is (= result
-    ;;            '(1609052959376 1609052958714 1608968448115 1608968448114 1608968448113)))))
-
-    ;; (testing "sort-by (created-at asc)"
-    ;;   (db/clear-query-state!)
-    ;;   (let [result (->> (dsl-query "(and (todo now later done)
-    ;;                              (sort-by created-at asc))")
-    ;;                     (map #(get-in % [:block/properties "created-at"])))]
-    ;;     (is (= result
-    ;;            '(1608968448113 1608968448114 1608968448115 1609052958714 1609052959376)))))
-
-    ;; (testing "sort-by (last-modified-at defaults to desc)"
-    ;;   (db/clear-query-state!)
-    ;;   (let [result (->> (dsl-query "(and (todo now later done)
-    ;;                              (sort-by last-modified-at))")
-    ;;                     (map #(get-in % [:block/properties "last-modified-at"])))]
-    ;;     (is (= result
-    ;;            '(1609052974285 1609052958714 1608968448120 1608968448115 1608968448113)))))
-
-    ;; (testing "sort-by (last-modified-at desc)"
-    ;;   (db/clear-query-state!)
-    ;;   (let [result (->> (dsl-query "(and (todo now later done)
-    ;;                              (sort-by last-modified-at desc))")
-    ;;                     (map #(get-in % [:block/properties "last-modified-at"])))]
-    ;;     (is (= result
-    ;;            '(1609052974285 1609052958714 1608968448120 1608968448115 1608968448113)))))
-
-    ;; (testing "sort-by (last-modified-at desc)"
-    ;;   (db/clear-query-state!)
-    ;;   (let [result (->> (dsl-query "(and (todo now later done)
-    ;;                              (sort-by last-modified-at asc))")
-    ;;                     (map #(get-in % [:block/properties "last-modified-at"])))]
-    ;;     (is (= result
-    ;;            '(1608968448113 1608968448115 1608968448120 1609052958714 1609052974285)))))
-    )
-
-#_(cljs.test/run-tests)
+(deftest sort-by-queries
+  (load-test-files [{:file/path "journals/2020_02_25.md"
+                     :file/content "rating:: 10"}
+                    {:file/path "journals/2020_12_26.md"
+                     :file/content "rating:: 8
+- DONE 26-b1
+created-at:: 1608968448113
+fruit:: plum
+- LATER 26-b2-modified-later
+created-at:: 1608968448114
+fruit:: apple
+- DONE 26-b3 has no fruit to test sorting of absent property value
+created-at:: 1608968448115
+- 26-b4
+created-at:: 1608968448116
+"}])
+
+  (testing "sort-by user block property fruit"
+    (let [result (->> (dsl-query "(and (task now later done) (sort-by fruit))")
+                      (map #(get-in % [:block/properties :fruit])))]
+      (is (= ["plum" "apple" nil]
+             result)
+          "sort-by correctly defaults to desc"))
+
+    (let [result (->> (dsl-query "(and (task now later done) (sort-by fruit desc))")
+                      (map #(get-in % [:block/properties :fruit])))]
+      (is (= ["plum" "apple" nil]
+             result)
+          "sort-by desc"))
+
+    (let [result (->> (dsl-query "(and (task now later done) (sort-by fruit asc))")
+                      (map #(get-in % [:block/properties :fruit])))]
+      (is (= ["apple" "plum" nil]
+             result)
+          "sort-by asc")))
+
+  (testing "sort-by hidden, built-in block property created-at"
+    (let [result (->> (dsl-query "(and (task now later done) (sort-by created-at desc))")
+                      (map #(get-in % [:block/properties :created-at])))]
+      (is (= [1608968448115 1608968448114 1608968448113]
+             result))
+      "sorted-by desc")
+
+    (let [result (->> (dsl-query "(and (todo now later done) (sort-by created-at asc))")
+                      (map #(get-in % [:block/properties :created-at])))]
+      (is (= [1608968448113 1608968448114 1608968448115]
+             result)
+          "sorted-by asc")))
+
+  (testing "user page property rating"
+    (is (= [10 8]
+           (->> (dsl-query "(and (page-property rating) (sort-by rating))")
+                (map #(get-in % [:block/properties :rating])))))))
 
 (comment
  (require '[clojure.pprint :as pprint])
  (test-helper/start-test-db!)
- (load-test-files-with-timestamps)
 
  (query-dsl/query test-db "(task done)")