Truncate Slack attachments (namely dashboard filters) that are too long (#25753)

[Fixes #25704]

src/metabase/pulse.clj

@@ -115,16 +115,16 @@
     :below (trs "gone below its goal")
     :rows  (trs "results")))
 
-(def ^:private ^:dynamic *slack-mrkdwn-length-limit*
-  3000)
+(def ^:private block-text-length-limit 3000)
+(def ^:private attachment-text-length-limit 2000)
 
 (defn- truncate-mrkdwn
   "If a mrkdwn string is greater than Slack's length limit, truncates it to fit the limit and
   adds an ellipsis character to the end."
-  [mrkdwn]
-  (if (> (count mrkdwn) *slack-mrkdwn-length-limit*)
+  [mrkdwn limit]
+  (if (> (count mrkdwn) limit)
     (-> mrkdwn
-        (subs 0 (dec *slack-mrkdwn-length-limit*))
+        (subs 0 (dec limit))
         (str "…"))
     mrkdwn))
 
@@ -146,7 +146,7 @@
                  (when (not (str/blank? mrkdwn))
                    {:blocks [{:type "section"
                               :text {:type "mrkdwn"
-                                     :text (truncate-mrkdwn mrkdwn)}}]})))))
+                                     :text (truncate-mrkdwn mrkdwn block-text-length-limit)}}]})))))
          (remove nil?))))
 
 (defn- subject
@@ -156,6 +156,12 @@
     name
     (trs "Pulse: {0}" name)))
 
+(defn- filter-text
+  [filter]
+  (truncate-mrkdwn
+   (format "*%s*\n%s" (:name filter) (params/value-string filter))
+   attachment-text-length-limit))
+
 (defn- slack-dashboard-header
   "Returns a block element that includes a dashboard's name, creator, and filters, for inclusion in a
   Slack dashboard subscription"
@@ -170,7 +176,7 @@
         filters         (params/parameters pulse dashboard)
         filter-fields   (for [filter filters]
                           {:type "mrkdwn"
-                           :text (str "*" (:name filter) "*\n" (params/value-string filter))})
+                           :text (filter-text filter)})
         filter-section  (when (seq filter-fields)
                           {:type   "section"
                            :fields filter-fields})]

test/metabase/dashboard_subscription_test.clj

