Merge pull request #3375 from evazion/feat-3372

Fix #3374: List approver in BUR approval messages

app/controllers/bulk_update_requests_controller.rb

@@ -38,7 +38,7 @@ class BulkUpdateRequestsController < ApplicationController
 
   def destroy
     if @bulk_update_request.editable?(CurrentUser.user)
-      @bulk_update_request.reject!
+      @bulk_update_request.reject!(CurrentUser.user)
       flash[:notice] = "Bulk update request rejected"
       respond_with(@bulk_update_request, :location => bulk_update_requests_path)
     else

app/logical/forum_updater.rb

@@ -21,9 +21,10 @@ class ForumUpdater
   end
 
   def create_response(body)
-    forum_topic.posts.create(
-      :body => body
-    )
+    forum_topic.posts.create({
+      :body => body,
+      :skip_mention_notifications => true
+    }, :without_protection => true)
   end
 
   def update_title(title_tag)
@@ -33,6 +34,6 @@ class ForumUpdater
   end
 
   def update_post(body)
-    forum_post.update(:body => "#{forum_post.body}\n\n#{body}")
+    forum_post.update({:body => "#{forum_post.body}\n\nEDIT: #{body}", :skip_mention_notifications => true }, :without_protection => true)
   end
 end

app/logical/mentionable.rb

@@ -9,7 +9,7 @@ module Mentionable
       @mentionable_options = options
 
       message_field = mentionable_option(:message_field)
-      after_save :queue_mention_messages, if: :"#{message_field}_changed?"
+      after_save :queue_mention_messages
     end
 
     def mentionable_option(key)
@@ -19,6 +19,9 @@ module Mentionable
 
   def queue_mention_messages
     message_field = self.class.mentionable_option(:message_field)
+    return if !send("#{message_field}_changed?")
+    return if self.skip_mention_notifications
+
     text = send(message_field)
     text_was = send("#{message_field}_was")
 

app/logical/tag_alias_request.rb

@@ -31,7 +31,9 @@ class TagAliasRequest
       @forum_topic = build_forum_topic(@tag_alias.id)
       @forum_topic.save
 
-      @tag_alias.update_attributes(forum_topic_id: @forum_topic.id, forum_post_id: @forum_topic.posts.first.id)
+      @tag_alias.forum_topic_id = @forum_topic.id
+      @tag_alias.forum_post_id = @forum_topic.posts.first.id
+      @tag_alias.save
     end
   end
 

app/logical/tag_implication_request.rb

@@ -31,7 +31,9 @@ class TagImplicationRequest
       @forum_topic = build_forum_topic(@tag_implication.id)
       @forum_topic.save
 
-      @tag_implication.update_attributes(:forum_topic_id => @forum_topic.id, :forum_post_id => @forum_topic.posts.first.id)
+      @tag_implication.forum_topic_id = @forum_topic.id
+      @tag_implication.forum_post_id = @forum_topic.posts.first.id
+      @tag_implication.save
     end
   end
 

app/models/bulk_update_request.rb

@@ -95,13 +95,13 @@ class BulkUpdateRequest < ApplicationRecord
       CurrentUser.scoped(approver) do
         AliasAndImplicationImporter.new(script, forum_topic_id, "1", true).process!
         update({ :status => "approved", :approver_id => CurrentUser.id, :skip_secondary_validations => true }, :as => CurrentUser.role)
-        forum_updater.update("The \"bulk update request ##{id}\":/bulk_update_requests?search%5Bid%5D=#{id} has been approved.", "APPROVED")
+        forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has been approved by @#{approver.name}.", "APPROVED")
       end
 
     rescue Exception => x
       self.approver = approver
       CurrentUser.scoped(approver) do
-        forum_updater.update("The \"Bulk update request ##{id}\":/bulk_update_requests?search%5Bid%5D=#{id} has failed: #{x.to_s}", "FAILED")
+        forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has failed: #{x.to_s}", "FAILED")
       end
     end
 
@@ -119,10 +119,14 @@ class BulkUpdateRequest < ApplicationRecord
       end
     end
 
-    def reject!
-      forum_updater.update("The \"bulk update request ##{id}\":/bulk_update_requests?search%5Bid%5D=#{id} has been rejected.", "REJECTED")
+    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")
     end
