From 1ddcc661e1092f534e64f3492ed4bea5823d8c3d Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Mon, 24 Aug 2020 16:41:52 -0500
Subject: [PATCH] BURs: clean up parsing and error handling.

* Don't raise exceptions when a BUR is invalid. Instead, use Rails
  validations to return errors. Fixes invalid BURs potentially raising
  exceptions in views. Also makes it so that each error in a BUR is
  reported, not just the first one.

* Revalidate the BUR whenever the script is edited, not just when the
  BUR is created. Ensures the BUR can't be broken by editing. Fixes a bug
  where forum threads could be broken by someone editing a BUR and
  breaking the syntax, thereby causing the BUR to raise an unparseable
  script error when the forum thread was viewed.

* Validate that removed aliases and implication actually exist.

* Validate that the tag actually exists when changing a tag's category.

* Combine bulk update request processor unit tests with main bulk update
  request unit tests.
---
 app/logical/bulk_update_request_processor.rb  | 137 +++++----
 app/models/bulk_update_request.rb             |  15 +-
 .../bulk_update_requests_controller_test.rb   |   8 +-
 .../bulk_update_request_processor_test.rb     | 128 ---------
 test/unit/bulk_update_request_test.rb         | 272 +++++++++++++-----
 5 files changed, 287 insertions(+), 273 deletions(-)
 delete mode 100644 test/unit/bulk_update_request_processor_test.rb

diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb
index 293320510..32c80fc27 100644
--- a/app/logical/bulk_update_request_processor.rb
+++ b/app/logical/bulk_update_request_processor.rb
@@ -1,109 +1,127 @@
 class BulkUpdateRequestProcessor
-  extend Memoist
+  include ActiveModel::Validations
 
   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
+  attr_reader :bulk_update_request
+  delegate :script, :forum_topic_id, :skip_secondary_validations, to: :bulk_update_request
+  validate :validate_script
+
+  def initialize(bulk_update_request)
+    @bulk_update_request = bulk_update_request
   end
 
-  def tokens
-    text.split(/\r\n|\r|\n/).reject(&:blank?).map do |line|
+  # Parse the script into a list of commands.
+  def commands
+    script.split(/\r\n|\r|\n/).reject(&:blank?).map do |line|
       line = line.gsub(/[[:space:]]+/, " ").strip
+      next if line.empty?
 
-      if line =~ /^(?:create alias|aliasing|alias) (\S+) -> (\S+)$/i
+      case line
+      when /\A(?:create alias|aliasing|alias) (\S+) -> (\S+)\z/i
         [:create_alias, $1, $2]
-      elsif line =~ /^(?:create implication|implicating|implicate|imply) (\S+) -> (\S+)$/i
+      when /\A(?:create implication|implicating|implicate|imply) (\S+) -> (\S+)\z/i
         [:create_implication, $1, $2]
-      elsif line =~ /^(?:remove alias|unaliasing|unalias) (\S+) -> (\S+)$/i
+      when /\A(?:remove alias|unaliasing|unalias) (\S+) -> (\S+)\z/i
         [:remove_alias, $1, $2]
-      elsif line =~ /^(?:remove implication|unimplicating|unimplicate|unimply) (\S+) -> (\S+)$/i
+      when /\A(?:remove implication|unimplicating|unimplicate|unimply) (\S+) -> (\S+)\z/i
         [:remove_implication, $1, $2]
-      elsif line =~ /^(?:mass update|updating|update|change) (.+?) -> (.*)$/i
+      when /\A(?:mass update|updating|update|change) (.+?) -> (.*)\z/i
         [:mass_update, $1, $2]
-      elsif line =~ /^category (\S+) -> (#{Tag.categories.regexp})/
+      when /\Acategory (\S+) -> (#{Tag.categories.regexp})\z/i
         [:change_category, $1, $2]
-      elsif line.strip.empty?
-        # do nothing
       else
-        raise Error, "Unparseable line: #{line}"
+        [:invalid_line, line]
       end
     end
   end
 
-  def validate!
-    tokens.map do |token|
-      case token[0]
+  def validate_script
+    commands.each do |command, *args|
+      case command
       when :create_alias
-        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})"
+        tag_alias = TagAlias.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1], skip_secondary_validations: skip_secondary_validations)
+        if tag_alias.invalid?
+          errors[:base] << "Can't create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name} (#{tag_alias.errors.full_messages.join("; ")})"
         end
 
       when :create_implication
