Authority Level on collections (formerly Typed collections) (metabase…

…#16191)

* Add type on collection

* Search with collection type

* Cleanup bounded heap accumulator

* Make search conditionally bump official collection items

* collection api and tests

* Put collection type onto hydrated cards on dashboards

* Move official collection type scoring into ee codebase

* ensure ee and oss agree when not official collection

* Mark Collections tree with type

* Remove unnecessary `and`s when no check for enhancements

* Tests for setting collection tree type

* Include hydration for collection type on dashboards

* Make sure created test collections are cleaned up

* Cleanup tests, don't search on collection_type

looks for all text fields and searches the term against that. official
would bring back all official types. no bueno

* Docstring on protocol and don't shadow comparator

* update to new ee impl var passing style

* Collection model ensures no type change on personal collection

* Check for personal collection when updating type

model checks for personal collection and rejects in the update but
doesn't check for children of personal collection.

* Update checking for unchangeable properties of a personal collection

* Cleanup: type collection tree, batched hydration, combine error checks

* Cleanup

* move bounded-heap accumulator to utils

* switch to test.check testing

* Bad test just to see what our CI will report

* remove purposeful failing test (was for CI)

* collection.type -> collection.authority_level

* Test the actual ee scoring impl, not the wrapped one

locally i had enhancements enabled so the tests were passing, but in
CI it was failing as it did not have enhancements enabled

.dir-locals.el

@@ -36,6 +36,7 @@
                               (p.types/deftype+ '(2 nil nil (:defn)))
                               (p/def-map-type '(2 nil nil (:defn)))
                               (p.types/defrecord+ '(2 nil nil (:defn)))
+                              (prop/for-all 1)
                               (tools.macro/macrolet '(1 (:defn))))))
                   (clojure-indent-style . always-align)
                   ;; if you're using clj-refactor (highly recommended!)

enterprise/backend/src/metabase_enterprise/search/scoring.clj

@@ -0,0 +1,26 @@
+(ns metabase-enterprise.search.scoring
+  (:require [metabase-enterprise.enhancements.ee-strategy-impl :as ee-strategy-impl]
+            [metabase.public-settings.metastore :as settings.metastore]
+            [metabase.search.scoring :as scoring]))
+
+(defn- official-collection-score
+  "A scorer for items in official collections"
+  [{:keys [collection_authority_level]}]
+  (if (contains? #{"official"} collection_authority_level)
+    1
+    0))
+
+(def scoring-impl
+  "Scoring implementation that adds score for items in official collections."
+  (reify scoring/ResultScore
+    (score-result [_ result]
+      (conj (scoring/score-result scoring/oss-score-impl result)
+            {:weight 2
+             :score  (official-collection-score result)
+             :name   "official collection score"}))))
+
+(def ee-scoring
+  "Enterprise scoring of results, falling back to the open source version if enterprise is not enabled."
+  (ee-strategy-impl/reify-ee-strategy-impl #'settings.metastore/enable-enhancements?
+                                           scoring-impl scoring/oss-score-impl
+                                           scoring/ResultScore))

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

@@ -0,0 +1,23 @@
+(ns metabase-enterprise.search.scoring-test
+  (:require [clojure.test :refer :all]
+            [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))
+            (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)))))))

resources/migrations/000_migrations.yaml

@@ -8340,6 +8340,20 @@ databaseChangeLog:
                                   'DateTimeField', 'DecimalField', 'DictionaryField', 'FloatField', 'IntegerField',
                                   'TextField', 'TimeField', 'UUIDField', 'UnknownField');
 
+  - changeSet:
+      id: 309
+      author: dpsutton
+      comment: 'Added 0.40.0 - Add type to collections'
+      changes:
+        - addColumn:
+            tableName: collection
+            columns:
+              - column:
+                  name: authority_level
+                  type: varchar(255)
+                  remarks: 'Nullable column to incidate collection''s authority level. Initially values are "official" and nil.'
+                  constraints:
+                    nullable: true
 # >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
 
 ########################################################################################################################

src/metabase/api/collection.clj