@@ -289,70 +289,71 @@
                   (pulse.test-util/thunk->boolean pulse-results)))))}}))
 
 (deftest dashboard-filter-test
-  (tests {:pulse     {:skip_if_empty false}
-          :dashboard pulse.test-util/test-dashboard}
-    "Dashboard subscription that includes a dashboard filters"
-    {:card (pulse.test-util/checkins-query-card {})
-
-     :fixture
-     (fn [_ thunk]
-       (mt/with-temporary-setting-values [site-name "Metabase Test"]
-          (thunk)))
-
-     :assert
-     {:email
-       (fn [_ _]
-         (testing "Markdown cards are included in email subscriptions"
-           (is (= (rasta-pulse-email {:body [{"Aviary KPIs" true
-                                              "<a class=\\\"title\\\" href=\\\"https://metabase.com/testmb/dashboard/\\d+\\?state=CA&amp;state=NY&amp;state=NJ&amp;quarter_and_year=Q1-2021\\\"" true}
-                                             pulse.test-util/png-attachment]})
-                  (mt/summarize-multipart-email #"Aviary KPIs"
-                                                #"<a class=\"title\" href=\"https://metabase.com/testmb/dashboard/\d+\?state=CA&amp;state=NY&amp;state=NJ&amp;quarter_and_year=Q1-2021\"")))))
-
-      :slack
-      (fn [{:keys [card-id dashboard-id]} [pulse-results]]
-        (testing "Markdown cards are included in attachments list as :blocks sublists, and markdown is
-                  converted to mrkdwn (Slack markup language)"
-          (is (= {:channel-id "#general"
-                  :attachments
-                  [{:blocks [{:type "header", :text {:type "plain_text", :text "Aviary KPIs", :emoji true}}
-                             {:type "section",
-                              :fields [{:type "mrkdwn", :text "*State*\nCA, NY and NJ"}
-                                       {:type "mrkdwn", :text "*Quarter and Year*\nQ1, 2021"}]}
-                             {:type "section", :fields [{:type "mrkdwn", :text "Sent by Rasta Toucan"}]}]}
-                   {:title           pulse.test-util/card-name
-                    :rendered-info   {:attachments false, :content true, :render/text true},
-                    :title_link      (str "https://metabase.com/testmb/question/" card-id)
-                    :attachment-name "image.png"
-                    :channel-id      "FOO"
-                    :fallback        pulse.test-util/card-name}
-                   {:blocks [{:type "divider"}
-                             {:type "context"
-                              :elements [{:type "mrkdwn"
-                                          :text (str "<https://metabase.com/testmb/dashboard/"
-                                                     dashboard-id
-                                                     "?state=CA&state=NY&state=NJ&quarter_and_year=Q1-2021|*Sent from Metabase Test*>")}]}]}]}
-                 (pulse.test-util/thunk->boolean pulse-results)))))}}))
+  (with-redefs [metabase.pulse/attachment-text-length-limit 15]
+    (tests {:pulse     {:skip_if_empty false}
+            :dashboard pulse.test-util/test-dashboard}
+      "Dashboard subscription that includes a dashboard filters"
+      {:card (pulse.test-util/checkins-query-card {})
+
+       :fixture
+       (fn [_ thunk]
+         (mt/with-temporary-setting-values [site-name "Metabase Test"]
+           (thunk)))
+
+       :assert
+       {:email
+        (fn [_ _]
+          (testing "Markdown cards are included in email subscriptions"
+            (is (= (rasta-pulse-email {:body [{"Aviary KPIs" true
+                                               "<a class=\\\"title\\\" href=\\\"https://metabase.com/testmb/dashboard/\\d+\\?state=CA&amp;state=NY&amp;state=NJ&amp;quarter_and_year=Q1-2021\\\"" true}
+                                              pulse.test-util/png-attachment]})
+                   (mt/summarize-multipart-email #"Aviary KPIs"
+                                                 #"<a class=\"title\" href=\"https://metabase.com/testmb/dashboard/\d+\?state=CA&amp;state=NY&amp;state=NJ&amp;quarter_and_year=Q1-2021\"")))))
+
+        :slack
+        (fn [{:keys [card-id dashboard-id]} [pulse-results]]
+          (testing "Markdown cards are included in attachments list as :blocks sublists, and markdown is
+                  converted to mrkdwn (Slack markup language) and truncated appropriately"
+            (is (= {:channel-id "#general"
+                    :attachments
+                    [{:blocks [{:type "header", :text {:type "plain_text", :text "Aviary KPIs", :emoji true}}
+                               {:type "section",
+                                :fields [{:type "mrkdwn", :text "*State*\nCA, NY…"}  ;; "*State*\nCA, NY and NJ"
+                                         {:type "mrkdwn", :text "*Quarter and Y…"}]} ;; "*Quarter and Year*\nQ1, 2021"
+                               {:type "section", :fields [{:type "mrkdwn", :text "Sent by Rasta Toucan"}]}]}
+                     {:title           pulse.test-util/card-name
+                      :rendered-info   {:attachments false, :content true, :render/text true},
+                      :title_link      (str "https://metabase.com/testmb/question/" card-id)
+                      :attachment-name "image.png"
+                      :channel-id      "FOO"
+                      :fallback        pulse.test-util/card-name}
+                     {:blocks [{:type "divider"}
+                               {:type "context"
+                                :elements [{:type "mrkdwn"
+                                            :text (str "<https://metabase.com/testmb/dashboard/"
+                                                       dashboard-id
+                                                       "?state=CA&state=NY&state=NJ&quarter_and_year=Q1-2021|*Sent from Metabase Test*>")}]}]}]}
+                   (pulse.test-util/thunk->boolean pulse-results)))))}})))
 
 (deftest mrkdwn-length-limit-test
-  (tests {:pulse {:skip_if_empty false}, :dashcard {:row 0, :col 0}}
-    "Dashboard subscription that includes a Markdown card that exceeds Slack's length limit when converted to mrkdwn"
-    {:card (pulse.test-util/checkins-query-card {})
-
-     :fixture
-     (fn [{dashboard-id :dashboard-id} thunk]
-       (mt/with-temp DashboardCard [_ {:dashboard_id dashboard-id
-                                       :row 1
-                                       :col 1
-                                       :visualization_settings {:text "abcdefghijklmnopqrstuvwxyz"}}]
-         (binding [metabase.pulse/*slack-mrkdwn-length-limit* 10]
-           (thunk))))
-
-     :assert
-     {:slack
-      (fn [_object-ids [pulse-results]]
-        (is (= {:blocks [{:type "section" :text {:type "mrkdwn" :text "abcdefghi…"}}]}
-               (nth (:attachments (pulse.test-util/thunk->boolean pulse-results)) 2))))}}))
+  (with-redefs [metabase.pulse/block-text-length-limit 10]
+    (tests {:pulse {:skip_if_empty false}, :dashcard {:row 0, :col 0}}
+           "Dashboard subscription that includes a Markdown card that exceeds Slack's length limit when converted to mrkdwn"
+           {:card (pulse.test-util/checkins-query-card {})
+
+            :fixture
+            (fn [{dashboard-id :dashboard-id} thunk]
+              (mt/with-temp DashboardCard [_ {:dashboard_id dashboard-id
+                                              :row 1
+                                              :col 1
+                                              :visualization_settings {:text "abcdefghijklmnopqrstuvwxyz"}}]
+                (thunk)))
+
+            :assert
+            {:slack
+             (fn [_object-ids [pulse-results]]
+               (is (= {:blocks [{:type "section" :text {:type "mrkdwn" :text "abcdefghi…"}}]}
+                      (nth (:attachments (pulse.test-util/thunk->boolean pulse-results)) 2))))}})))
 
 (deftest archived-dashboard-test
   (tests {:dashboard {:archived true}}