From 61caca3c5bfe6fa1d0a643bb85609876856515f6 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 29 Jun 2017 17:50:54 +0100 Subject: [PATCH] Use new DiscourseChat::Rule model throughout the backend code --- .../admin-plugins-chat-edit-rule.js.es6 | 22 +++++- assets/javascripts/admin/models/rule.js.es6 | 15 ++-- .../modal/admin-plugins-chat-edit-rule.hbs | 6 +- lib/manager.rb | 72 +++---------------- lib/rule.rb | 2 +- plugin.rb | 16 +++-- spec/lib/manager_spec.rb | 69 ++++++------------ 7 files changed, 79 insertions(+), 123 deletions(-) diff --git a/assets/javascripts/admin/controllers/modals/admin-plugins-chat-edit-rule.js.es6 b/assets/javascripts/admin/controllers/modals/admin-plugins-chat-edit-rule.js.es6 index 2092186..2bcd8e1 100644 --- a/assets/javascripts/admin/controllers/modals/admin-plugins-chat-edit-rule.js.es6 +++ b/assets/javascripts/admin/controllers/modals/admin-plugins-chat-edit-rule.js.es6 @@ -1,14 +1,32 @@ import Rule from 'discourse/plugins/discourse-chat/admin/models/rule' +import ModalFunctionality from 'discourse/mixins/modal-functionality'; import { ajax } from 'discourse/lib/ajax'; +import { extractError } from 'discourse/lib/ajax-error'; -export default Ember.Controller.extend({ +export default Ember.Controller.extend(ModalFunctionality, { model: Rule.create({}), actions: { cancel: function(){ - this.send('closeModal'); }, + + save: function(){ + + const self = this; + + this.get('model').update().then(function(result) { + self.send('closeModal'); + if (result.responseJson.success) { + self.transitionToRoute('tags.show', result.responseJson.tag.id); + } else { + self.flash(extractError(result.responseJson.errors[0]), 'error'); + } + }).catch(function(error) { + self.flash(extractError(error), 'error'); + }); + + } } }); \ No newline at end of file diff --git a/assets/javascripts/admin/models/rule.js.es6 b/assets/javascripts/admin/models/rule.js.es6 index 3c2744b..5074bf0 100644 --- a/assets/javascripts/admin/models/rule.js.es6 +++ b/assets/javascripts/admin/models/rule.js.es6 @@ -10,21 +10,26 @@ export default RestModel.extend({ ], category_id: null, + tags: null, provider: '', channel: '', filter: null, @computed('category_id') - categoryName(categoryId) { - if (categoryId) - return Category.findById(categoryId).get('name'); - else { - return I18n.t('slack.choose.all_categories'); + category(categoryId) { + if (categoryId){ + return Category.findById(categoryId); + }else { + return false; } }, @computed('filter') filterName(filter) { return I18n.t(`slack.present.${filter}`); + }, + + updateProperties() { + return ['category_id','provider','channel', 'tags','filter']; } }); diff --git a/assets/javascripts/admin/templates/modal/admin-plugins-chat-edit-rule.hbs b/assets/javascripts/admin/templates/modal/admin-plugins-chat-edit-rule.hbs index 7f07fee..a07fc95 100644 --- a/assets/javascripts/admin/templates/modal/admin-plugins-chat-edit-rule.hbs +++ b/assets/javascripts/admin/templates/modal/admin-plugins-chat-edit-rule.hbs @@ -6,9 +6,11 @@ {{category-chooser name="category" - value=model.categoryId + value=model.category_id rootNoneLabel="slack.choose.all_categories" - rootNone=true}} + rootNone=true + allowUncategorized="true" + }} {{#if siteSettings.tagging_enabled}} diff --git a/lib/manager.rb b/lib/manager.rb index 4b3843c..244b14e 100644 --- a/lib/manager.rb +++ b/lib/manager.rb @@ -1,50 +1,10 @@ module DiscourseChat module Manager - KEY_PREFIX = 'category_'.freeze def self.guardian Guardian.new(User.find_by(username: SiteSetting.chat_discourse_username)) end - def self.get_store_key(cat_id = nil) - "#{KEY_PREFIX}#{cat_id.present? ? cat_id : '*'}" - end - - def self.get_all_rules - rules = [] - PluginStoreRow.where(plugin_name: DiscourseChat::PLUGIN_NAME) - .where("key ~* :pattern", pattern: "^#{DiscourseChat::Manager::KEY_PREFIX}.*") - .each do |row| - PluginStore.cast_value(row.type_name, row.value).each_with_index do |rule, thisIndex| - category_id = - if row.key == DiscourseChat::Manager.get_store_key - nil - else - row.key.gsub!(DiscourseChat::Manager::KEY_PREFIX, '') - row.key - end - - rules << { - id: "#{(category_id || 'all')}_#{thisIndex}", - provider: rule[:provider], - channel: rule[:channel], - filter: rule[:filter], - category_id: category_id, - tags: rule[:tags] - } - end - end - return rules - end - - def self.get_rules_for_provider(provider) - get_all_rules.select { |rule| rule[:provider] == provider } - end - - def self.get_rules_for_category(cat_id = nil) - PluginStore.get(DiscourseChat::PLUGIN_NAME, get_store_key(cat_id)) || [] - end - def self.trigger_notifications(post_id) Rails.logger.info("Triggering chat notifications for post #{post_id}") @@ -62,37 +22,37 @@ module DiscourseChat return if topic.blank? || topic.archetype == Archetype.private_message # Load all the rules that apply to this topic's category - matching_rules = get_rules_for_category(topic.category_id) + matching_rules = DiscourseChat::Rule.all_for_category(topic.category_id) if topic.category # Also load the rules for the wildcard category - matching_rules += get_rules_for_category(nil) + matching_rules += DiscourseChat::Rule.all_for_category(nil) end # If tagging is enabled, thow away rules that don't apply to this topic if SiteSetting.tagging_enabled topic_tags = topic.tags.present? ? topic.tags.pluck(:name) : [] matching_rules = matching_rules.select do |rule| - next true if rule[:tags].nil? or rule[:tags].empty? # Filter has no tags specified - any_tags_match = !((rule[:tags] & topic_tags).empty?) + next true if rule.tags.nil? or rule.tags.empty? # Filter has no tags specified + any_tags_match = !((rule.tags & topic_tags).empty?) next any_tags_match # If any tags match, keep this filter, otherwise throw away end end # Sort by order of precedence (mute always wins; watch beats follow) precedence = { 'mute' => 0, 'watch' => 1, 'follow' => 2} - sort_func = proc { |a, b| precedence[a[:filter]] <=> precedence[b[:filter]] } + sort_func = proc { |a, b| precedence[a.filter] <=> precedence[b.filter] } matching_rules = matching_rules.sort(&sort_func) # Take the first rule for each channel - uniq_func = proc { |rule| rule.values_at(:provider, :channel) } + uniq_func = proc { |rule| [rule.provider, rule.channel] } matching_rules = matching_rules.uniq(&uniq_func) # If a matching rule is set to mute, we can discard it now - matching_rules = matching_rules.select { |rule| rule[:filter] != "mute" } + matching_rules = matching_rules.select { |rule| rule.filter != "mute" } # If this is not the first post, discard all "follow" rules if not post.is_first_post? - matching_rules = matching_rules.select { |rule| rule[:filter] != "follow" } + matching_rules = matching_rules.select { |rule| rule.filter != "follow" } end # All remaining rules now require a notification to be sent @@ -101,10 +61,10 @@ 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]) + Rails.logger.info("Sending notification to provider #{rule.provider}, channel #{rule.channel}") + provider = ::DiscourseChat::Provider.get_by_name(rule.provider) if provider - provider.trigger_notification(post, rule[:channel]) + provider.trigger_notification(post, rule.channel) else puts "Can't find provider" # TODO: Handle when the provider does not exist @@ -113,16 +73,6 @@ module DiscourseChat end - def self.create_rule(provider, channel, filter, category_id, tags) - raise "Invalid filter" if !['mute','follow','watch'].include?(filter) - - data = get_rules_for_category(category_id) - tags = Tag.where(name: tags).pluck(:name) - tags = nil if tags.blank? - - data.push(provider: provider, channel: channel, filter: filter, tags: tags) - PluginStore.set(DiscourseChat::PLUGIN_NAME, get_store_key(category_id), data) - end end end \ No newline at end of file diff --git a/lib/rule.rb b/lib/rule.rb index 8c2432c..465d212 100644 --- a/lib/rule.rb +++ b/lib/rule.rb @@ -79,7 +79,7 @@ end # Use JSON selectors like this: - # Rule.where("value::json->>'provider'=?", "telegram") + # Rule.where("value::json->>'provider'=?", "slack") def self.where(*args) rows = self._all_raw.where(*args) self._from_psr_rows(rows) diff --git a/plugin.rb b/plugin.rb index 7071f2b..66951ff 100644 --- a/plugin.rb +++ b/plugin.rb @@ -5,6 +5,7 @@ enabled_site_setting :chat_enabled + after_initialize do module ::DiscourseChat @@ -60,19 +61,24 @@ after_initialize do requested_provider = params[:provider] if requested_provider.nil? - rules = DiscourseChat::Manager.get_all_rules() + rules = DiscourseChat::Rule.all elsif providers.include? requested_provider - rules = DiscourseChat::Manager.get_rules_for_provider(requested_provider) + rules = DiscourseChat::Rule.all_for_provider(requested_provider) else raise Discourse::NotFound end - render json: rules, root: 'rules' + render_serialized rules, DiscourseChat::RuleSerializer, root: 'rules' end + def update_rule + render json: {success: false} + end end - + class DiscourseChat::RuleSerializer < ActiveModel::Serializer + attributes :id, :provider, :channel, :category_id, :tags, :filter + end require_dependency 'admin_constraint' @@ -82,7 +88,9 @@ after_initialize do DiscourseChat::Engine.routes.draw do get "" => "chat#respond" get '/providers' => "chat#list_providers" + get '/rules' => "chat#list_rules" + put 'rules/:id' => "chat#update_rule" get "/:provider" => "chat#respond" end diff --git a/spec/lib/manager_spec.rb b/spec/lib/manager_spec.rb index 2fad16d..72e3093 100644 --- a/spec/lib/manager_spec.rb +++ b/spec/lib/manager_spec.rb @@ -31,10 +31,15 @@ RSpec.describe DiscourseChat::Manager do let(:provider) {::DiscourseChat::Provider::DummyProvider} + def create_rule(provider, channel, filter, category_id, tags) # Just shorthand for testing purposes + DiscourseChat::Rule.new({provider: provider, channel: channel, filter:filter, category_id:category_id, tags:tags}).save + end + it "should send a notification to watched and following channels for new topic" do - manager.create_rule('dummy', 'chan1', 'watch', category.id, nil) - manager.create_rule('dummy', 'chan2', 'follow', category.id, nil) - manager.create_rule('dummy', 'chan3', 'mute', category.id, nil) + + create_rule('dummy', 'chan1', 'watch', category.id, nil) + create_rule('dummy', 'chan2', 'follow', category.id, nil) + create_rule('dummy', 'chan3', 'mute', category.id, nil) manager.trigger_notifications(first_post.id) @@ -42,9 +47,9 @@ RSpec.describe DiscourseChat::Manager do end it "should send a notification only to watched for reply" do - manager.create_rule('dummy', 'chan1', 'watch', category.id, nil) - manager.create_rule('dummy', 'chan2', 'follow', category.id, nil) - manager.create_rule('dummy', 'chan3', 'mute', category.id, nil) + create_rule('dummy', 'chan1', 'watch', category.id, nil) + create_rule('dummy', 'chan2', 'follow', category.id, nil) + create_rule('dummy', 'chan3', 'mute', category.id, nil) manager.trigger_notifications(second_post.id) @@ -52,7 +57,7 @@ RSpec.describe DiscourseChat::Manager do end it "should respect wildcard category settings" do - manager.create_rule('dummy', 'chan1', 'watch', nil, nil) + create_rule('dummy', 'chan1', 'watch', nil, nil) manager.trigger_notifications(first_post.id) @@ -60,8 +65,8 @@ RSpec.describe DiscourseChat::Manager do end it "should respect mute over watch" do - manager.create_rule('dummy', 'chan1', 'watch', nil, nil) # Wildcard watch - manager.create_rule('dummy', 'chan1', 'mute', category.id, nil) # Specific mute + create_rule('dummy', 'chan1', 'watch', nil, nil) # Wildcard watch + create_rule('dummy', 'chan1', 'mute', category.id, nil) # Specific mute manager.trigger_notifications(first_post.id) @@ -69,8 +74,8 @@ RSpec.describe DiscourseChat::Manager do end it "should respect watch over follow" do - manager.create_rule('dummy', 'chan1', 'follow', nil, nil) - manager.create_rule('dummy', 'chan1', 'watch', category.id, nil) + create_rule('dummy', 'chan1', 'follow', nil, nil) + create_rule('dummy', 'chan1', 'watch', category.id, nil) manager.trigger_notifications(second_post.id) @@ -78,7 +83,7 @@ RSpec.describe DiscourseChat::Manager do end it "should not notify about private messages" do - manager.create_rule('dummy', 'chan1', 'watch', nil, nil) + create_rule('dummy', 'chan1', 'watch', nil, nil) private_post = Fabricate(:private_message_post) manager.trigger_notifications(private_post.id) @@ -87,7 +92,7 @@ RSpec.describe DiscourseChat::Manager do end it "should not notify about private messages" do - manager.create_rule('dummy', 'chan1', 'watch', nil, nil) + create_rule('dummy', 'chan1', 'watch', nil, nil) private_post = Fabricate(:private_message_post) manager.trigger_notifications(private_post.id) @@ -96,7 +101,7 @@ RSpec.describe DiscourseChat::Manager do end it "should not notify about posts the chat_user cannot see" do - manager.create_rule('dummy', 'chan1', 'watch', nil, nil) + create_rule('dummy', 'chan1', 'watch', nil, nil) # Create a group & user group = Fabricate(:group, name: "friends") @@ -139,7 +144,7 @@ RSpec.describe DiscourseChat::Manager do end it 'should still work for rules without any tags specified' do - manager.create_rule('dummy', 'chan1', 'watch', category.id, nil) + create_rule('dummy', 'chan1', 'watch', category.id, nil) manager.trigger_notifications(first_post.id) manager.trigger_notifications(tagged_first_post.id) @@ -148,7 +153,7 @@ RSpec.describe DiscourseChat::Manager do end it 'should only match tagged topics when rule has tags' do - manager.create_rule('dummy', 'chan1', 'watch', category.id, [tag.name]) + create_rule('dummy', 'chan1', 'watch', category.id, [tag.name]) manager.trigger_notifications(first_post.id) manager.trigger_notifications(tagged_first_post.id) @@ -159,36 +164,4 @@ RSpec.describe DiscourseChat::Manager do end end - - describe '.create_rule' do - it 'should add new rule correctly' do - expect do - manager.create_rule('dummy', 'chan1', 'watch', category.id, nil) - end.to change { manager.get_rules_for_category(category.id).length }.by(1) - - expect do - manager.create_rule('dummy', 'chan2', 'follow', category.id, nil) - end.to change { manager.get_rules_for_category(category.id).length }.by(1) - end - - it 'should accept tags correctly' do - tag = Fabricate(:tag) - expect do - manager.create_rule('dummy', 'chan1', 'watch', category.id, [tag.name, 'faketag']) - end.to change { manager.get_rules_for_category(category.id).length }.by(1) - - expect(manager.get_rules_for_category(category.id).first[:tags]).to contain_exactly(tag.name) - - end - - it 'should error on invalid filter strings' do - expect do - manager.create_rule('dummy', 'chan1', 'invalid_filter', category.id, nil) - end.to raise_error(RuntimeError, "Invalid filter") - end - - - end - - end \ No newline at end of file