From d97d35fd0db382128fc631801f7238c5f5043428 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 4 Jul 2017 19:37:56 +0100 Subject: [PATCH] Handle errors on trigger_notification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../admin-plugins-chat-provider.js.es6 | 13 ++++++++ assets/javascripts/admin/models/rule.js.es6 | 1 + .../templates/admin/plugins-chat-provider.hbs | 19 ++++++++++-- .../stylesheets/chat-integration-admin.scss | 13 ++++++++ config/locales/client.en.yml | 5 +++ lib/discourse_chat/manager.rb | 23 ++++++++++++-- lib/discourse_chat/provider.rb | 9 ++++++ .../provider/slack/slack_provider.rb | 14 ++++++++- lib/discourse_chat/rule.rb | 24 ++++++++------ plugin.rb | 4 ++- spec/lib/discourse_chat/manager_spec.rb | 31 ++++++++++++++++++- 11 files changed, 139 insertions(+), 17 deletions(-) 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 09d3a34..e1dd35a 100644 --- a/assets/javascripts/admin/controllers/admin-plugins-chat-provider.js.es6 +++ b/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)); + } } diff --git a/assets/javascripts/admin/models/rule.js.es6 b/assets/javascripts/admin/models/rule.js.es6 index c58fc04..2ee1523 100644 --- a/assets/javascripts/admin/models/rule.js.es6 +++ b/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) { diff --git a/assets/javascripts/discourse/templates/admin/plugins-chat-provider.hbs b/assets/javascripts/discourse/templates/admin/plugins-chat-provider.hbs index 6c10e92..2fcd602 100644 --- a/assets/javascripts/discourse/templates/admin/plugins-chat-provider.hbs +++ b/assets/javascripts/discourse/templates/admin/plugins-chat-provider.hbs @@ -1,4 +1,10 @@ + {{#if anyErrors}} +
+ + {{i18n "chat_integration.rules_with_errors"}} +
+ {{/if}} @@ -16,10 +22,19 @@ + {{#each model.rules as |rule|}} - + {{/each}} diff --git a/assets/stylesheets/chat-integration-admin.scss b/assets/stylesheets/chat-integration-admin.scss index 27a6276..d5aaad3 100644 --- a/assets/stylesheets/chat-integration-admin.scss +++ b/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{ diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 93266fc..10e374c 100644 --- a/config/locales/client.en.yml +++ b/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 ########### diff --git a/lib/discourse_chat/manager.rb b/lib/discourse_chat/manager.rb index b9952d9..76b8098 100644 --- a/lib/discourse_chat/manager.rb +++ b/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 diff --git a/lib/discourse_chat/provider.rb b/lib/discourse_chat/provider.rb index 41119a4..364bca5 100644 --- a/lib/discourse_chat/provider.rb +++ b/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| diff --git a/lib/discourse_chat/provider/slack/slack_provider.rb b/lib/discourse_chat/provider/slack/slack_provider.rb index 4800361..a8674df 100644 --- a/lib/discourse_chat/provider/slack/slack_provider.rb +++ b/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) diff --git a/lib/discourse_chat/rule.rb b/lib/discourse_chat/rule.rb index d8355ac..03d82b5 100644 --- a/lib/discourse_chat/rule.rb +++ b/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 diff --git a/plugin.rb b/plugin.rb index 57ebf47..14999c3 100644 --- a/plugin.rb +++ b/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' diff --git a/spec/lib/discourse_chat/manager_spec.rb b/spec/lib/discourse_chat/manager_spec.rb index 78157ea..1a910e7 100644 --- a/spec/lib/discourse_chat/manager_spec.rb +++ b/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)
{{rule.channel}} + {{#if rule.error_key}} + + {{d-button action="showError" actionParam=rule.error_key class="delete btn-danger" icon="exclamation-triangle"}} + + {{/if}} + + {{rule.channel}} + {{rule.filterName}} @@ -44,7 +59,7 @@ {{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"}}