-        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})"
+        tag_implication = TagImplication.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1], skip_secondary_validations: skip_secondary_validations)
+        if tag_implication.invalid?
+          errors[:base] << "Can't create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name} (#{tag_implication.errors.full_messages.join("; ")})"
         end
 
-      when :remove_alias, :remove_implication, :mass_update, :change_category
+      when :remove_alias
+        tag_alias = TagAlias.active.find_by(antecedent_name: args[0], consequent_name: args[1])
+        if tag_alias.nil?
+          errors[:base] << "Can't remove alias #{args[0]} -> #{args[1]} (alias doesn't exist)"
+        end
+
+      when :remove_implication
+        tag_implication = TagImplication.active.find_by(antecedent_name: args[0], consequent_name: args[1])
+        if tag_implication.nil?
+          errors[:base] << "Can't remove implication #{args[0]} -> #{args[1]} (implication doesn't exist)"
+        end
+
+      when :change_category
+        tag = Tag.find_by_name(args[0])
+        if tag.nil?
+          errors[:base] << "Can't change category #{args[0]} -> #{args[1]} (the '#{args[0]}' tag doesn't exist)"
+        end
+
+      when :mass_update
         # okay
 
+      when :invalid_line
+        errors[:base] << "Invalid line: #{args[0]}"
+
       else
-        raise NotImplementedError, "Unknown token: #{token[0]}" # should never happen
+        # should never happen
+        raise Error, "Unknown command: #{command}"
       end
     end
   end
 
   def process!(approver)
     ActiveRecord::Base.transaction do
-      tokens.map do |token|
-        case token[0]
+      validate!
+
+      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: 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 = 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)
 
         when :create_implication
-          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
+          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)
 
         when :remove_alias
-          tag_alias = TagAlias.active.find_by(antecedent_name: token[1], consequent_name: token[2])
-          raise Error, "Alias for #{token[1]} not found" if tag_alias.nil?
+          tag_alias = TagAlias.active.find_by!(antecedent_name: args[0], consequent_name: args[1])
           tag_alias.reject!
 
         when :remove_implication
-          tag_implication = TagImplication.active.find_by(antecedent_name: token[1], consequent_name: token[2])
-          raise Error, "Implication for #{token[1]} not found" if tag_implication.nil?
+          tag_implication = TagImplication.active.find_by!(antecedent_name: args[0], consequent_name: args[1])
           tag_implication.reject!
 
         when :mass_update
-          TagBatchChangeJob.perform_later(token[1], token[2], User.system, "127.0.0.1")
+          TagBatchChangeJob.perform_later(args[0], args[1], User.system, "127.0.0.1")
 
         when :change_category
-          tag = Tag.find_or_create_by_name(token[1])
-          tag.category = Tag.categories.value_for(token[2])
-          tag.save
+          tag = Tag.find_or_create_by_name(args[0])
+          tag.update!(category: Tag.categories.value_for(args[1]))
 
         else
-          raise Error, "Unknown token: #{token[0]}"
+          # should never happen
+          raise Error, "Unknown command: #{command}"
         end
       end
     end
   end
 
   def affected_tags
-    tokens.flat_map do |type, *args|
-      case type
+    commands.flat_map do |command, *args|
+      case command
       when :create_alias, :remove_alias, :create_implication, :remove_implication
         [args[0], args[1]]
       when :mass_update
@@ -113,13 +131,11 @@ class BulkUpdateRequestProcessor
         args[0]
       end
     end.sort.uniq
-  rescue Error
-    []
   end
 
   def is_tag_move_allowed?
-    tokens.all? do |type, *args|
-      case type
+    commands.all? do |command, *args|
+      case command
       when :create_alias
         BulkUpdateRequestProcessor.is_tag_move_allowed?(args[0], args[1])
       when :mass_update
@@ -134,16 +150,17 @@ class BulkUpdateRequestProcessor
   end
 
   def to_dtext
-    tokens.map do |token|
-      case token[0]
+    commands.map do |command, *args|
+      case command
       when :create_alias, :create_implication, :remove_alias, :remove_implication
-        "#{token[0].to_s.tr("_", " ")} [[#{token[1]}]] -> [[#{token[2]}]]"
+        "#{command.to_s.tr("_", " ")} [[#{args[0]}]] -> [[#{args[1]}]]"
       when :mass_update
