From 027f565989a9208ea8d0266bee5d65f2635b97e2 Mon Sep 17 00:00:00 2001
From: Case Nelson <snoe@snoe.ca>
Date: Wed, 13 Apr 2022 15:02:11 -0600
Subject: [PATCH] Add bookmark ordering (#21652)
* Add bookmark ordering
Bookmark ordering is applied to bookmark fetches order-by clauses.
To save ordering send a sequence of `[{type, item_id}]` values.
There are no foreign keys to the `*_bookmark` tables and
bookmark ordering handle extra or missing rows gracefully by ignoring
them or falling back to order by created_at as a secondary sort. Pruning
is done on save.
* Fix missing docstring
* Fix refer order
* Use varchar for type because h2 is unhappy with text index
* Cast untyped literal in union for h2
* Use hx/literal thanks @dpsutton
---
resources/migrations/000_migrations.yaml | 73 ++++++++++++++++++++++++
src/metabase/api/bookmark.clj | 12 ++++
src/metabase/cmd/copy.clj | 7 ++-
src/metabase/models.clj | 1 +
src/metabase/models/bookmark.clj | 63 +++++++++++---------
test/metabase/api/bookmark_test.clj | 63 +++++++++++++++++---
6 files changed, 180 insertions(+), 39 deletions(-)
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index 65fd4dfa7b0..5d306090963 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -11372,6 +11372,79 @@ databaseChangeLog:
nullable: false
tableName: timeline
+ - changeSet:
+ id: v43.00-052
+ author: snoe
+ comment: Added 0.43.0 - bookmark ordering
+ changes:
+ - createTable:
+ tableName: bookmark_ordering
+ remarks: Table holding ordering information for various bookmark tables
+ columns:
+ - column:
+ name: id
+ type: int
+ autoIncrement: true
+ constraints:
+ primaryKey: true
+ nullable: false
+ - column:
+ name: user_id
+ type: int
+ remarks: 'ID of the User who ordered bookmarks'
+ constraints:
+ nullable: false
+ references: core_user(id)
+ foreignKeyName: fk_bookmark_ordering_user_id
+ deleteCascade: true
+ - column:
+ name: type
+ type: varchar(255)
+ remarks: 'type of the Bookmark'
+ constraints:
+ nullable: false
+ - column:
+ name: item_id
+ type: int
+ remarks: 'id of the item being bookmarked (Card, Collection, Dashboard, ...) no FK, so may no longer exist'
+ constraints:
+ nullable: false
+ - column:
+ name: ordering
+ type: int
+ remarks: 'order of bookmark for user'
+ constraints:
+ nullable: false
+ - changeSet:
+ id: v43.00-053
+ author: snoe
+ comment: Added 0.43.0 - bookmark ordering
+ changes:
+ - addUniqueConstraint:
+ tableName: bookmark_ordering
+ columnNames: user_id, type, item_id
+ constraintName: unique_bookmark_user_id_type_item_id
+ - changeSet:
+ id: v43.00-054
+ author: snoe
+ comment: Added 0.43.0 - bookmark ordering
+ changes:
+ - addUniqueConstraint:
+ tableName: bookmark_ordering
+ columnNames: user_id, ordering
+ constraintName: unique_bookmark_user_id_ordering
+ - changeSet:
+ id: v43.00-055
+ author: snoe
+ comment: Added 0.43.0 - bookmark ordering
+ changes:
+ - createIndex:
+ tableName: bookmark_ordering
+ columns:
+ - column:
+ name: user_id
+ indexName: idx_bookmark_ordering_user_id
+
# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
########################################################################################################################
diff --git a/src/metabase/api/bookmark.clj b/src/metabase/api/bookmark.clj
index 37496a6da6a..1edf7f9eebd 100644
--- a/src/metabase/api/bookmark.clj
+++ b/src/metabase/api/bookmark.clj
@@ -19,6 +19,11 @@
"Schema enumerating bookmarkable models."
(s/enum "card" "dashboard" "collection"))
+(def BookmarkOrderings
+ "Schema for an ordered of boomark orderings"
+ [{:type Models
+ :item_id su/IntGreaterThanZero}])
+
(def ^:private lookup
"Lookup map from model as a string to [model bookmark-model item-id-key]."
{"card" [Card CardBookmark :card_id]
@@ -56,4 +61,11 @@
item-key id)
api/generic-204-no-content))
+(api/defendpoint PUT "/ordering"
+ "Sets the order of bookmarks for user."
+ [:as {{:keys [orderings]} :body}]
+ {orderings BookmarkOrderings}
+ (bookmarks/save-ordering api/*current-user-id* orderings)
+ api/generic-204-no-content)
+
(api/define-routes)
diff --git a/src/metabase/cmd/copy.clj b/src/metabase/cmd/copy.clj
index fbe0e01ac40..c22762ef4a9 100644
--- a/src/metabase/cmd/copy.clj
+++ b/src/metabase/cmd/copy.clj
@@ -9,8 +9,10 @@
[metabase.db.connection :as mdb.conn]
[metabase.db.data-migrations :refer [DataMigrations]]
[metabase.db.setup :as mdb.setup]
- [metabase.models :refer [Activity Card CardBookmark Collection CollectionBookmark CollectionPermissionGraphRevision Dashboard DashboardBookmark
- DashboardCard DashboardCardSeries Database Dependency Dimension Field FieldValues
+ [metabase.models :refer [Activity BookmarkOrdering Card CardBookmark
+ Collection CollectionBookmark CollectionPermissionGraphRevision
+ Dashboard DashboardBookmark DashboardCard DashboardCardSeries
+ Database Dependency Dimension Field FieldValues
GeneralPermissionsRevision LoginHistory Metric MetricImportantField ModerationReview NativeQuerySnippet
Permissions PermissionsGroup PermissionsGroupMembership PermissionsRevision Pulse PulseCard
PulseChannel PulseChannelRecipient Revision Secret Segment Session Setting Table
@@ -64,6 +66,7 @@
CardBookmark
DashboardBookmark
CollectionBookmark
+ BookmarkOrdering
DashboardCard
DashboardCardSeries
Activity
diff --git a/src/metabase/models.clj b/src/metabase/models.clj
index c875c984227..d2846fc6bb8 100644
--- a/src/metabase/models.clj
+++ b/src/metabase/models.clj
@@ -88,6 +88,7 @@
[bookmark CardBookmark]
[bookmark DashboardBookmark]
[bookmark CollectionBookmark]
+ [bookmark BookmarkOrdering]
[card Card]
[collection Collection]
[c-perm-revision CollectionPermissionGraphRevision]
diff --git a/src/metabase/models/bookmark.clj b/src/metabase/models/bookmark.clj
index a398f0620f4..27de8f024c5 100644
--- a/src/metabase/models/bookmark.clj
+++ b/src/metabase/models/bookmark.clj
@@ -13,6 +13,7 @@
(models/defmodel CardBookmark :card_bookmark)
(models/defmodel DashboardBookmark :dashboard_bookmark)
(models/defmodel CollectionBookmark :collection_bookmark)
+(models/defmodel BookmarkOrdering :bookmark_ordering)
(defn- unqualify-key
[k]
@@ -23,7 +24,7 @@
id. This is required for the frontend entity loading system and does not refer to any particular bookmark id,
although the compound key can be inferred from it."
{:id s/Str
- :type (s/enum :card :collection :dashboard)
+ :type (s/enum "card" "collection" "dashboard")
:item_id su/IntGreaterThanZero
:name su/NonBlankString
(s/optional-key :dataset) (s/maybe s/Bool)
@@ -37,74 +38,80 @@
description."
[result]
(let [result (into {} (remove (comp nil? second) result))
- lookup {"report_card" "card" "report_dashboard" "dashboard" "collection" "collection"}
- ttype (-> (keys result)
- first
- name
- (str/split #"\.")
- first
- lookup
- keyword)
normalized-result (zipmap (map unqualify-key (keys result)) (vals result))
- item-id-str (str (:item_id normalized-result))]
- (merge
- {:id (str (name ttype) "-" item-id-str)
- :type ttype}
- (select-keys normalized-result [:item_id :name :dataset :description :display :authority_level]))))
+ id-str (str (:type normalized-result) "-" (:item_id normalized-result))]
+ (-> normalized-result
+ (select-keys [:item_id :type :name :dataset :description :display :authority_level])
+ (assoc :id id-str))))
(defn- bookmarks-union-query
- [id]
+ [user-id]
(let [as-null (when (= (mdb/db-type) :postgres) (hx/->integer nil))]
{:union-all [{:select [:card_id
[as-null :dashboard_id]
[as-null :collection_id]
- :id
+ [:card_id :item_id]
+ [(hx/literal "card") :type]
:created_at]
:from [CardBookmark]
- :where [:= :user_id id]}
+ :where [:= :user_id user-id]}
{:select [[as-null :card_id]
:dashboard_id
[as-null :collection_id]
- :id
+ [:dashboard_id :item_id]
+ [(hx/literal "dashboard") :type]
:created_at]
:from [DashboardBookmark]
- :where [:= :user_id id]}
+ :where [:= :user_id user-id]}
{:select [[as-null :card_id]
[as-null :dashboard_id]
:collection_id
- :id
+ [:collection_id :item_id]
+ [(hx/literal "collection") :type]
:created_at]
:from [CollectionBookmark]
- :where [:= :user_id id]}]}))
+ :where [:= :user_id user-id]}]}))
(s/defn bookmarks-for-user :- [BookmarkResult]
"Get all bookmarks for a user. Each bookmark will have a string id made of the model and model-id, a type, and
item_id, name, and description from the underlying bookmarked item."
- [id]
+ [user-id]
(->> (db/query
{:select [[:bookmark.created_at :created_at]
- [:card.id (db/qualify 'Card :item_id)]
+ [:bookmark.type :type]
+ [:bookmark.item_id :item_id]
[:card.name (db/qualify 'Card :name)]
[:card.dataset (db/qualify 'Card :dataset)]
[:card.display (db/qualify 'Card :display)]
[:card.description (db/qualify 'Card :description)]
[:card.archived (db/qualify 'Card :archived)]
- [:dashboard.id (db/qualify 'Dashboard :item_id)]
[:dashboard.name (db/qualify 'Dashboard :name)]
[:dashboard.description (db/qualify 'Dashboard :description)]
[:dashboard.archived (db/qualify 'Dashboard :archived)]
- [:collection.id (db/qualify 'Collection :item_id)]
[:collection.name (db/qualify 'Collection :name)]
[:collection.authority_level (db/qualify 'Collection :authority_level)]
[:collection.description (db/qualify 'Collection :description)]
[:collection.archived (db/qualify 'Collection :archived)]]
- :from [[(bookmarks-union-query id) :bookmark]]
+ :from [[(bookmarks-union-query user-id) :bookmark]]
:left-join [[Card :card] [:= :bookmark.card_id :card.id]
[Dashboard :dashboard] [:= :bookmark.dashboard_id :dashboard.id]
- [Collection :collection] [:= :bookmark.collection_id :collection.id]]
+ [Collection :collection] [:= :bookmark.collection_id :collection.id]
+ [BookmarkOrdering :bookmark_ordering] [:and
+ [:= :bookmark_ordering.user_id user-id]
+ [:= :bookmark_ordering.type :bookmark.type]
+ [:= :bookmark_ordering.item_id :bookmark.item_id]]]
:where (into [:and]
(for [table [:card :dashboard :collection]
:let [field (keyword (str (name table) "." "archived"))]]
[:or [:= field false] [:= field nil]]))
- :order-by [[:created_at :desc]]})
+ :order-by [[:bookmark_ordering.ordering :asc-nulls-last] [:created_at :desc]]})
(map normalize-bookmark-result)))
+
+(defn save-ordering
+ "Saves a bookmark ordering of shape `[{:type, :item_id}]`
+ Deletes all existing orderings for user so should be given a total ordering."
+ [user-id orderings]
+ (db/delete! BookmarkOrdering :user_id user-id)
+ (db/insert-many! BookmarkOrdering (->> orderings
+ (map #(select-keys % [:type :item_id]))
+ (map-indexed #(assoc %2 :user_id user-id :ordering %1)))))
diff --git a/test/metabase/api/bookmark_test.clj b/test/metabase/api/bookmark_test.clj
index 36e1d156010..c515c79fac4 100644
--- a/test/metabase/api/bookmark_test.clj
+++ b/test/metabase/api/bookmark_test.clj
@@ -1,12 +1,15 @@
(ns metabase.api.bookmark-test
"Tests for /api/bookmark endpoints."
(:require [clojure.test :refer :all]
- [metabase.models.bookmark :refer [CardBookmark CollectionBookmark DashboardBookmark]]
+ [metabase.models.bookmark :refer [BookmarkOrdering CardBookmark
+ CollectionBookmark
+ DashboardBookmark]]
[metabase.models.card :refer [Card]]
[metabase.models.collection :refer [Collection]]
[metabase.models.dashboard :refer [Dashboard]]
[metabase.test :as mt]
- [metabase.util :as u]))
+ [metabase.util :as u]
+ [toucan.db :as db]))
(deftest bookmarks-test
(testing "POST /api/bookmark/:model/:model-id"
@@ -50,19 +53,61 @@
(map :type)
set)))))))
+(defn bookmark-models [user-id & models]
+ (doseq [model models]
+ (cond
+ (instance? (class Collection) model)
+ (db/insert! CollectionBookmark
+ {:user_id user-id
+ :collection_id (u/the-id model)})
+
+ (instance? (class Card) model)
+ (db/insert! CardBookmark
+ {:user_id user-id
+ :card_id (u/the-id model)})
+
+ (instance? (class Dashboard) model)
+ (db/insert! DashboardBookmark
+ {:user_id user-id
+ :dashboard_id (u/the-id model)})
+
+ :else
+ (throw (ex-info "Unknown type" {:user-id user-id :model model})))))
+
(deftest bookmarks-on-archived-items-test
(testing "POST /api/bookmark/:model/:model-id"
(mt/with-temp* [Collection [archived-collection {:name "Test Collection" :archived true}]
Card [archived-card {:name "Test Card" :archived true}]
- Dashboard [archived-dashboard {:name "Test Dashboard" :archived true}]
- CardBookmark [card-bookmark {:user_id (mt/user->id :rasta)
- :card_id (u/the-id archived-card)}]
- CollectionBookmark [collection-bookmark {:user_id (mt/user->id :rasta)
- :collection_id (u/the-id archived-collection)}]
- DashboardBookmark [dashboard-bookmark {:user_id (mt/user->id :rasta)
- :dashboard_id (u/the-id archived-dashboard)}]]
+ Dashboard [archived-dashboard {:name "Test Dashboard" :archived true}]]
+ (bookmark-models (mt/user->id :rasta) archived-collection archived-card archived-dashboard)
(testing "check that we don't receive bookmarks of archived items"
(is (= #{}
(->> (mt/user-http-request :rasta :get 200 "bookmark")
(map :type)
set)))))))
+
+(deftest bookmarks-ordering-test
+ (testing "PUT /api/bookmark/ordering"
+ (mt/with-temp* [Collection [collection {:name "Test Collection"}]
+ Card [card {:name "Test Card"}]
+ Dashboard [dashboard {:name "Test Dashboard"}]]
+ (mt/with-model-cleanup [BookmarkOrdering]
+ (bookmark-models (mt/user->id :rasta) collection card dashboard)
+ (testing "Check that ordering works"
+ (is (= nil
+ (mt/user-http-request :rasta :put 204 "bookmark/ordering"
+ {:orderings [{:type "dashboard" :item_id (u/the-id dashboard)}
+ {:type "card" :item_id (u/the-id card)}
+ {:type "collection" :item_id (u/the-id collection)}]})))
+ (is (= ["dashboard" "card" "collection"]
+ (map :type
+ (mt/user-http-request :rasta :get 200 "bookmark")))))
+ (testing "Check that re-ordering works"
+ (is (= nil
+ (mt/user-http-request :rasta :put 204 "bookmark/ordering"
+ {:orderings [{:type "card" :item_id (u/the-id card)}
+ {:type "collection" :item_id (u/the-id collection)}
+ {:type "dashboard" :item_id (u/the-id dashboard)}]})))
+ (is (= ["card" "collection" "dashboard"]
+ (map :type
+ (mt/user-http-request :rasta :get 200 "bookmark")))))))))
--
GitLab