Store errors per-channel rather than per-rule, and update UI to match

app/controllers/chat_controller.rb

@@ -91,7 +91,7 @@ class DiscourseChat::ChatController < ApplicationController
   def update_channel
     begin
       channel = DiscourseChat::Channel.find(params[:id].to_i)
-      # rule.error_key = nil # Reset any error on the rule
+      channel.error_key = nil # Reset any error on the rule
 
       allowed_keys = DiscourseChat::Provider.get_by_name(channel.provider)::CHANNEL_PARAMETERS.map{|p| p[:key].to_sym}
 
@@ -134,7 +134,6 @@ class DiscourseChat::ChatController < ApplicationController
   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).permit(:filter, :category_id, tags:[])
       
       if not rule.update(hash)

app/models/channel.rb

@@ -2,7 +2,7 @@ class DiscourseChat::Channel < DiscourseChat::PluginModel
   KEY_PREFIX = 'channel:'
 
   # Setup ActiveRecord::Store to use the JSON field to read/write these values
-  store :value, accessors: [ :provider, :data ], coder: JSON
+  store :value, accessors: [ :provider, :error_key, :data ], coder: JSON
 
   after_initialize :init_data
 

app/models/rule.rb

@@ -2,7 +2,7 @@ class DiscourseChat::Rule < DiscourseChat::PluginModel
   KEY_PREFIX = 'rule:'
 
   # Setup ActiveRecord::Store to use the JSON field to read/write these values
-  store :value, accessors: [ :channel_id, :category_id, :tags, :filter, :error_key ], coder: JSON
+  store :value, accessors: [ :channel_id, :category_id, :tags, :filter ], coder: JSON
 
   after_initialize :init_filter
 
@@ -67,9 +67,9 @@ class DiscourseChat::Rule < DiscourseChat::PluginModel
   end
 
   # Mock foreign key
-  # Could raise RecordNotFound
+  # Could return nil
   def channel
-    DiscourseChat::Channel.find(channel_id)
+    DiscourseChat::Channel.find_by(id:channel_id)
   end
   def channel=(val)
     self.channel_id = val.id

app/serializers/channel_serializer.rb

@@ -1,7 +1,7 @@
 require_relative './rule_serializer'
 
 class DiscourseChat::ChannelSerializer < ApplicationSerializer
-  attributes :id, :provider, :data, :rules
+  attributes :id, :provider, :error_key, :data, :rules
 
   def rules
     object.rules.order_by_precedence.map do |rule|

app/serializers/rule_serializer.rb

@@ -1,3 +1,3 @@
 class DiscourseChat::RuleSerializer < ApplicationSerializer
-  attributes :id, :channel_id, :category_id, :tags, :filter, :error_key
+  attributes :id, :channel_id, :category_id, :tags, :filter
 end

app/services/manager.rb

@@ -61,35 +61,31 @@ module DiscourseChat
 
       # Loop through each rule, and trigger appropriate notifications
       matching_rules.each do |rule|
-        channel = rule.channel
+        # If there are any issues, skip to the next rule
-        provider = ::DiscourseChat::Provider.get_by_name(channel.provider)
+        next unless channel = rule.channel
-        is_enabled = ::DiscourseChat::Provider.is_enabled(provider)
+        next unless provider = ::DiscourseChat::Provider.get_by_name(channel.provider)
+        next unless is_enabled = ::DiscourseChat::Provider.is_enabled(provider)
 
-        if provider and is_enabled
+        begin
-          begin
+          provider.trigger_notification(post, channel)
-            provider.trigger_notification(post, rule.channel)
+          channel.update_attribute('error_key', nil) if channel.error_key
-            rule.update_attribute('error_key', nil) if rule.error_key
+        rescue => e
-          rescue => e
+          if e.class == DiscourseChat::ProviderError and e.info.key?(:error_key) and !e.info[:error_key].nil?
-            if e.class == DiscourseChat::ProviderError and e.info.key?(:error_key) and !e.info[:error_key].nil?
+            channel.update_attribute('error_key', e.info[:error_key])
-              rule.update_attribute('error_key', e.info[:error_key])
+          else
-            else
+            channel.update_attribute('error_key','chat_integration.channel_exception')
-              rule.update_attribute('error_key','chat_integration.rule_exception')
-            end
-
-            # 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
+          # Log the error
-        else
+          Discourse.handle_job_exception(e,
-          # TODO: Handle when the provider does not exist
+            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
+        
       end
 
     end

assets/javascripts/admin/components/channel-details.js.es6

@@ -28,9 +28,11 @@ export default Ember.Component.extend({
     createRule(channel){
       var newRule = this.get('store').createRecord('rule',{channel_id: channel.id});
       channel.rules.pushObject(newRule)
-    }
+    },
-
 
+    showError(error_key){
+      bootbox.alert(I18n.t(error_key));
+    },
 
   }
 });

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

@@ -34,11 +34,6 @@ export default Ember.Controller.extend({
       var model = {channel:channel}
       showModal('admin-plugins-chat-test', { model: model, admin: true });
     },
-    showError(error_key){
-      bootbox.alert(I18n.t(error_key));
-    },
-
-
 
   }
 

assets/javascripts/discourse/templates/components/channel-details.hbs

@@ -7,6 +7,10 @@
     {{d-button class='cancel' action="delete" actionParam=channel icon="trash" title="chat_integration.delete_channel" label="chat_integration.delete_channel"}}
   </div>
   <span class='channel-title'>
+    {{#if channel.error_key}}
+      {{d-button action="showError" actionParam=channel.error_key class="delete btn-danger" icon="exclamation-triangle"}}
+    {{/if}}
+
 
     {{# each provider.channel_parameters as |param|}}
         <b>{{i18n (concat 'chat_integration.provider.' channel.provider '.param.' param.key '.title')}}:</b>

spec/controllers/chat_controller_spec.rb

@@ -103,7 +103,8 @@ describe 'Chat Controller', type: :request do
           "id" => channel.id,
           "provider" => 'dummy',
           "data" => {},
-          "rules" => [{"id" => rule.id, "filter" => "follow", "channel_id" => channel.id, "category_id" => category.id, "tags" => [tag.name], "error_key" => nil}]
+          "error_key" => nil,
+          "rules" => [{"id" => rule.id, "filter" => "follow", "channel_id" => channel.id, "category_id" => category.id, "tags" => [tag.name]}]
         )
       end
 

spec/services/manager_spec.rb

@@ -28,18 +28,18 @@ RSpec.describe DiscourseChat::Manager do
       provider.set_raise_exception(DiscourseChat::ProviderError.new info: {error_key:"hello"})
       manager.trigger_notifications(first_post.id)
       expect(provider.sent_to_channel_ids).to contain_exactly()
-      expect(DiscourseChat::Rule.all.first.error_key).to eq('hello')
+      expect(DiscourseChat::Channel.all.first.error_key).to eq('hello')
 
       # Triggering a different error should set the error_key to a generic message
       provider.set_raise_exception(StandardError.new "hello")
       manager.trigger_notifications(first_post.id)
       expect(provider.sent_to_channel_ids).to contain_exactly()
-      expect(DiscourseChat::Rule.all.first.error_key).to eq('chat_integration.rule_exception')
+      expect(DiscourseChat::Channel.all.first.error_key).to eq('chat_integration.channel_exception')
 
       provider.set_raise_exception(nil)
 
       manager.trigger_notifications(first_post.id)
-      expect(DiscourseChat::Rule.all.first.error_key.nil?).to be true      
+      expect(DiscourseChat::Channel.all.first.error_key.nil?).to be true      
     end
 
     it "should not send notifications when provider is disabled" do