FIX: Improve error handling for slack transcript generation (#63)

- Stop using `break` in a block - it doesn't work. The failure here was hidden because it was within a `defer` block, so would not cause a server error
- Refactor the error handling so that the error is passed back to Slack and displayed to the user
- Return specific error messages for user / message-history / message errors
- Tidy up the SlackCommandController to make all non-requestable methods private
- Add a test to ensure error messages are passed correctly to Slack

config/locales/server.en.yml

@@ -167,6 +167,9 @@ en:
           change_first_message: "Change first message..."
           change_last_message: "Change last message..."
           loading: "Loading the transcript..."
+          error_users: "Error: unable to fetch users from Slack"
+          error_history: "Error: unable to fetch channel history from Slack"
+          error_ts: "Error: unable to fetch requested message from Slack"
 
       #######################################
       ########## TELEGRAM STRINGS ###########

lib/discourse_chat/provider/slack/slack_command_controller.rb

@@ -19,6 +19,15 @@ module DiscourseChat::Provider::SlackProvider
       render json: message
     end
 
+    def interactive
+      json = JSON.parse(params[:payload], symbolize_names: true)
+      process_interactive(json)
+
+      render json: { text: I18n.t("chat_integration.provider.slack.transcript.loading") }
+    end
+
+    private
+
     def process_command(params)
 
       tokens = params[:text].split(" ")
@@ -58,6 +67,18 @@ module DiscourseChat::Provider::SlackProvider
       end
 
       Scheduler::Defer.later "Processing slack transcript request" do
+        response = build_post_request_response(channel, tokens, slack_channel_id, channel_name, response_url)
+        http = Net::HTTP.new("slack.com", 443)
+        http.use_ssl = true
+        req = Net::HTTP::Post.new(URI(response_url), 'Content-Type' => 'application/json')
+        req.body = response.to_json
+        http.request(req)
+      end
+
+      { text: I18n.t("chat_integration.provider.slack.transcript.loading") }
+    end
+
+    def build_post_request_response(channel, tokens, slack_channel_id, channel_name, response_url)
       requested_messages = nil
       first_message_ts = nil
       requested_thread_ts = nil
@@ -75,39 +96,25 @@ module DiscourseChat::Provider::SlackProvider
         begin
           requested_messages = Integer(tokens[1], 10)
         rescue ArgumentError
-            break { text: I18n.t("chat_integration.provider.slack.parse_error") }
+          return { text: I18n.t("chat_integration.provider.slack.parse_error") }
         end
       end
 
-        error_message = { text: I18n.t("chat_integration.provider.slack.transcript.error") }
+      error_key = "chat_integration.provider.slack.transcript.error"
 
-        break error_message unless transcript = SlackTranscript.new(channel_name: channel_name, channel_id: slack_channel_id, requested_thread_ts: requested_thread_ts)
-        break error_message unless transcript.load_user_data
-        break error_message unless transcript.load_chat_history
+      return { text: I18n.t(error_key) } unless transcript = SlackTranscript.new(channel_name: channel_name, channel_id: slack_channel_id, requested_thread_ts: requested_thread_ts)
+      return { text: I18n.t("#{error_key}_users") } unless transcript.load_user_data
+      return { text: I18n.t("#{error_key}_history") } unless transcript.load_chat_history
 
       if first_message_ts
-          break error_message unless transcript.set_first_message_by_ts(first_message_ts)
+        return { text: I18n.t("#{error_key}_ts") } unless transcript.set_first_message_by_ts(first_message_ts)
       elsif requested_messages
         transcript.set_first_message_by_index(-requested_messages)
       else
         transcript.set_first_message_by_index(-10) unless transcript.guess_first_message
       end
 
-        http = Net::HTTP.new("slack.com", 443)
-        http.use_ssl = true
-        req = Net::HTTP::Post.new(URI(response_url), 'Content-Type' => 'application/json')
-        req.body = transcript.build_slack_ui.to_json
-        response = http.request(req)
-      end
-
-      { text: I18n.t("chat_integration.provider.slack.transcript.loading") }
-    end
-
-    def interactive
-      json = JSON.parse(params[:payload], symbolize_names: true)
-      process_interactive(json)
-
-      render json: { text: I18n.t("chat_integration.provider.slack.transcript.loading") }
+      transcript.build_slack_ui
     end
 
     def process_interactive(json)

spec/lib/discourse_chat/provider/slack/slack_command_controller_spec.rb

@@ -307,6 +307,10 @@ describe 'Slack Command Controller', type: :request do
         end
 
         it 'deals with failed API calls correctly' do
+          command_stub = stub_request(:post, "https://slack.com/commands/1234")
+            .with(body: { text: I18n.t("chat_integration.provider.slack.transcript.error_users") })
+            .to_return(body: { ok: true }.to_json)
+
           stub_request(:post, "https://slack.com/api/users.list").to_return(status: 403)
 
           post "/chat-integration/slack/command.json", params: {
@@ -320,6 +324,8 @@ describe 'Slack Command Controller', type: :request do
           json = response.parsed_body
 
           expect(json["text"]).to include(I18n.t("chat_integration.provider.slack.transcript.loading"))
+
+          expect(command_stub).to have_been_requested
         end
 
         it 'errors correctly if there is no api key' do