-        "mass update {{#{token[1]}}} -> #{token[2]}"
+        "mass update {{#{args[0]}}} -> #{args[1]}"
       when :change_category
-        "category [[#{token[1]}]] -> #{token[2]}"
+        "category [[#{args[0]}]] -> #{args[1]}"
       else
-        raise "Unknown token: #{token[0]}"
+        # should never happen
+        raise Error, "Unknown command: #{command}"
       end
     end.join("\n")
   end
@@ -155,6 +172,4 @@ class BulkUpdateRequestProcessor
     (antecedent_tag.blank? || antecedent_tag.empty? || (antecedent_tag.artist? && antecedent_tag.post_count <= 100)) &&
     (consequent_tag.blank? || consequent_tag.empty? || (consequent_tag.artist? && consequent_tag.post_count <= 100))
   end
-
-  memoize :tokens
 end
diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb
index 2070dc1db..531d10be2 100644
--- a/app/models/bulk_update_request.rb
+++ b/app/models/bulk_update_request.rb
@@ -9,14 +9,14 @@ class BulkUpdateRequest < ApplicationRecord
   belongs_to :approver, optional: true, class_name: "User"
 
   before_validation :normalize_text
-  before_validation :update_tags
   validates_presence_of :reason, on: :create
   validates_presence_of :script
   validates_presence_of :title, if: ->(rec) {rec.forum_topic_id.blank?}
   validates_presence_of :forum_topic, if: ->(rec) {rec.forum_topic_id.present?}
   validates_inclusion_of :status, :in => %w(pending approved rejected)
-  validate :validate_script, :on => :create
+  validate :validate_script, if: :script_changed?
 
+  before_save :update_tags, if: :script_changed?
   after_create :create_forum_topic
 
   scope :pending_first, -> { order(Arel.sql("(case status when 'pending' then 0 when 'approved' then 1 else 2 end)")) }
@@ -108,17 +108,14 @@ class BulkUpdateRequest < ApplicationRecord
     end
   end
 
-  module ValidationMethods
-    def validate_script
-      processor.validate!
-    rescue BulkUpdateRequestProcessor::Error => e
-      errors[:base] << e.message
+  def validate_script
+    if processor.invalid?
+      errors[:base] << processor.errors.full_messages.join("; ")
     end
   end
 
   extend SearchMethods
   include ApprovalMethods
-  include ValidationMethods
 
   def normalize_text
     self.script = script.downcase
@@ -133,7 +130,7 @@ class BulkUpdateRequest < ApplicationRecord
   end
 
   def processor
-    @processor ||= BulkUpdateRequestProcessor.new(script, forum_topic_id: forum_topic_id, skip_secondary_validations: skip_secondary_validations)
+    @processor ||= BulkUpdateRequestProcessor.new(self)
   end
 
   def is_tag_move_allowed?
diff --git a/test/functional/bulk_update_requests_controller_test.rb b/test/functional/bulk_update_requests_controller_test.rb
index 5b91dd92f..5303719ca 100644
--- a/test/functional/bulk_update_requests_controller_test.rb
+++ b/test/functional/bulk_update_requests_controller_test.rb
@@ -7,7 +7,7 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest
       @builder = create(:builder_user)
       @admin = create(:admin_user)
       as(@admin) { @forum_topic = create(:forum_topic, id: 100, category_id: 0) }
-      as(@user) { @bulk_update_request = create(:bulk_update_request, user: @user, forum_topic: @forum_topic) }
+      as(@user) { @bulk_update_request = create(:bulk_update_request, user: @user, forum_topic: @forum_topic, script: "create alias aaa -> bbb") }
     end
 
     context "#new" do
@@ -71,6 +71,12 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest
         assert_response 403
         assert_equal("create alias aaa -> bbb", @bulk_update_request.reload.script)
       end
+
+      should "fail for an invalid script" do
+        put_auth bulk_update_request_path(@bulk_update_request.id), @user, params: { bulk_update_request: { script: "create alis gray -> grey" }}
+        assert_response :success
+        assert_equal("create alias aaa -> bbb", @bulk_update_request.reload.script)
+      end
     end
 
     context "#index" do
