Set approver of aliases/implications in BURs.

Previously only the BUR's approver was set when a BUR was approved. Set
the approver for each alias/implication in the BUR as well.

Additionally:

* Refactor `approve!` to take a user instead of just a user id.
* Be mass-assignment permissions aware when setting approver_id.

app/controllers/bulk_update_requests_controller.rb

@@ -31,7 +31,7 @@ class BulkUpdateRequestsController < ApplicationController
   end
 
   def approve
-    @bulk_update_request.approve!(CurrentUser.user.id)
+    @bulk_update_request.approve!(CurrentUser.user)
     respond_with(@bulk_update_request, :location => bulk_update_requests_path)
   end
 

app/controllers/tag_aliases_controller.rb

@@ -43,7 +43,7 @@ class TagAliasesController < ApplicationController
 
   def approve
     @tag_alias = TagAlias.find(params[:id])
-    @tag_alias.approve!(CurrentUser.user.id)
+    @tag_alias.approve!(CurrentUser.user)
     respond_with(@tag_alias, :location => tag_alias_path(@tag_alias))
   end
 

app/controllers/tag_implications_controller.rb

@@ -48,7 +48,7 @@ class TagImplicationsController < ApplicationController
 
   def approve
     @tag_implication = TagImplication.find(params[:id])
-    @tag_implication.approve!(CurrentUser.user.id)
+    @tag_implication.approve!(CurrentUser.user)
     respond_with(@tag_implication, :location => tag_implication_path(@tag_implication))
   end
 

app/logical/alias_and_implication_importer.rb

@@ -8,9 +8,9 @@ class AliasAndImplicationImporter
     @skip_secondary_validations = skip_secondary_validations
   end
 
-  def process!
+  def process!(approver = CurrentUser.user)
     tokens = AliasAndImplicationImporter.tokenize(text)
-    parse(tokens)
+    parse(tokens, approver)
   end
 
   def validate!
@@ -77,7 +77,7 @@ class AliasAndImplicationImporter
 
 private
 
-  def parse(tokens)
+  def parse(tokens, approver)
     ActiveRecord::Base.transaction do
       tokens.map do |token|
         case token[0]
@@ -87,14 +87,14 @@ private
             raise "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.delay(:queue => "default").process!(false)
+          tag_alias.approve!(approver, update_topic: false)
 
         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})"
           end
-          tag_implication.delay(:queue => "default").process!(false)
+          tag_implication.approve!(approver)
 
         when :remove_alias
           tag_alias = TagAlias.where("antecedent_name = ?", token[1]).first

app/models/bulk_update_request.rb

@@ -33,12 +33,10 @@ class BulkUpdateRequest < ActiveRecord::Base
 
   extend SearchMethods
 
-  def approve!(approver_id)
-    self.approver_id = approver_id
-    AliasAndImplicationImporter.new(script, forum_topic_id, "1", true).process!
-    self.status = "approved"
-    self.skip_secondary_validations = true
-    save
+  def approve!(approver)
+    AliasAndImplicationImporter.new(script, forum_topic_id, "1", true).process!(approver)
+
+    update({ :status => "approved", :approver_id => approver.id, :skip_secondary_validations => true }, :as => approver.role)
     update_forum_topic_for_approve
 
   rescue Exception => x

app/models/tag_alias.rb

@@ -21,7 +21,7 @@ class TagAlias < ActiveRecord::Base
   belongs_to :approver, :class_name => "User"
   belongs_to :forum_topic
   attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :skip_secondary_validations
-  attr_accessible :status, :as => [:admin]
+  attr_accessible :status, :approver_id, :as => [:admin]
 
   module SearchMethods
     def name_matches(name)
@@ -88,9 +88,9 @@ class TagAlias < ActiveRecord::Base
     end.uniq
   end
 
