From a1a6652133f39dab02bdf9ae04cc12b278a41a3e Mon Sep 17 00:00:00 2001
From: adam-james <21064735+adam-james-v@users.noreply.github.com>
Date: Fri, 8 Apr 2022 12:10:14 -0700
Subject: [PATCH] Start page be simplified popular items (#21553)
* Add `:is_installer` key to users returned from api/user/current
* Add `:has_question_and_dashboard` Key to current users via hydration
This is also part of the starting page project, it's a key that helps the frontend serve more specific data to the
user.
* Moved `permissions-set` function up so I can use it in hydration
* Adjust recents query and first pass at popular items query
Recents:
Before, the recent query would look at the ViewLog, which records a view on cards even when it was only 'viewed' via a
dashboard.
For dashboards and tables, we don't need to make a change.
For cards, we now query the QueryExecution table and use started_at as a proxy for the last viewed timestamp, and can
then only grab cards with an execution context of 'question'.
Popular:
We now want to have a notion of 'popular' items as well, which can look different than 'recents'. This is the first
attempt at a reasonable query with scoring system.
The score takes into account:
- recency of view
- count of views total
- if the card/dashboard is 'verified' (enterprise)
- if the cad/dashboard is inside an 'official' Collection (enterprise)
The popular score currently uses only the *current-user-id* to fetch items, so popularity is (in this commit, at
least) a per-user concept.
* Fixed mistake and renamed endpoint
* Typo fix
* Clean up QueryExecution in tests
* Indent, naming, and some simple defs
* try to reduce db calls for :has_question_and_dashboard
I've moved the fn out of the hydration system to guarantee that it only runs for a single user, and is only used in
`GET /api/user/current` (and not accidentally used in other spots via hydration mechanism).
Still don't know how to check Card and Dashboard tables more efficiently
* ViewLog and QueryExecution have different timestamps. Can't compare
Pushing this for some review, but might want to build a proper way to compare, so that recent cards and recent
dashboards/tables are sorted equally
* fix namespace sorting
* Fix the endpoint name
* Sorting mixed date time formats 'works' but not ideal
This formats the timestamps into strings and uses those for comparison. Not perfect.
Pushing in case people are trying the branch
* Use simpler db function
* Let views and runs query work with one user or all users
Popular_items are an all-users notion, but recent_views applies only to the current user.
* Unify view_log.timestamp to query_execution.started_at type
these used to both be `DATETIME` in the migration file. Then migration
168 bumped up the type of the view_log.timestamp column:
```
- changeSet:
id: 168
author: camsaul
comment: Added 0.36.0
changes:
- modifyDataType:
tableName: query_execution
columnName: started_at
newDataType: ${timestamp_type}
```
So these were no longer the same on h2 (and possibly mysql). But sorting
viewlogs and query_executions would fail.
```clojure
activity=> (->> (ViewLog) first :timestamp #_type)
"0x6a33e42e"
"2022-04-04T21:57:07.471"]
activity=> (->> (ViewLog) first :timestamp type)
java.time.LocalDateTime
activity=> (->> (QueryExecution) first :started_at #_type)
"0x7af249ac"
"2022-04-04T21:57:07.625738Z"]
activity=> (->> (QueryExecution) first :started_at type)
java.time.OffsetDateTime
```
The LocalDateTime and OffsetDateTime were not comparable. So make them
identical types again.
* Bookmarked results should not show up in recents/ popular.
This is done in a db query to still try fetch enough items from the db to present to the user.
Filtering bookmarked items later may result in an empty list. It's possible that the firt N results are all
bookmarked, and then the frontend would have no items to show. Filtering bookmarked results out from the beginning
increases the chances of a non-empty result.
* :first_login populated with the earliest login timestamp.
If there is a first login timestamp, that is used. If one does not exist, we assume it's the first time logging in and
use an OffsetDateTime (now). This is possible since the login_history is done off thread, so on a real first login, we
might hit the db before the login is logged, resulting in an empty list returned.
On subsequent checks, we should see a proper timestamp from the db.
Since this is used to check if a user is 'new' (within 7 days of first logging in), the accuracy of the timestamp
matters on the order of days, not milliseconds, so this should be ok.
* Passing test
* Popular_items test
Tweak the create-views! function to more consistently order items. And creates views for dashboards/tables (ViewLog)
and cards (QueryExecution) in a unified way, meaning we can reliably order views, and write tests more easily.
Note that the popular_items test may have to change if the scoring math changes.
* Fix e2e test
* Fix nit and bug
- forgot to remove '0' from the and clause, so we didn't get the expected boolean
- popular_items not WIP
* another nit
* Fix popular items on the frontend
Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>
Co-authored-by: dan sutton <dan@dpsutton.com>
---
.../src/metabase/entities/popular-items.js | 1 -
.../components/HomeContent/HomeContent.tsx | 16 +-
.../HomeContent/HomeContent.unit.spec.tsx | 4 +
.../containers/HomeContent/HomeContent.tsx | 2 +
.../HomePopularSection/HomePopularSection.tsx | 2 +-
.../onboarding/home/homepage.cy.spec.js | 14 +-
.../search/recently-viewed.cy.spec.js | 4 +-
.../smoketest/admin_setup.cy.spec.js | 9 +-
resources/migrations/000_migrations.yaml | 10 +
src/metabase/api/activity.clj | 171 +++++++++++++++---
src/metabase/api/user.clj | 28 ++-
src/metabase/models/user.clj | 45 +++--
test/metabase/api/activity_test.clj | 114 ++++++------
test/metabase/api/user_test.clj | 5 +-
14 files changed, 311 insertions(+), 114 deletions(-)
diff --git a/frontend/src/metabase/entities/popular-items.js b/frontend/src/metabase/entities/popular-items.js
index a6b5805c69a..6e1a5d70dd0 100644
--- a/frontend/src/metabase/entities/popular-items.js
+++ b/frontend/src/metabase/entities/popular-items.js
@@ -21,7 +21,6 @@ const PopularItems = createEntity({
nameOne: "popularItem",
path: "/api/activity/popular_items",
schema: PopularItemSchema,
-
wrapEntity(item, dispatch = null) {
const entity = getEntity(item);
return entity.wrapEntity(item, dispatch);
diff --git a/frontend/src/metabase/home/homepage/components/HomeContent/HomeContent.tsx b/frontend/src/metabase/home/homepage/components/HomeContent/HomeContent.tsx
index 41139c5e97c..255304d1582 100644
--- a/frontend/src/metabase/home/homepage/components/HomeContent/HomeContent.tsx
+++ b/frontend/src/metabase/home/homepage/components/HomeContent/HomeContent.tsx
@@ -2,7 +2,7 @@ import React from "react";
import moment from "moment";
import { parseTimestamp } from "metabase/lib/time";
import { isSyncCompleted } from "metabase/lib/syncing";
-import { Database, RecentItem, User } from "metabase-types/api";
+import { Database, PopularItem, RecentItem, User } from "metabase-types/api";
import HomePopularSection from "../../containers/HomePopularSection";
import HomeRecentSection from "../../containers/HomeRecentSection";
import HomeXraySection from "../../containers/HomeXraySection";
@@ -11,6 +11,7 @@ export interface HomeContentProps {
user: User;
databases: Database[];
recentItems: RecentItem[];
+ popularItems: PopularItem[];
}
const HomeContent = (props: HomeContentProps): JSX.Element | null => {
@@ -29,23 +30,28 @@ const HomeContent = (props: HomeContentProps): JSX.Element | null => {
return null;
};
-const isPopularSection = ({ user, recentItems }: HomeContentProps) => {
+const isPopularSection = ({
+ user,
+ recentItems,
+ popularItems,
+}: HomeContentProps): boolean => {
return (
!user.is_installer &&
user.has_question_and_dashboard &&
+ popularItems.length > 0 &&
(isWithinWeek(user.first_login) || !recentItems.length)
);
};
-const isRecentSection = ({ recentItems }: HomeContentProps) => {
+const isRecentSection = ({ recentItems }: HomeContentProps): boolean => {
return recentItems.length > 0;
};
-const isXraySection = ({ databases }: HomeContentProps) => {
+const isXraySection = ({ databases }: HomeContentProps): boolean => {
return databases.some(isSyncCompleted);
};
-const isWithinWeek = (timestamp: string) => {
+const isWithinWeek = (timestamp: string): boolean => {
const date = parseTimestamp(timestamp);
const weekAgo = moment().subtract(1, "week");
return date.isAfter(weekAgo);
diff --git a/frontend/src/metabase/home/homepage/components/HomeContent/HomeContent.unit.spec.tsx b/frontend/src/metabase/home/homepage/components/HomeContent/HomeContent.unit.spec.tsx
index 4856f0426c7..f0dc05f60d5 100644
--- a/frontend/src/metabase/home/homepage/components/HomeContent/HomeContent.unit.spec.tsx
+++ b/frontend/src/metabase/home/homepage/components/HomeContent/HomeContent.unit.spec.tsx
@@ -2,6 +2,7 @@ import React from "react";
import { render, screen } from "@testing-library/react";
import {
createMockDatabase,
+ createMockPopularItem,
createMockRecentItem,
createMockUser,
} from "metabase-types/api/mocks";
@@ -35,6 +36,7 @@ describe("HomeContent", () => {
}),
databases: [createMockDatabase()],
recentItems: [createMockRecentItem()],
+ popularItems: [createMockPopularItem()],
});
render(<HomeContent {...props} />);
@@ -51,6 +53,7 @@ describe("HomeContent", () => {
}),
databases: [createMockDatabase()],
recentItems: [],
+ popularItems: [createMockPopularItem()],
});
render(<HomeContent {...props} />);
@@ -111,5 +114,6 @@ const getProps = (opts?: Partial<HomeContentProps>): HomeContentProps => ({
user: createMockUser(),
databases: [],
recentItems: [],
+ popularItems: [],
...opts,
});
diff --git a/frontend/src/metabase/home/homepage/containers/HomeContent/HomeContent.tsx b/frontend/src/metabase/home/homepage/containers/HomeContent/HomeContent.tsx
index 3e8119cfb42..413e0d4d411 100644
--- a/frontend/src/metabase/home/homepage/containers/HomeContent/HomeContent.tsx
+++ b/frontend/src/metabase/home/homepage/containers/HomeContent/HomeContent.tsx
@@ -2,6 +2,7 @@ import { connect } from "react-redux";
import _ from "underscore";
import Databases from "metabase/entities/databases";
import RecentItems from "metabase/entities/recent-items";
+import PopularItems from "metabase/entities/popular-items";
import { getUser } from "metabase/selectors/user";
import { State } from "metabase-types/store";
import HomeContent from "../../components/HomeContent";
@@ -13,5 +14,6 @@ const mapStateToProps = (state: State) => ({
export default _.compose(
Databases.loadList(),
RecentItems.loadList({ reload: true }),
+ PopularItems.loadList({ reload: true }),
connect(mapStateToProps),
)(HomeContent);
diff --git a/frontend/src/metabase/home/homepage/containers/HomePopularSection/HomePopularSection.tsx b/frontend/src/metabase/home/homepage/containers/HomePopularSection/HomePopularSection.tsx
index 636ced3e075..cd38a635049 100644
--- a/frontend/src/metabase/home/homepage/containers/HomePopularSection/HomePopularSection.tsx
+++ b/frontend/src/metabase/home/homepage/containers/HomePopularSection/HomePopularSection.tsx
@@ -1,4 +1,4 @@
import PopularItems from "metabase/entities/popular-items";
import HomePopularSection from "../../components/HomePopularSection";
-export default PopularItems.loadList({ reload: true })(HomePopularSection);
+export default PopularItems.loadList()(HomePopularSection);
diff --git a/frontend/test/metabase/scenarios/onboarding/home/homepage.cy.spec.js b/frontend/test/metabase/scenarios/onboarding/home/homepage.cy.spec.js
index 97b414cca4b..f91a9990074 100644
--- a/frontend/test/metabase/scenarios/onboarding/home/homepage.cy.spec.js
+++ b/frontend/test/metabase/scenarios/onboarding/home/homepage.cy.spec.js
@@ -58,8 +58,8 @@ describe("scenarios > home > homepage", () => {
it("should display recent items", () => {
restore("default");
- cy.signInAsAdmin();
+ cy.signInAsAdmin();
visitDashboard(1);
cy.findByText("Orders in a dashboard");
@@ -72,14 +72,18 @@ describe("scenarios > home > homepage", () => {
cy.findByText("Orders");
});
- it.skip("should display popular items for a new user", () => {
+ it("should display popular items for a new user", () => {
restore("default");
- cy.signInAsNormalUser();
+ cy.signInAsAdmin();
+ visitDashboard(1);
+ cy.findByText("Orders in a dashboard");
+ cy.signOut();
+
+ cy.signInAsNormalUser();
cy.visit("/");
cy.wait("@getPopularItems");
- cy.findByText("Here are some popular items");
-
+ cy.findByText("Here are some popular dashboards");
cy.findByText("Orders in a dashboard").click();
cy.wait("@getDashboard");
cy.findByText("Orders");
diff --git a/frontend/test/metabase/scenarios/onboarding/search/recently-viewed.cy.spec.js b/frontend/test/metabase/scenarios/onboarding/search/recently-viewed.cy.spec.js
index b78e12d7eda..75d3705b93e 100644
--- a/frontend/test/metabase/scenarios/onboarding/search/recently-viewed.cy.spec.js
+++ b/frontend/test/metabase/scenarios/onboarding/search/recently-viewed.cy.spec.js
@@ -34,13 +34,13 @@ describe(`search > recently viewed`, () => {
cy.findByPlaceholderText("Search…").click();
cy.findByTestId("loading-spinner").should("not.exist");
- assertRecentlyViewedItem(0, "Orders", "Question", "/question/1-orders");
assertRecentlyViewedItem(
- 1,
+ 0,
"Orders in a dashboard",
"Dashboard",
"/dashboard/1-orders-in-a-dashboard",
);
+ assertRecentlyViewedItem(1, "Orders", "Question", "/question/1-orders");
assertRecentlyViewedItem(2, "People", "Table", "/question#?db=1&table=3");
});
});
diff --git a/frontend/test/metabase/scenarios/smoketest/admin_setup.cy.spec.js b/frontend/test/metabase/scenarios/smoketest/admin_setup.cy.spec.js
index 099e0517f0d..6380ba8765e 100644
--- a/frontend/test/metabase/scenarios/smoketest/admin_setup.cy.spec.js
+++ b/frontend/test/metabase/scenarios/smoketest/admin_setup.cy.spec.js
@@ -525,8 +525,8 @@ describe("smoketest > admin_setup", () => {
// Check table names and visibility
- cy.contains("People");
- cy.contains("Test");
+ cy.findByText("Here are some popular items");
+ cy.findByText("Test Table");
cy.findByText("Reviews").should("not.exist");
// Check question names and descriptions
@@ -596,9 +596,7 @@ describe("smoketest > admin_setup", () => {
cy.signIn("nocollection");
cy.visit("/");
- cy.wait(2000).findByText(
- "Try out these sample x-rays to see what Metabase can do.",
- );
+ cy.wait(2000).findByText("Here are some popular tables");
cy.contains("Test Table");
cy.contains("Reviews").should("not.exist");
@@ -808,7 +806,6 @@ describe("smoketest > admin_setup", () => {
// No collection user sees Test Table and People table
cy.contains("Test Table");
- cy.contains("People");
cy.contains("Reviews").should("not.exist");
});
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index 065beb88e9c..5a6497354b6 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -11330,6 +11330,16 @@ databaseChangeLog:
AND p.object = '/general/subscription/'
WHERE p.object IS NULL;
+ - changeSet:
+ id: v43.00-049
+ author: dpsutton
+ comment: Added 0.43.0 - Unify datatype with query_execution.started_at so comparable (see 168).
+ changes:
+ - modifyDataType:
+ tableName: view_log
+ columnName: timestamp
+ newDataType: ${timestamp_type}
+
- changeSet:
id: v43.00-051
author: adam-james
diff --git a/src/metabase/api/activity.clj b/src/metabase/api/activity.clj
index cb97160746c..a6806f51a71 100644
--- a/src/metabase/api/activity.clj
+++ b/src/metabase/api/activity.clj
@@ -1,14 +1,19 @@
(ns metabase.api.activity
(:require [clojure.set :as set]
+ [clojure.string :as str]
[compojure.core :refer [GET]]
[medley.core :as m]
[metabase.api.common :refer [*current-user-id* defendpoint define-routes]]
[metabase.models.activity :refer [Activity]]
+ [metabase.models.bookmark :refer [CardBookmark DashboardBookmark]]
[metabase.models.card :refer [Card]]
+ [metabase.models.collection :refer [Collection]]
[metabase.models.dashboard :refer [Dashboard]]
[metabase.models.interface :as mi]
+ [metabase.models.query-execution :refer [QueryExecution]]
[metabase.models.table :refer [Table]]
[metabase.models.view-log :refer [ViewLog]]
+ [metabase.util.honeysql-extensions :as hx]
[toucan.db :as db]
[toucan.hydrate :refer [hydrate]]))
@@ -86,19 +91,33 @@
(hydrate :user :table :database)
add-model-exists-info)))
+(defn- models-query
+ [model ids]
+ (db/select
+ (case model
+ "card" [Card
+ :id :name :collection_id :description :display
+ :dataset_query :dataset :archived
+ (db/qualify Collection :authority_level)]
+ "dashboard" [Dashboard
+ :id :name :collection_id :description
+ :archived
+ (db/qualify Collection :authority_level)]
+ "table" [Table
+ :id :name :db_id
+ :display_name :initial_sync_status
+ :visibility_type])
+ (cond-> {:where [:in (db/qualify (symbol (str/capitalize model)) :id) ids]}
+ (not= model "table")
+ (merge {:left-join [Collection [:= (db/qualify Collection :id) :collection_id]]}))))
+
(defn- models-for-views
"Returns a map of {model {id instance}} for activity views suitable for looking up by model and id to get a model."
[views]
(letfn [(select-items! [model ids]
(when (seq ids)
- (db/select
- (case model
- "card" [Card :id :name :collection_id :description :display
- :dataset_query :dataset :archived]
- "dashboard" [Dashboard :id :name :collection_id :description :archived]
- "table" [Table :id :name :db_id :display_name :initial_sync_status
- :visibility_type])
- {:where [:in :id ids]})))
+ (-> (models-query model ids)
+ (hydrate :moderation_reviews))))
(by-id [models] (m/index-by :id models))]
(into {} (map (fn [[model models]]
[model (->> models
@@ -107,31 +126,137 @@
(by-id))]))
(group-by :model views))))
+(defn- views-and-runs
+ "Common query implementation for `recent_views` and `popular_items`. Tables and Dashboards have a query limit of `views-limit`.
+ Cards have a query limit of `card-runs-limit`.
+
+ The expected output of the query is a single row per unique model viewed by the current user including a `:max_ts` which
+ has the most recent view timestamp of the item and `:cnt` which has total views. We order the results by most recently
+ viewed then hydrate the basic details of the model. Bookmarked cards and dashboards are *not* included in the result.
+
+ Viewing a Dashboard will add entries to the view log for all cards on that dashboard so all card views are instead derived
+ from the query_execution table. The query context is always a `:question`. The results are normalized and concatenated to the
+ query results for dashboard and table views."
+ [views-limit card-runs-limit all-users?]
+ (let [dashboard-and-table-views (db/select [ViewLog :user_id :model :model_id
+ [:%count.* :cnt] [:%max.timestamp :max_ts]]
+ {:group-by [(db/qualify ViewLog :user_id) :model :model_id]
+ :where [:and
+ (when-not all-users? [:= (db/qualify ViewLog :user_id) *current-user-id*])
+ [:in :model #{"dashboard" "table"}]
+ [:= :bm.id nil]]
+ :order-by [[:max_ts :desc]]
+ :limit views-limit
+ :left-join [[DashboardBookmark :bm]
+ [:and
+ [:not [:= :model "table"]]
+ [:= :bm.user_id *current-user-id*]
+ [:= :model_id :bm.dashboard_id]]]})
+ card-runs (->> (db/select [QueryExecution [:executor_id :user_id] [(db/qualify QueryExecution :card_id) :model_id]
+ [:%count.* :cnt] [:%max.started_at :max_ts]]
+ {:group-by [:executor_id (db/qualify QueryExecution :card_id) :context]
+ :where [:and
+ (when-not all-users? [:= :executor_id *current-user-id*])
+ [:= :context (hx/literal :question)]
+ [:= :bm.id nil]]
+ :order-by [[:max_ts :desc]]
+ :limit card-runs-limit
+ :left-join [[CardBookmark :bm]
+ [:and
+ [:= :bm.user_id *current-user-id*]
+ [:= (db/qualify QueryExecution :card_id) :bm.card_id]]]})
+ (map #(dissoc % :row_count))
+ (map #(assoc % :model "card")))]
+ (->> (concat card-runs dashboard-and-table-views)
+ (sort-by :max_ts)
+ reverse)))
+
+(def ^:private views-limit 8)
+(def ^:private card-runs-limit 8)
+
(defendpoint GET "/recent_views"
- "Get the list of 5 things the current user has been viewing most recently. Query takes 8 and limits to 5 so that if it
- finds anything archived, deleted, etc it can hopefully still get 5."
+ "Get the list of 5 things the current user has been viewing most recently."
[]
- ;; expected output of the query is a single row per unique model viewed by the current user
- ;; including a `:max_ts` which has the most recent view timestamp of the item and `:cnt` which has total views
- ;; and we order the results by most recently viewed then hydrate the basic details of the model
- (let [views (db/select [ViewLog :user_id :model :model_id
- [:%count.* :cnt] [:%max.timestamp :max_ts]]
- {:group-by [:user_id :model :model_id]
- :where [:and
- [:= :user_id *current-user-id*]
- [:in :model #{"card" "dashboard" "table"}]]
- :order-by [[:max_ts :desc]]
- :limit 8})
+ (let [views (views-and-runs views-limit card-runs-limit false)
model->id->items (models-for-views views)]
(->> (for [{:keys [model model_id] :as view-log} views
- :let [model-object (get-in model->id->items [model model_id])]
+ :let [model-object (-> (get-in model->id->items [model model_id])
+ (dissoc :dataset_query))]
:when (and model-object
(mi/can-read? model-object)
;; hidden tables, archived cards/dashboards
(not (or (:archived model-object)
(= (:visibility_type model-object) :hidden))))]
- (cond-> (assoc view-log :model_object (dissoc model-object :dataset_query))
+ (cond-> (assoc view-log :model_object model-object)
(:dataset model-object) (assoc :model "dataset")))
(take 5))))
+(defn- official?
+ "Returns true if the item belongs to an official collection. False otherwise. Assumes that `:authority_level` exists
+ if the item can be placed in a collection."
+ [{:keys [authority_level]}]
+ (boolean
+ (when authority_level
+ (#{"official"} authority_level))))
+
+(defn- verified?
+ "Return true if the item is verified, false otherwise. Assumes that `:moderation_reviews` is hydrated.
+ Assumes that moderation reviews are ordered so that the most recent is the first. This is the case
+ from the hydration function for moderation_reviews."
+ [{:keys [moderation_reviews]}]
+ (boolean
+ (when moderation_reviews
+ (-> moderation_reviews first :status #{"verified"}))))
+
+(defn- score-items
+ [items]
+ (when (seq items)
+ (let [n-items (count items)
+ max-count (apply max (map :cnt items))]
+ (for [[recency-pos {:keys [cnt model_object] :as item}] (zipmap (range) items)]
+ (let [verified-wt 1
+ official-wt 1
+ recency-wt 2
+ views-wt 4
+ scores [;; cards and dashboards? can be 'verified' in enterprise
+ (if (verified? model_object) verified-wt 0)
+ ;; items may exist in an 'official' collection in enterprise
+ (if (official? model_object) official-wt 0)
+ ;; most recent item = 1 * recency-wt, least recent item of 10 items = 1/10 * recency-wt
+ (* (/ (- n-items recency-pos) n-items) recency-wt)
+ ;; item with highest count = 1 * views-wt, lowest = item-view-count / max-view-count * views-wt
+
+ ;; NOTE: the query implementation `views-and-runs` has an order-by clause using most recent timestamp
+ ;; this has an effect on the outcomes. Consider an item with a massively high viewcount but a last view by the user
+ ;; a long time ago. This may not even make it into the firs 10 items from the query, even though it might be worth showing
+ (* (/ cnt max-count) views-wt)]]
+ (assoc item :score (double (reduce + scores))))))))
+
+(defendpoint GET "/popular_items"
+ "Get the list of 5 popular things for the current user. Query takes 8 and limits to 5 so that if it
+ finds anything archived, deleted, etc it can hopefully still get 5."
+ []
+ ;; we can do a weighted score which incorporates:
+ ;; total count -> higher = higher score
+ ;; recently viewed -> more recent = higher score
+ ;; official/verified -> yes = higher score
+ (let [views (views-and-runs views-limit card-runs-limit true)
+ model->id->items (models-for-views views)
+ filtered-views (for [{:keys [model model_id] :as view-log} views
+ :let [model-object (-> (get-in model->id->items [model model_id])
+ (dissoc :dataset_query))]
+ :when (and model-object
+ (mi/can-read? model-object)
+ ;; hidden tables, archived cards/dashboards
+ (not (or (:archived model-object)
+ (= (:visibility_type model-object) :hidden))))]
+ (cond-> (assoc view-log :model_object model-object)
+ (:dataset model-object) (assoc :model "dataset")))
+ scored-views (score-items filtered-views)]
+ (->> scored-views
+ (sort-by :score)
+ reverse
+ (take 5)
+ (map #(dissoc % :score)))))
+
(define-routes)
diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj
index 1ebbc4cbd33..8d2863b5dfe 100644
--- a/src/metabase/api/user.clj
+++ b/src/metabase/api/user.clj
@@ -4,12 +4,14 @@
[clojure.string :as str]
[compojure.core :refer [DELETE GET POST PUT]]
[honeysql.helpers :as hh]
+ [java-time :as t]
[metabase.analytics.snowplow :as snowplow]
[metabase.api.common :as api]
[metabase.email.messages :as email]
[metabase.integrations.google :as google]
[metabase.integrations.ldap :as ldap]
[metabase.models.collection :as collection :refer [Collection]]
+ [metabase.models.login-history :refer [LoginHistory]]
[metabase.models.permissions-group :as group]
[metabase.models.user :as user :refer [User]]
[metabase.plugins.classloader :as classloader]
@@ -161,11 +163,35 @@
user
((resolve 'metabase-enterprise.advanced-permissions.common/with-advanced-permissions) user)))
+(defn- add-has-question-and-dashboard
+ "True when the user has permissions for at least one un-archived question and one un-archived dashboard."
+ [user]
+ (let [coll-ids-filter (collection/visible-collection-ids->honeysql-filter-clause
+ :collection_id
+ (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*))
+ perms-query {:where [:and
+ [:= :archived false]
+ coll-ids-filter]}]
+ (assoc user :has_question_and_dashboard (and (db/exists? 'Card (perms-query user))
+ (db/exists? 'Dashboard (perms-query user))))))
+
+(defn- add-first-login
+ "Adds `first_login` key to the `User` with a timestamp value."
+ [{:keys [user_id] :as user}]
+ (let [ts (or
+ (:timestamp (db/select-one [LoginHistory :timestamp] :user_id user_id
+ {:limit 1
+ :order-by [[:timestamp :desc]]}))
+ (t/offset-date-time))]
+ (assoc user :first_login ts)))
+
(api/defendpoint GET "/current"
"Fetch the current `User`."
[]
(-> (api/check-404 @api/*current-user*)
- (hydrate :personal_collection_id :group_ids :has_invited_second_user)
+ (hydrate :personal_collection_id :group_ids :is_installer :has_invited_second_user)
+ add-has-question-and-dashboard
+ add-first-login
maybe-add-general-permissions))
(api/defendpoint GET "/:id"
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index 09f475aad7a..70c513331dc 100644
--- a/src/metabase/models/user.clj
+++ b/src/metabase/models/user.clj
@@ -143,6 +143,25 @@
(when user-or-id
(db/select-field :group_id PermissionsGroupMembership :user_id (u/the-id user-or-id))))
+
+;;; -------------------------------------------------- Permissions ---------------------------------------------------
+
+(defn permissions-set
+ "Return a set of all permissions object paths that `user-or-id` has been granted access to. (2 DB Calls)"
+ [user-or-id]
+ (set (when-let [user-id (u/the-id user-or-id)]
+ (concat
+ ;; Current User always gets readwrite perms for their Personal Collection and for its descendants! (1 DB Call)
+ (map perms/collection-readwrite-path (collection/user->personal-collection-and-descendant-ids user-or-id))
+ ;; include the other Perms entries for any Group this User is in (1 DB Call)
+ (map :object (db/query {:select [:p.object]
+ :from [[:permissions_group_membership :pgm]]
+ :join [[:permissions_group :pg] [:= :pgm.group_id :pg.id]
+ [:permissions :p] [:= :p.group_id :pg.id]]
+ :where [:= :pgm.user_id user-id]}))))))
+
+;;; --------------------------------------------------- Hydration ----------------------------------------------------
+
(defn add-group-ids
"Efficiently add PermissionsGroup `group_ids` to a collection of `users`."
{:batched-hydrate :group_ids}
@@ -165,6 +184,15 @@
(assoc user :has_invited_second_user (and (= (:id user) 1)
(> user-count 1)))))))
+(defn add-is-installer
+ "Adds the `is_installer` flag to a collection of `users`. This should be `true` for only the user who
+ underwent the initial app setup flow (with an ID of 1). This is used to modify the experience of the
+ starting page for users."
+ {:batched-hydrate :is_installer}
+ [users]
+ (when (seq users)
+ (for [user users]
+ (assoc user :is_installer (= (:id user) 1)))))
;;; --------------------------------------------------- Helper Fns ---------------------------------------------------
@@ -277,20 +305,3 @@
(doseq [group-id to-add]
(db/insert! PermissionsGroupMembership {:user_id user-id, :group_id group-id})))
true)))
-
-
-;;; -------------------------------------------------- Permissions ---------------------------------------------------
-
-(defn permissions-set
- "Return a set of all permissions object paths that `user-or-id` has been granted access to. (2 DB Calls)"
- [user-or-id]
- (set (when-let [user-id (u/the-id user-or-id)]
- (concat
- ;; Current User always gets readwrite perms for their Personal Collection and for its descendants! (1 DB Call)
- (map perms/collection-readwrite-path (collection/user->personal-collection-and-descendant-ids user-or-id))
- ;; include the other Perms entries for any Group this User is in (1 DB Call)
- (map :object (db/query {:select [:p.object]
- :from [[:permissions_group_membership :pgm]]
- :join [[:permissions_group :pg] [:= :pgm.group_id :pg.id]
- [:permissions :p] [:= :p.group_id :pg.id]]
- :where [:= :pgm.user_id user-id]}))))))
diff --git a/test/metabase/api/activity_test.clj b/test/metabase/api/activity_test.clj
index 62b056d48de..09081a6f0b7 100644
--- a/test/metabase/api/activity_test.clj
+++ b/test/metabase/api/activity_test.clj
@@ -7,8 +7,10 @@
[metabase.models.card :refer [Card]]
[metabase.models.dashboard :refer [Dashboard]]
[metabase.models.interface :as models]
+ [metabase.models.query-execution :refer [QueryExecution]]
[metabase.models.table :refer [Table]]
[metabase.models.view-log :refer [ViewLog]]
+ [metabase.query-processor.util :as qputil]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
[metabase.util :as u]
@@ -87,14 +89,25 @@
;; 4. we filter out entries where `:model_object` is nil (object doesn't exist)
(defn- create-views!
- "Insert views [user-id model model-id]. Reviews are entered a second apart with last review as most recent."
+ "Insert views [user-id model model-id]. Views are entered a second apart with last view as most recent."
[views]
- (let [views (map (fn [[user model model-id] hours-ago]
- {:user_id user, :model model, :model_id model-id
- :timestamp (t/plus (t/local-date-time) (t/seconds (- hours-ago)))})
- (reverse views)
- (range))]
- (db/insert-many! ViewLog views)))
+ (let [start-time (t/offset-date-time)
+ views (->> (map (fn [[user model model-id] seconds-ago]
+ (case model
+ "card" {:executor_id user :card_id model-id
+ :context :question
+ :hash (qputil/query-hash {})
+ :running_time 1
+ :result_rows 1
+ :native false
+ :started_at (t/plus start-time (t/seconds (- seconds-ago)))}
+ {:user_id user, :model model, :model_id model-id
+ :timestamp (t/plus start-time (t/seconds (- seconds-ago)))}))
+ (reverse views)
+ (range))
+ (group-by #(if (:card_id %) :card :other)))]
+ (db/insert-many! ViewLog (:other views))
+ (db/insert-many! QueryExecution (:card views))))
(deftest recent-views-test
(mt/with-temp* [Card [card1 {:name "rand-name"
@@ -109,7 +122,7 @@
Dashboard [dash1 {:name "rand-name"
:description "rand-name"
:creator_id (mt/user->id :crowberto)}]
- Table [table1 {:name "rand-name"}]
+ Table [table1 {:name "rand-name"}]
Table [hidden-table {:name "hidden table"
:visibility_type "hidden"}]
Card [dataset {:name "rand-name"
@@ -117,7 +130,7 @@
:creator_id (mt/user->id :crowberto)
:display "table"
:visualization_settings {}}]]
- (mt/with-model-cleanup [ViewLog]
+ (mt/with-model-cleanup [ViewLog QueryExecution]
(create-views! [[(mt/user->id :crowberto) "card" (:id dataset)]
[(mt/user->id :crowberto) "dashboard" (:id dash1)]
[(mt/user->id :crowberto) "card" (:id card1)]
@@ -127,50 +140,49 @@
[(mt/user->id :crowberto) "card" (:id archived)]
[(mt/user->id :crowberto) "table" (:id hidden-table)]
[(mt/user->id :rasta) "card" (:id card1)]])
- (is (= [{:cnt 1,
- :model "table",
- :model_id (:id table1),
- :model_object {:db_id (:db_id table1),
- :id (:id table1),
- :visibility_type nil
- :name (:name table1)
- :display_name (:display_name table1)
- :initial_sync_status "incomplete"},
- :user_id (mt/user->id :crowberto)}
- {:cnt 1
- :user_id (mt/user->id :crowberto)
- :model "card"
- :model_id (:id card1)
- :model_object {:id (:id card1)
- :name (:name card1)
- :archived false
- :collection_id nil
- :dataset false
- :description (:description card1)
- :display (name (:display card1))}}
- {:cnt 1
- :user_id (mt/user->id :crowberto)
- :model "dashboard"
- :model_id (:id dash1)
- :model_object {:id (:id dash1)
- :name (:name dash1)
- :archived false
- :collection_id nil
- :description (:description dash1)}}
- {:cnt 1
- :user_id (mt/user->id :crowberto)
- :model "dataset"
- :model_id (:id dataset)
- :model_object {:id (:id dataset)
- :name (:name dataset)
- :archived false
- :dataset true
- :collection_id nil
- :description (:description dataset)
- :display (name (:display dataset))}}]
+ (is (= [["table" (:id table1)]
+ ["card" (:id card1)]
+ ["dashboard" (:id dash1)]
+ ["dataset" (:id dataset)]]
(for [recent-view (mt/user-http-request :crowberto :get 200 "activity/recent_views")]
- (dissoc recent-view :max_ts)))))))
+ ((juxt :model :model_id) recent-view)))))))
+(deftest popular-items-test
+ (mt/with-temp* [Card [card1 {:name "rand-name"
+ :creator_id (mt/user->id :crowberto)
+ :display "table"
+ :visualization_settings {}}]
+ Card [archived {:name "archived-card"
+ :creator_id (mt/user->id :crowberto)
+ :display "table"
+ :archived true
+ :visualization_settings {}}]
+ Dashboard [dash1 {:name "rand-name"
+ :description "rand-name"
+ :creator_id (mt/user->id :crowberto)}]
+ Table [table1 {:name "rand-name"}]
+ Table [hidden-table {:name "hidden table"
+ :visibility_type "hidden"}]
+ Card [dataset {:name "rand-name"
+ :dataset true
+ :creator_id (mt/user->id :crowberto)
+ :display "table"
+ :visualization_settings {}}]]
+ (mt/with-model-cleanup [ViewLog QueryExecution]
+ (create-views! (concat
+ ;; one item with many views is considered more popular
+ (repeat 10 [(mt/user->id :rasta) "card" (:id dataset)])
+ [[(mt/user->id :rasta) "dashboard" (:id dash1)]
+ [(mt/user->id :rasta) "card" (:id card1)]
+ [(mt/user->id :rasta) "table" (:id table1)]
+ [(mt/user->id :rasta) "card" (:id card1)]]))
+ (is (= [["dataset" (:id dataset)]
+ ["card" (:id card1)]
+ ["table" (:id table1)]
+ ["dashboard" (:id dash1)]]
+ ;; all views are from :rasta, but :crowberto can still see popular items
+ (for [popular-item (mt/user-http-request :crowberto :get 200 "activity/popular_items")]
+ ((juxt :model :model_id) popular-item)))))))
;;; activities->referenced-objects, referenced-objects->existing-objects, add-model-exists-info
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index a4ca247f92a..9a2be40661d 100644
--- a/test/metabase/api/user_test.clj
+++ b/test/metabase/api/user_test.clj
@@ -253,11 +253,12 @@
:common_name "Rasta Toucan"
:group_ids [(u/the-id (group/all-users))]
:personal_collection_id true
+ :is_installer (= 1 (mt/user->id :rasta))
:has_invited_second_user (= 1 (mt/user->id :rasta))})
- (dissoc :is_qbnewb :last_login))
+ (dissoc :is_qbnewb :last_login :has_question_and_dashboard))
(-> (mt/user-http-request :rasta :get 200 "user/current")
mt/boolean-ids-and-timestamps
- (dissoc :is_qbnewb :last_login)))))))
+ (dissoc :is_qbnewb :first_login :last_login :has_question_and_dashboard)))))))
(deftest get-user-test
(testing "GET /api/user/:id"
--
GitLab