diff --git a/test/unit/bulk_update_request_processor_test.rb b/test/unit/bulk_update_request_processor_test.rb
deleted file mode 100644
index 664431e93..000000000
--- a/test/unit/bulk_update_request_processor_test.rb
+++ /dev/null
@@ -1,128 +0,0 @@
-require 'test_helper'
-
-class BulkUpdateRequestProcessorTest < ActiveSupport::TestCase
-  context "The bulk update request processor" do
-    setup do
-      CurrentUser.user = create(:admin_user)
-      CurrentUser.ip_addr = "127.0.0.1"
-    end
-
-    teardown do
-      CurrentUser.user = nil
-      CurrentUser.ip_addr = nil
-    end
-
-    context "category command" do
-      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)
-
-        assert_equal(Tag.categories.value_for("artist"), @tag.reload.category)
-      end
-    end
-
-    context "#affected_tags" do
-      setup do
-        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" +
-          "remove alias ccc -> 222\n" +
-          "remove implication ddd -> 333\n" +
-          "mass update eee -> 444\n"
-      end
-
-      should "return the correct tags" do
-        @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
-      should "process it" do
-        @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
-      should "throw an exception" do
-        @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
-      should "throw an exception" do
-        @list = "remove alias zzz -> yyy\n"
-        @processor = BulkUpdateRequestProcessor.new(@list)
-
-        assert_raises(BulkUpdateRequestProcessor::Error) do
-          @processor.process!(CurrentUser.user)
-        end
-      end
-    end
-
-    should "rename an aliased tag's artist entry and wiki page" do
-      tag1 = create(:tag, name: "aaa", category: 1)
-      tag2 = create(:tag, name: "bbb")
-      wiki = create(:wiki_page, title: "aaa")
-      artist = create(:artist, name: "aaa")
-
-      @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 = 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
-        }
-        @processor = BulkUpdateRequestProcessor.new(@script)
-      end
-
-      should "set aliases and implications as deleted" do
-        @processor.process!(CurrentUser.user)
-
-        assert_equal("deleted", @ta.reload.status)
-        assert_equal("deleted", @ti.reload.status)
-      end
-
-      should "create modactions for each removal" do
-        assert_difference(-> { ModAction.count }, 2) do
-          @processor.process!(CurrentUser.user)
-        end
-      end
-
-      should "only remove active aliases and implications" do
-        @ta2 = create(:tag_alias, antecedent_name: "a", consequent_name: "b", status: "pending")
-        @ti2 = create(:tag_implication, antecedent_name: "c", consequent_name: "d", status: "pending")
-
-        @processor.process!(CurrentUser.user)
-        assert_equal("pending", @ta2.reload.status)
-        assert_equal("pending", @ti2.reload.status)
-      end
-    end
-  end
-end
diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb
index c4763e8e1..7351a22be 100644
--- a/test/unit/bulk_update_request_test.rb
+++ b/test/unit/bulk_update_request_test.rb
@@ -13,24 +13,209 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
       CurrentUser.ip_addr = nil
     end
 
-    should "parse the tags inside the script" do
-      @bur = create(:bulk_update_request, script: %{
-        create alias aaa -> 000
-        create implication bbb -> 111
-        remove alias ccc -> 222
-        remove implication ddd -> 333
-        mass update eee id:1 -fff ~ggg hhh* -> 444 -555
-        category iii -> meta
-      })
-
-      assert_equal(%w[000 111 222 333 444 aaa bbb ccc ddd eee iii], @bur.tags)
-    end
-
     should_eventually "parse tags with tag type prefixes inside the script" do
       @bur = create(:bulk_update_request, script: "mass update aaa -> artist:bbb")
       assert_equal(%w[aaa bbb], @bur.tags)
     end
 
