diff --git a/app/controllers/tag_aliases_controller.rb b/app/controllers/tag_aliases_controller.rb index 9bd339a2e..27957e3b3 100644 --- a/app/controllers/tag_aliases_controller.rb +++ b/app/controllers/tag_aliases_controller.rb @@ -15,7 +15,7 @@ class TagAliasesController < ApplicationController @tag_alias = TagAlias.find(params[:id]) if @tag_alias.is_pending? && @tag_alias.editable_by?(CurrentUser.user) - @tag_alias.update_attributes(params[:tag_alias]) + @tag_alias.update_attributes(update_params) end respond_with(@tag_alias) @@ -46,4 +46,10 @@ class TagAliasesController < ApplicationController @tag_alias.approve!(CurrentUser.user.id) respond_with(@tag_alias, :location => tag_alias_path(@tag_alias)) end + +private + + def update_params + params.require(:tag_alias).permit(:antecedent_name, :consequent_name, :forum_topic_id) + end end diff --git a/app/controllers/tag_implications_controller.rb b/app/controllers/tag_implications_controller.rb index a5cfafc68..a893b53db 100644 --- a/app/controllers/tag_implications_controller.rb +++ b/app/controllers/tag_implications_controller.rb @@ -15,7 +15,7 @@ class TagImplicationsController < ApplicationController @tag_implication = TagImplication.find(params[:id]) if @tag_implication.is_pending? && @tag_implication.editable_by?(CurrentUser.user) - @tag_implication.update_attributes(params[:tag_implication]) + @tag_implication.update_attributes(update_params) end respond_with(@tag_implication) @@ -51,4 +51,10 @@ class TagImplicationsController < ApplicationController @tag_implication.approve!(CurrentUser.user.id) respond_with(@tag_implication, :location => tag_implication_path(@tag_implication)) end + +private + + def update_params + params.require(:tag_implication).permit(:antecedent_name, :consequent_name, :forum_topic_id) + end end diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index ebbe6d148..d7fa41245 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -6,7 +6,11 @@ class TagAlias < ActiveRecord::Base after_destroy :clear_all_cache before_validation :initialize_creator, :on => :create before_validation :normalize_names + validates_format_of :status, :with => /\A(active|deleted|pending|processing|queued|error: .*)\Z/ validates_presence_of :creator_id, :antecedent_name, :consequent_name + validates :creator, presence: { message: "must exist" }, if: lambda { creator_id.present? } + validates :approver, presence: { message: "must exist" }, if: lambda { approver_id.present? } + validates :forum_topic, presence: { message: "must exist" }, if: lambda { forum_topic_id.present? } validates_uniqueness_of :antecedent_name validate :absence_of_transitive_relation validate :antecedent_and_consequent_are_different @@ -15,7 +19,8 @@ class TagAlias < ActiveRecord::Base belongs_to :creator, :class_name => "User" belongs_to :approver, :class_name => "User" belongs_to :forum_topic - attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :status, :skip_secondary_validations + attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :skip_secondary_validations + attr_accessible :status, :as => [:admin] module SearchMethods def name_matches(name) diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 58aa90aff..bf033b0e6 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -9,14 +9,19 @@ class TagImplication < ActiveRecord::Base belongs_to :forum_topic before_validation :initialize_creator, :on => :create before_validation :normalize_names + validates_format_of :status, :with => /\A(active|deleted|pending|processing|queued|error: .*)\Z/ validates_presence_of :creator_id, :antecedent_name, :consequent_name + validates :creator, presence: { message: "must exist" }, if: lambda { creator_id.present? } + validates :approver, presence: { message: "must exist" }, if: lambda { approver_id.present? } + validates :forum_topic, presence: { message: "must exist" }, if: lambda { forum_topic_id.present? } validates_uniqueness_of :antecedent_name, :scope => :consequent_name validate :absence_of_circular_relation validate :antecedent_is_not_aliased validate :consequent_is_not_aliased validate :antecedent_and_consequent_are_different validate :wiki_pages_present, :on => :create - attr_accessible :antecedent_name, :consequent_name, :descendant_names, :forum_topic_id, :status, :forum_topic, :skip_secondary_validations + attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :skip_secondary_validations + attr_accessible :status, :as => [:admin] module DescendantMethods extend ActiveSupport::Concern diff --git a/test/functional/tag_aliases_controller_test.rb b/test/functional/tag_aliases_controller_test.rb index 30c99ce70..1d099e8d9 100644 --- a/test/functional/tag_aliases_controller_test.rb +++ b/test/functional/tag_aliases_controller_test.rb @@ -41,6 +41,15 @@ class TagAliasesControllerTest < ActionController::TestCase @tag_alias.reload assert_equal("xxx", @tag_alias.antecedent_name) end + + should "not allow changing the status" do + post :update, {:id => @tag_alias.id, :tag_alias => {:status => "active"}}, {:user_id => @user.id} + @tag_alias.reload + assert_equal("pending", @tag_alias.status) + end + + # TODO: Broken in shoulda-matchers 2.8.0. Need to upgrade to 3.1.1. + should_eventually permit(:antecedent_name, :consequent_name, :forum_topic_id).for(:update) end context "for an approved alias" do diff --git a/test/functional/tag_implications_controller_test.rb b/test/functional/tag_implications_controller_test.rb index 8fa68f1d7..e6408eb02 100644 --- a/test/functional/tag_implications_controller_test.rb +++ b/test/functional/tag_implications_controller_test.rb @@ -42,6 +42,15 @@ class TagImplicationsControllerTest < ActionController::TestCase @tag_implication.reload assert_equal("xxx", @tag_implication.antecedent_name) end + + should "not allow changing the status" do + post :update, {:id => @tag_implication.id, :tag_implication => {:status => "active"}}, {:user_id => @user.id} + @tag_implication.reload + assert_equal("pending", @tag_implication.status) + end + + # TODO: Broken in shoulda-matchers 2.8.0. Need to upgrade to 3.1.1. + should_eventually permit(:antecedent_name, :consequent_name, :forum_topic_id).for(:update) end context "for an approved implication" do diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index 5ad89ca74..16bce66c9 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -17,6 +17,36 @@ class TagAliasTest < ActiveSupport::TestCase CurrentUser.ip_addr = nil end + context "on validation" do + subject do + FactoryGirl.create(:tag, :name => "aaa") + FactoryGirl.create(:tag, :name => "bbb") + FactoryGirl.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb") + end + + should allow_value('active').for(:status) + should allow_value('deleted').for(:status) + should allow_value('pending').for(:status) + should allow_value('processing').for(:status) + should allow_value('queued').for(:status) + should allow_value('error: derp').for(:status) + + should_not allow_value('ACTIVE').for(:status) + should_not allow_value('error').for(:status) + should_not allow_value('derp').for(:status) + + should allow_value(nil).for(:forum_topic_id) + should_not allow_value(-1).for(:forum_topic_id).with_message("must exist", against: :forum_topic) + + should allow_value(nil).for(:approver_id) + should_not allow_value(-1).for(:approver_id).with_message("must exist", against: :approver) + + should_not allow_value(nil).for(:creator_id) + should_not allow_value(-1).for(:creator_id).with_message("must exist", against: :creator) + + should_not allow_mass_assignment_of(:status).as(:member) + end + should "populate the creator information" do ta = FactoryGirl.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb") assert_equal(CurrentUser.user.id, ta.creator_id) diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index 5a4fc922b..910dd8604 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -16,6 +16,38 @@ class TagImplicationTest < ActiveSupport::TestCase CurrentUser.ip_addr = nil end + context "on validation" do + subject do + FactoryGirl.create(:tag, :name => "aaa") + FactoryGirl.create(:tag, :name => "bbb") + FactoryGirl.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") + end + + should allow_value('active').for(:status) + should allow_value('deleted').for(:status) + should allow_value('pending').for(:status) + should allow_value('processing').for(:status) + should allow_value('queued').for(:status) + should allow_value('error: derp').for(:status) + + should_not allow_value('ACTIVE').for(:status) + should_not allow_value('error').for(:status) + should_not allow_value('derp').for(:status) + + should allow_value(nil).for(:forum_topic_id) + should_not allow_value(-1).for(:forum_topic_id).with_message("must exist", against: :forum_topic) + + should allow_value(nil).for(:approver_id) + should_not allow_value(-1).for(:approver_id).with_message("must exist", against: :approver) + + should_not allow_value(nil).for(:creator_id) + should_not allow_value(-1).for(:creator_id).with_message("must exist", against: :creator) + + should_not allow_mass_assignment_of(:status).as(:member) + should_not allow_mass_assignment_of(:forum_topic).as(:member) + should_not allow_mass_assignment_of(:descendant_names).as(:member) + end + should "ignore pending implications when building descendant names" do ti2 = FactoryGirl.build(:tag_implication, :antecedent_name => "b", :consequent_name => "c", :status => "pending") ti2.save