Merge pull request #2733 from evazion/fix-tag-normalization

Fix metatag editing so that rating:s obeys rate locks, source:blah updates pixiv id

app/models/post.rb

@@ -7,27 +7,31 @@ class Post < ActiveRecord::Base
   class RevertError < Exception ; end
   class SearchError < Exception ; end
 
-  attr_accessor :old_tag_string, :old_parent_id, :old_source, :old_rating, :has_constraints, :disable_versioning, :view_count
+  before_validation :initialize_uploader, :on => :create
-  after_destroy :remove_iqdb_async
+  before_validation :merge_old_changes
-  after_destroy :delete_files
+  before_validation :normalize_tags
-  after_destroy :delete_remote_files
+  before_validation :strip_source
-  after_save :create_version
+  before_validation :parse_pixiv_id
-  after_save :update_parent_on_save
+  before_validation :blank_out_nonexistent_parents
-  after_save :apply_post_metatags
+  before_validation :remove_parent_loops
-  after_save :expire_essential_tag_string_cache
+  validates_uniqueness_of :md5
-  after_create :update_iqdb_async
+  validates_inclusion_of :rating, in: %w(s q e), message: "rating must be s, q, or e"
-  after_commit :notify_pubsub
+  validate :post_is_not_its_own_parent
-  before_save :merge_old_changes
+  validate :updater_can_change_rating
-  before_save :normalize_tags
   before_save :update_tag_post_counts
   before_save :set_tag_counts
   before_save :set_pool_category_pseudo_tags
   before_create :autoban
-  before_validation :strip_source
+  after_create :update_iqdb_async
-  before_validation :initialize_uploader, :on => :create
+  after_save :create_version
-  before_validation :parse_pixiv_id
+  after_save :update_parent_on_save
-  before_validation :blank_out_nonexistent_parents
+  after_save :apply_post_metatags
-  before_validation :remove_parent_loops
+  after_save :expire_essential_tag_string_cache
+  after_destroy :remove_iqdb_async
+  after_destroy :delete_files
+  after_destroy :delete_remote_files
+  after_commit :notify_pubsub
+
   belongs_to :updater, :class_name => "User"
   belongs_to :approver, :class_name => "User"
   belongs_to :uploader, :class_name => "User"
@@ -44,13 +48,10 @@ class Post < ActiveRecord::Base
   has_many :children, lambda {order("posts.id")}, :class_name => "Post", :foreign_key => "parent_id"
   has_many :disapprovals, :class_name => "PostDisapproval", :dependent => :destroy
   has_many :favorites, :dependent => :destroy
-  validates_uniqueness_of :md5
-  validates_inclusion_of :rating, in: %w(s q e), message: "rating must be s, q, or e"
-  validate :post_is_not_its_own_parent
-  validate :updater_can_change_rating
   attr_accessible :source, :rating, :tag_string, :old_tag_string, :old_parent_id, :old_source, :old_rating, :parent_id, :has_embedded_notes, :as => [:member, :builder, :gold, :platinum, :janitor, :moderator, :admin, :default]
   attr_accessible :is_rating_locked, :is_note_locked, :as => [:builder, :janitor, :moderator, :admin]
   attr_accessible :is_status_locked, :as => [:admin]
+  attr_accessor :old_tag_string, :old_parent_id, :old_source, :old_rating, :has_constraints, :disable_versioning, :view_count
 
   module FileMethods
     def distribute_files

test/unit/post_test.rb

@@ -107,7 +107,7 @@ class PostTest < ActiveSupport::TestCase
   end
 
   context "Parenting:" do
-    context "Assignining a parent to a post" do
+    context "Assigning a parent to a post" do
       should "update the has_children flag on the parent" do
         p1 = FactoryGirl.create(:post)
         assert(!p1.has_children?, "Parent should not have any children")
@@ -460,7 +460,7 @@ class PostTest < ActiveSupport::TestCase
 
       context "as a new user" do
         setup do
-          @post.update_attribute(:tag_string, "aaa bbb ccc ddd tagme")
+          @post.update(:tag_string => "aaa bbb ccc ddd tagme")
           CurrentUser.user = FactoryGirl.create(:user)
         end
 
@@ -478,8 +478,10 @@ class PostTest < ActiveSupport::TestCase
 
       context "with a banned artist" do
         setup do
-          @artist = FactoryGirl.create(:artist)
+          CurrentUser.scoped(FactoryGirl.create(:admin_user)) do
-          @artist.ban!
+            @artist = FactoryGirl.create(:artist)
+            @artist.ban!
+          end
           @post = FactoryGirl.create(:post, :tag_string => @artist.name)
         end
 