+
+    def bulk_update_request_link
+      %{"bulk update request ##{id}":/bulk_update_requests?search%5Bid%5D=#{id}}
+    end
   end
 
   module ValidationMethods

app/models/forum_post.rb

@@ -3,6 +3,7 @@ class ForumPost < ApplicationRecord
 
   attr_accessible :body, :topic_id, :as => [:member, :builder, :gold, :platinum, :admin, :moderator, :default]
   attr_accessible :is_locked, :is_sticky, :is_deleted, :as => [:admin, :moderator]
+  attr_accessor :skip_mention_notifications # used by `Mentionable::queue_mention_messages`
   attr_readonly :topic_id
   belongs_to :creator, :class_name => "User"
   belongs_to :updater, :class_name => "User"

app/models/tag_alias.rb

@@ -42,26 +42,6 @@ class TagAlias < TagRelationship
         delay(:queue => "default").process!(update_topic: update_topic)
       end
     end
-
-    def approval_message
-      "The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) has been approved."
-    end
-
-    def failure_message(e = nil)
-      "The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) failed during processing. Reason: #{e}"
-    end
-
-    def reject_message
-      "The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) has been rejected."
-    end
-
-    def conflict_message
-      "The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) has conflicting wiki pages. [[#{consequent_name}]] should be updated to include information from [[#{antecedent_name}]] if necessary."
-    end
-
-    def date_timestamp
-      Time.now.strftime("%Y-%m-%d")
-    end
   end
 
   module ForumMethods
@@ -106,7 +86,7 @@ class TagAlias < TagRelationship
         clear_all_cache
         ensure_category_consistency
         update_posts
-        forum_updater.update(approval_message, "APPROVED") if update_topic
+        forum_updater.update(approval_message(approver), "APPROVED") if update_topic
         rename_wiki_and_artist
         update({ :status => "active", :post_count => consequent_tag.post_count }, :as => CurrentUser.role)
       end
@@ -243,7 +223,7 @@ class TagAlias < TagRelationship
   def reject!
     update({ :status => "deleted" }, :as => CurrentUser.role)
     clear_all_cache
-    forum_updater.update(reject_message, "REJECTED")
+    forum_updater.update(reject_message(CurrentUser.user), "REJECTED")
     destroy
   end
 

app/models/tag_implication.rb

@@ -148,7 +148,7 @@ class TagImplication < TagRelationship
           update_posts
           update({ :status => "active" }, :as => CurrentUser.role)
           update_descendant_names_for_parents
-          forum_updater.update("The tag implication #{antecedent_name} -> #{consequent_name} has been approved.", "APPROVED") if update_topic
+          forum_updater.update(approval_message(approver), "APPROVED") if update_topic
         end
       rescue Exception => e
         if tries < 5
@@ -157,7 +157,7 @@ class TagImplication < TagRelationship
           retry
         end
 
-        forum_updater.update("The tag implication #{antecedent_name} -> #{consequent_name} failed during processing. Reason: #{e}", "FAILED") if update_topic
+        forum_updater.update(failure_message(e), "FAILED") if update_topic
         update({ :status => "error: #{e}" }, :as => CurrentUser.role)
 
         if Rails.env.production?
@@ -186,7 +186,7 @@ class TagImplication < TagRelationship
 
     def reject!
       update({ :status => "deleted", }, :as => CurrentUser.role)
-      forum_updater.update("The tag implication #{antecedent_name} -> #{consequent_name} has been rejected.", "REJECTED")
+      forum_updater.update(reject_message(CurrentUser.user), "REJECTED")
       destroy
     end
 
@@ -210,10 +210,6 @@ class TagImplication < TagRelationship
       end
     end
 
-    def date_timestamp
-      Time.now.strftime("%Y-%m-%d")
-    end
-
     def forum_updater
       @forum_updater ||= begin
         post = if forum_topic

app/models/tag_relationship.rb

@@ -117,5 +117,37 @@ class TagRelationship < ApplicationRecord
     end
   end
 
+  module MessageMethods
+    def relationship
+      # "TagAlias" -> "tag alias", "TagImplication" -> "tag implication"
+      self.class.name.underscore.tr("_", " ")
+    end
+
+    def approval_message(approver)
+      "The #{relationship} [[#{antecedent_name}]] -> [[#{consequent_name}]] #{forum_link} has been approved by @#{approver.name}."
+    end
+
+    def failure_message(e = nil)
+      "The #{relationship} [[#{antecedent_name}]] -> [[#{consequent_name}]] #{forum_link} failed during processing. Reason: #{e}"
+    end
+
+    def reject_message(rejector)
+      "The #{relationship} [[#{antecedent_name}]] -> [[#{consequent_name}]] #{forum_link} has been rejected by @#{rejector.name}."
+    end
+
+    def conflict_message
+      "The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] #{forum_link} has conflicting wiki pages. [[#{consequent_name}]] should be updated to include information from [[#{antecedent_name}]] if necessary."
+    end
+
+    def date_timestamp
+      Time.now.strftime("%Y-%m-%d")
+    end
+
+    def forum_link
+      "(forum ##{forum_post.id})" if forum_post.present?
+    end
+  end
+
   extend SearchMethods
+  include MessageMethods
 end

test/unit/bulk_update_request_test.rb

@@ -127,13 +127,19 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
         @req.approver_id = @admin.id
 
         assert_difference("ForumPost.count") do
-          @req.reject!
+          @req.reject!(@admin)
         end
 
         @topic.reload
         @post.reload
         assert_match(/\[REJECTED\]/, @topic.title)
       end
+
+      should "not send @mention dmails to the approver" do
+        assert_no_difference("Dmail.count") do
+          @req.approve!(@admin)
+        end
+      end
     end
 
     context "when searching" do

test/unit/tag_alias_request_test.rb

@@ -46,5 +46,11 @@ class TagAliasRequestTest < ActiveSupport::TestCase
         tar.create
       end
     end
+
+    should "save the forum post id" do
+      tar = TagAliasRequest.new(:antecedent_name => "aaa", :consequent_name => "bbb", :reason => "reason", :skip_secondary_validations => true)
+      tar.create
+      assert_equal(tar.forum_topic.posts.first.id, tar.tag_alias.forum_post.id)
+    end
   end
 end

test/unit/tag_alias_test.rb

@@ -66,11 +66,11 @@ class TagAliasTest < ActiveSupport::TestCase
       tag1 = FactoryGirl.create(:tag, :name => "aaa")
       tag2 = FactoryGirl.create(:tag, :name => "bbb")
       ta = FactoryGirl.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb")
-      assert_nil(Cache.get("ta:aaa"))
+      assert_nil(Cache.get("ta:#{Cache.hash("aaa")}"))
       TagAlias.to_aliased(["aaa"])
-      assert_equal("bbb", Cache.get("ta:aaa"))
+      assert_equal("bbb", Cache.get("ta:#{Cache.hash("aaa")}"))
       ta.destroy
-      assert_nil(Cache.get("ta:aaa"))
+      assert_nil(Cache.get("ta:#{Cache.hash("aaa")}"))
     end
 
     should "move saved searches" do