From 56da3639ab40801dcfa0f09a200585eef9313c53 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 12 Jul 2017 18:28:45 +0100 Subject: [PATCH] =?UTF-8?q?Use=20real=20ActiveRecord=20for=20the=20?= =?UTF-8?q?=E2=80=9CRule=E2=80=9D=20model=20by=20extending=20the=20PluginS?= =?UTF-8?q?toreRow=20class.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This gives us all the activerecord awesomeness for free, like validations, safe form params etc. --- lib/discourse_chat/helper.rb | 6 +- lib/discourse_chat/manager.rb | 11 +- lib/discourse_chat/provider.rb | 4 + lib/discourse_chat/rule.rb | 265 ++++++++---------------- plugin.rb | 13 +- spec/chat_controller_spec.rb | 6 +- spec/lib/discourse_chat/manager_spec.rb | 2 +- spec/lib/discourse_chat/rule_spec.rb | 35 ++-- 8 files changed, 132 insertions(+), 210 deletions(-) diff --git a/lib/discourse_chat/helper.rb b/lib/discourse_chat/helper.rb index 34bf1b9..ba96b35 100644 --- a/lib/discourse_chat/helper.rb +++ b/lib/discourse_chat/helper.rb @@ -3,7 +3,7 @@ module DiscourseChat # Produce a string with a list of all rules associated with a channel def self.status_for_channel(provider, channel) - rules = DiscourseChat::Rule.all_for_channel(provider, channel) + rules = DiscourseChat::Rule.with_channel(provider, channel) text = I18n.t("chat_integration.provider.#{provider}.status.header") + "\n" @@ -44,7 +44,7 @@ module DiscourseChat # Delete a rule based on its (1 based) index as seen in the # status_for_channel function def self.delete_by_index(provider, channel, index) - rules = DiscourseChat::Rule.all_for_channel(provider, channel) + rules = DiscourseChat::Rule.with_channel(provider, channel) return false if index < 1 or index > rules.size @@ -59,7 +59,7 @@ module DiscourseChat # :created if a new rule has been created # false if there was an error def self.smart_create_rule(provider:, channel:, filter:, category_id:, tags:) - existing_rules = DiscourseChat::Rule.all_for_channel(provider, channel) + existing_rules = DiscourseChat::Rule.with_channel(provider, channel) # Select the ones that have the same category same_category = existing_rules.select { |rule| rule.category_id == category_id } diff --git a/lib/discourse_chat/manager.rb b/lib/discourse_chat/manager.rb index 76b8098..5368acc 100644 --- a/lib/discourse_chat/manager.rb +++ b/lib/discourse_chat/manager.rb @@ -22,10 +22,10 @@ module DiscourseChat return if topic.blank? || topic.archetype == Archetype.private_message # Load all the rules that apply to this topic's category - matching_rules = DiscourseChat::Rule.all_for_category(topic.category_id) + matching_rules = DiscourseChat::Rule.with_category(topic.category_id) if topic.category # Also load the rules for the wildcard category - matching_rules += DiscourseChat::Rule.all_for_category(nil) + matching_rules += DiscourseChat::Rule.with_category(nil) end # If tagging is enabled, thow away rules that don't apply to this topic @@ -67,14 +67,13 @@ module DiscourseChat if provider and is_enabled begin provider.trigger_notification(post, rule.channel) - rule.update({error_key: nil}, false) if rule.error_key + 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.error_key = e.info[:error_key] + rule.update_attribute('error_key', e.info[:error_key]) else - rule.error_key = 'chat_integration.rule_exception' + rule.update_attribute('error_key','chat_integration.rule_exception') end - rule.save(false) # Save without validations # Log the error Discourse.handle_job_exception(e, diff --git a/lib/discourse_chat/provider.rb b/lib/discourse_chat/provider.rb index f81f1fb..2e42c86 100644 --- a/lib/discourse_chat/provider.rb +++ b/lib/discourse_chat/provider.rb @@ -21,6 +21,10 @@ module DiscourseChat end end + def self.provider_names + self.providers.map {|x| x::PROVIDER_NAME} + end + def self.enabled_provider_names self.enabled_providers.map {|x| x::PROVIDER_NAME} end diff --git a/lib/discourse_chat/rule.rb b/lib/discourse_chat/rule.rb index 0de9f99..24baf26 100644 --- a/lib/discourse_chat/rule.rb +++ b/lib/discourse_chat/rule.rb @@ -1,186 +1,105 @@ - # Similar to an ActiveRecord class, but uses PluginStore for storage instead. Adapted from discourse-data-explorer - # 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, :error_key +class DiscourseChat::Rule < PluginStoreRow + PLUGIN_NAME = 'discourse-chat-integration' + KEY_PREFIX = 'rule:' + + # Restrict the scope to JSON PluginStoreRows which are for this plugin, and this model + default_scope { where(type_name: 'JSON') + .where(plugin_name: PLUGIN_NAME) + .where("key like?", "#{KEY_PREFIX}%") + } - def initialize(h={}) - @provider = '' - @channel = '' - @category_id = nil - @tags = nil - @filter = 'watch' - @error_key = nil - h.each {|k,v| public_send("#{k}=",v)} + # Setup ActiveRecord::Store to use the JSON field to read/write these values + store :value, accessors: [ :provider, :channel, :category_id, :tags, :filter, :error_key ], coder: JSON + + before_save :set_key + + after_initialize :init + + def init + self.filter ||= 'watch' + end + + validates :filter, :inclusion => { :in => %w(watch follow mute), + :message => "%{value} is not a valid filter" } + + validate :provider_and_channel_valid?, :category_valid?, :tags_valid? + + def provider_and_channel_valid? + # Validate provider + if not ::DiscourseChat::Provider.provider_names.include? provider + errors.add(:provider, "#{provider} is not a valid provider") + return + end + + # Validate channel + if channel.blank? + errors.add(:channel, "channel cannot be blank") + return end - def tags=(array) - if array.nil? or array.empty? - @tags = nil - else - @tags = array + provider_class = ::DiscourseChat::Provider.get_by_name(provider) + if defined? provider_class::PROVIDER_CHANNEL_REGEX + channel_regex = Regexp.new provider_class::PROVIDER_CHANNEL_REGEX + if not channel_regex.match?(channel) + errors.add(:channel, "#{channel} is not a valid channel for provider #{provider}") end end + end - def category_id=(val) - if val.nil? or val.blank? - @category_id = nil - else - @category_id = val.to_i + def category_valid? + # Validate category + if not (category_id.nil? or Category.where(id: category_id).exists?) + errors.add(:category_id, "#{category_id} is not a valid category id") + end + end + + def tags_valid? + # Validate tags + return if tags.nil? + tags.each do |tag| + if not Tag.where(name: tag).exists? + errors.add(:tags, "#{tag} is not a valid tag") end end + end - # saving/loading functions - def self.alloc_id - DistributedMutex.synchronize('discourse-chat_rule-id') do - max_id = DiscourseChat.pstore_get("rule:_id") + # We never want an empty array, set it to nil instead + def tags=(array) + if array.nil? or array.empty? + super(nil) + else + super(array) + end + end + + # Don't want this to end up as anything other than an integer + def category_id=(val) + if val.nil? or val.blank? + super(nil) + else + super(val.to_i) + end + end + + scope :with_provider, ->(provider) { where("value::json->>'provider'=?", provider)} + + scope :with_channel, ->(provider, channel) { with_provider(provider).where("value::json->>'channel'=?", channel)} + + scope :with_category, ->(category_id) { category_id.nil? ? where("(value::json->'category_id') IS NULL OR json_typeof(value::json->'category_id')='null'") : where("value::json->>'category_id'=?", category_id.to_s)} + + private + + def set_key + self.key ||= alloc_key + end + + def alloc_key + DistributedMutex.synchronize("#{PLUGIN_NAME}_#{KEY_PREFIX}_id") do + max_id = PluginStore.get(PLUGIN_NAME, "#{KEY_PREFIX}_id") max_id = 1 unless max_id - DiscourseChat.pstore_set("rule:_id", max_id + 1) - max_id + PluginStore.set(PLUGIN_NAME, "#{KEY_PREFIX}_id", max_id + 1) + "#{KEY_PREFIX}#{max_id}" end end - def self.from_hash(h) - rule = DiscourseChat::Rule.new - [:provider, :channel, :category_id, :tags, :filter, :error_key].each do |sym| - rule.send("#{sym}=", h[sym]) if h[sym] - end - if h[:id] - rule.id = h[:id].to_i - end - rule - end - - def to_hash - { - id: @id, - provider: @provider, - channel: @channel, - category_id: @category_id, - tags: @tags, - filter: @filter, - error_key: @error_key, - } - end - - def self.find(id, opts={}) - hash = DiscourseChat.pstore_get("rule:#{id}") - unless hash - return DiscourseChat::Rule.new if opts[:ignore_deleted] - raise Discourse::NotFound - end - from_hash hash - end - - 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(validate) - end - - def save(validate=true) - if validate - return false if not valid? - end - - unless @id && @id > 0 - @id = self.class.alloc_id - end - DiscourseChat.pstore_set "rule:#{id}", to_hash - return self - end - - def save!(validate=true) - if not save(validate) - raise 'Rule not valid' - end - return self - end - - def valid? - - # Validate provider - return false if not ::DiscourseChat::Provider.providers.map {|x| x::PROVIDER_NAME}.include? @provider - - # Validate channel - return false if @channel.blank? - - provider = ::DiscourseChat::Provider.get_by_name(@provider) - if defined? provider::PROVIDER_CHANNEL_REGEX - channel_regex = Regexp.new provider::PROVIDER_CHANNEL_REGEX - return false if not channel_regex.match?(@channel) - end - - # Validate category - return false if not (@category_id.nil? or Category.where(id: @category_id).exists?) - - # Validate tags - if not @tags.nil? - @tags.each do |tag| - return false if not Tag.where(name: tag).exists? - end - end - - # Validate filter - return false if not ['watch','follow','mute'].include? @filter - - return true - end - - def destroy - DiscourseChat.pstore_delete "rule:#{id}" - end - - def read_attribute_for_serialization(attr) - self.send(attr) - end - - def self.all_for_provider(provider) - self.where("value::json->>'provider'=?", provider) - end - - def self.all_for_channel(provider, channel) - self.where("value::json->>'provider'=? AND value::json->>'channel'=?", provider, channel) - end - - def self.all_for_category(category_id) - if category_id.nil? - self.where("json_typeof(value::json->'category_id')='null'") - else - self.where("value::json->>'category_id'=?", category_id.to_s) - end - end - - # Use JSON selectors like this: - # Rule.where("value::json->>'provider'=?", "slack") - def self.where(*args) - rows = self._all_raw.where(*args) - self._from_psr_rows(rows) - end - - def self.all - self._from_psr_rows(self._all_raw) - end - - def self._all_raw - PluginStoreRow.where(plugin_name: DiscourseChat.plugin_name) - .where("key LIKE 'rule:%'") - .where("key != 'rule:_id'") - end - - def self._from_psr_rows(raw) - rules = raw.map do |psr| - from_hash PluginStore.cast_value(psr.type_name, psr.value) - end - - filter_order = ["mute", "watch", "follow"] - rules = rules.sort_by{ |r| [r.channel, r.category_id.nil? ? 0 : r.category_id, filter_order.index(r.filter)] } - return rules - end - - def self.destroy_all - self._all_raw().destroy_all - end - end \ No newline at end of file +end \ No newline at end of file diff --git a/plugin.rb b/plugin.rb index f68ab68..fbbe0e6 100644 --- a/plugin.rb +++ b/plugin.rb @@ -119,7 +119,7 @@ after_initialize do requested_provider = params[:provider] if providers.include? requested_provider - rules = DiscourseChat::Rule.all_for_provider(requested_provider) + rules = DiscourseChat::Rule.with_provider(requested_provider) else raise Discourse::NotFound end @@ -129,10 +129,11 @@ after_initialize do def create_rule begin - rule = DiscourseChat::Rule.new() - hash = params.require(:rule) + hash = params.require(:rule).permit(:provider, :channel, :filter, :category_id, tags:[]) - if not rule.update(hash) + rule = DiscourseChat::Rule.new(hash) + + if not rule.save(hash) raise Discourse::InvalidParameters, 'Rule is not valid' end @@ -146,8 +147,8 @@ after_initialize do begin rule = DiscourseChat::Rule.find(params[:id].to_i) rule.error_key = nil # Reset any error on the rule - hash = params.require(:rule) - + hash = params.require(:rule).permit(:provider, :channel, :filter, :category_id, tags:[]) + if not rule.update(hash) raise Discourse::InvalidParameters, 'Rule is not valid' end diff --git a/spec/chat_controller_spec.rb b/spec/chat_controller_spec.rb index f3351e3..bc2c1dc 100644 --- a/spec/chat_controller_spec.rb +++ b/spec/chat_controller_spec.rb @@ -87,7 +87,7 @@ describe 'Chat Controller', type: :request do end it 'should return the right response' do - rule = DiscourseChat::Rule.new({provider: 'dummy', channel: '#general', filter:'follow', category_id:category.id, tags:[tag.name]}).save! + rule = DiscourseChat::Rule.create({provider: 'dummy', channel: '#general', filter:'follow', category_id:category.id, tags:[tag.name]}) get '/admin/plugins/chat/rules.json', provider:'dummy' @@ -165,7 +165,7 @@ describe 'Chat Controller', type: :request do end describe 'updating a rule' do - let(:rule){DiscourseChat::Rule.new({provider: 'dummy', channel: '#general', filter:'follow', category_id:category.id, tags:[tag.name]}).save!} + let(:rule){DiscourseChat::Rule.create({provider: 'dummy', channel: '#general', filter:'follow', category_id:category.id, tags:[tag.name]})} include_examples 'admin constraints', 'put', "/admin/plugins/chat/rules/1.json" @@ -214,7 +214,7 @@ describe 'Chat Controller', type: :request do end describe 'deleting a rule' do - let(:rule){DiscourseChat::Rule.new({provider: 'dummy', channel: '#general', filter:'follow', category_id:category.id, tags:[tag.name]}).save!} + let(:rule){DiscourseChat::Rule.create({provider: 'dummy', channel: '#general', filter:'follow', category_id:category.id, tags:[tag.name]})} include_examples 'admin constraints', 'delete', "/admin/plugins/chat/rules/1.json" diff --git a/spec/lib/discourse_chat/manager_spec.rb b/spec/lib/discourse_chat/manager_spec.rb index 525e761..29ade51 100644 --- a/spec/lib/discourse_chat/manager_spec.rb +++ b/spec/lib/discourse_chat/manager_spec.rb @@ -18,7 +18,7 @@ RSpec.describe DiscourseChat::Manager do end 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! + DiscourseChat::Rule.create({provider: provider, channel: channel, filter:filter, category_id:category_id, tags:tags}) end it "should fail gracefully when a provider throws an exception" do diff --git a/spec/lib/discourse_chat/rule_spec.rb b/spec/lib/discourse_chat/rule_spec.rb index 2d6abee..595f0c0 100644 --- a/spec/lib/discourse_chat/rule_spec.rb +++ b/spec/lib/discourse_chat/rule_spec.rb @@ -1,29 +1,28 @@ require 'rails_helper' - - RSpec.describe DiscourseChat::Rule do + let(:tag1){Fabricate(:tag)} let(:tag2){Fabricate(:tag)} - describe '.alloc_id' do + describe '.alloc_key' do it 'should return sequential numbers' do - expect( DiscourseChat::Rule.alloc_id ).to eq(1) - expect( DiscourseChat::Rule.alloc_id ).to eq(2) - expect( DiscourseChat::Rule.alloc_id ).to eq(3) + expect( DiscourseChat::Rule.create(provider:'slack',channel:'#general').key ).to eq("rule:1") + expect( DiscourseChat::Rule.create(provider:'slack',channel:'#general').key ).to eq("rule:2") + expect( DiscourseChat::Rule.create(provider:'slack',channel:'#general').key ).to eq("rule:3") end end it 'should save and load successfully' do expect(DiscourseChat::Rule.all.length).to eq(0) - rule = DiscourseChat::Rule.new({ + rule = DiscourseChat::Rule.create({ provider:"slack", channel: "#general", category_id: 1, tags: [tag1.name, tag2.name], filter: 'watch' - }).save! + }) expect(DiscourseChat::Rule.all.length).to eq(1) @@ -39,12 +38,12 @@ RSpec.describe DiscourseChat::Rule do describe 'general operations' do before do - rule = DiscourseChat::Rule.new({ + rule = DiscourseChat::Rule.create({ provider:"slack", channel: "#general", category_id: 1, tags: [tag1.name, tag2.name] - }).save! + }) end it 'can be modified' do @@ -86,8 +85,8 @@ RSpec.describe DiscourseChat::Rule do expect(DiscourseChat::Rule.all.length).to eq(3) - expect(DiscourseChat::Rule.all_for_provider('slack').length).to eq(2) - expect(DiscourseChat::Rule.all_for_provider('telegram').length).to eq(1) + expect(DiscourseChat::Rule.with_provider('slack').length).to eq(2) + expect(DiscourseChat::Rule.with_provider('telegram').length).to eq(1) end it 'can be filtered by channel' do @@ -98,8 +97,8 @@ RSpec.describe DiscourseChat::Rule do expect(DiscourseChat::Rule.all.length).to eq(5) - expect(DiscourseChat::Rule.all_for_channel('slack','#general').length).to eq(3) - expect(DiscourseChat::Rule.all_for_channel('slack', '#blah').length).to eq(1) + expect(DiscourseChat::Rule.with_channel('slack','#general').length).to eq(3) + expect(DiscourseChat::Rule.with_channel('slack', '#blah').length).to eq(1) end it 'can be filtered by category' do @@ -108,20 +107,20 @@ RSpec.describe DiscourseChat::Rule do expect(DiscourseChat::Rule.all.length).to eq(3) - expect(DiscourseChat::Rule.all_for_category(1).length).to eq(2) - expect(DiscourseChat::Rule.all_for_category(nil).length).to eq(1) + expect(DiscourseChat::Rule.with_category(1).length).to eq(2) + expect(DiscourseChat::Rule.with_category(nil).length).to eq(1) end end describe 'validations' do let(:rule) do - DiscourseChat::Rule.new({ + DiscourseChat::Rule.create({ filter: 'watch', provider:"slack", channel: "#general", category_id: 1, - }).save! + }) end it 'validates provider correctly' do