-  def approve!(approver_id)
-    update_attributes(:status => "queued", :approver_id => approver_id)
-    delay(:queue => "default").process!(true)
+  def approve!(approver = CurrentUser.user, update_topic: true)
+    update({ :status => "queued", :approver_id => approver.id }, :as => approver.role)
+    delay(:queue => "default").process!(update_topic)
   end
 
   def process!(update_topic=true)
@@ -102,9 +102,8 @@ class TagAlias < ActiveRecord::Base
     forum_message = []
 
     begin
-      admin = CurrentUser.user || approver || User.admins.first
-      CurrentUser.scoped(admin, "127.0.0.1") do
-        update({ :status => "processing" }, :as => CurrentUser.role)
+      CurrentUser.scoped(approver, CurrentUser.ip_addr) do
+        update({ :status => "processing" }, :as => approver.role)
         move_aliases_and_implications
         move_saved_searches
         clear_all_cache
@@ -112,7 +111,7 @@ class TagAlias < ActiveRecord::Base
         update_posts
         forum_message << "The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) has been approved."
         forum_message << rename_wiki_and_artist
-        update({ :status => "active", :post_count => consequent_tag.post_count }, :as => CurrentUser.role)
+        update({ :status => "active", :post_count => consequent_tag.post_count }, :as => approver.role)
       end
     rescue Exception => e
       if tries < 5

app/models/tag_implication.rb

@@ -22,7 +22,7 @@ class TagImplication < ActiveRecord::Base
   validate :antecedent_and_consequent_are_different
   validate :wiki_pages_present, :on => :create
   attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :skip_secondary_validations
-  attr_accessible :status, :as => [:admin]
+  attr_accessible :status, :approver_id, :as => [:admin]
 
   module DescendantMethods
     extend ActiveSupport::Concern
@@ -138,11 +138,10 @@ class TagImplication < ActiveRecord::Base
     tries = 0
 
     begin
-      admin = CurrentUser.user || approver || User.admins.first
-      CurrentUser.scoped(admin, "127.0.0.1") do
-        update({ :status => "processing" }, :as => CurrentUser.role)
+      CurrentUser.scoped(approver, CurrentUser.ip_addr) do
+        update({ :status => "processing" }, :as => approver.role)
         update_posts
-        update({ :status => "active" }, :as => CurrentUser.role)
+        update({ :status => "active" }, :as => approver.role)
         update_descendant_names_for_parents
         update_forum_topic_for_approve if update_topic
       end
@@ -283,11 +282,9 @@ class TagImplication < ActiveRecord::Base
     end
   end
 
-  def approve!(approver_id)
-    self.status = "queued"
-    self.approver_id = approver_id
-    save
-    delay(:queue => "default").process!(true)
+  def approve!(approver = CurrentUser.user, update_topic: true)
+    update({ :status => "queued", :approver_id => approver.id }, :as => approver.role)
+    delay(:queue => "default").process!(update_topic)
   end
 
   def reject!

test/unit/bulk_update_request_test.rb

@@ -22,7 +22,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
         )
 
         @bur = FactoryGirl.create(:bulk_update_request, :script => @script)
-        @bur.approve!(@admin.id)
+        @bur.approve!(@admin)
 
         @ta = TagAlias.where(:antecedent_name => "foo", :consequent_name => "bar").first
         @ti = TagImplication.where(:antecedent_name => "bar", :consequent_name => "baz").first
@@ -72,7 +72,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
       should "handle errors gracefully" do
         @req.stubs(:update_forum_topic_for_approve).raises(RuntimeError.new("blah"))
         assert_difference("Dmail.count", 1) do
-          @req.approve!(@admin.id)
+          @req.approve!(@admin)
         end
         assert_match(/Exception: RuntimeError/, Dmail.last.body)
         assert_match(/Message: blah/, Dmail.last.body)
@@ -84,7 +84,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
 
       should "update the topic when processed" do
         assert_difference("ForumPost.count") do
-          @req.approve!(@admin.id)
+          @req.approve!(@admin)
         end
       end