From 669adc3a4ff022602df15be2c6e3312b9380daf7 Mon Sep 17 00:00:00 2001
From: adam-james <21064735+adam-james-v@users.noreply.github.com>
Date: Tue, 12 Mar 2024 11:54:52 -0700
Subject: [PATCH] Adam backend triage todo changes (#40022)
* Add issue number to todos in models.card
* Add issue number to todos in models.pulse
* Add issue number to todos in models.dashboard-card
---
src/metabase/models/card.clj | 8 ++++----
src/metabase/models/dashboard_card.clj | 6 +++---
src/metabase/models/pulse.clj | 4 ++--
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index 42056699f62..33616c93950 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -282,7 +282,7 @@
;; TODO: move this to [[metabase.query-processor.card]] or MLv2 so the logic can be shared between the backend and frontend
;; NOTE: this should mirror `getTemplateTagParameters` in frontend/src/metabase-lib/parameters/utils/template-tags.ts
-;; If this function moves you should update the comment that links to this one
+;; If this function moves you should update the comment that links to this one (#40013)
(defn template-tag-parameters
"Transforms native query's `template-tags` into `parameters`.
An older style was to not include `:template-tags` onto cards as parameters. I think this is a mistake and they should always be there. Apparently lots of e2e tests are sloppy about this so this is included as a convenience."
@@ -348,7 +348,7 @@
{:status-code 400})))))
nil)
-;; TODO -- consider whether we should validate the Card query when you save/update it??
+;; TODO -- consider whether we should validate the Card query when you save/update it?? (#40013)
(defn- pre-insert [card]
(let [defaults {:parameters []
:parameter_mappings []}
@@ -357,7 +357,7 @@
;; make sure this Card doesn't have circular source query references
(check-for-circular-source-query-references card)
(check-field-filter-fields-are-from-correct-database card)
- ;; TODO: add a check to see if all id in :parameter_mappings are in :parameters
+ ;; TODO: add a check to see if all id in :parameter_mappings are in :parameters (#40013)
(assert-valid-type card)
(params/assert-valid-parameters card)
(params/assert-valid-parameter-mappings card)
@@ -431,7 +431,7 @@
(defn- pre-update [{archived? :archived, id :id, :as changes}]
;; TODO - don't we need to be doing the same permissions check we do in `pre-insert` if the query gets changed? Or
- ;; does that happen in the `PUT` endpoint?
+ ;; does that happen in the `PUT` endpoint? (#40013)
(u/prog1 changes
(let [;; Fetch old card data if necessary, and share the data between multiple checks.
old-card-info (when (or (contains? changes :type)
diff --git a/src/metabase/models/dashboard_card.clj b/src/metabase/models/dashboard_card.clj
index 14187bd4af0..efdb41880fe 100644
--- a/src/metabase/models/dashboard_card.clj
+++ b/src/metabase/models/dashboard_card.clj
@@ -193,16 +193,16 @@
[:and
[:map-of :keyword :any]
[:map
- ;; TODO -- validate `:target` as well... breaks a few tests tho so those will have to be fixed
+ ;; TODO -- validate `:target` as well... breaks a few tests tho so those will have to be fixed (#40021)
[:parameter_id ms/NonBlankString]
#_[:target :any]]])
(def ^:private NewDashboardCard
- ;; TODO - make the rest of the options explicit instead of just allowing whatever for other keys
+ ;; TODO - make the rest of the options explicit instead of just allowing whatever for other keys (#40021)
[:map
[:dashboard_id ms/PositiveInt]
[:action_id {:optional true} [:maybe ms/PositiveInt]]
- ;; TODO - use ParamMapping. Breaks too many tests right now tho
+ ;; TODO - use ParamMapping. Breaks too many tests right now tho (#40021)
[:parameter_mappings {:optional true} [:maybe [:sequential map?]]]
[:visualization_settings {:optional true} [:maybe map?]]
[:series {:optional true} [:maybe [:sequential ms/PositiveInt]]]])
diff --git a/src/metabase/models/pulse.clj b/src/metabase/models/pulse.clj
index 5df808f0ba9..cafa38622d2 100644
--- a/src/metabase/models/pulse.clj
+++ b/src/metabase/models/pulse.clj
@@ -271,7 +271,7 @@
(dissoc notification :alert_condition :alert_above_goal :alert_first_only))
;; TODO - do we really need this function? Why can't we just use `t2/select` and `hydrate` like we do for everything
-;; else?
+;; else? (#40016)
(mu/defn retrieve-pulse :- [:maybe (mi/InstanceOf Pulse)]
"Fetch a single *Pulse*, and hydrate it with a set of 'standard' hydrations; remove Alert columns, since this is a
*Pulse* and they will all be unset."
@@ -588,7 +588,7 @@
(dissoc :card))
(seq cards) (assoc :cards cards))))
-;; TODO - why do we make sure to strictly validate everything when we create a PULSE but not when we create an ALERT?
+;; TODO - why do we make sure to strictly validate everything when we create a PULSE but not when we create an ALERT? (#40016)
(defn update-alert!
"Updates the given `alert` and returns it"
[alert]
--
GitLab