Handle errors on trigger_notification

Providers can define their own errors, and these are presented in the user interface. e.g. Slack can define an error that says “That channel doesn’t exist”.
Errors in the UI disappear once a message has been sent successfully, or the rule is edited.

assets/javascripts/admin/controllers/admin-plugins-chat-provider.js.es6

@@ -8,6 +8,16 @@ import { popupAjaxError } from 'discourse/lib/ajax-error';
 export default Ember.Controller.extend({
 	modalShowing: false,
 	
+  anyErrors: function(){
+    var anyErrors = false;
+    this.get('model.rules').forEach(function(rule){
+      if(rule.error_key){
+        anyErrors = true;
+      }
+    });
+    return anyErrors;
+  }.property('model.rules'),
+
   actions:{
   	create(){
   		this.set('modalShowing', true);
@@ -25,6 +35,9 @@ export default Ember.Controller.extend({
         self.send('refresh');
       }).catch(popupAjaxError)
   	},
+    showError(error_key){
+      bootbox.alert(I18n.t(error_key));
+    }
 
   }
 

assets/javascripts/admin/models/rule.js.es6

@@ -14,6 +14,7 @@ export default RestModel.extend({
   provider: '',
   channel: '',
   filter: null,
+  error_key: null,
 
   @computed('category_id')
   category(categoryId) {

assets/javascripts/discourse/templates/admin/plugins-chat-provider.hbs

@@ -1,4 +1,10 @@
 
+  {{#if anyErrors}}
+    <div class="error">
+      <i class="fa fa-exclamation-triangle"></i>
+      <span class="error-message">{{i18n "chat_integration.rules_with_errors"}}</span>
+    </div>
+  {{/if}}
 
   <table>
     <tr>
@@ -16,10 +22,19 @@
       <th></th>
     </tr>
 
+
     {{#each model.rules as |rule|}}
 
       <tr>
-				<td>{{rule.channel}}</td>
+				<td>
+          {{#if rule.error_key}}
+
+            {{d-button action="showError" actionParam=rule.error_key class="delete btn-danger" icon="exclamation-triangle"}}
+            
+          {{/if}}
+
+          {{rule.channel}}
+        </td>
         <td>{{rule.filterName}}</td>
 
         <td>
@@ -44,7 +59,7 @@
         
         <td>
           {{d-button action="edit" actionParam=rule icon="pencil" class="edit" title="chat_integration.rule_table.edit_rule"}}
-          {{d-button action="delete" actionParam=rule icon="trash-o" class="delete btn-danger" title="chat_integration.rule_table.delete_rule"}}
+          {{d-button action="delete" actionParam=rule icon="trash-o" class="delete" title="chat_integration.rule_table.delete_rule"}}
         </td>
       </tr>
     {{/each}}

assets/stylesheets/chat-integration-admin.scss

@@ -13,6 +13,19 @@
 		margin-top: 10px;
 	}
 
+	div.error {
+		font-size: 1.1em;
+		font-weight:bold;
+		max-width: 100%;
+    margin-top: 10px;
+    margin-bottom: 10px;
+    background-color: dark-light-diff($quaternary, $secondary, 70%, -70%);
+    padding: 15px;
+
+    
+  }
+
+
 }
 
 #chat_integration_edit_rule_modal{

config/locales/client.en.yml

@@ -4,6 +4,8 @@ en:
       menu_title: "Chat Integrations"
       settings: "Settings"
       no_providers: "You need to enable some providers in the plugin settings"
+      rules_with_errors: "Some rules for this provider failed last time they were triggered. Click the error icon(s) to learn more."
+      rule_exception: "An unknown error occured when this rule was last triggered. Check the site logs for more information."
       all_categories: "(all categories)"
       all_tags: "(all tags)"
       create_rule: "Create Rule"
@@ -43,6 +45,9 @@ en:
         slack:
           title: "Slack"
           channel_instructions: "e.g. #channel, @username."
+          errors:
+            action_prohibited: "The bot does not have permission to post to that channel"
+            channel_not_found: "The specified channel does not exist on slack"
       
         #######################################
         ########## TELEGRAM STRINGS ###########

lib/discourse_chat/manager.rb

@@ -61,11 +61,30 @@ module DiscourseChat
 
       # Loop through each rule, and trigger appropriate notifications
       matching_rules.each do |rule|
-        Rails.logger.info("Sending notification to provider #{rule.provider}, channel #{rule.channel}")
         provider = ::DiscourseChat::Provider.get_by_name(rule.provider)
         is_enabled = ::DiscourseChat::Provider.is_enabled(provider)
+
         if provider and is_enabled
-          provider.trigger_notification(post, rule.channel)
+          begin
+            provider.trigger_notification(post, rule.channel)
+            rule.update({error_key: nil}, false) if rule.error_key
+          rescue => e
+            if e.class == DiscourseChat::ProviderError and e.info.key?(:error_key) and !e.info[:error_key].nil?
+              rule.error_key = e.info[:error_key]
+            else
+              rule.error_key = 'chat_integration.rule_exception'
+            end
+            rule.save(false) # Save without validations
+
+            # Log the error
+            Discourse.handle_job_exception(e,
+              message: "Triggering notifications failed",
+              extra: { provider_name: provider::PROVIDER_NAME,
+                       channel: rule.channel,
+                       post_id: post.id,
+                       error_info: e.class == DiscourseChat::ProviderError ? e.info : nil }
+            )
+          end
         elsif provider
           # Provider is disabled, don't do anything
         else

lib/discourse_chat/provider.rb

@@ -1,4 +1,13 @@
 module DiscourseChat
+  class ProviderError < StandardError
+    attr_accessor :info
+
+    def initialize(message = nil, info: nil)
+      super(message)
+      self.info = info.nil? ? {} : info
+    end
+  end
+
   module Provider
     def self.providers
       constants.select do |constant|

lib/discourse_chat/provider/slack/slack_provider.rb

@@ -105,7 +105,19 @@ module DiscourseChat::Provider::SlackProvider
   	req = Net::HTTP::Post.new(URI(SiteSetting.chat_integration_slack_outbound_webhook_url), 'Content-Type' =>'application/json')
     req.body = message.to_json
     response = http.request(req)
-    response
+
+    unless response.kind_of? Net::HTTPSuccess
+      if response.code.to_s == '403'
+        error_key = 'chat_integration.provider.slack.errors.action_prohibited'
+      elsif response.body == 'channel_not_found' or response.body == 'channel_is_archived'
+        error_key = 'chat_integration.provider.slack.errors.channel_not_found'
+      else
+        error_key = nil
+      end
+      raise ::DiscourseChat::ProviderError.new info: {error_key: error_key, request: req.body, response_code:response.code, response_body:response.body}
+    end
+
+    
   end
 
   def self.trigger_notification(post, channel)

lib/discourse_chat/rule.rb

@@ -2,7 +2,7 @@
   # Using this means we can use a standard serializer for sending JSON to the client, and also have convenient save/update/delete methods
   # Since this is now being used in two plugins, maybe it should be built into core somehow
   class DiscourseChat::Rule
-    attr_accessor :id, :provider, :channel, :category_id, :tags, :filter
+    attr_accessor :id, :provider, :channel, :category_id, :tags, :filter, :error_key
 
     def initialize(h={})
       @provider = ''
@@ -10,6 +10,7 @@
       @category_id = nil
       @tags = []
       @filter = 'watch'
+      @error_key = nil
       h.each {|k,v| public_send("#{k}=",v)}
     end
 
@@ -41,7 +42,7 @@
 
     def self.from_hash(h)
       rule = DiscourseChat::Rule.new
-      [:provider, :channel, :category_id, :tags, :filter].each do |sym|
+      [:provider, :channel, :category_id, :tags, :filter, :error_key].each do |sym|
         rule.send("#{sym}=", h[sym]) if h[sym]
       end
       if h[:id]
@@ -58,6 +59,7 @@
         category_id: @category_id,
         tags: @tags,
         filter: @filter,
+        error_key: @error_key,
       }
     end
 
@@ -70,16 +72,18 @@
       from_hash hash
     end
 
-    def update(h)
-      [:provider, :channel, :category_id, :tags, :filter].each do |sym|
-        public_send("#{sym}=", h[sym]) if h[sym]
+    def update(h, validate=true)
+      [:provider, :channel, :category_id, :tags, :filter, :error_key].each do |sym|
+        public_send("#{sym}=", h[sym]) if h.key?(sym)
       end
 
-      save
+      save(validate)
     end
 
-    def save
-      return false if not valid?
+    def save(validate=true)
+      if validate
+        return false if not valid?
+      end
 
       unless @id && @id > 0
         @id = self.class.alloc_id
@@ -88,8 +92,8 @@
       return self
     end
 
-    def save!
-      if not save
+    def save!(validate=true)
+      if not save(validate)
         raise 'Rule not valid'
       end
       return self

plugin.rb

@@ -43,6 +43,7 @@ after_initialize do
 
   module ::Jobs
     class NotifyChats < Jobs::Base
+      sidekiq_options retry: false # Don't retry, could result in duplicate notifications for some providers
       def execute(args)
         return if not SiteSetting.chat_integration_enabled? # Plugin may have been disabled since job triggered
 
@@ -113,6 +114,7 @@ after_initialize do
     def update_rule
       begin
         rule = DiscourseChat::Rule.find(params[:id].to_i)
+        rule.error_key = nil # Reset any error on the rule
         hash = params.require(:rule)
 
         if not rule.update(hash)
@@ -135,7 +137,7 @@ after_initialize do
   end
 
   class DiscourseChat::RuleSerializer < ActiveModel::Serializer
-    attributes :id, :provider, :channel, :category_id, :tags, :filter
+    attributes :id, :provider, :channel, :category_id, :tags, :filter, :error_key
   end
 
   require_dependency 'admin_constraint'

spec/lib/discourse_chat/manager_spec.rb

@@ -19,14 +19,22 @@ RSpec.describe DiscourseChat::Manager do
         PROVIDER_NAME = "dummy".freeze
         PROVIDER_ENABLED_SETTING = :chat_integration_enabled # Tie to main plugin enabled setting
         @@sent_messages = []
+        @@raise_exception = nil
 
         def self.trigger_notification(post, channel)
+          if @@raise_exception
+            raise @@raise_exception
+          end
           @@sent_messages.push(post: post.id, channel: channel)
         end
 
         def self.sent_messages
           @@sent_messages
         end
+
+        def self.set_raise_exception(bool)
+          @@raise_exception = bool
+        end
       end
     end
 
@@ -40,7 +48,28 @@ RSpec.describe DiscourseChat::Manager do
       DiscourseChat::Rule.new({provider: provider, channel: channel, filter:filter, category_id:category_id, tags:tags}).save!
     end
 
-    it "should only send notifications when provider is enabled" do
+    it "should fail gracefully when a provider throws an exception" do
+      create_rule('dummy', 'chan1', 'watch', category.id, nil)
+
+      # Triggering a ProviderError should set the error_key to the error message
+      ::DiscourseChat::Provider::DummyProvider.set_raise_exception(DiscourseChat::ProviderError.new info: {error_key:"hello"})
+      manager.trigger_notifications(first_post.id)
+      expect(provider.sent_messages.map{|x| x[:channel]}).to contain_exactly()
+      expect(DiscourseChat::Rule.all.first.error_key).to eq('hello')
+
+      # Triggering a different error should set the error_key to a generic message
+      ::DiscourseChat::Provider::DummyProvider.set_raise_exception(StandardError.new "hello")
+      manager.trigger_notifications(first_post.id)
+      expect(provider.sent_messages.map{|x| x[:channel]}).to contain_exactly()
+      expect(DiscourseChat::Rule.all.first.error_key).to eq('chat_integration.rule_exception')
+
+      ::DiscourseChat::Provider::DummyProvider.set_raise_exception(nil)
+
+      manager.trigger_notifications(first_post.id)
+      expect(DiscourseChat::Rule.all.first.error_key.nil?).to be true      
+    end
+
+    it "should not send notifications when provider is disabled" do
       SiteSetting.chat_integration_enabled = false
       create_rule('dummy', 'chan1', 'watch', category.id, nil)