diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj
index 1dfc56e0856d666d965c217a6771de75465b444d..1479d56a4d742305805f4759b7ad389929c5438c 100644
--- a/src/metabase/api/pulse.clj
+++ b/src/metabase/api/pulse.clj
@@ -167,7 +167,9 @@
(future (slack/refresh-channels-and-usernames-when-needed!))
(assoc-in chan-types
[:slack :fields 0 :options]
- (slack/slack-cached-channels-and-usernames))
+ (->> (slack/slack-cached-channels-and-usernames)
+ :channels
+ (map :display-name)))
(catch Throwable e
(assoc-in chan-types [:slack :error] (.getMessage e)))))}))
diff --git a/src/metabase/integrations/slack.clj b/src/metabase/integrations/slack.clj
index 57da89b21bc424f75cf267ff90409f00805d1d3f..f242c242faa6c0f6d3a151ac8d19ce52a554cd26 100644
--- a/src/metabase/integrations/slack.clj
+++ b/src/metabase/integrations/slack.clj
@@ -163,6 +163,14 @@
(lazy-seq
(paged-list-request endpoint response->data (assoc params :cursor next-cursor)))))))))
+(defn channel-transform
+ "Transformation from slack's api representation of a channel to our own."
+ [channel]
+ {:display-name (str \# (:name channel))
+ :name (:name channel)
+ :id (:id channel)
+ :type "channel"})
+
(defn conversations-list
"Calls Slack API `conversations.list` and returns list of available 'conversations' (channels and direct messages).
By default only fetches channels, and returns them with their # prefix. Note the call to [[paged-list-request]] will
@@ -171,19 +179,16 @@
(let [params (merge {:exclude_archived true, :types "public_channel"} query-parameters)]
(paged-list-request "conversations.list"
;; response -> channel names
- #(->> %
- :channels
- (map (fn [channel]
- (str \# (:name channel)))))
+ #(->> % :channels (map channel-transform))
params)))
(defn channel-exists?
"Returns true if the channel it exists."
[channel-name]
- (and channel-name
- (contains?
- (set (slack-cached-channels-and-usernames))
- (str \# channel-name))))
+ (let [channel-names (into #{} (comp (map (juxt :name :id))
+ cat)
+ (:channels (slack-cached-channels-and-usernames)))]
+ (and channel-name (contains? channel-names channel-name))))
(s/defn valid-token?
"Check whether a Slack token is valid by checking if the `conversations.list` Slack api accepts it."
@@ -196,16 +201,21 @@
false
(throw e)))))
+(defn user-transform
+ "Tranformation from slack api user to our own internal representation."
+ [member]
+ {:display-name (str \@ (:name member))
+ :type "user"
+ :name (:name member)
+ :id (:id member)})
+
(defn users-list
"Calls Slack API `users.list` endpoint and returns the list of available users with their @ prefix. Note the call
to [[paged-list-request]] will only fetch the first [[max-list-results]] items."
[& {:as query-parameters}]
(->> (paged-list-request "users.list"
;; response -> user names
- #(->> %
- :members
- (map (fn [member]
- (str \@ (:name member)))))
+ #(->> % :members (map user-transform))
query-parameters)
;; remove deleted users and bots. At the time of this writing there's no way to do this in the Slack API
;; itself so we need to do it after the fact.
@@ -223,7 +233,7 @@
"Clear the Slack channels cache, and reset its last-updated timestamp to its default value (the Unix epoch)."
[]
(slack-channels-and-usernames-last-updated! zoned-time-epoch)
- (slack-cached-channels-and-usernames! []))
+ (slack-cached-channels-and-usernames! {:channels []}))
(defn refresh-channels-and-usernames!
"Refreshes users and conversations in slack-cache. finds both in parallel, sets
@@ -233,7 +243,7 @@
(log/info "Refreshing slack channels and usernames.")
(let [users (future (vec (users-list)))
conversations (future (vec (conversations-list)))]
- (slack-cached-channels-and-usernames! (concat @conversations @users))
+ (slack-cached-channels-and-usernames! {:channels (concat @conversations @users)})
(slack-channels-and-usernames-last-updated! (t/zoned-date-time)))))
(defn refresh-channels-and-usernames-when-needed!
@@ -272,6 +282,18 @@
[channel-id :- su/NonBlankString]
(POST "conversations.join" {:form-params {:channel channel-id}}))
+(defn- maybe-lookup-id
+ "Slack requires the slack app to be in the channel that we post all of our attachments to. Slack changed (around June
+ 2022 #23229) the \"conversations.join\" api to require the internal slack id rather than the common name. This makes
+ a lot of sense to ensure we continue to operate despite channel renames. Attempt to look up the channel-id in the
+ list of channels to obtain the internal id. Fallback to using the current channel-id."
+ [channel-id cached-channels]
+ (let [name->id (into {} (comp (filter (comp #{"channel"} :type))
+ (map (juxt :name :id)))
+ (:channels cached-channels))
+ channel-id' (get name->id channel-id channel-id)]
+ channel-id'))
+
(s/defn upload-file!
"Calls Slack API `files.upload` endpoint and returns the URL of the uploaded file."
[file :- NonEmptyByteArray, filename :- su/NonBlankString, channel-id :- su/NonBlankString]
@@ -285,7 +307,9 @@
;; If file upload fails with a "not_in_channel" error, we join the channel and try again.
;; This is expected to happen the first time a Slack subscription is sent.
(if (= "not_in_channel" (:error-code (ex-data e)))
- (do (join-channel! channel-id)
+ (do (-> channel-id
+ (maybe-lookup-id (slack-cached-channels-and-usernames))
+ join-channel!)
(POST "files.upload" request))
(throw e))))]
(u/prog1 (get-in response [:file :url_private])
diff --git a/src/metabase/task/refresh_slack_channel_user_cache.clj b/src/metabase/task/refresh_slack_channel_user_cache.clj
index abbef1e1554ad891236ab6ca17c8225f1a72c060..21259b68c996fc264ac84551244d887bcd019a41 100644
--- a/src/metabase/task/refresh_slack_channel_user_cache.clj
+++ b/src/metabase/task/refresh_slack_channel_user_cache.clj
@@ -14,7 +14,7 @@
start-ms (System/currentTimeMillis)
_ (slack/refresh-channels-and-usernames!)]
(log/info (trs "Slack user/channel startup cache refreshed with {0} entries, took {1}ms."
- (count (slack/slack-cached-channels-and-usernames))
+ (count (:channels (slack/slack-cached-channels-and-usernames)))
(- (System/currentTimeMillis) start-ms))))
(log/info (trs "Slack is not configured, not refreshing slack user/channel cache."))))
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index 8a97e676095bd0e4b956bd2348a2fae678cd4e8f..61d0836111857e9602cc18ffd8701824008f1e04 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -972,17 +972,28 @@
(deftest form-input-test
(testing "GET /api/pulse/form_input"
(testing "Check that Slack channels come back when configured"
- (mt/with-temporary-setting-values [slack-token nil
- slack-app-token "something"]
- (with-redefs [slack/conversations-list (constantly ["#foo" "#two" "#general"])
- slack/users-list (constantly ["@bar" "@baz"])]
- ;; set the cache to these values
- (slack/slack-cached-channels-and-usernames! (concat (slack/conversations-list) (slack/users-list)))
- ;; don't let the cache refresh itself (it will refetch if it is too old)
- (slack/slack-channels-and-usernames-last-updated! (t/zoned-date-time))
- (is (= [{:name "channel", :type "select", :displayName "Post to", :options ["#foo" "#two" "#general" "@bar" "@baz"], :required true}]
- (-> (mt/user-http-request :rasta :get 200 "pulse/form_input")
- (get-in [:channels :slack :fields])))))))
+ (mt/with-temporary-setting-values [slack/slack-channels-and-usernames-last-updated
+ (t/zoned-date-time)
+
+ slack/slack-app-token "test-token"
+
+ slack/slack-cached-channels-and-usernames
+ {:channels [{:type "channel"
+ :name "foo"
+ :display-name "#foo"
+ :id "CAAS3DD9XND"}
+ {:type "channel"
+ :name "general"
+ :display-name "#general"
+ :id "C3MJRZ9EUVA"}
+ {:type "user"
+ :name "user1"
+ :id "U1DYU9W3WZ2"
+ :display-name "@user1"}]}]
+ (is (= [{:name "channel", :type "select", :displayName "Post to",
+ :options ["#foo" "#general" "@user1"], :required true}]
+ (-> (mt/user-http-request :rasta :get 200 "pulse/form_input")
+ (get-in [:channels :slack :fields]))))))
(testing "When slack is not configured, `form_input` returns no channels"
(mt/with-temporary-setting-values [slack-token nil
diff --git a/test/metabase/api/slack_test.clj b/test/metabase/api/slack_test.clj
index adb38a34a84b8b92dd01262876577bca149d5d30..725d9e3bc74d5f125899ea4a0f63ff94683b3e85 100644
--- a/test/metabase/api/slack_test.clj
+++ b/test/metabase/api/slack_test.clj
@@ -10,9 +10,9 @@
(testing "PUT /api/slack/settings"
(testing "An admin can set a valid Slack app token to the slack-app-token setting, and any value in the
`slack-token` setting is cleared"
- (with-redefs [slack/valid-token? (constantly true)
- slack/channel-exists? (constantly true)
- slack/refresh-channels-and-usernames! (constantly nil)
+ (with-redefs [slack/valid-token? (constantly true)
+ slack/channel-exists? (constantly true)
+ slack/refresh-channels-and-usernames! (constantly nil)
slack/refresh-channels-and-usernames-when-needed! (constantly nil)]
(mt/with-temporary-setting-values [slack-app-token nil
slack-token "fake-token"]
@@ -22,16 +22,17 @@
(testing "A 400 error is returned if the Slack app token is invalid"
(mt/with-temporary-setting-values [slack-app-token nil]
- (with-redefs [slack/valid-token? (constantly false)
+ (with-redefs [slack/valid-token? (constantly false)
;; Token validation is skipped by default in test environments; overriding `is-test?` ensures
;; that validation occurs
- config/is-test? false
- slack/refresh-channels-and-usernames! (constantly nil)
+ config/is-test? false
+ slack/refresh-channels-and-usernames! (constantly nil)
slack/refresh-channels-and-usernames-when-needed! (constantly nil)]
(let [response (mt/user-http-request :crowberto :put 400 "slack/settings" {:slack-app-token "fake-token"})]
(is (= {:slack-app-token "invalid token"} (:errors response)))
(is (= nil (slack/slack-app-token)))
- (is (= [] (slack/slack-cached-channels-and-usernames)))))))
+ (is (= {:channels []}
+ (slack/slack-cached-channels-and-usernames)))))))
(testing "The Slack files channel setting can be set by an admin, and the leading # is stripped if it is present"
(mt/with-temporary-setting-values [slack-files-channel nil
@@ -59,7 +60,8 @@
;; The files channel is reset to its default value
(is (= "metabase_files" (slack/slack-files-channel)))
;; The cache is empty, and its last-updated value is reset to its default value
- (is (= [] (slack/slack-cached-channels-and-usernames)))
+ (is (= {:channels []}
+ (slack/slack-cached-channels-and-usernames)))
(is (= @#'slack/zoned-time-epoch (slack/slack-channels-and-usernames-last-updated)))))
diff --git a/test/metabase/integrations/slack_test.clj b/test/metabase/integrations/slack_test.clj
index 5ffb4fd82f2109b341d1ff2a94ca1da4e82ffd1f..6e221f30d1b160a82aa8906fc67afe17fae61e74 100644
--- a/test/metabase/integrations/slack_test.clj
+++ b/test/metabase/integrations/slack_test.clj
@@ -1,7 +1,6 @@
(ns metabase.integrations.slack-test
(:require [cheshire.core :as json]
[clj-http.fake :as http-fake]
- [clojure.java.io :as io]
[clojure.test :refer :all]
[medley.core :as m]
[metabase.email.messages :as messages]
@@ -10,7 +9,6 @@
[metabase.test.util :as tu]
[schema.core :as s])
(:import java.nio.charset.Charset
- org.apache.commons.io.IOUtils
org.apache.http.client.utils.URLEncodedUtils
org.apache.http.NameValuePair))
@@ -83,9 +81,8 @@
(testing "should be able to fetch channels and paginate"
(http-fake/with-fake-routes {conversations-endpoint (comp mock-200-response mock-conversations-response-body)}
- (let [expected-result (map
- (comp #(str \# %) :name)
- (concat (mock-conversations) (mock-conversations)))]
+ (let [expected-result (map slack/channel-transform
+ (concat (mock-conversations) (mock-conversations)))]
(tu/with-temporary-setting-values [slack-token "test-token"
slack-app-token nil]
(is (= expected-result
@@ -136,9 +133,8 @@
(testing "should be able to fetch list of users and page"
(http-fake/with-fake-routes {users-endpoint (comp mock-200-response mock-users-response-body)}
- (let [expected-result (map
- (comp #(str \@ %) :name)
- (concat (mock-users) (mock-users)))]
+ (let [expected-result (map slack/user-transform
+ (concat (mock-users) (mock-users)))]
(tu/with-temporary-setting-values [slack-token nil
slack-app-token "test-token"]
(is (= expected-result
@@ -152,10 +148,14 @@
(testing "files-channel"
(testing "Should be able to get the files-channel from the cache (if it exists)"
(tu/with-temporary-setting-values [slack-files-channel "general"
- slack-cached-channels-and-usernames ["#general" "#random" "#off-topic" "#cooking" "@john" "@james" "@jordan"]]
+ slack-cached-channels-and-usernames
+ {:channels (mapv (fn [c] {:name c :id c})
+ ["general" "random" "off-topic"
+ "cooking" "john" "james" "jordan"])}]
(is (= "general" (slack/files-channel))))
(tu/with-temporary-setting-values [slack-files-channel "not_in_the_cache"
- slack-cached-channels-and-usernames ["#general"]]
+ slack-cached-channels-and-usernames
+ {:channels [{:name "general" :id "C0G9QKBBL"}]}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Slack channel named.*is missing.*"
@@ -163,8 +163,7 @@
(deftest upload-file!-test
(testing "upload-file!"
- (let [image-bytes (with-open [is (io/input-stream (io/resource "frontend_client/favicon.ico"))]
- (IOUtils/toByteArray is))
+ (let [image-bytes (.getBytes "fake-picture")
filename "wow.gif"
channel-id "C13372B6X"]
(http-fake/with-fake-routes {#"^https://slack.com/api/files\.upload.*"
@@ -181,7 +180,65 @@
(tu/with-temporary-setting-values [slack-token nil
slack-app-token "test-token"]
(is (= "https://files.slack.com/files-pri/T078VLEET-F017C3TSBK6/wow.gif"
- (slack/upload-file! image-bytes filename channel-id))))))))
+ (slack/upload-file! image-bytes filename channel-id)))))))
+ (testing (str "upload-file! will attempt to join channels by internal slack id"
+ " but we can continue to use the channel name for posting")
+ (let [filename "wow.gif"
+ channel-id "metabase_files"
+ slack-id "CQXPZKNQ3RK"
+ joined? (atom false)
+ channel-info [{:display-name "#random",
+ :name "random",
+ :id "CT2FNGZSRPL",
+ :type "channel"}
+ {:display-name "#general",
+ :name "general",
+ :id "C4Q6LXLRA46",
+ :type "channel"}
+ {:display-name "#metabase_files",
+ :name channel-id,
+ ;; must look up "metabase_files" and find the id below
+ :id slack-id,
+ :type "channel"}]]
+ (tu/with-temporary-setting-values [slack/slack-app-token "slack-configured?"
+ slack/slack-cached-channels-and-usernames
+ {:channels channel-info}]
+ (with-redefs [slack/POST (fn [endpoint payload]
+ (case endpoint
+ "files.upload"
+ (if @joined?
+ {:file {:url_private filename}}
+ (throw (ex-info "Not in that channel"
+ {:error-code "not_in_channel"})))
+ "conversations.join"
+ (reset! joined? (= (-> payload :form-params :channel)
+ slack-id))))]
+ (slack/upload-file! (.getBytes "fake-picture") filename channel-id)
+ (is @joined? (str "Did not attempt to join with slack-id " slack-id)))))))
+
+(deftest maybe-lookup-id-test
+ (let [f (var-get #'slack/maybe-lookup-id)]
+ (testing "On new v2 shape"
+ (testing "Returns original if not found"
+ (is (= "needle"
+ (f "needle" {:channels [{:display-name "#other1"
+ :name "other1"
+ :type "channel"
+ :id "CR65C4ZJVIW"}
+ {:display-name "#other2"
+ :name "other2"
+ :type "channel"
+ :id "C87LQNL0Y23"}]}))))
+ (testing "Returns the slack internal id if found"
+ (is (= "slack-id"
+ (f "needle" {:channels [{:display-name "#other1"
+ :name "other1"
+ :type "channel"
+ :id "CR65C4ZJVIW"}
+ {:display-name "#needle"
+ :name "needle"
+ :type "channel"
+ :id "slack-id"}]})))))))
(deftest post-chat-message!-test
(testing "post-chat-message!"