Automagic dashboards #7048
Automagic dashboards #7048
Conversation
…abase-init into automagic-dashboards-stage1
| ["dimension" "Foo"] | ||
| ["DIMENSION" "Foo"] | ||
| 42 | ||
| [:baz :bar]])) |
There was a problem hiding this comment.
can you just make these all separate tests? Why combine them all into one test?
| (expect | ||
| [true | ||
| false] | ||
| (map ga-dimension? ["ga:foo" "foo"])) |
There was a problem hiding this comment.
I'm not really sure what's going on in this namespace -- what's the reasoning behind trying to combine a bunch of tests that anywhere else in the codebase would be separate into one single test? I just don't understand why
test/metabase/util_test.clj
Outdated
| [{:foo 2} | ||
| {:foo {:bar 3}}] | ||
| [(update-in-when {:foo 2} [:foo :bar] inc) | ||
| (update-in-when {:foo {:bar 2}} [:foo :bar] inc)]) |
There was a problem hiding this comment.
multiple test combined into one :(
| (s/defn ^:private save-field-updates! | ||
| (def ^:private FieldOrTableInstance (s/either i/FieldInstance i/TableInstance)) | ||
|
|
||
| (s/defn ^:private save-model-updates! | ||
| "Save the updates in UPDATED-FIELD." |
There was a problem hiding this comment.
If you're going to change the function make sure you change the docstring
| (log/debug (format "Based on classification, updating these values of %s: %s" | ||
| (sync-util/name-for-logging original-field) | ||
| (sync-util/name-for-logging original-model) | ||
| values-to-set)) | ||
| ;; Check that we're not trying to set anything that we're not allowed to | ||
| (doseq [k (keys values-to-set)] | ||
| (when-not (contains? values-that-can-be-set k) | ||
| (throw (Exception. (format "Classifiers are not allowed to set the value of %s." k))))) | ||
| ;; cool, now we should be ok to update the Field |
There was a problem hiding this comment.
make sure you change the comments too
| [#"vendor" :entity/CompanyTable]]) | ||
|
|
||
| (s/defn ^:always-validate infer-entity-type :- i/TableInstance | ||
| [table :- i/TableInstance] |
There was a problem hiding this comment.
s/defn generates basic docstrings which is why the linter didn't complain but it would still be nice to have a brief blurb explaining what exactly is going on here
|
This looks good to me except for:
|
| @@ -44,6 +44,16 @@ const FieldSidebar = ({ | |||
| icon="document" | |||
| name={t`Details`} | |||
| /> | |||
|
|
|||
| { | |||
There was a problem hiding this comment.
Any reason to keep this around?
There was a problem hiding this comment.
left it commented out while we wait on the endpoint being done.
| :visualization_settings {} | ||
| :id (gensym)})))))) | ||
|
|
||
| (def ^:private ^Long title-height 2) |
| nil)) | ||
|
|
||
| (defmethod automagic-analysis (type Field) | ||
| [field] |
There was a problem hiding this comment.
A comment around why this is empty would be good, maybe a nil too if that's what is intended
| ([context bindings filters metrics dimensions limit order_by] | ||
| (walk/postwalk | ||
| (fn [subform] | ||
| (if (rules/dimension-form? subform) |
There was a problem hiding this comment.
Where are you expecting to find dimension-forms? Seems like it will be traversing a lot of stuff that couldn't possibly be a dimension form (i.e. :type and :query and [:type :query])
There was a problem hiding this comment.
Anywhere in :query so it's just 2 keys, :type and :database which doesn't fell like that much.
| true | ||
| false | ||
| false] | ||
| (map (comp boolean #'rules/dimension-form?) [[:dimension "Foo"] |
There was a problem hiding this comment.
This function is no longer private
| :fk_target_field_id [:not= nil] | ||
| :table_id from-table) | ||
| (filter (comp #{(:table_id to-field)} :table_id Field :fk_target_field_id)) | ||
| (map :id))) |
There was a problem hiding this comment.
I'm intentionally using :id as it will always be an object. Unless we have a convention that we always prefer u/get-id
| (let [filter-id (-> candidate hash str) | ||
| dashcards (:ordered_cards dashboard) | ||
| dashcards-new (map #(add-filter % filter-id candidate) dashcards)] | ||
| (cond-> dashboard |
There was a problem hiding this comment.
dashboard -*> dashcards -*> series -*> fk fields and each fk field will cause a separate Fields query to fire. Any idea how many we're talking here? We've been battling n+1 issues lately
There was a problem hiding this comment.
It sucks. I'll rework it so it's one query per filter.
| (defn- filter-type | ||
| [{:keys [base_type special_type]}] | ||
| (cond | ||
| (isa? base_type :type/DateTime) "date/all-options" |
There was a problem hiding this comment.
Similar question as above, no unix timestamps?
| (def ^Long default-card-width | ||
| "Default card width." | ||
| 6) | ||
| (def ^Long default-card-height |
|
|
||
| (defn- create-collection! | ||
| [title color description] | ||
| (when api/*is-superuser?* |
There was a problem hiding this comment.
What's the intended behavior of non superusers here? Does everything have to deal with possible nil values from this?
There was a problem hiding this comment.
Permissions aren't really thought out yet. But in this case it works ok, as :collection_id = nil is the default anyway.
There was a problem hiding this comment.
I'm not sure I'd characterize users spewing cards into the everything questions space as working ok ;)
…abase into automagic-dashboards-stage1
This supersedes #6455, #6840, #6853, #6887, #6786, and #6978.
Automagic filters are no longer exposed via api, but only used internally. Added the option of explicitly defining which filters to have in dashboard heuristics (dashboard_filter field).
Things that need to be fixed before merging