Merge pull request #8844 from metabase/no-csv-default-when-xls-is-specified

Only include a CSV attachment for pulses when XLS is not selected

src/metabase/pulse/render.clj

@@ -160,9 +160,10 @@
 
 (defn include-csv-attachment?
   "Returns true if this card and resultset should include a CSV attachment"
-  [{:keys [include_csv] :as card} {:keys [cols rows] :as result-data}]
+  [card {:keys [cols rows] :as result-data}]
   (or (:include_csv card)
-      (and (= :table (detect-pulse-card-type card result-data))
+      (and (not (:include_xls card))
+           (= :table (detect-pulse-card-type card result-data))
            (or
             ;; If some columns are not shown, include an attachment
             (some (complement show-in-table?) cols)

test/metabase/pulse_test.clj

@@ -764,6 +764,53 @@
      (send-pulse! (retrieve-notification pulse-id))
      (et/summarize-multipart-email test-card-regex))))
 
+;; With a "rows" type of pulse (table visualization) we should include the CSV by default
+(expect
+  (-> (rasta-pulse-email)
+      ;; There's no PNG with a table visualization, remove it from the expected results
+      (update-in ["rasta@metabase.com" 0 :body] (comp vector first))
+      (add-rasta-attachment csv-attachment))
+
+  (tt/with-temp* [Card                 [{card-id :id}    {:name          card-name
+                                                          :dataset_query {:database (data/id)
+                                                                          :type     :query
+                                                                          :query    {:source-table (data/id :checkins)}}}]
+                  Pulse                [{pulse-id :id} {:name          "Pulse Name"
+                                                        :skip_if_empty false}]
+                  PulseCard             [_             {:pulse_id    pulse-id
+                                                        :card_id     card-id
+                                                        :position    0}]
+                  PulseChannel          [{pc-id :id}   {:pulse_id pulse-id}]
+                  PulseChannelRecipient [_             {:user_id          (rasta-id)
+                                                        :pulse_channel_id pc-id}]]
+    (email-test-setup
+     (send-pulse! (retrieve-pulse pulse-id))
+     (et/summarize-multipart-email #"Pulse Name"))))
+
+;; If the pulse is already configured to send an XLS, no need to include a CSV
+(expect
+  (-> (rasta-pulse-email)
+      ;; There's no PNG with a table visualization, remove it from the expected results
+      (update-in ["rasta@metabase.com" 0 :body] (comp vector first))
+      (add-rasta-attachment xls-attachment))
+
+  (tt/with-temp* [Card                 [{card-id :id}    {:name          card-name
+                                                          :dataset_query {:database (data/id)
+                                                                          :type     :query
+                                                                          :query    {:source-table (data/id :checkins)}}}]
+                  Pulse                [{pulse-id :id} {:name          "Pulse Name"
+                                                        :skip_if_empty false}]
+                  PulseCard             [_             {:pulse_id    pulse-id
+                                                        :card_id     card-id
+                                                        :position    0
+                                                        :include_xls true}]
+                  PulseChannel          [{pc-id :id}   {:pulse_id pulse-id}]
+                  PulseChannelRecipient [_             {:user_id          (rasta-id)
+                                                        :pulse_channel_id pc-id}]]
+    (email-test-setup
+     (send-pulse! (retrieve-pulse pulse-id))
+     (et/summarize-multipart-email #"Pulse Name"))))
+
 ;; Basic test of card with CSV and XLS attachments, but no data. Should not include an attachment
 (expect
   (rasta-pulse-email)