diff --git a/app/logical/alias_and_implication_importer.rb b/app/logical/bulk_update_request_processor.rb similarity index 73% rename from app/logical/alias_and_implication_importer.rb rename to app/logical/bulk_update_request_processor.rb index 6285d6bd3..951495048 100644 --- a/app/logical/alias_and_implication_importer.rb +++ b/app/logical/bulk_update_request_processor.rb @@ -1,65 +1,50 @@ -class AliasAndImplicationImporter - class Error < RuntimeError; end - attr_accessor :text, :commands, :forum_id, :skip_secondary_validations +class BulkUpdateRequestProcessor + extend Memoist - def initialize(text, forum_id, skip_secondary_validations = true) - @forum_id = forum_id + class Error < StandardError; end + attr_accessor :text, :forum_topic_id, :skip_secondary_validations + + def initialize(text, forum_topic_id: nil, skip_secondary_validations: true) + @forum_topic_id = forum_topic_id @text = text @skip_secondary_validations = skip_secondary_validations end - def process!(approver = CurrentUser.user) - tokens = AliasAndImplicationImporter.tokenize(text) - parse(tokens, approver) - end - - def validate! - tokens = AliasAndImplicationImporter.tokenize(text) - validate(tokens) - end - - def self.tokenize(text) + def tokens text.split(/\r\n|\r|\n/).reject(&:blank?).map do |line| line = line.gsub(/[[:space:]]+/, " ").strip if line =~ /^(?:create alias|aliasing|alias) (\S+) -> (\S+)$/i [:create_alias, $1, $2] - elsif line =~ /^(?:create implication|implicating|implicate|imply) (\S+) -> (\S+)$/i [:create_implication, $1, $2] - elsif line =~ /^(?:remove alias|unaliasing|unalias) (\S+) -> (\S+)$/i [:remove_alias, $1, $2] - elsif line =~ /^(?:remove implication|unimplicating|unimplicate|unimply) (\S+) -> (\S+)$/i [:remove_implication, $1, $2] - elsif line =~ /^(?:mass update|updating|update|change) (.+?) -> (.*)$/i [:mass_update, $1, $2] - elsif line =~ /^category (\S+) -> (#{Tag.categories.regexp})/ [:change_category, $1, $2] - elsif line.strip.empty? # do nothing - else raise Error, "Unparseable line: #{line}" end end end - def validate(tokens) + def validate! tokens.map do |token| case token[0] when :create_alias - tag_alias = TagAlias.new(creator: User.system, forum_topic_id: forum_id, status: "pending", antecedent_name: token[1], consequent_name: token[2], skip_secondary_validations: skip_secondary_validations) + tag_alias = TagAlias.new(creator: User.system, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: token[1], consequent_name: token[2], skip_secondary_validations: skip_secondary_validations) unless tag_alias.valid? raise Error, "Error: #{tag_alias.errors.full_messages.join("; ")} (create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name})" end when :create_implication - tag_implication = TagImplication.new(creator: User.system, forum_topic_id: forum_id, status: "pending", antecedent_name: token[1], consequent_name: token[2], skip_secondary_validations: skip_secondary_validations) + tag_implication = TagImplication.new(creator: User.system, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: token[1], consequent_name: token[2], skip_secondary_validations: skip_secondary_validations) unless tag_implication.valid? raise Error, "Error: #{tag_implication.errors.full_messages.join("; ")} (create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name})" end @@ -73,37 +58,19 @@ class AliasAndImplicationImporter end end - def affected_tags - AliasAndImplicationImporter.tokenize(text).flat_map do |type, *args| - case type - when :create_alias, :remove_alias, :create_implication, :remove_implication - [args[0], args[1]] - when :mass_update - tags = PostQueryBuilder.new(args[0]).tags + PostQueryBuilder.new(args[1]).tags - tags.reject(&:negated).reject(&:optional).reject(&:wildcard).map(&:name) - when :change_category - args[0] - end - end.sort.uniq - rescue Error - [] - end - - private - - def parse(tokens, approver) + def process!(approver) ActiveRecord::Base.transaction do tokens.map do |token| case token[0] when :create_alias - tag_alias = TagAlias.create(creator: approver, forum_topic_id: forum_id, status: "pending", antecedent_name: token[1], consequent_name: token[2], skip_secondary_validations: skip_secondary_validations) + tag_alias = TagAlias.create(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: token[1], consequent_name: token[2], skip_secondary_validations: skip_secondary_validations) unless tag_alias.valid? raise Error, "Error: #{tag_alias.errors.full_messages.join("; ")} (create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name})" end tag_alias.approve!(approver: approver) when :create_implication - tag_implication = TagImplication.create(creator: approver, forum_topic_id: forum_id, status: "pending", antecedent_name: token[1], consequent_name: token[2], skip_secondary_validations: skip_secondary_validations) + tag_implication = TagImplication.create(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: token[1], consequent_name: token[2], skip_secondary_validations: skip_secondary_validations) unless tag_implication.valid? raise Error, "Error: #{tag_implication.errors.full_messages.join("; ")} (create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name})" end @@ -133,4 +100,37 @@ class AliasAndImplicationImporter end end end + + def affected_tags + tokens.flat_map do |type, *args| + case type + when :create_alias, :remove_alias, :create_implication, :remove_implication + [args[0], args[1]] + when :mass_update + tags = PostQueryBuilder.new(args[0]).tags + PostQueryBuilder.new(args[1]).tags + tags.reject(&:negated).reject(&:optional).reject(&:wildcard).map(&:name) + when :change_category + args[0] + end + end.sort.uniq + rescue Error + [] + end + + def to_dtext + tokens.map do |token| + case token[0] + when :create_alias, :create_implication, :remove_alias, :remove_implication + "#{token[0].to_s.tr("_", " ")} [[#{token[1]}]] -> [[#{token[2]}]]" + when :mass_update + "mass update {{#{token[1]}}} -> #{token[2]}" + when :change_category + "category [[#{token[1]}]] -> #{token[2]}" + else + raise "Unknown token: #{token[0]}" + end + end.join("\n") + end + + memoize :tokens end diff --git a/app/logical/d_text.rb b/app/logical/d_text.rb index 04de53969..70cfa88d1 100644 --- a/app/logical/d_text.rb +++ b/app/logical/d_text.rb @@ -105,9 +105,9 @@ class DText end elsif obj.is_a?(BulkUpdateRequest) if obj.script.size < 700 - embedded_script = obj.script_with_links + embedded_script = obj.processor.to_dtext else - embedded_script = "[expand]#{obj.script_with_links}[/expand]" + embedded_script = "[expand]#{obj.processor.to_dtext}[/expand]" end if obj.is_approved? diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index eafcf7f6a..96b095a74 100644 --- a/app/models/bulk_update_request.rb +++ b/app/models/bulk_update_request.rb @@ -75,12 +75,12 @@ class BulkUpdateRequest < ApplicationRecord def approve!(approver) transaction do CurrentUser.scoped(approver) do - AliasAndImplicationImporter.new(script, forum_topic_id, true).process! + processor.process!(approver) update!(status: "approved", approver: approver, skip_secondary_validations: true) forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has been approved by @#{approver.name}.") end end - rescue AliasAndImplicationImporter::Error => x + rescue BulkUpdateRequestProcessor::Error => x self.approver = approver CurrentUser.scoped(approver) do forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has failed: #{x}") @@ -110,8 +110,8 @@ class BulkUpdateRequest < ApplicationRecord module ValidationMethods def validate_script - AliasAndImplicationImporter.new(script, forum_topic_id, skip_secondary_validations).validate! - rescue AliasAndImplicationImporter::Error => e + processor.validate! + rescue BulkUpdateRequestProcessor::Error => e errors[:base] << e.message end end @@ -120,38 +120,22 @@ class BulkUpdateRequest < ApplicationRecord include ApprovalMethods include ValidationMethods - def script_with_links - tokens = AliasAndImplicationImporter.tokenize(script) - lines = tokens.map do |token| - case token[0] - when :create_alias, :create_implication, :remove_alias, :remove_implication - "#{token[0].to_s.tr("_", " ")} [[#{token[1]}]] -> [[#{token[2]}]]" - - when :mass_update - "mass update {{#{token[1]}}} -> #{token[2]}" - - when :change_category - "category [[#{token[1]}]] -> #{token[2]}" - - else - raise "Unknown token: #{token[0]}" - end - end - lines.join("\n") - end - def normalize_text self.script = script.downcase end def update_tags - self.tags = AliasAndImplicationImporter.new(script, nil).affected_tags + self.tags = processor.affected_tags end def skip_secondary_validations=(v) @skip_secondary_validations = v.to_s.truthy? end + def processor + @processor ||= BulkUpdateRequestProcessor.new(script, forum_topic_id: forum_topic_id, skip_secondary_validations: skip_secondary_validations) + end + def is_pending? status == "pending" end diff --git a/app/views/bulk_update_requests/_listing.html.erb b/app/views/bulk_update_requests/_listing.html.erb index 61d4a5e2a..ed710f8e5 100644 --- a/app/views/bulk_update_requests/_listing.html.erb +++ b/app/views/bulk_update_requests/_listing.html.erb @@ -1,4 +1,4 @@ -<% dtext_data = DText.preprocess(bulk_update_requests.map(&:script_with_links)) %> +<% dtext_data = DText.preprocess(bulk_update_requests.map(&:processor).map(&:to_dtext)) %> <%= table_for bulk_update_requests, width: "100%" do |t| %> <% t.column "Request" do |request| %> @@ -9,7 +9,7 @@ <% end %>
- <%= format_text(request.script_with_links, data: dtext_data) %> + <%= format_text(request.processor.to_dtext, data: dtext_data) %>
<% end %> <% t.column "Votes" do |request| %> diff --git a/app/views/bulk_update_requests/show.html.erb b/app/views/bulk_update_requests/show.html.erb index f074030ab..f81923425 100644 --- a/app/views/bulk_update_requests/show.html.erb +++ b/app/views/bulk_update_requests/show.html.erb @@ -14,7 +14,7 @@

Script

- <%= format_text @bulk_update_request.script_with_links %> + <%= format_text @bulk_update_request.processor.to_dtext %>
<%= render "bur_edit_links", bur: @bulk_update_request %> diff --git a/script/fixes/064_initialize_bulk_update_request_tags.rb b/script/fixes/064_initialize_bulk_update_request_tags.rb index c68e9fef7..a398b9a96 100755 --- a/script/fixes/064_initialize_bulk_update_request_tags.rb +++ b/script/fixes/064_initialize_bulk_update_request_tags.rb @@ -4,7 +4,7 @@ require_relative "../../config/environment" BulkUpdateRequest.transaction do BulkUpdateRequest.find_each do |request| - request.tags = AliasAndImplicationImporter.new(request.script, nil).affected_tags + request.tags = request.processor.affected_tags request.save!(validate: false) puts "bur id=#{request.id} tags=#{request.tags}" end diff --git a/test/unit/alias_and_implication_importer_test.rb b/test/unit/bulk_update_request_processor_test.rb similarity index 52% rename from test/unit/alias_and_implication_importer_test.rb rename to test/unit/bulk_update_request_processor_test.rb index 60c8a7f62..664431e93 100644 --- a/test/unit/alias_and_implication_importer_test.rb +++ b/test/unit/bulk_update_request_processor_test.rb @@ -1,9 +1,9 @@ require 'test_helper' -class AliasAndImplicationImporterTest < ActiveSupport::TestCase - context "The alias and implication importer" do +class BulkUpdateRequestProcessorTest < ActiveSupport::TestCase + context "The bulk update request processor" do setup do - CurrentUser.user = FactoryBot.create(:admin_user) + CurrentUser.user = create(:admin_user) CurrentUser.ip_addr = "127.0.0.1" end @@ -13,26 +13,23 @@ class AliasAndImplicationImporterTest < ActiveSupport::TestCase end context "category command" do - setup do - @tag = Tag.find_or_create_by_name("hello") - @list = "category hello -> artist\n" - @importer = AliasAndImplicationImporter.new(@list, nil) - end + should "change the tag's category" do + @tag = create(:tag, name: "hello") + @script = "category hello -> artist\n" + @processor = BulkUpdateRequestProcessor.new(@script) + @processor.process!(CurrentUser.user) - should "work" do - @importer.process! - @tag.reload - assert_equal(Tag.categories.value_for("artist"), @tag.category) + assert_equal(Tag.categories.value_for("artist"), @tag.reload.category) end end context "#affected_tags" do setup do - FactoryBot.create(:post, tag_string: "aaa") - FactoryBot.create(:post, tag_string: "bbb") - FactoryBot.create(:post, tag_string: "ccc") - FactoryBot.create(:post, tag_string: "ddd") - FactoryBot.create(:post, tag_string: "eee") + create(:post, tag_string: "aaa") + create(:post, tag_string: "bbb") + create(:post, tag_string: "ccc") + create(:post, tag_string: "ddd") + create(:post, tag_string: "eee") @script = "create alias aaa -> 000\n" + "create implication bbb -> 111\n" + @@ -41,48 +38,41 @@ class AliasAndImplicationImporterTest < ActiveSupport::TestCase "mass update eee -> 444\n" end - subject { AliasAndImplicationImporter.new(@script, nil) } - should "return the correct tags" do - assert_equal(%w(aaa 000 bbb 111 ccc 222 ddd 333 eee 444), subject.affected_tags) + @processor = BulkUpdateRequestProcessor.new(@script) + assert_equal(%w(000 111 222 333 444 aaa bbb ccc ddd eee), @processor.affected_tags) end end context "given a valid list" do - setup do - @list = "create alias abc -> def\ncreate implication aaa -> bbb\n" - @importer = AliasAndImplicationImporter.new(@list, nil) - end - should "process it" do - @importer.process! + @list = "create alias abc -> def\ncreate implication aaa -> bbb\n" + @processor = BulkUpdateRequestProcessor.new(@list) + @processor.process!(CurrentUser.user) + assert(TagAlias.exists?(antecedent_name: "abc", consequent_name: "def")) assert(TagImplication.exists?(antecedent_name: "aaa", consequent_name: "bbb")) end end context "given a list with an invalid command" do - setup do - @list = "zzzz abc -> def\n" - @importer = AliasAndImplicationImporter.new(@list, nil) - end - should "throw an exception" do - assert_raises(RuntimeError) do - @importer.process! + @list = "zzzz abc -> def\n" + @processor = BulkUpdateRequestProcessor.new(@list) + + assert_raises(BulkUpdateRequestProcessor::Error) do + @processor.process!(CurrentUser.user) end end end context "given a list with a logic error" do - setup do - @list = "remove alias zzz -> yyy\n" - @importer = AliasAndImplicationImporter.new(@list, nil) - end - should "throw an exception" do - assert_raises(RuntimeError) do - @importer.process! + @list = "remove alias zzz -> yyy\n" + @processor = BulkUpdateRequestProcessor.new(@list) + + assert_raises(BulkUpdateRequestProcessor::Error) do + @processor.process!(CurrentUser.user) end end end @@ -92,26 +82,28 @@ class AliasAndImplicationImporterTest < ActiveSupport::TestCase tag2 = create(:tag, name: "bbb") wiki = create(:wiki_page, title: "aaa") artist = create(:artist, name: "aaa") - @importer = AliasAndImplicationImporter.new("create alias aaa -> bbb", "") - @importer.process! + + @processor = BulkUpdateRequestProcessor.new("create alias aaa -> bbb") + @processor.process!(CurrentUser.user) perform_enqueued_jobs + assert_equal("bbb", artist.reload.name) assert_equal("bbb", wiki.reload.title) end context "remove alias and remove implication commands" do setup do - @ta = FactoryBot.create(:tag_alias, antecedent_name: "a", consequent_name: "b", status: "active") - @ti = FactoryBot.create(:tag_implication, antecedent_name: "c", consequent_name: "d", status: "active") + @ta = create(:tag_alias, antecedent_name: "a", consequent_name: "b", status: "active") + @ti = create(:tag_implication, antecedent_name: "c", consequent_name: "d", status: "active") @script = %{ remove alias a -> b remove implication c -> d } - @importer = AliasAndImplicationImporter.new(@script, nil) + @processor = BulkUpdateRequestProcessor.new(@script) end should "set aliases and implications as deleted" do - @importer.process! + @processor.process!(CurrentUser.user) assert_equal("deleted", @ta.reload.status) assert_equal("deleted", @ti.reload.status) @@ -119,15 +111,15 @@ class AliasAndImplicationImporterTest < ActiveSupport::TestCase should "create modactions for each removal" do assert_difference(-> { ModAction.count }, 2) do - @importer.process! + @processor.process!(CurrentUser.user) end end should "only remove active aliases and implications" do - @ta2 = FactoryBot.create(:tag_alias, antecedent_name: "a", consequent_name: "b", status: "pending") - @ti2 = FactoryBot.create(:tag_implication, antecedent_name: "c", consequent_name: "d", status: "pending") + @ta2 = create(:tag_alias, antecedent_name: "a", consequent_name: "b", status: "pending") + @ti2 = create(:tag_implication, antecedent_name: "c", consequent_name: "d", status: "pending") - @importer.process! + @processor.process!(CurrentUser.user) assert_equal("pending", @ta2.reload.status) assert_equal("pending", @ti2.reload.status) end diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index c3d3f44f1..eb95c7418 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -150,7 +150,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end should "gracefully handle validation errors during approval" do - @req.stubs(:update!).raises(AliasAndImplicationImporter::Error.new("blah")) + @req.stubs(:update!).raises(BulkUpdateRequestProcessor::Error.new("blah")) assert_difference("ForumPost.count", 1) do @req.approve!(@admin) end