+    context "when approving a BUR" do
+      context "the category command" do
+        should "change the tag's category" do
+          @tag = create(:tag, name: "hello")
+          @bur = create(:bulk_update_request, script: "category hello -> artist")
+          @bur.approve!(@admin)
+
+          assert_equal(true, @bur.valid?)
+          assert_equal(true, @tag.reload.artist?)
+        end
+
+        should "fail if the tag doesn't already exist" do
+          @bur = build(:bulk_update_request, script: "category hello -> artist")
+
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Can't change category hello -> artist (the 'hello' tag doesn't exist)"], @bur.errors[:base])
+        end
+      end
+
+      context "the create alias command" do
+        setup do
+          @wiki = create(:wiki_page, title: "foo")
+          @artist = create(:artist, name: "foo")
+          @bur = create(:bulk_update_request, script: "create alias foo -> bar")
+
+          @bur.approve!(@admin)
+          perform_enqueued_jobs
+        end
+
+        should "create an alias" do
+          @alias = TagAlias.find_by(antecedent_name: "foo", consequent_name: "bar")
+          assert_equal(true, @alias.present?)
+          assert_equal(true, @alias.is_active?)
+        end
+
+        should "rename the aliased tag's artist entry and wiki page" do
+          assert_equal("bar", @artist.reload.name)
+          assert_equal("bar", @wiki.reload.title)
+        end
+
+        should "fail if the alias is invalid" do
+          create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb")
+          @bur = build(:bulk_update_request, script: "create alias bbb -> aaa")
+
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Can't create alias bbb -> aaa (A tag alias for aaa already exists)"], @bur.errors.full_messages)
+        end
+      end
+
+      context "the create implication command" do
+        should "create an implication" do
+          @bur = create(:bulk_update_request, script: "create implication foo -> bar")
+          @bur.approve!(@admin)
+          perform_enqueued_jobs
+
+          @implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar")
+          assert_equal(true, @implication.present?)
+          assert_equal(true, @implication.is_active?)
+        end
+
+        should "fail for an implication that is redundant with an existing implication" do
+          create(:tag_implication, antecedent_name: "a", consequent_name: "b")
+          create(:tag_implication, antecedent_name: "b", consequent_name: "c")
+          @bur = build(:bulk_update_request, script: "imply a -> c")
+
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Can't create implication a -> c (a already implies c through another implication)"], @bur.errors.full_messages)
+        end
+
+        should_eventually "fail for an implication that is redundant with another implication in the same BUR" do
+          create(:tag_implication, antecedent_name: "b", consequent_name: "c")
+          @bur = build(:bulk_update_request, script: "imply a -> b\nimply a -> c")
+
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Can't create implication a -> c (a already implies c through another implication)"], @bur.errors.full_messages)
+        end
+      end
+
+      context "the remove alias command" do
+        should "remove an alias" do
+          create(:tag_alias, antecedent_name: "foo", consequent_name: "bar")
+          @bur = create(:bulk_update_request, script: "remove alias foo -> bar")
+          @bur.approve!(@admin)
+
+          @alias = TagAlias.find_by(antecedent_name: "foo", consequent_name: "bar")
+          assert_equal(true, @alias.present?)
+          assert_equal(true, @alias.is_deleted?)
+        end
+
+        should "fail if the alias isn't active" do
+          create(:tag_alias, antecedent_name: "foo", consequent_name: "bar", status: "pending")
+          @bur = build(:bulk_update_request, script: "remove alias foo -> bar")
+
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Can't remove alias foo -> bar (alias doesn't exist)"], @bur.errors[:base])
+        end
+
+        should "fail if the alias doesn't already exist" do
+          @bur = build(:bulk_update_request, script: "remove alias foo -> bar")
+
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Can't remove alias foo -> bar (alias doesn't exist)"], @bur.errors[:base])
+        end
+      end
+
+      context "the remove implication command" do
+        should "remove an implication" do
+          create(:tag_implication, antecedent_name: "foo", consequent_name: "bar", status: "active")
+          @bur = create(:bulk_update_request, script: "remove implication foo -> bar")
+          @bur.approve!(@admin)
+
+          @implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar")
+          assert_equal(true, @implication.present?)
+          assert_equal(true, @implication.is_deleted?)
+        end
+
+        should "fail if the implication isn't active" do
+          create(:tag_implication, antecedent_name: "foo", consequent_name: "bar", status: "pending")
+          @bur = build(:bulk_update_request, script: "remove implication foo -> bar")
+
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Can't remove implication foo -> bar (implication doesn't exist)"], @bur.errors[:base])
+        end
+
+        should "fail if the implication doesn't already exist" do
+          @bur = build(:bulk_update_request, script: "remove implication foo -> bar")
+
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Can't remove implication foo -> bar (implication doesn't exist)"], @bur.errors[:base])
+        end
+      end
+
+      context "the mass update command" do
+        setup do
+          @post = create(:post, tag_string: "foo")
+          @bur = create(:bulk_update_request, script: "mass update foo -> bar")
+          @bur.approve!(@admin)
+          perform_enqueued_jobs
+        end
+
+        should "update the tags" do
+          assert_equal("bar", @post.reload.tag_string)
+        end
+      end
+    end
+
+    context "when validating a script" do
+      context "an unparseable script" do
+        should "fail validation" do
+          @script = <<~EOS
+            create alias aaa -> 000
+            create alias bbb > 111
+            create alias ccc -> 222
+          EOS
+
+          @bur = build(:bulk_update_request, script: @script)
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Invalid line: create alias bbb > 111"], @bur.errors[:base])
+        end
+      end
+
+      context "a script with extra whitespace" do
+        should "validate" do
+          @script = %{
+            create alias aaa -> 000 
+
+            create alias bbb -> 111 
+          }
+
+          @bur = create(:bulk_update_request, script: @script)
+          assert_equal(true, @bur.valid?)
+        end
+      end
+    end
+
+    context "when the script is updated" do
+      should "update the BUR's list of affected tags" do
+        create(:tag_alias, antecedent_name: "ccc", consequent_name: "222")
+        create(:tag_implication, antecedent_name: "ddd", consequent_name: "333")
+        create(:tag, name: "iii")
+
+        @script = <<~EOS
+          create alias aaa -> 000
+          create implication bbb -> 111
+          remove alias ccc -> 222
+          remove implication ddd -> 333
+          mass update eee id:1 -fff ~ggg hhh* -> 444 -555
+          category iii -> meta
+        EOS
+
+        @bur = create(:bulk_update_request, script: "create alias aaa -> bbb")
+        assert_equal(%w[aaa bbb], @bur.tags)
+
+        @bur.update!(script: @script)
+        assert_equal(%w(000 111 222 333 444 aaa bbb ccc ddd eee iii), @bur.tags)
+      end
+    end
+
     context "on approval" do
       setup do
         @post = create(:post, tag_string: "foo aaa")
