From 015c6dc7db07c0318b92e16c14502867edbd2f49 Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 10 Sep 2022 02:37:40 -0500 Subject: [PATCH] Fix #4965: Account for metatag prefixes when searching/linking Drop the ability to write e.g. `create alias foo -> char:bar` in a BUR to change the tag's type as a side effect. You can only use these tag type prefixes in tag edits now. This feature was only intended to be used in tag edits. The fact it worked elsewhere was unintended behavior. This feature was problematic because it relied on `Tag.find_or_create_by_name` automagically changing the tag's category when the tag name contained a tag category prefix, e.g. `char:hatsune_miku`. This meant that merely looking up a tag could have the side effect of changing its category. It was also bad because `find_or_create_by_name` had a hidden dependency on the current user, which may not be set or available in all contexts. --- app/models/artist.rb | 2 +- app/models/post.rb | 2 +- app/models/tag.rb | 37 +++++---------------------- test/unit/artist_test.rb | 4 +++ test/unit/bulk_update_request_test.rb | 7 +++++ test/unit/tag_test.rb | 10 ++++---- 6 files changed, 24 insertions(+), 38 deletions(-) diff --git a/app/models/artist.rb b/app/models/artist.rb index e5d8a789c..7dbebb5a5 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -215,7 +215,7 @@ class Artist < ApplicationRecord # potential race condition but unlikely unless TagImplication.where(:antecedent_name => name, :consequent_name => "banned_artist").exists? - Tag.find_or_create_by_name("artist:banned_artist") # ensure the banned_artist exists and is an artist tag. + Tag.find_or_create_by_name("banned_artist", category: "artist", current_user: banner) TagImplication.approve!(antecedent_name: name, consequent_name: "banned_artist", approver: banner) end diff --git a/app/models/post.rb b/app/models/post.rb index 52014a443..d871fce65 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -590,7 +590,7 @@ class Post < ApplicationRecord self.source = value in category, name if category.in?(PostEdit::CATEGORIZATION_METATAGS) - Tag.find_or_create_by_name("#{category}:#{name}", creator: CurrentUser.user) + Tag.find_or_create_by_name(name, category: category, current_user: CurrentUser.user) else nil diff --git a/app/models/tag.rb b/app/models/tag.rb index 1e149d0a2..6cc6bf766 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -193,39 +193,14 @@ class Tag < ApplicationRecord names.map {|x| find_or_create_by_name(x).name} end - def find_or_create_by_name(name, creator: CurrentUser.user) - name = normalize_name(name) - category = nil + def find_or_create_by_name(name, category: nil, current_user: nil) + tag = find_or_create_by(name: normalize_name(name)) - if name =~ /\A(#{categories.regexp}):(.+)\Z/ - category = $1 - name = $2 + if category.present? && current_user.present? && Pundit.policy!(current_user, tag).can_change_category? + tag.update!(category: categories.value_for(category)) end - tag = find_by_name(name) - - if tag - if category - category_id = categories.value_for(category) - - # in case a category change hasn't propagated to this server yet, - # force an update the local cache. This may get overwritten in the - # next few lines if the category is changed. - tag.update_category_cache - - if Pundit.policy!(creator, tag).can_change_category? - tag.update(category: category_id) - end - end - - tag - else - Tag.new.tap do |t| - t.name = name - t.category = categories.value_for(category) - t.save - end - end + tag end end end @@ -440,7 +415,7 @@ class Tag < ApplicationRecord concerning :CosplayTagMethods do def create_character_tag_for_cosplay_tag character_name = name.delete_suffix("_(cosplay)") - Tag.find_or_create_by_name("char:#{character_name}") + Tag.find_or_create_by_name(character_name, category: "character", current_user: CurrentUser.user) end def is_cosplay_tag? diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index 316b4bc7b..b91c72da9 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -98,6 +98,10 @@ class ArtistTest < ActiveSupport::TestCase assert_equal("aaa banned_artist", @post.reload.tag_string) end + should "create the banned_artist tag if it doesn't already exist" do + assert_equal(true, Tag.exists?(name: "banned_artist", category: Tag.categories.artist)) + end + should "set the approver of the banned_artist implication" do ta = TagImplication.where(:antecedent_name => "aaa", :consequent_name => "banned_artist").first assert_equal(@admin.id, ta.approver.id) diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index c942c11fb..8fa1425a5 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -165,6 +165,13 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_equal(["Can't create alias tag -> tag_ ('tag_' cannot end with an underscore)"], @bur.errors.full_messages) end + should "fail if the consequent name contains a tag type prefix" do + @bur = build(:bulk_update_request, script: "alias blah -> char:bar") + + assert_equal(false, @bur.valid?) + assert_equal(["Can't create alias blah -> char:bar ('char:bar' cannot begin with 'char:')"], @bur.errors.full_messages) + end + should "be case-insensitive" do @bur = create_bur!("CREATE ALIAS AAA -> BBB", @admin) diff --git a/test/unit/tag_test.rb b/test/unit/tag_test.rb index 47dbf21dc..f737c781b 100644 --- a/test/unit/tag_test.rb +++ b/test/unit/tag_test.rb @@ -93,7 +93,7 @@ class TagTest < ActiveSupport::TestCase tag = FactoryBot.create(:tag) assert_difference("Tag.count", 0) do assert_equal(Tag.categories.general, tag.category) - Tag.find_or_create_by_name("artist:#{tag.name}") + Tag.find_or_create_by_name(tag.name, category: "artist", current_user: @builder) tag.reload assert_equal(Tag.categories.artist, tag.category) end @@ -101,14 +101,14 @@ class TagTest < ActiveSupport::TestCase should "not change category when the tag is too large to be changed by a builder" do tag = FactoryBot.create(:tag, post_count: 1001) - Tag.find_or_create_by_name("artist:#{tag.name}", creator: @builder) + Tag.find_or_create_by_name(tag.name, category: "artist", current_user: @builder) assert_equal(0, tag.reload.category) end should "not change category when the tag is too large to be changed by a member" do tag = FactoryBot.create(:tag, post_count: 51) - Tag.find_or_create_by_name("artist:#{tag.name}", creator: FactoryBot.create(:member_user)) + Tag.find_or_create_by_name(tag.name, category: "artist", current_user: create(:member_user)) assert_equal(0, tag.reload.category) end @@ -118,7 +118,7 @@ class TagTest < ActiveSupport::TestCase assert_equal(1, post.tag_count_general) assert_equal(0, post.tag_count_character) - tag = Tag.find_or_create_by_name("char:test") + tag = Tag.find_or_create_by_name("test", category: "char", current_user: @builder) post.reload assert_equal(0, post.tag_count_general) assert_equal(1, post.tag_count_character) @@ -134,7 +134,7 @@ class TagTest < ActiveSupport::TestCase should "be created with the type when one doesn't exist" do assert_difference("Tag.count", 1) do - tag = Tag.find_or_create_by_name("artist:hoge") + tag = Tag.find_or_create_by_name("hoge", category: "artist", current_user: @builder) assert_equal("hoge", tag.name) assert_equal(Tag.categories.artist, tag.category) end