diff --git a/app/jobs/process_tag_alias_job.rb b/app/jobs/process_tag_alias_job.rb index 9098dd14e..120171928 100644 --- a/app/jobs/process_tag_alias_job.rb +++ b/app/jobs/process_tag_alias_job.rb @@ -1,7 +1,7 @@ class ProcessTagAliasJob < ApplicationJob queue_as :bulk_update - def perform(tag_alias) - tag_alias.process! + 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 index 41bff5c45..457f12386 100644 --- a/app/jobs/process_tag_implication_job.rb +++ b/app/jobs/process_tag_implication_job.rb @@ -1,7 +1,7 @@ class ProcessTagImplicationJob < ApplicationJob queue_as :bulk_update - def perform(tag_implication) - tag_implication.process! + def perform(tag_implication, approver) + tag_implication.process!(approver) end end diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb index ecbd5997c..2d072640d 100644 --- a/app/logical/bulk_update_request_processor.rb +++ b/app/logical/bulk_update_request_processor.rb @@ -98,11 +98,11 @@ class BulkUpdateRequestProcessor 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], skip_secondary_validations: skip_secondary_validations) - tag_alias.approve!(approver: approver) + tag_alias.approve!(approver) 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], skip_secondary_validations: skip_secondary_validations) - tag_implication.approve!(approver: approver) + tag_implication.approve!(approver) 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 dccde5f68..dfee85f2d 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -193,7 +193,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", skip_secondary_validations: true, creator: banner) - tag_implication.approve!(approver: banner) + tag_implication.approve!(banner) end update!(is_banned: true) diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index 40a0c7759..6e7303d5d 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -3,15 +3,10 @@ class TagAlias < TagRelationship validates_uniqueness_of :antecedent_name, scope: :status, conditions: -> { active } validate :absence_of_transitive_relation - module ApprovalMethods - def approve!(approver: CurrentUser.user) - update(approver: approver, status: "queued") - ProcessTagAliasJob.perform_later(self) - end + def approve!(approver) + ProcessTagAliasJob.perform_later(self, approver) end - include ApprovalMethods - def self.to_aliased(names) names = Array(names).map(&:to_s) return [] if names.empty? @@ -19,10 +14,10 @@ class TagAlias < TagRelationship names.map { |name| aliases[name] || name } end - def process!(user: User.system) - update!(status: "processing") + def process!(approver) + update!(approver: approver, status: "processing") move_aliases_and_implications - TagMover.new(antecedent_name, consequent_name, user: user).move! + TagMover.new(antecedent_name, consequent_name, user: User.system).move! update!(status: "active") rescue Exception => e update!(status: "error: #{e}") diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index e4c0321b6..c91605004 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -120,13 +120,13 @@ class TagImplication < TagRelationship end module ApprovalMethods - def process! + def process!(approver) unless valid? raise errors.full_messages.join("; ") end CurrentUser.scoped(User.system) do - update(status: "processing") + update(approver: approver, status: "processing") update_posts update(status: "active") end @@ -135,9 +135,8 @@ class TagImplication < TagRelationship DanbooruLogger.log(e, tag_implication_id: id, antecedent_name: antecedent_name, consequent_name: consequent_name) end - def approve!(approver: CurrentUser.user) - update(approver: approver, status: "queued") - ProcessTagImplicationJob.perform_later(self) + def approve!(approver) + ProcessTagImplicationJob.perform_later(self, approver) end def create_mod_action diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index 0afc16ff9..02675032c 100644 --- a/app/models/tag_relationship.rb +++ b/app/models/tag_relationship.rb @@ -16,7 +16,7 @@ class TagRelationship < ApplicationRecord belongs_to :consequent_wiki, class_name: "WikiPage", foreign_key: "consequent_name", primary_key: "title", optional: true scope :active, -> {approved} - scope :approved, -> {where(status: %w[active processing queued])} + scope :approved, -> {where(status: %w[active processing])} 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)} @@ -24,7 +24,7 @@ class TagRelationship < ApplicationRecord scope :retired, -> {where(status: "retired")} before_validation :normalize_names - validates_format_of :status, :with => /\A(active|deleted|pending|processing|queued|retired|error: .*)\Z/ + validates_format_of :status, :with => /\A(active|deleted|pending|processing|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? } @@ -36,7 +36,7 @@ class TagRelationship < ApplicationRecord end def is_approved? - status.in?(%w[active processing queued]) + status.in?(%w[active processing]) end def is_rejected? @@ -76,7 +76,7 @@ class TagRelationship < ApplicationRecord status = status.downcase if status == "approved" - where(status: %w[active processing queued]) + where(status: %w[active processing]) else where(status: status) end @@ -89,7 +89,7 @@ class TagRelationship < ApplicationRecord def pending_first # unknown statuses return null and are sorted first - order(Arel.sql("array_position(array['queued', 'processing', 'pending', 'active', 'deleted', 'retired'], status::text) NULLS FIRST, id DESC")) + order(Arel.sql("array_position(array['processing', 'pending', 'active', 'deleted', 'retired'], status::text) NULLS FIRST, id DESC")) end def search(params) diff --git a/app/views/tag_relationships/_search.html.erb b/app/views/tag_relationships/_search.html.erb index 58389ce77..32d197ae0 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[Approved Active Pending Deleted Retired Processing Queued], include_blank: true, selected: params[:search][:status] %> + <%= f.input :status, label: "Status", collection: %w[Approved Active Pending Deleted Retired Processing], 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.submit "Search" %> <% end %> diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index da2e1221d..ce7e05f8a 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -226,6 +226,20 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end end end + + context "that contains a mass update followed by an alias" do + should "make the alias take effect after the mass update" do + @bur = create(:bulk_update_request, script: "mass update maid_dress -> maid dress\nalias maid_dress -> maid") + @p1 = create(:post, tag_string: "maid_dress") + @p2 = create(:post, tag_string: "maid") + + @bur.approve!(@admin) + perform_enqueued_jobs + + assert_equal("dress maid", @p1.reload.tag_string) + assert_equal("maid", @p2.reload.tag_string) + end + end end context "when validating a script" do diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index bd5af29d1..9b36d76c9 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -28,7 +28,6 @@ class TagAliasTest < ActiveSupport::TestCase 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) @@ -93,7 +92,7 @@ class TagAliasTest < ActiveSupport::TestCase @ss5 = create(:saved_search, query: "123 ...", user: CurrentUser.user) @ta = create(:tag_alias, antecedent_name: "...", consequent_name: "bbb") - @ta.approve!(approver: @admin) + @ta.approve!(@admin) perform_enqueued_jobs assert_equal("123 bbb 456", @ss1.reload.query) @@ -115,7 +114,7 @@ class TagAliasTest < ActiveSupport::TestCase @u7 = create(:user, blacklisted_tags: "111 ...\r\n222 333\n") @ta = create(:tag_alias, antecedent_name: "...", consequent_name: "aaa") - @ta.approve!(approver: @admin) + @ta.approve!(@admin) perform_enqueued_jobs assert_equal("111 aaa 222", @u1.reload.blacklisted_tags) @@ -133,7 +132,7 @@ class TagAliasTest < ActiveSupport::TestCase post2 = FactoryBot.create(:post, :tag_string => "ccc ddd") ta = FactoryBot.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "ccc") - ta.approve!(approver: @admin) + ta.approve!(@admin) perform_enqueued_jobs assert_equal("bbb ccc", post1.reload.tag_string) @@ -155,7 +154,7 @@ class TagAliasTest < ActiveSupport::TestCase @wiki = create(:wiki_page, title: "aaa") @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - @ta.approve!(approver: @admin) + @ta.approve!(@admin) perform_enqueued_jobs assert_equal("active", @ta.reload.status) @@ -167,7 +166,7 @@ class TagAliasTest < ActiveSupport::TestCase @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!(approver: @admin) + @ta.approve!(@admin) perform_enqueued_jobs assert_equal("active", @ta.reload.status) @@ -185,7 +184,7 @@ class TagAliasTest < ActiveSupport::TestCase @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!(approver: @admin) + @ta.approve!(@admin) perform_enqueued_jobs assert_equal("active", @ta.reload.status) @@ -202,7 +201,7 @@ class TagAliasTest < ActiveSupport::TestCase @wiki = create(:wiki_page, body: "foo [[aaa]] bar") @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb") - @ta.approve!(approver: @admin) + @ta.approve!(@admin) perform_enqueued_jobs assert_equal("active", @ta.reload.status) @@ -215,7 +214,7 @@ class TagAliasTest < ActiveSupport::TestCase @artist = create(:artist, name: "aaa") @ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending") - @ta.approve!(approver: @admin) + @ta.approve!(@admin) perform_enqueued_jobs assert_equal("active", @ta.reload.status) @@ -228,7 +227,7 @@ class TagAliasTest < ActiveSupport::TestCase @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!(approver: @admin) + @ta.approve!(@admin) perform_enqueued_jobs assert_equal("active", @ta.reload.status) @@ -248,7 +247,7 @@ class TagAliasTest < ActiveSupport::TestCase @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!(approver: @admin) + @ta.approve!(@admin) perform_enqueued_jobs assert_equal("active", @ta.reload.status) @@ -270,8 +269,8 @@ class TagAliasTest < ActiveSupport::TestCase ta2 = FactoryBot.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc", :status => "pending") # XXX this is broken, it depends on the order the jobs are executed in. - ta2.approve!(approver: @admin) - ta1.approve!(approver: @admin) + ta2.approve!(@admin) + ta1.approve!(@admin) perform_enqueued_jobs assert_equal("ccc", ta1.reload.consequent_name) @@ -280,7 +279,7 @@ class TagAliasTest < ActiveSupport::TestCase should "move existing implications" do ti = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") ta = FactoryBot.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc") - ta.approve!(approver: @admin) + ta.approve!(@admin) perform_enqueued_jobs assert_equal("ccc", ti.reload.consequent_name) @@ -291,7 +290,7 @@ class TagAliasTest < ActiveSupport::TestCase tag2 = create(:tag, name: "artist", category: 1) ta = create(:tag_alias, antecedent_name: "general", consequent_name: "artist") - ta.approve!(approver: @admin) + ta.approve!(@admin) perform_enqueued_jobs assert_equal(1, tag1.reload.category) @@ -303,7 +302,7 @@ class TagAliasTest < ActiveSupport::TestCase tag2 = create(:tag, name: "general", category: 0) ta = create(:tag_alias, antecedent_name: "artist", consequent_name: "general") - ta.approve!(approver: @admin) + ta.approve!(@admin) perform_enqueued_jobs assert_equal(1, tag1.reload.category) @@ -315,7 +314,7 @@ class TagAliasTest < ActiveSupport::TestCase tag2 = create(:tag, name: "copyright", category: 3) ta = create(:tag_alias, antecedent_name: "character", consequent_name: "copyright") - ta.approve!(approver: @admin) + ta.approve!(@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 6dddde7a6..508c66221 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -3,8 +3,8 @@ require 'test_helper' class TagImplicationTest < ActiveSupport::TestCase context "A tag implication" do setup do - user = FactoryBot.create(:admin_user) - CurrentUser.user = user + @admin = create(:admin_user) + CurrentUser.user = @admin CurrentUser.ip_addr = "127.0.0.1" end @@ -24,7 +24,6 @@ class TagImplicationTest < ActiveSupport::TestCase 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) @@ -135,8 +134,8 @@ class TagImplicationTest < ActiveSupport::TestCase ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "xxx") ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "yyy") - ti1.approve! - ti2.approve! + ti1.approve!(@admin) + ti2.approve!(@admin) perform_enqueued_jobs assert_equal("aaa bbb ccc xxx yyy", p1.reload.tag_string)