From c5fdebd1bc4c5b8088745eb22dd2d356c1ee44de Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 18 Jul 2017 23:08:06 +0100 Subject: [PATCH] Store errors per-channel rather than per-rule, and update UI to match --- app/controllers/chat_controller.rb | 3 +- app/models/channel.rb | 2 +- app/models/rule.rb | 6 +-- app/serializers/channel_serializer.rb | 2 +- app/serializers/rule_serializer.rb | 2 +- app/services/manager.rb | 48 +++++++++---------- .../admin/components/channel-details.js.es6 | 6 ++- .../admin-plugins-chat-provider.js.es6 | 5 -- .../templates/components/channel-details.hbs | 4 ++ spec/controllers/chat_controller_spec.rb | 3 +- spec/services/manager_spec.rb | 6 +-- 11 files changed, 42 insertions(+), 45 deletions(-) diff --git a/app/controllers/chat_controller.rb b/app/controllers/chat_controller.rb index a605a53..63e9b9e 100644 --- a/app/controllers/chat_controller.rb +++ b/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) diff --git a/app/models/channel.rb b/app/models/channel.rb index a3238d0..2337ade 100644 --- a/app/models/channel.rb +++ b/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 diff --git a/app/models/rule.rb b/app/models/rule.rb index 79baa64..6ccd4f4 100644 --- a/app/models/rule.rb +++ b/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 diff --git a/app/serializers/channel_serializer.rb b/app/serializers/channel_serializer.rb index 2317434..e24831f 100644 --- a/app/serializers/channel_serializer.rb +++ b/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| diff --git a/app/serializers/rule_serializer.rb b/app/serializers/rule_serializer.rb index d8b3f77..57a0581 100644 --- a/app/serializers/rule_serializer.rb +++ b/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 \ No newline at end of file diff --git a/app/services/manager.rb b/app/services/manager.rb index ce49d5f..78bee22 100644 --- a/app/services/manager.rb +++ b/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 - provider = ::DiscourseChat::Provider.get_by_name(channel.provider) - is_enabled = ::DiscourseChat::Provider.is_enabled(provider) + # If there are any issues, skip to the next rule + next unless channel = rule.channel + 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 - provider.trigger_notification(post, rule.channel) - rule.update_attribute('error_key', nil) if rule.error_key - rescue => e - if e.class == DiscourseChat::ProviderError and e.info.key?(:error_key) and !e.info[:error_key].nil? - rule.update_attribute('error_key', e.info[:error_key]) - else - 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 } - ) + begin + provider.trigger_notification(post, channel) + channel.update_attribute('error_key', nil) if channel.error_key + rescue => e + 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]) + else + channel.update_attribute('error_key','chat_integration.channel_exception') end - elsif provider - # Provider is disabled, don't do anything - else - # TODO: Handle when the provider does not exist + + # 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 + end end diff --git a/assets/javascripts/admin/components/channel-details.js.es6 b/assets/javascripts/admin/components/channel-details.js.es6 index 6e3be76..08fcd1b 100644 --- a/assets/javascripts/admin/components/channel-details.js.es6 +++ b/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)); + }, } }); \ No newline at end of file diff --git a/assets/javascripts/admin/controllers/admin-plugins-chat-provider.js.es6 b/assets/javascripts/admin/controllers/admin-plugins-chat-provider.js.es6 index 335d400..f52965c 100644 --- a/assets/javascripts/admin/controllers/admin-plugins-chat-provider.js.es6 +++ b/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)); - }, - - } diff --git a/assets/javascripts/discourse/templates/components/channel-details.hbs b/assets/javascripts/discourse/templates/components/channel-details.hbs index d3ce21d..204fd02 100644 --- a/assets/javascripts/discourse/templates/components/channel-details.hbs +++ b/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"}} + {{#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|}} {{i18n (concat 'chat_integration.provider.' channel.provider '.param.' param.key '.title')}}: diff --git a/spec/controllers/chat_controller_spec.rb b/spec/controllers/chat_controller_spec.rb index a8f4e08..c94e518 100644 --- a/spec/controllers/chat_controller_spec.rb +++ 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 diff --git a/spec/services/manager_spec.rb b/spec/services/manager_spec.rb index 0055960..2180c04 100644 --- a/spec/services/manager_spec.rb +++ b/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