@@ -493,9 +495,9 @@ class PostTest < ActiveSupport::TestCase
           CurrentUser.user = FactoryGirl.create(:builder_user)
           Delayed::Worker.delay_jobs = false
           @post = Post.find(@post.id)
-          @post.update_attribute(:tag_string, "art:abc")
+          @post.update(:tag_string => "art:abc")
           @post = Post.find(@post.id)
-          @post.update_attribute(:tag_string, "copy:abc")
+          @post.update(:tag_string => "copy:abc")
           @post.reload
         end
 
@@ -522,7 +524,7 @@ class PostTest < ActiveSupport::TestCase
         setup do
           FactoryGirl.create(:tag_alias, :antecedent_name => "abc", :consequent_name => "xyz")
           @post = Post.find(@post.id)
-          @post.update_attribute(:tag_string, "art:abc")
+          @post.update(:tag_string => "art:abc")
           @post.reload
         end
 
@@ -544,6 +546,27 @@ class PostTest < ActiveSupport::TestCase
             assert_equal(@parent.id, @post.parent_id)
             assert(@parent.has_children?)
           end
+
+          should "not allow self-parenting" do
+            @post.update(:tag_string => "parent:#{@post.id}")
+            assert_nil(@post.parent_id)
+          end
+
+          should "clear the parent with parent:none" do
+            @post.update(:parent_id => @parent.id)
+            assert_equal(@parent.id, @post.parent_id)
+
+            @post.update(:tag_string => "parent:none")
+            assert_nil(@post.parent_id)
+          end
+
+          should "clear the parent with -parent:1234" do
+            @post.update(:parent_id => @parent.id)
+            assert_equal(@parent.id, @post.parent_id)
+
+            @post.update(:tag_string => "-parent:#{@parent.id}")
+            assert_nil(@post.parent_id)
+          end
         end
 
         context "for a pool" do
@@ -622,7 +645,7 @@ class PostTest < ActiveSupport::TestCase
 
         context "for a rating" do
           context "that is valid" do
-            should "update the rating" do
+            should "update the rating if the post is unlocked" do
               @post.update_attributes(:tag_string => "aaa rating:e")
               @post.reload
               assert_equal("e", @post.rating)
@@ -636,13 +659,34 @@ class PostTest < ActiveSupport::TestCase
               assert_equal("q", @post.rating)
             end
           end
+
+          context "that is locked" do
+            should "change the rating if locked in the same update" do
+              @post.update({ :tag_string => "rating:e", :is_rating_locked => true }, :as => :builder)
+
+              assert(@post.valid?)
+              assert_equal("e", @post.reload.rating)
+            end
+
+            should "not change the rating if locked previously" do
+              @post.is_rating_locked = true
+              @post.save
+
+              @post.update(:tag_string => "rating:e")
+
+              assert(@post.invalid?)
+              assert_not_equal("e", @post.reload.rating)
+            end
+          end
         end
 
         context "for a fav" do
-          should "add the current user to the post's favorite listing" do
+          should "add/remove the current user to the post's favorite listing" do
             @post.update_attributes(:tag_string => "aaa fav:self")
-            @post.reload
             assert_equal("fav:#{@user.id}", @post.fav_string)
+
+            @post.update_attributes(:tag_string => "aaa -fav:self")
+            assert_equal("", @post.fav_string)
           end
         end
 
@@ -659,6 +703,34 @@ class PostTest < ActiveSupport::TestCase
             assert(@post.has_children?)
           end
         end
+
+        context "for a source" do
+          should "set the source with source:foo_bar_baz" do
+            @post.update(:tag_string => "source:foo_bar_baz")
+            assert_equal("foo_bar_baz", @post.source)
+          end
+
+          should 'set the source with source:"foo bar baz"' do
+            @post.update(:tag_string => 'source:"foo bar baz"')
+            assert_equal("foo bar baz", @post.source)
+          end
+
+          should 'strip the source with source:"  foo bar baz  "' do
+            @post.update(:tag_string => 'source:"  foo bar baz  "')
+            assert_equal("foo bar baz", @post.source)
+          end
+
+          should "clear the source with source:none" do
+            @post.update(:source => "foobar")
+            @post.update(:tag_string => "source:none")
+            assert_nil(@post.source)
+          end
+
+          should "set the pixiv id with source:https://img18.pixiv.net/img/evazion/14901720.png" do
+            @post.update(:tag_string => "source:https://img18.pixiv.net/img/evazion/14901720.png")
+            assert_equal(14901720, @post.pixiv_id)
+          end
+        end
       end
 
       context "tagged with a negated tag" do