From 8717c319abb44d194d046a99240ebb3457ed5ae2 Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 1 Dec 2020 15:30:42 -0600 Subject: [PATCH] aliases/implications: remove 'pending' state. Remove the pending status from tag aliases and implications. Previously aliases would be created first in the pending state then changed to active when the alias was later processed in a delayed job. This meant that BURs weren't processed completely sequentially; first all the aliases in a BUR would be created in one go, then later they would be processed and set to active sequentially. This was problematic in complex BURs that tried to reverse or swap around aliases, since new pending aliases could be created before old conflicting aliases were removed. --- app/jobs/process_tag_alias_job.rb | 7 --- app/jobs/process_tag_implication_job.rb | 7 --- app/jobs/process_tag_relationship_job.rb | 9 +++ app/logical/bulk_update_request_processor.rb | 8 +-- app/models/artist.rb | 3 +- app/models/tag_alias.rb | 7 +-- app/models/tag_implication.rb | 11 +--- app/models/tag_relationship.rb | 18 ++---- app/views/tag_relationships/_search.html.erb | 4 +- ...nge_default_status_on_tag_relationships.rb | 6 ++ db/structure.sql | 7 ++- test/functional/artists_controller_test.rb | 7 ++- .../bulk_update_requests_controller_test.rb | 6 +- test/functional/posts_controller_test.rb | 1 + .../functional/tag_aliases_controller_test.rb | 4 +- .../tag_implications_controller_test.rb | 4 +- test/unit/artist_test.rb | 1 - test/unit/tag_alias_test.rb | 56 ++++++------------- test/unit/tag_implication_test.rb | 18 +++--- 19 files changed, 70 insertions(+), 114 deletions(-) delete mode 100644 app/jobs/process_tag_alias_job.rb delete mode 100644 app/jobs/process_tag_implication_job.rb create mode 100644 app/jobs/process_tag_relationship_job.rb create mode 100644 db/migrate/20201201211748_change_default_status_on_tag_relationships.rb diff --git a/app/jobs/process_tag_alias_job.rb b/app/jobs/process_tag_alias_job.rb deleted file mode 100644 index 120171928..000000000 --- a/app/jobs/process_tag_alias_job.rb +++ /dev/null @@ -1,7 +0,0 @@ -class ProcessTagAliasJob < ApplicationJob - queue_as :bulk_update - - def perform(tag_alias, approver) - tag_alias.process!(approver) - end -end diff --git a/app/jobs/process_tag_implication_job.rb b/app/jobs/process_tag_implication_job.rb deleted file mode 100644 index 457f12386..000000000 --- a/app/jobs/process_tag_implication_job.rb +++ /dev/null @@ -1,7 +0,0 @@ -class ProcessTagImplicationJob < ApplicationJob - queue_as :bulk_update - - def perform(tag_implication, approver) - tag_implication.process!(approver) - end -end diff --git a/app/jobs/process_tag_relationship_job.rb b/app/jobs/process_tag_relationship_job.rb new file mode 100644 index 000000000..01b22a233 --- /dev/null +++ b/app/jobs/process_tag_relationship_job.rb @@ -0,0 +1,9 @@ +class ProcessTagRelationshipJob < ApplicationJob + queue_as :bulk_update + + def perform(class_name:, approver:, antecedent_name:, consequent_name:, forum_topic: nil) + relation_class = Kernel.const_get(class_name) + tag_relationship = relation_class.create!(creator: approver, approver: approver, antecedent_name: antecedent_name, consequent_name: consequent_name, forum_topic: forum_topic) + tag_relationship.process! + end +end diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb index f9604f1f0..0b6d96b18 100644 --- a/app/logical/bulk_update_request_processor.rb +++ b/app/logical/bulk_update_request_processor.rb @@ -4,7 +4,7 @@ class BulkUpdateRequestProcessor class Error < StandardError; end attr_reader :bulk_update_request - delegate :script, :forum_topic_id, to: :bulk_update_request + delegate :script, :forum_topic, to: :bulk_update_request validate :validate_script def initialize(bulk_update_request) @@ -105,12 +105,10 @@ class BulkUpdateRequestProcessor commands.map do |command, *args| case command when :create_alias - tag_alias = TagAlias.create!(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: args[0], consequent_name: args[1]) - tag_alias.approve!(approver) + TagAlias.approve!(antecedent_name: args[0], consequent_name: args[1], approver: approver, forum_topic: forum_topic) when :create_implication - tag_implication = TagImplication.create!(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: args[0], consequent_name: args[1]) - tag_implication.approve!(approver) + TagImplication.approve!(antecedent_name: args[0], consequent_name: args[1], approver: approver, forum_topic: forum_topic) when :remove_alias tag_alias = TagAlias.active.find_by!(antecedent_name: args[0], consequent_name: args[1]) diff --git a/app/models/artist.rb b/app/models/artist.rb index d059a8367..ab533b37b 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -192,8 +192,7 @@ class Artist < ApplicationRecord # potential race condition but unlikely unless TagImplication.where(:antecedent_name => name, :consequent_name => "banned_artist").exists? - tag_implication = TagImplication.create!(antecedent_name: name, consequent_name: "banned_artist", creator: banner) - tag_implication.approve!(banner) + TagImplication.approve!(antecedent_name: name, consequent_name: "banned_artist", approver: banner) end update!(is_banned: true) diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index 9b3b0f42d..72fee2ccb 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -3,10 +3,6 @@ class TagAlias < TagRelationship validates_uniqueness_of :antecedent_name, scope: :status, conditions: -> { active } validate :absence_of_transitive_relation - def approve!(approver) - ProcessTagAliasJob.perform_later(self, approver) - end - def self.to_aliased(names) names = Array(names).map(&:to_s) return [] if names.empty? @@ -14,8 +10,7 @@ class TagAlias < TagRelationship names.map { |name| aliases[name] || name } end - def process!(approver) - update!(approver: approver, status: "active") + def process! TagMover.new(antecedent_name, consequent_name, user: User.system).move! rescue Exception => e update!(status: "error: #{e}") diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index c5be0ac91..052dac101 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -109,13 +109,8 @@ class TagImplication < TagRelationship end module ApprovalMethods - def process!(approver) - unless valid? - raise errors.full_messages.join("; ") - end - + def process! CurrentUser.scoped(User.system) do - update!(approver: approver, status: "active") update_posts end rescue Exception => e @@ -123,10 +118,6 @@ class TagImplication < TagRelationship DanbooruLogger.log(e, tag_implication_id: id, antecedent_name: antecedent_name, consequent_name: consequent_name) end - def approve!(approver) - ProcessTagImplicationJob.perform_later(self, approver) - end - def create_mod_action implication = %("tag implication ##{id}":[#{Rails.application.routes.url_helpers.tag_implication_path(self)}]: [[#{antecedent_name}]] -> [[#{consequent_name}]]) diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index 7eb6c020c..a1cab6891 100644 --- a/app/models/tag_relationship.rb +++ b/app/models/tag_relationship.rb @@ -17,11 +17,10 @@ class TagRelationship < ApplicationRecord scope :deleted, -> {where(status: "deleted")} scope :expired, -> {where("created_at < ?", EXPIRY.days.ago)} scope :old, -> {where("created_at >= ? and created_at < ?", EXPIRY.days.ago, EXPIRY_WARNING.days.ago)} - scope :pending, -> {where(status: "pending")} scope :retired, -> {where(status: "retired")} before_validation :normalize_names - validates_format_of :status, :with => /\A(active|deleted|pending|retired|error: .*)\Z/ + validates_format_of :status, :with => /\A(active|deleted|retired|error: .*)\Z/ validates_presence_of :antecedent_name, :consequent_name validates :approver, presence: { message: "must exist" }, if: -> { approver_id.present? } validates :forum_topic, presence: { message: "must exist" }, if: -> { forum_topic_id.present? } @@ -44,10 +43,6 @@ class TagRelationship < ApplicationRecord status == "deleted" end - def is_pending? - status == "pending" - end - def is_active? status == "active" end @@ -74,11 +69,6 @@ class TagRelationship < ApplicationRecord where(field => Tag.search(params).reorder(nil).select(:name)) end - def pending_first - # unknown statuses return null and are sorted first - order(Arel.sql("array_position(array['pending', 'active', 'deleted', 'retired'], status::text) NULLS FIRST, id DESC")) - end - def search(params) q = super q = q.search_attributes(params, :antecedent_name, :consequent_name) @@ -107,8 +97,6 @@ class TagRelationship < ApplicationRecord q = q.order("antecedent_name asc, consequent_name asc") when "tag_count" q = q.joins(:consequent_tag).order("tags.post_count desc, antecedent_name asc, consequent_name asc") - when "status" - q = q.pending_first else q = q.apply_default_order(params) end @@ -144,6 +132,10 @@ class TagRelationship < ApplicationRecord end end + def self.approve!(antecedent_name:, consequent_name:, approver:, forum_topic: nil) + ProcessTagRelationshipJob.perform_later(class_name: self.name, approver: approver, antecedent_name: antecedent_name, consequent_name: consequent_name, forum_topic: forum_topic) + end + def self.model_restriction(table) super.where(table[:status].eq("active")) end diff --git a/app/views/tag_relationships/_search.html.erb b/app/views/tag_relationships/_search.html.erb index 719c921e3..21f2bb4cc 100644 --- a/app/views/tag_relationships/_search.html.erb +++ b/app/views/tag_relationships/_search.html.erb @@ -9,7 +9,7 @@ <%= f.simple_fields_for :consequent_tag do |fa| %> <%= fa.input :category, label: "To Category", collection: TagCategory.canonical_mapping.to_a, include_blank: true, selected: params.dig(:search, :consequent_tag, :category) %> <% end %> - <%= f.input :status, label: "Status", collection: %w[Active Pending Deleted Retired], include_blank: true, selected: params[:search][:status] %> - <%= f.input :order, label: "Order", collection: [%w[Created created_at], %w[Updated updated_at], %w[Name name], %w[Tag\ count tag_count], %w[Status status]], include_blank: true, selected: params[:search][:order] %> + <%= f.input :status, label: "Status", collection: %w[Active Deleted Retired], include_blank: true, selected: params[:search][:status] %> + <%= f.input :order, label: "Order", collection: [%w[Created created_at], %w[Updated updated_at], %w[Name name], %w[Tag\ count tag_count]], include_blank: true, selected: params[:search][:order] %> <%= f.submit "Search" %> <% end %> diff --git a/db/migrate/20201201211748_change_default_status_on_tag_relationships.rb b/db/migrate/20201201211748_change_default_status_on_tag_relationships.rb new file mode 100644 index 000000000..549f4ea87 --- /dev/null +++ b/db/migrate/20201201211748_change_default_status_on_tag_relationships.rb @@ -0,0 +1,6 @@ +class ChangeDefaultStatusOnTagRelationships < ActiveRecord::Migration[6.0] + def change + change_column_default(:tag_aliases, :status, from: "pending", to: "active") + change_column_default(:tag_implications, :status, from: "pending", to: "active") + end +end diff --git a/db/structure.sql b/db/structure.sql index c5813449e..dd4227258 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2857,7 +2857,7 @@ CREATE TABLE public.tag_aliases ( consequent_name character varying NOT NULL, creator_id integer NOT NULL, forum_topic_id integer, - status text DEFAULT 'pending'::text NOT NULL, + status text DEFAULT 'active'::text NOT NULL, created_at timestamp without time zone NOT NULL, updated_at timestamp without time zone NOT NULL, approver_id integer, @@ -2894,7 +2894,7 @@ CREATE TABLE public.tag_implications ( consequent_name character varying NOT NULL, creator_id integer NOT NULL, forum_topic_id integer, - status text DEFAULT 'pending'::text NOT NULL, + status text DEFAULT 'active'::text NOT NULL, created_at timestamp without time zone NOT NULL, updated_at timestamp without time zone NOT NULL, approver_id integer, @@ -7419,6 +7419,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20200427190519'), ('20200520060951'), ('20200803022359'), -('20200816175151'); +('20200816175151'), +('20201201211748'); diff --git a/test/functional/artists_controller_test.rb b/test/functional/artists_controller_test.rb index 11ebae670..352d34a78 100644 --- a/test/functional/artists_controller_test.rb +++ b/test/functional/artists_controller_test.rb @@ -107,10 +107,13 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest context "ban action" do should "ban an artist" do - put_auth ban_artist_path(@artist.id), @admin + perform_enqueued_jobs do + put_auth ban_artist_path(@artist.id), @admin + end + assert_redirected_to(@artist) assert_equal(true, @artist.reload.is_banned?) - assert_equal(true, TagImplication.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) + assert_equal(true, TagImplication.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist", status: "active")) end should "not allow non-admins to ban artists" do diff --git a/test/functional/bulk_update_requests_controller_test.rb b/test/functional/bulk_update_requests_controller_test.rb index f4c47f79d..b72db21bc 100644 --- a/test/functional/bulk_update_requests_controller_test.rb +++ b/test/functional/bulk_update_requests_controller_test.rb @@ -156,12 +156,14 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest create(:tag, name: "artist2a", category: Tag.categories.artist, post_count: 20) @bulk_update_request = create(:bulk_update_request, script: "mass update artist1a -> artist1b\ncreate alias artist2a -> artist2b") - post_auth approve_bulk_update_request_path(@bulk_update_request), @builder + perform_enqueued_jobs do + post_auth approve_bulk_update_request_path(@bulk_update_request), @builder + end assert_redirected_to(bulk_update_requests_path) assert_equal("approved", @bulk_update_request.reload.status) assert_equal(@builder, @bulk_update_request.approver) - assert_equal(true, TagAlias.where(antecedent_name: "artist2a", consequent_name: "artist2b").exists?) + assert_equal(true, TagAlias.exists?(antecedent_name: "artist2a", consequent_name: "artist2b", status: "active")) end end diff --git a/test/functional/posts_controller_test.rb b/test/functional/posts_controller_test.rb index d0f36c3f3..18ad1ba37 100644 --- a/test/functional/posts_controller_test.rb +++ b/test/functional/posts_controller_test.rb @@ -161,6 +161,7 @@ class PostsControllerTest < ActionDispatch::IntegrationTest end should "show a notice for a single tag search with a pending BUR" do + create(:tag, name: "foo") create(:bulk_update_request, script: "create alias foo -> bar") get_auth posts_path(tags: "foo"), @user assert_select ".tag-change-notice" diff --git a/test/functional/tag_aliases_controller_test.rb b/test/functional/tag_aliases_controller_test.rb index 4e6111d83..2db17a743 100644 --- a/test/functional/tag_aliases_controller_test.rb +++ b/test/functional/tag_aliases_controller_test.rb @@ -18,7 +18,7 @@ class TagAliasesControllerTest < ActionDispatch::IntegrationTest @antecedent_wiki = create(:wiki_page, title: "touhou", body: "zun project") @consequent_wiki = create(:wiki_page, title: "touhou_project") - @other_alias = create(:tag_alias, antecedent_name: "touhou", consequent_name: "touhou_project", creator: @user, status: "pending", forum_topic: @forum_topic, forum_post: @forum_post) + @other_alias = create(:tag_alias, antecedent_name: "touhou", consequent_name: "touhou_project", creator: @user, status: "deleted", forum_topic: @forum_topic, forum_post: @forum_post) @unrelated_alias = create(:tag_alias) end @@ -30,7 +30,7 @@ class TagAliasesControllerTest < ActionDispatch::IntegrationTest should respond_to_search({}).with { [@unrelated_alias, @other_alias, @tag_alias] } should respond_to_search(antecedent_name: "aaa").with { @tag_alias } should respond_to_search(consequent_name: "bbb").with { @tag_alias } - should respond_to_search(status: "pending").with { @other_alias } + should respond_to_search(status: "deleted").with { @other_alias } context "using includes" do should respond_to_search(antecedent_tag: {post_count: 1000}).with { @other_alias } diff --git a/test/functional/tag_implications_controller_test.rb b/test/functional/tag_implications_controller_test.rb index f7f9b4187..e7355b422 100644 --- a/test/functional/tag_implications_controller_test.rb +++ b/test/functional/tag_implications_controller_test.rb @@ -18,7 +18,7 @@ class TagImplicationsControllerTest < ActionDispatch::IntegrationTest @antecedent_wiki = create(:wiki_page, title: "cannon", body: "made of fun") @consequent_wiki = create(:wiki_page, title: "weapon") - @other_implication = create(:tag_implication, antecedent_name: "cannon", consequent_name: "weapon", creator: @user, status: "pending", forum_topic: @forum_topic, forum_post: @forum_post) + @other_implication = create(:tag_implication, antecedent_name: "cannon", consequent_name: "weapon", creator: @user, status: "deleted", forum_topic: @forum_topic, forum_post: @forum_post) @unrelated_implication = create(:tag_implication) end @@ -30,7 +30,7 @@ class TagImplicationsControllerTest < ActionDispatch::IntegrationTest should respond_to_search({}).with { [@unrelated_implication, @other_implication, @tag_implication] } should respond_to_search(antecedent_name: "aaa").with { @tag_implication } should respond_to_search(consequent_name: "bbb").with { @tag_implication } - should respond_to_search(status: "pending").with { @other_implication } + should respond_to_search(status: "deleted").with { @other_implication } context "using includes" do should respond_to_search(antecedent_tag: {post_count: 10}).with { @other_implication } diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index f63613241..7fcc08e9d 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -94,7 +94,6 @@ class ArtistTest < ActiveSupport::TestCase end should "create a new tag implication" do - perform_enqueued_jobs assert_equal(1, TagImplication.where(:antecedent_name => "aaa", :consequent_name => "banned_artist").count) assert_equal("aaa banned_artist", @post.reload.tag_string) end diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index 604ce57fa..87dfe6228 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -26,7 +26,6 @@ class TagAliasTest < ActiveSupport::TestCase should allow_value('active').for(:status) should allow_value('deleted').for(:status) - should allow_value('pending').for(:status) should allow_value('error: derp').for(:status) should_not allow_value('ACTIVE').for(:status) @@ -47,18 +46,17 @@ class TagAliasTest < ActiveSupport::TestCase ta2 = FactoryBot.create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "retired") ta3 = FactoryBot.create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "deleted") ta4 = FactoryBot.create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "deleted") - ta5 = FactoryBot.create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - [ta1, ta2, ta3, ta4, ta5].each { |ta| assert(ta.valid?) } + [ta1, ta2, ta3, ta4].each { |ta| assert(ta.valid?) } - ta5.update(status: "active") - assert_includes(ta5.errors[:antecedent_name], "has already been taken") + ta4.update(status: "active") + assert_includes(ta4.errors[:antecedent_name], "has already been taken") end end context "#reject!" do should "not be blocked by validations" do ta1 = create(:tag_alias, antecedent_name: "kitty", consequent_name: "kitten", status: "active") - ta2 = build(:tag_alias, antecedent_name: "cat", consequent_name: "kitty", status: "pending") + ta2 = build(:tag_alias, antecedent_name: "cat", consequent_name: "kitty", status: "active") ta2.reject! assert_equal("deleted", ta2.reload.status) @@ -89,9 +87,8 @@ class TagAliasTest < ActiveSupport::TestCase @ss3 = create(:saved_search, query: "123 ~... 456", user: CurrentUser.user) @ss4 = create(:saved_search, query: "... 456", user: CurrentUser.user) @ss5 = create(:saved_search, query: "123 ...", user: CurrentUser.user) - @ta = create(:tag_alias, antecedent_name: "...", consequent_name: "bbb") - @ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "...", consequent_name: "bbb", approver: @admin) perform_enqueued_jobs assert_equal("123 bbb 456", @ss1.reload.query) @@ -111,9 +108,8 @@ class TagAliasTest < ActiveSupport::TestCase @u5 = create(:user, blacklisted_tags: "111 ...") @u6 = create(:user, blacklisted_tags: "111 222\n\n... 333\n") @u7 = create(:user, blacklisted_tags: "111 ...\r\n222 333\n") - @ta = create(:tag_alias, antecedent_name: "...", consequent_name: "aaa") - @ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "...", consequent_name: "aaa", approver: @admin) perform_enqueued_jobs assert_equal("111 aaa 222", @u1.reload.blacklisted_tags) @@ -130,8 +126,7 @@ class TagAliasTest < ActiveSupport::TestCase post1 = FactoryBot.create(:post, :tag_string => "aaa bbb") post2 = FactoryBot.create(:post, :tag_string => "ccc ddd") - ta = FactoryBot.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "ccc") - ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "aaa", consequent_name: "ccc", approver: @admin) perform_enqueued_jobs assert_equal("bbb ccc", post1.reload.tag_string) @@ -151,11 +146,9 @@ class TagAliasTest < ActiveSupport::TestCase context "when the tags have wikis" do should "rename the old wiki if there is no conflict" do @wiki = create(:wiki_page, title: "aaa") - @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - @ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin) perform_enqueued_jobs - assert_equal("active", @ta.reload.status) assert_equal("bbb", @wiki.reload.title) end @@ -163,11 +156,9 @@ class TagAliasTest < ActiveSupport::TestCase should "merge existing wikis if there is a conflict" do @wiki1 = create(:wiki_page, title: "aaa", other_names: "111 222", body: "first") @wiki2 = create(:wiki_page, title: "bbb", other_names: "111 333", body: "second") - @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - @ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin) perform_enqueued_jobs - assert_equal("active", @ta.reload.status) assert_equal(true, @wiki1.reload.is_deleted) assert_equal([], @wiki1.other_names) @@ -181,11 +172,9 @@ class TagAliasTest < ActiveSupport::TestCase should "ignore the old wiki if it has been deleted" do @wiki1 = create(:wiki_page, title: "aaa", other_names: "111 222", body: "first", is_deleted: true) @wiki2 = create(:wiki_page, title: "bbb", other_names: "111 333", body: "second") - @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - @ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin) perform_enqueued_jobs - assert_equal("active", @ta.reload.status) assert_equal(true, @wiki1.reload.is_deleted) assert_equal(%w[111 222], @wiki1.other_names) @@ -198,11 +187,9 @@ class TagAliasTest < ActiveSupport::TestCase should "rewrite links in other wikis to use the new tag" do @wiki = create(:wiki_page, body: "foo [[aaa]] bar") - @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb") - @ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin) perform_enqueued_jobs - assert_equal("active", @ta.reload.status) assert_equal("foo [[bbb]] bar", @wiki.reload.body) end @@ -211,11 +198,9 @@ class TagAliasTest < ActiveSupport::TestCase context "when the tags have artist entries" do should "rename the old artist entry if there is no conflict" do @artist = create(:artist, name: "aaa") - @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - @ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin) perform_enqueued_jobs - assert_equal("active", @ta.reload.status) assert_equal("bbb", @artist.reload.name) end @@ -224,11 +209,9 @@ class TagAliasTest < ActiveSupport::TestCase @tag = create(:tag, name: "aaa", category: Tag.categories.artist) @artist1 = create(:artist, name: "aaa", group_name: "g_aaa", other_names: "111 222", url_string: "https://twitter.com/111\n-https://twitter.com/222") @artist2 = create(:artist, name: "bbb", other_names: "111 333", url_string: "https://twitter.com/111\n-https://twitter.com/333\nhttps://twitter.com/444") - @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - @ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin) perform_enqueued_jobs - assert_equal("active", @ta.reload.status) assert_equal(true, @artist1.reload.is_deleted) assert_equal([@artist2.name], @artist1.other_names) @@ -244,11 +227,9 @@ class TagAliasTest < ActiveSupport::TestCase should "ignore the old artist if it has been deleted" do @artist1 = create(:artist, name: "aaa", group_name: "g_aaa", other_names: "111 222", url_string: "https://twitter.com/111\n-https://twitter.com/222", is_deleted: true) @artist2 = create(:artist, name: "bbb", other_names: "111 333", url_string: "https://twitter.com/111\n-https://twitter.com/333\nhttps://twitter.com/444") - @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - @ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin) perform_enqueued_jobs - assert_equal("active", @ta.reload.status) assert_equal(true, @artist1.reload.is_deleted) assert_equal(%w[111 222], @artist1.other_names) @@ -265,9 +246,8 @@ class TagAliasTest < ActiveSupport::TestCase should "push the consequent's category to the antecedent if the antecedent is general" do tag1 = create(:tag, name: "general", category: 0) tag2 = create(:tag, name: "artist", category: 1) - ta = create(:tag_alias, antecedent_name: "general", consequent_name: "artist") - ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "general", consequent_name: "artist", approver: @admin) perform_enqueued_jobs assert_equal(1, tag1.reload.category) @@ -277,9 +257,8 @@ class TagAliasTest < ActiveSupport::TestCase should "push the antecedent's category to the consequent if the consequent is general" do tag1 = create(:tag, name: "artist", category: 1) tag2 = create(:tag, name: "general", category: 0) - ta = create(:tag_alias, antecedent_name: "artist", consequent_name: "general") - ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "artist", consequent_name: "general", approver: @admin) perform_enqueued_jobs assert_equal(1, tag1.reload.category) @@ -289,9 +268,8 @@ class TagAliasTest < ActiveSupport::TestCase should "not change either tag category when neither the antecedent or consequent are general" do tag1 = create(:tag, name: "character", category: 4) tag2 = create(:tag, name: "copyright", category: 3) - ta = create(:tag_alias, antecedent_name: "character", consequent_name: "copyright") - ta.approve!(@admin) + TagAlias.approve!(antecedent_name: "character", consequent_name: "copyright", approver: @admin) perform_enqueued_jobs assert_equal(4, tag1.reload.category) diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index a71852814..f284d489b 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -22,7 +22,6 @@ class TagImplicationTest < ActiveSupport::TestCase should allow_value('active').for(:status) should allow_value('deleted').for(:status) - should allow_value('pending').for(:status) should allow_value('error: derp').for(:status) should_not allow_value('ACTIVE').for(:status) @@ -43,17 +42,16 @@ class TagImplicationTest < ActiveSupport::TestCase ti2 = FactoryBot.create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", status: "retired") ti3 = FactoryBot.create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", status: "deleted") ti4 = FactoryBot.create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", status: "deleted") - ti5 = FactoryBot.create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - [ti1, ti2, ti3, ti4, ti5].each { |ti| assert(ti.valid?) } + [ti1, ti2, ti3, ti4].each { |ti| assert(ti.valid?) } - ti5.update(status: "active") - assert_includes(ti5.errors[:antecedent_name], "Implication already exists") + ti4.update(status: "active") + assert_includes(ti4.errors[:antecedent_name], "Implication already exists") end end context "#reject!" do should "not be blocked by alias validations" do - ti = create(:tag_implication, antecedent_name: "cat", consequent_name: "animal", status: "pending") + ti = create(:tag_implication, antecedent_name: "cat", consequent_name: "animal", status: "active") ta = create(:tag_alias, antecedent_name: "cat", consequent_name: "kitty", status: "active") ti.reject! @@ -120,11 +118,9 @@ class TagImplicationTest < ActiveSupport::TestCase should "update any affected post upon save" do p1 = FactoryBot.create(:post, :tag_string => "aaa bbb ccc") - ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "xxx") - ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "yyy") - ti1.approve!(@admin) - ti2.approve!(@admin) + TagImplication.approve!(antecedent_name: "aaa", consequent_name: "xxx", approver: @admin) + TagImplication.approve!(antecedent_name: "aaa", consequent_name: "yyy", approver: @admin) perform_enqueued_jobs assert_equal("aaa bbb ccc xxx yyy", p1.reload.tag_string) @@ -149,7 +145,7 @@ class TagImplicationTest < ActiveSupport::TestCase should "not include inactive implications" do create(:tag_implication, antecedent_name: "a", consequent_name: "b", status: "active") - create(:tag_implication, antecedent_name: "b", consequent_name: "c", status: "pending") + create(:tag_implication, antecedent_name: "b", consequent_name: "c", status: "deleted") create(:tag_implication, antecedent_name: "c", consequent_name: "d", status: "active") assert_equal(["b"], TagImplication.tags_implied_by("a").map(&:name))