@@ -361,15 +361,18 @@
 
 (api/defendpoint POST "/"
   "Create a new Collection."
-  [:as {{:keys [name color description parent_id namespace]} :body}]
-  {name        su/NonBlankString
-   color       collection/hex-color-regex
-   description (s/maybe su/NonBlankString)
-   parent_id   (s/maybe su/IntGreaterThanZero)
-   namespace   (s/maybe su/NonBlankString)}
+  [:as {{:keys [name color description parent_id namespace authority_level]} :body}]
+  {name            su/NonBlankString
+   color           collection/hex-color-regex
+   description     (s/maybe su/NonBlankString)
+   parent_id       (s/maybe su/IntGreaterThanZero)
+   namespace       (s/maybe su/NonBlankString)
+   authority_level collection/AuthorityLevel}
   ;; To create a new collection, you need write perms for the location you are going to be putting it in...
   (write-check-collection-or-root-collection parent_id)
   ;; Now create the new Collection :)
+  (api/check-403 (or (nil? authority_level)
+                     (and api/*is-superuser?* authority_level)))
   (db/insert! Collection
     (merge
      {:name        name
@@ -428,23 +431,40 @@
 
 (api/defendpoint PUT "/:id"
   "Modify an existing Collection, including archiving or unarchiving it, or moving it."
-  [id, :as {{:keys [name color description archived parent_id], :as collection-updates} :body}]
-  {name        (s/maybe su/NonBlankString)
-   color       (s/maybe collection/hex-color-regex)
-   description (s/maybe su/NonBlankString)
-   archived    (s/maybe s/Bool)
-   parent_id   (s/maybe su/IntGreaterThanZero)}
+  [id, :as {{:keys [name color description archived parent_id authority_level update_collection_tree_authority_level], :as collection-updates} :body}]
+  {name                                   (s/maybe su/NonBlankString)
+   color                                  (s/maybe collection/hex-color-regex)
+   description                            (s/maybe su/NonBlankString)
+   archived                               (s/maybe s/Bool)
+   parent_id                              (s/maybe su/IntGreaterThanZero)
+   authority_level                        collection/AuthorityLevel
+   update_collection_tree_authority_level (s/maybe s/Bool)}
   ;; do we have perms to edit this Collection?
   (let [collection-before-update (api/write-check Collection id)]
     ;; if we're trying to *archive* the Collection, make sure we're allowed to do that
     (check-allowed-to-archive-or-unarchive collection-before-update collection-updates)
+    (when (or (and (contains? collection-updates :authority_level)
+                   (not= authority_level (:authority_level collection-before-update)))
+              update_collection_tree_authority_level)
+      (api/check-403 (and api/*is-superuser?*
+                          ;; pre-update of model checks if the collection is a personal collection and rejects changes
+                          ;; to authority_level, but it doesn't check if it is a sub-collection of a personal one so we add that
+                          ;; here
+                          (not (collection/is-personal-collection-or-descendant-of-one? collection-before-update)))))
     ;; ok, go ahead and update it! Only update keys that were specified in the `body`. But not `parent_id` since
     ;; that's not actually a property of Collection, and since we handle moving a Collection separately below.
-    (let [updates (u/select-keys-when collection-updates :present [:name :color :description :archived])]
+    (let [updates (u/select-keys-when collection-updates :present [:name :color :description :archived :authority_level])]
       (when (seq updates)
         (db/update! Collection id updates)))
     ;; if we're trying to *move* the Collection (instead or as well) go ahead and do that
     (move-collection-if-needed! collection-before-update collection-updates)
+    ;; mark the tree after moving so the new tree is what is marked as official
+    (when update_collection_tree_authority_level
+      (db/execute! {:update Collection
+                    :set    {:authority_level authority_level}
+                    :where  [:or
+                             [:= :id id]
+                             [:like :location (hx/literal (format "%%/%d/%%" id))]]}))
     ;; if we *did* end up archiving this Collection, we most post a few notifications
     (maybe-send-archived-notificaitons! collection-before-update collection-updates))
   ;; finally, return the updated object

src/metabase/api/dashboard.clj

@@ -205,7 +205,7 @@
   [id]
   (-> (Dashboard id)
       api/check-404
-      (hydrate [:ordered_cards :card :series] :can_write :param_fields :param_values)
+      (hydrate [:ordered_cards :card :series] :collection_authority_level :can_write :param_fields :param_values)
       api/read-check
       api/check-not-archived
       hide-unreadable-cards

src/metabase/api/search.clj

@@ -75,6 +75,7 @@
    ;; returned for Card, Dashboard, Pulse, and Collection
    :collection_id       :integer
    :collection_name     :text
+   :collection_authority_level :text
    ;; returned for Card and Dashboard
    :collection_position :integer
    :favorite            :boolean
@@ -323,7 +324,8 @@
   (let [match             (wildcard-match (scoring/normalize query))
         columns-to-search (->> all-search-columns
                                (filter (fn [[k v]] (= v :text)))
-                               (map first))
+                               (map first)
+                               (remove #{:collection_authority_level}))
         case-clauses      (as-> columns-to-search <>
                                 (map (fn [col] [:like (hsql/call :lower col) match]) <>)
                                 (interleave <> (repeat 0))

src/metabase/models/collection.clj

@@ -33,6 +33,10 @@
 
 (models/defmodel Collection :collection)
 
+(def AuthorityLevel
+  "Schema for valid collection authority levels"
+  (s/maybe (s/enum "official")))
+
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                         Slug & Hex Color & Validation                                          |
@@ -370,7 +374,7 @@
   "Given a `collection` return a location path that should match the `:location` value of all the children of the
   Collection.
 
-     (children-location collection) ; -> \"/10/20/30/;
+     (children-location collection) ; -> \"/10/20/30/\";
 
      ;; To get children of this collection:
      (db/select Collection :location \"/10/20/30/\")"
@@ -613,7 +617,7 @@
    :personal_owner_id (s/maybe su/IntGreaterThanZero)
    s/Keyword          s/Any})
 
-(s/defn ^:private is-personal-collection-or-descendant-of-one? :- s/Bool
+(s/defn is-personal-collection-or-descendant-of-one? :- s/Bool
   "Is `collection` a Personal Collection, or a descendant of one?"
   [collection :- CollectionWithLocationAndPersonalOwnerID]
   (boolean
@@ -688,28 +692,19 @@
   [collection-before-updates :- CollectionWithLocationAndIDOrRoot, collection-updates :- su/Map]
   ;; you're not allowed to change the `:personal_owner_id` of a Collection!
   ;; double-check and make sure it's not just the existing value getting passed back in for whatever reason
-  (when (api/column-will-change? :personal_owner_id collection-before-updates collection-updates)
-    (throw
-     (ex-info (tru "You're not allowed to change the owner of a Personal Collection.")
-       {:status-code 400
-        :errors      {:personal_owner_id (tru "You're not allowed to change the owner of a Personal Collection.")}})))
-  ;;
-  ;; The checks below should be redundant because the `perms-for-moving` and `perms-for-archiving` functions also
-  ;; check to make sure you're not operating on Personal Collections. But as an extra safety net it doesn't hurt to
-  ;; check here too.
-  ;;
-  ;; You also definitely cannot *move* a Personal Collection
-  (when (api/column-will-change? :location collection-before-updates collection-updates)
-    (throw
-     (ex-info (tru "You're not allowed to move a Personal Collection.")
-       {:status-code 400
-        :errors      {:location (tru "You're not allowed to move a Personal Collection.")}})))
-  ;; You also can't archive a Personal Collection
-  (when (api/column-will-change? :archived collection-before-updates collection-updates)
-    (throw
-     (ex-info (tru "You cannot archive a Personal Collection.")
-       {:status-code 400
-        :errors      {:archived (tru "You cannot archive a Personal Collection.")}}))))
+  (let [unchangeable {:personal_owner_id (tru "You are not allowed to change the owner of a Personal Collection.")
+                      :authority_level   (tru "You are not allowed to change the authority level of a Personal Collection.")
+                      ;; The checks below should be redundant because the `perms-for-moving` and `perms-for-archiving`
+                      ;; functions also check to make sure you're not operating on Personal Collections. But as an extra safety net it
+                      ;; doesn't hurt to check here too.
+                      :location          (tru "You are not allowed to move a Personal Collection.")
+                      :archived          (tru "You cannot archive a Personal Collection.")}]
+    (when-let [[k msg] (->> unchangeable
+                            (filter (fn [[k _msg]]
+                                      (api/column-will-change? k collection-before-updates collection-updates)))
+                            first)]
+      (throw
+       (ex-info msg {:status-code 400 :errors {k msg}})))))
 
 (s/defn ^:private maybe-archive-or-unarchive!
   "If `:archived` specified in the updates map, archive/unarchive as needed."
@@ -865,7 +860,8 @@
   models/IModel
   (merge models/IModelDefaults
          {:hydration-keys (constantly [:collection])
-          :types          (constantly {:namespace :keyword})
+          :types          (constantly {:namespace       :keyword
+                                       :authority_level :keyword})
           :pre-insert     pre-insert
           :post-insert    post-insert
           :pre-update     pre-update

src/metabase/models/dashboard.clj

@@ -7,7 +7,7 @@
             [metabase.automagic-dashboards.populate :as magic.populate]
             [metabase.events :as events]
             [metabase.models.card :as card :refer [Card]]
-            [metabase.models.collection :as collection]
+            [metabase.models.collection :as collection :refer [Collection]]
             [metabase.models.dashboard-card :as dashboard-card :refer [DashboardCard]]
             [metabase.models.field-values :as field-values]
             [metabase.models.interface :as i]
@@ -34,20 +34,32 @@
   {:hydrate :ordered_cards}
   [dashboard-or-id]
   (db/do-post-select DashboardCard
-    (db/query {:select    [:dashcard.*]
+    (db/query {:select    [:dashcard.* [:collection.authority_level :collection_authority_level]]
                :from      [[DashboardCard :dashcard]]
-               :left-join [[Card :card] [:= :dashcard.card_id :card.id]]
+               :left-join [[Card :card] [:= :dashcard.card_id :card.id]
+                           [Collection :collection] [:= :collection.id :card.collection_id]]
                :where     [:and
                            [:= :dashcard.dashboard_id (u/the-id dashboard-or-id)]
                            [:or
                             [:= :card.archived false]
                             [:= :card.archived nil]]] ; e.g. DashCards with no corresponding Card, e.g. text Cards
                :order-by  [[:dashcard.created_at :asc]]})))
 
-
-;;; ----------------------------------------------- Entity & Lifecycle -----------------------------------------------
+(defn collections-authority-level
+  "Efficiently hydrate the `:collection_authority_level` of a sequence of dashboards."
+  {:batched-hydrate :collection_authority_level}
+  [dashboards]
+  (let [coll-id->level (into {}
+                             (map (juxt :id :authority_level))
+                             (db/query {:select    [:dashboard.id :collection.authority_level]
+                                        :from      [[:report_dashboard :dashboard]]
+                                        :left-join [[Collection :collection] [:= :collection.id :dashboard.collection_id]]
+                                        :where     [:in :dashboard.id (into #{} (map u/the-id) dashboards)]}))]
+    (for [dashboard dashboards]
+      (assoc dashboard :collection_authority_level (get coll-id->level (u/the-id dashboard))))))
 
 (models/defmodel Dashboard :report_dashboard)
+;;; ----------------------------------------------- Entity & Lifecycle -----------------------------------------------
 
 (defn- assert-valid-parameters [{:keys [parameters]}]
   (when (s/check (s/maybe [{:id su/NonBlankString, s/Keyword s/Any}]) parameters)

src/metabase/search/config.clj

@@ -122,12 +122,16 @@
 
 (defmethod columns-for-model (class Card)
   [_]
-  (conj default-columns :collection_id :collection_position [:collection.name :collection_name] :dataset_query
+  (conj default-columns :collection_id :collection_position :dataset_query
+        [:collection.name :collection_name]
+        [:collection.authority_level :collection_authority_level]
         favorite-col dashboardcard-count-col))
 
 (defmethod columns-for-model (class Dashboard)
   [_]
-  (conj default-columns :collection_id :collection_position [:collection.name :collection_name] favorite-col))
+  (conj default-columns :collection_id :collection_position favorite-col
+        [:collection.name :collection_name]
+        [:collection.authority_level :collection_authority_level]))
 
 (defmethod columns-for-model (class Database)
   [_]
@@ -139,7 +143,8 @@
 
 (defmethod columns-for-model (class Collection)
   [_]
-  (conj (remove #{:updated_at} default-columns) [:id :collection_id] [:name :collection_name]))
+  (conj (remove #{:updated_at} default-columns) [:id :collection_id] [:name :collection_name]
+        [:authority_level :collection_authority_level]))
 
 (defmethod columns-for-model (class Segment)
   [_]