New Recents API Format

[escherize]

resolves #41957

The way this works is by querying recent_views for the last 20 of each sort of model. This query joins with report_card so we can differentiate questions (cards) from datasets (models).

Then each one is passed through post-process, a multimethod that dispatches on the model (with a distinction between cards and datasets) which does some classification, and adds details to each RecentViews/Item.

Tasks

• update return from /activity/recent_views
• track models as distinct from cards
• update recents to preserve inside category (not just last 30 whatevers, but 20 cards, dashboards, models, collections, and tables)
• add collection events for:
  • collection creation,
  • moving card,
  • dash,
  • collection)
• make sure we are pruning recent_views properly
• update /popular_items return shape to match recent_views
• tests
• return moderated_status on cards, models, (and maybe dashboards)
• filter out archived / inactive values
• add description to all models
• more tests

[dpsutton]

Some comments:

A nice ns docstring:

(ns metabase.models.recent-views
  "The Recent Views table is used to track the most recent views of objects such as Cards, Models, Tables, Dashboards,
  and Collections for each user. It offers a simple api to add a recent, and fetch the list of recents.

  Fetch recent items: `(recent-view/get-list <user-id>)`
  add recent item: `(recent-views/update-users-recent-views! <user-id> <model> <model-id>)`

  When adding a recent item, duplicates will be removed, and [[*recent-views-stored-per-user-per-model*]] (20
  currently) are kept of each entity type. Ie, if you were to view lots of cards, it would not push collections and
  dashboards out of your recents."
,,,)

(note, enumerating the API makes metabase.models.recent-views/user-recent-views an awkward function to have around)

distinct ids

I see you're doing most of this in SQL. Want to throw out perhaps an easier impl.

recent-views=> (->> (set/index (t2/select :model/RecentViews
                                          :user_id 1
                                          {:order-by [[:timestamp :desc]]})
                               [:model :model_id])
                    (mapcat (comp next val))
                    (map :id))
(40
 61
 48
 49
,,,)

I think mine is easier to read. And since we always load this all in memory, felt appropriate. Up to you to accept or decline the change.

Issue in spec

@@ -212,7 +228,7 @@
      :model :table
      :can_write (mi/can-write? :model/Table model_id)
      :timestamp (str timestamp)
-     :database {:id (:database-id table)
+     :database {:id (:db_id table)
                 :name (:name table)}}))
 

Tables have a db_id not a database-id. And this is causing

[iethree]

I'm getting this error:

{
    "cause": "Invalid output: [{:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} nil nil {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} nil nil nil nil {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}}]",
    "via": [
        {
            "type": "clojure.lang.ExceptionInfo",
            "message": "Invalid output: [{:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:database {:id [\"should be an integer, got: nil\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} nil nil {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} nil nil nil nil {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}} {:parent_collection {:authority_level [\"should be either official or , got: :official\"]}}]",
            "data": {...}
}

[replay-io]

Status	Complete ↗︎
Commit	94b7f0a
Results	⚠️ 6 Flaky ✅ 2502 Passed

[noahmoss]

nit - might be a bit more readable to break this out into two separate maps, instead of nested threading macros? and/or to use a transducer for the whole pipeline

[escherize]

I landed here because I found the transducer style to be harder to read, though it does have the memory pressure benefit. But then in the other direction: for doing 2 maps we'd be creating another intermediate sequence.

[noahmoss]

Would it make sense for the impls of this multimethod to live in the model namespaces for each model? That way each model is responsible for what information is included in recent views, rather than blowing up this namespace with all the implementations. That's the pattern I followed with audit-log/model-details for instance.

Don't need to block on this, just a suggestion but it could go either way.

[escherize]

Glad you asked! I thought about that too, but I figured that I wanted these methods to be seen from the POV of a recent view item, instead of a model. Part of the reason for that is the impedence mismatch between :model/Card being both questions and models (and metrics etc if we ever add those?). So I thought it was more simple to handle things as they are on the api side, instead of the way they are on our model/instance side.

[github-actions]

@escherize Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?