@@ -81,67 +266,6 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
       assert_match(/zzz/, bur.forum_post.body)
     end
 
-    context "that has an invalid alias" do
-      setup do
-        @alias1 = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", creator: @admin)
-        @req = FactoryBot.build(:bulk_update_request, :script => "create alias bbb -> aaa")
-      end
-
-      should "not validate" do
-        assert_difference("TagAlias.count", 0) do
-          @req.save
-        end
-        assert_equal(["Error: A tag alias for aaa already exists (create alias bbb -> aaa)"], @req.errors.full_messages)
-      end
-    end
-
-    context "for an implication that is redundant with an existing implication" do
-      should "not validate" do
-        FactoryBot.create(:tag_implication, :antecedent_name => "a", :consequent_name => "b")
-        FactoryBot.create(:tag_implication, :antecedent_name => "b", :consequent_name => "c")
-        bur = FactoryBot.build(:bulk_update_request, :script => "imply a -> c")
-        bur.save
-
-        assert_equal(["Error: a already implies c through another implication (create implication a -> c)"], bur.errors.full_messages)
-      end
-    end
-
-    context "for an implication that is redundant with another implication in the same BUR" do
-      setup do
-        FactoryBot.create(:tag_implication, :antecedent_name => "b", :consequent_name => "c")
-        @bur = FactoryBot.build(:bulk_update_request, :script => "imply a -> b\nimply a -> c")
-        @bur.save
-      end
-
-      should "not process" do
-        assert_no_difference("TagImplication.count") do
-          @bur.approve!(@admin)
-        end
-      end
-
-      should_eventually "not validate" do
-        assert_equal(["Error: a already implies c through another implication (create implication a -> c)"], @bur.errors.full_messages)
-      end
-    end
-
-    context "for a `category <tag> -> type` change" do
-      should "work" do
-        tag = Tag.find_or_create_by_name("tagme")
-        bur = FactoryBot.create(:bulk_update_request, :script => "category tagme -> meta")
-        bur.approve!(@admin)
-
-        assert_equal(Tag.categories.meta, tag.reload.category)
-      end
-
-      should "work for a new tag" do
-        bur = FactoryBot.create(:bulk_update_request, :script => "category new_tag -> meta")
-        bur.approve!(@admin)
-
-        assert_not_nil(Tag.find_by_name("new_tag"))
-        assert_equal(Tag.categories.meta, Tag.find_by_name("new_tag").category)
-      end
-    end
-
     context "with an associated forum topic" do
       setup do
         @topic = create(:forum_topic, title: "[bulk] hoge", creator: @admin)