From 0b7cd71d424ea86521b1e663e5f8a0e3ab1894b7 Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 21 Oct 2016 04:49:36 -0500 Subject: [PATCH 1/5] Add some tests for tagging posts with metatags. Exercise a few bugs: * rating:safe should obey on rating locks. * source:blah should update the pixiv id. * source:" foo bar baz " should trim leading/trailing whitespace. The other tests are for metatags that work but didn't have tests. --- test/unit/post_test.rb | 78 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 74 insertions(+), 4 deletions(-) diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index d78c34ebc..f3e038bdc 100644 --- a/test/unit/post_test.rb +++ b/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") @@ -544,6 +544,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 +643,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 +657,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 +701,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 From 899f008c1de1c1fe4a279c574ecc3594af6837fe Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 21 Oct 2016 01:11:11 -0500 Subject: [PATCH 2/5] Reorganize Post callbacks into calling order. Reorder callbacks into the same order Rails runs them in: * before_validation * validate * before_save * before_create * after_create * after_save * after_commit This doesn't change the behavior of anything, it simply rearranges callbacks so their running order is less confusing. --- app/models/post.rb | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index c5c7099f0..4f58c59aa 100644 --- a/app/models/post.rb +++ b/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 - after_destroy :remove_iqdb_async - after_destroy :delete_files - after_destroy :delete_remote_files - after_save :create_version - after_save :update_parent_on_save - after_save :apply_post_metatags - after_save :expire_essential_tag_string_cache - after_create :update_iqdb_async - after_commit :notify_pubsub + before_validation :strip_source + before_validation :initialize_uploader, :on => :create + before_validation :parse_pixiv_id + before_validation :blank_out_nonexistent_parents + before_validation :remove_parent_loops + 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 before_save :merge_old_changes 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 - before_validation :initialize_uploader, :on => :create - before_validation :parse_pixiv_id - before_validation :blank_out_nonexistent_parents - before_validation :remove_parent_loops + after_create :update_iqdb_async + after_save :create_version + after_save :update_parent_on_save + after_save :apply_post_metatags + 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 From 07921d2c88f4e464ecf73be7f4a1cfcec981dc1d Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 21 Oct 2016 04:57:07 -0500 Subject: [PATCH 3/5] Make rating:s obey rate locks; make source:blah update pixiv id. Move normalize_tags (which processes metatags) from before_save to before_validation. This is so that it runs as early as possible, before strip_source / parse_pixiv_ids / updater_can_change_rating, so these callbacks can handle source/rating changes from metatags. Fixes a couple bugs: * Ratings locks were ignored when using rating:s metatag (regression in 0006b76) * Pixiv ids weren't updated when using source:blah metatag. Note: this means that `post.update_attribute(:tag_string => "art:bkub)` is now wrong. This is because update_attribute runs callbacks but not validations, so it doesn't process metatags from the tag string. `update` or `update_attributes` must be used instead. --- app/models/post.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 4f58c59aa..73efe797e 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -7,8 +7,10 @@ class Post < ActiveRecord::Base class RevertError < Exception ; end class SearchError < Exception ; end - before_validation :strip_source before_validation :initialize_uploader, :on => :create + before_validation :merge_old_changes + before_validation :normalize_tags + before_validation :strip_source before_validation :parse_pixiv_id before_validation :blank_out_nonexistent_parents before_validation :remove_parent_loops @@ -16,8 +18,6 @@ class Post < ActiveRecord::Base 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 - before_save :merge_old_changes - before_save :normalize_tags before_save :update_tag_post_counts before_save :set_tag_counts before_save :set_pool_category_pseudo_tags From 8b4672616649a57375d449b3b5159f57cc5c40f0 Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 21 Oct 2016 04:41:05 -0500 Subject: [PATCH 4/5] Fix tests for `before_validation :normalize_tags`. `update_attribute` doesn't trigger `before_validation` callbacks, which is where metatag processing happens. `update` or `update_attributes` must be used instead. AFAIK the test suite is the only place where `post.update_attribute(:tag_string => "stuff")` is used, the actual code doesn't use it. --- test/unit/post_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index f3e038bdc..685bf442d 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -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 @@ -493,9 +493,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 +522,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 From 52491f4486d526f4d2ac18c0eb70cf79a90e1773 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 20 Oct 2016 21:58:52 -0500 Subject: [PATCH 5/5] Fix @artist.ban! test case. Must use an admin in this test now because admin privileges are needed to create the `banned_artist` tag implication. (fixes regression in 7e3284c) --- test/unit/post_test.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index 685bf442d..8fddae6b4 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -478,8 +478,10 @@ class PostTest < ActiveSupport::TestCase context "with a banned artist" do setup do - @artist = FactoryGirl.create(:artist) - @artist.ban! + CurrentUser.scoped(FactoryGirl.create(:admin_user)) do + @artist = FactoryGirl.create(:artist) + @artist.ban! + end @post = FactoryGirl.create(:post, :tag_string => @artist.name) end