Merge pull request #3564 from evazion/fix-3554

Fix #3554: BUR approval bugs

app/logical/alias_and_implication_importer.rb

@@ -1,4 +1,5 @@
 class AliasAndImplicationImporter
+  class Error < RuntimeError; end
   attr_accessor :text, :commands, :forum_id, :rename_aliased_pages, :skip_secondary_validations
 
   def initialize(text, forum_id, rename_aliased_pages = "0", skip_secondary_validations = true)
@@ -50,7 +51,7 @@ class AliasAndImplicationImporter
         # do nothing
 
       else
-        raise "Unparseable line: #{line}"
+        raise Error, "Unparseable line: #{line}"
       end
     end
   end
@@ -61,20 +62,20 @@ class AliasAndImplicationImporter
       when :create_alias
         tag_alias = TagAlias.new(:forum_topic_id => forum_id, :status => "pending", :antecedent_name => token[1], :consequent_name => token[2], :skip_secondary_validations => skip_secondary_validations)
         unless tag_alias.valid?
-          raise "Error: #{tag_alias.errors.full_messages.join("; ")} (create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name})"
+          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(:forum_topic_id => forum_id, :status => "pending", :antecedent_name => token[1], :consequent_name => token[2], :skip_secondary_validations => skip_secondary_validations)
         unless tag_implication.valid?
-          raise "Error: #{tag_implication.errors.full_messages.join("; ")} (create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name})"
+          raise Error, "Error: #{tag_implication.errors.full_messages.join("; ")} (create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name})"
         end
 
       when :remove_alias, :remove_implication, :mass_update, :change_category
         # okay
 
       else
-        raise "Unknown token: #{token[0]}"
+        raise Error, "Unknown token: #{token[0]}"
       end
     end
   end
@@ -88,7 +89,7 @@ private
         when :create_alias
           tag_alias = TagAlias.create(:forum_topic_id => forum_id, :status => "pending", :antecedent_name => token[1], :consequent_name => token[2], :skip_secondary_validations => skip_secondary_validations)
           unless tag_alias.valid?
-            raise "Error: #{tag_alias.errors.full_messages.join("; ")} (create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name})"
+            raise Error, "Error: #{tag_alias.errors.full_messages.join("; ")} (create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name})"
           end
           tag_alias.rename_wiki_and_artist if rename_aliased_pages?
           tag_alias.approve!(approver: approver, update_topic: false)
@@ -96,18 +97,18 @@ private
         when :create_implication
           tag_implication = TagImplication.create(:forum_topic_id => forum_id, :status => "pending", :antecedent_name => token[1], :consequent_name => token[2], :skip_secondary_validations => skip_secondary_validations)
           unless tag_implication.valid?
-            raise "Error: #{tag_implication.errors.full_messages.join("; ")} (create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name})"
+            raise Error, "Error: #{tag_implication.errors.full_messages.join("; ")} (create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name})"
           end
           tag_implication.approve!(approver: approver, update_topic: false)
 
         when :remove_alias
           tag_alias = TagAlias.where("antecedent_name = ?", token[1]).first
-          raise "Alias for #{token[1]} not found" if tag_alias.nil?
+          raise Error, "Alias for #{token[1]} not found" if tag_alias.nil?
           tag_alias.destroy
 
         when :remove_implication
           tag_implication = TagImplication.where("antecedent_name = ? and consequent_name = ?", token[1], token[2]).first
-          raise "Implication for #{token[1]} not found" if tag_implication.nil?
+          raise Error, "Implication for #{token[1]} not found" if tag_implication.nil?
           tag_implication.destroy
 
         when :mass_update
@@ -119,7 +120,7 @@ private
           tag.save
 
         else
-          raise "Unknown token: #{token[0]}"
+          raise Error, "Unknown token: #{token[0]}"
         end
       end
     end

app/models/bulk_update_request.rb

@@ -90,13 +90,15 @@ class BulkUpdateRequest < ApplicationRecord
     end
 
     def approve!(approver)
-      CurrentUser.scoped(approver) do
-        AliasAndImplicationImporter.new(script, forum_topic_id, "1", true).process!
-        update(status: "approved", approver: CurrentUser.user, skip_secondary_validations: true)
-        forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has been approved by @#{approver.name}.", "APPROVED")
+      transaction do
+        CurrentUser.scoped(approver) do
+          AliasAndImplicationImporter.new(script, forum_topic_id, "1", true).process!
+          update(status: "approved", approver: CurrentUser.user, skip_secondary_validations: true)
+          forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has been approved by @#{approver.name}.", "APPROVED")
+        end
       end
 
-    rescue Exception => x
+    rescue AliasAndImplicationImporter::Error => x
       self.approver = approver
       CurrentUser.scoped(approver) do
         forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has failed: #{x.to_s}", "FAILED")
@@ -118,8 +120,10 @@ class BulkUpdateRequest < ApplicationRecord
     end
 
     def reject!(rejector)
-      forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has been rejected by @#{rejector.name}.", "REJECTED")
-      update_attribute(:status, "rejected")
+      transaction do
+        update(status: "rejected")
+        forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has been rejected by @#{rejector.name}.", "REJECTED")
+      end
     end
 
     def bulk_update_request_link

test/unit/bulk_update_request_test.rb

@@ -112,15 +112,21 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
         @req = FactoryGirl.create(:bulk_update_request, :script => "create alias AAA -> BBB", :forum_topic_id => @topic.id, :forum_post_id => @post.id, :title => "[bulk] hoge")
       end
 
-      should "handle errors gracefully" do
-        @req.stubs(:update).raises(RuntimeError.new("blah"))
+      should "gracefully handle validation errors during approval" do
+        @req.stubs(:update).raises(AliasAndImplicationImporter::Error.new("blah"))
         assert_difference("ForumPost.count", 1) do
           @req.approve!(@admin)
         end
 
-        @topic.reload
-        @post.reload
-        assert_match(/\[FAILED\]/, @topic.title)
+        assert_equal("pending", @req.reload.status)
+        assert_match(/\[FAILED\]/, @topic.reload.title)
+      end
+
+      should "leave the BUR pending if there is an unexpected error during approval" do
+        @req.forum_updater.stubs(:update).raises(RuntimeError.new("blah"))
+        assert_raises(RuntimeError) { @req.approve!(@admin) }
+
+        assert_equal("pending", @req.reload.status)
       end
 
       should "downcase the text" do