Don't compute only on non-zero scores (#16598)

* Don't compute only on non-zero scores

I was removing a helper function when refactoring the scoring. That
had a filter on :score?. I'm not sure why i translated this to only
interpret the denominator of scoring to only include scores that were
positive. This is obviously wrong.

Also, previously, all databases would be returned in the archived
query which seems obviously wrong. Databases cannot be archived and so
should never appear when searching for archived items.

* Add some tests

- had been asserting that ee-score = os-score if not in an official
collection. This is actually hard to do under the current scheme, and
actually not important. The important bit is that the relative
ordering is the same. It had been adding 0 score 2 weight previously,
and removing all scores with a score of 0. This created a bad way to
score, as something with 1/2, 0/3, 0/4, 0/10 (net score 1/2=0.5) would
appear higher than something with 1/2, 1/3, 1/4, 3/10 (net score
6/19=0.315) and it really shouldn't. The former really should
be (1/19=0.05). And since we are adding in a denominator of
2 (official collection weight) all of the scores are actually smaller,
but that's ok, as they are all proportionately smaller.

enterprise/backend/test/metabase_enterprise/search/scoring_test.clj

 (ns metabase-enterprise.search.scoring-test
   (:require [clojure.test :refer :all]
+            [java-time :as t]
             [metabase-enterprise.search.scoring :as ee-scoring]
             [metabase.search.scoring :as scoring]))
 
 (deftest official-collection-tests
   (testing "it should bump up the value of items in official collections"
     ;; using the ee implementation that isn't wrapped by enable-enhancements? check
-    (let [score          (comp :score (partial scoring/score-and-result ee-scoring/scoring-impl ""))
-          items          [{:name                "needle"
-                           :dashboardcard_count 0
-                           :model               "card"}
-                          {:name  "foo"
-                           :model "dashboard"}
-                          {:name  "foo2"
-                           :model "pulse"}]]
-      (doseq [item items]
-        (is (> (score (assoc item :collection_authority_level "official")) (score item))
+    (let [search-string     "custom expression examples"
+          ee-score          (comp :score (partial scoring/score-and-result ee-scoring/scoring-impl search-string))
+          os-score          (comp :score (partial scoring/score-and-result scoring/oss-score-impl search-string))
+          labeled-results   {:a {:name "custom expression examples" :model "dashboard"}
+                             :b {:name "examples of custom expressions" :model "dashboard"}
+                             :c {:name                "customer success stories"
+                                 :dashboardcard_count 50
+                                 :updated_at          (t/offset-date-time)
+                                 :collection_position 1
+                                 :model               "dashboard"}
+                             :d {:name "customer examples of bad sorting" :model "dashboard"}}
+          {:keys [a b c d]} labeled-results]
+      (doseq [item [a b c d]]
+        (is (> (ee-score (assoc item :collection_authority_level "official")) (ee-score item))
             (str "Item not greater for model: " (:model item))))
-      (doseq [item items]
-        (is (= (score item)
-               ((comp :score (partial scoring/score-and-result scoring/oss-score-impl ""))
-                item)))))))
+      (let [items (shuffle [a b c d])]
+        (is (= (sort-by os-score items)
+               ;; assert that the ordering remains the same even if scores are slightly different
+               (sort-by ee-score items)))
+        (is (= ["customer examples of bad sorting"
+                "customer success stories"
+                "examples of custom expressions"
+                "custom expression examples"]
+               (map :name (sort-by os-score [a b c d]))))
+        (is (= ["customer success stories"
+                "customer examples of bad sorting" ;; bumped up slightly in results
+                "examples of custom expressions"
+                "custom expression examples"]
+               (map :name (sort-by ee-score [a b c
+                                             (assoc d :collection_authority_level "official")]))))))))

src/metabase/api/search.clj

@@ -163,7 +163,7 @@
 ;; Databases can't be archived
 (defmethod archived-where-clause (class Database)
   [model archived?]
-  [:= 1 1])
+  [:= 1 (if archived? 2 1)])
 
 ;; Table has an `:active` flag, but no `:archived` flag; never return inactive Tables
 (defmethod archived-where-clause (class Table)

src/metabase/search/scoring.clj

@@ -281,7 +281,7 @@
                                {:score (:score text-score), :weight 10 :name "text score"})
                          (filter :score))]
      {:score  (/ (reduce + (map (fn [{:keys [weight score]}] (* weight score)) scores))
-                 (reduce + (map :weight (filter (comp pos? :score) scores))))
+                 (reduce + (map :weight scores)))
       :result (serialize result text-score scores)})))
 
 (defn top-results

test/metabase/search/scoring_test.clj

@@ -263,3 +263,11 @@
                 reverse
                 (map :result)
                 (map :name))))))
+
+(deftest score-and-result-test
+  (testing "If all scores are 0, does not divide by zero"
+    (let [scorer (reify search/ResultScore
+                   (score-result [_ search-result]
+                     [{:weight 100 :score 0 :name "Some score type"}
+                      {:weight 100 :score 0 :name "Some other score type"}]))]
+      (is (= 0 (:score (search/score-and-result scorer "" {:name "racing yo" :model "card"})))))))