From 19b8d41d09ec3aa0049838aa2992dc7c2dd12a14 Mon Sep 17 00:00:00 2001 From: r888888888 Date: Fri, 31 Mar 2017 17:42:14 -0700 Subject: [PATCH] refactor forum notifications for tag changes --- app/controllers/tag_aliases_controller.rb | 2 +- .../tag_implications_controller.rb | 2 +- app/logical/alias_and_implication_importer.rb | 4 +- app/logical/forum_updater.rb | 38 +++ app/logical/tag_alias_request.rb | 14 +- app/logical/tag_implication_request.rb | 14 +- app/models/artist.rb | 2 +- app/models/bulk_update_request.rb | 176 +++++------ app/models/tag_alias.rb | 96 ++++-- app/models/tag_implication.rb | 292 +++++++++--------- ...30231_add_forum_post_id_to_tag_requests.rb | 9 + db/structure.sql | 18 +- test/factories/tag_alias.rb | 2 +- test/factories/tag_implication.rb | 2 +- test/unit/bulk_update_request_test.rb | 26 +- test/unit/tag_alias_test.rb | 27 +- test/unit/tag_implication_test.rb | 15 +- 17 files changed, 422 insertions(+), 317 deletions(-) create mode 100644 app/logical/forum_updater.rb create mode 100644 db/migrate/20170330230231_add_forum_post_id_to_tag_requests.rb diff --git a/app/controllers/tag_aliases_controller.rb b/app/controllers/tag_aliases_controller.rb index 01bcbea99..8b9739350 100644 --- a/app/controllers/tag_aliases_controller.rb +++ b/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) + @tag_alias.approve!(approver: CurrentUser.user) respond_with(@tag_alias, :location => tag_alias_path(@tag_alias)) end diff --git a/app/controllers/tag_implications_controller.rb b/app/controllers/tag_implications_controller.rb index 9fc755e38..12f268e8a 100644 --- a/app/controllers/tag_implications_controller.rb +++ b/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) + @tag_implication.approve!(approver: CurrentUser.user) respond_with(@tag_implication, :location => tag_implication_path(@tag_implication)) end diff --git a/app/logical/alias_and_implication_importer.rb b/app/logical/alias_and_implication_importer.rb index 602112044..e081d1c2f 100644 --- a/app/logical/alias_and_implication_importer.rb +++ b/app/logical/alias_and_implication_importer.rb @@ -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.approve!(approver, update_topic: false) + tag_alias.approve!(approver: 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.approve!(approver, update_topic: false) + tag_implication.approve!(approver: approver, update_topic: false) when :remove_alias tag_alias = TagAlias.where("antecedent_name = ?", token[1]).first diff --git a/app/logical/forum_updater.rb b/app/logical/forum_updater.rb new file mode 100644 index 000000000..0951eec38 --- /dev/null +++ b/app/logical/forum_updater.rb @@ -0,0 +1,38 @@ +class ForumUpdater + attr_reader :forum_topic, :forum_post, :expected_title + + def initialize(forum_topic, options = {}) + @forum_topic = forum_topic + @forum_post = options[:forum_post] + @expected_title = options[:expected_title] + end + + def update(message, title_tag = nil) + return if forum_topic.nil? + + CurrentUser.scoped(User.system) do + create_response(message) + update_title(title_tag) if title_tag + + if forum_post + update_post(message) + end + end + end + + def create_response(body) + forum_topic.posts.create( + :body => body + ) + end + + def update_title(title_tag) + if forum_topic.title == expected_title + forum_topic.update(:title => "[#{title_tag}] #{forum_topic.title}") + end + end + + def update_post(body) + forum_post.update(:body => "#{forum_post.body}\n\n#{body}") + end +end diff --git a/app/logical/tag_alias_request.rb b/app/logical/tag_alias_request.rb index 34476800c..6a238b634 100644 --- a/app/logical/tag_alias_request.rb +++ b/app/logical/tag_alias_request.rb @@ -6,6 +6,14 @@ class TagAliasRequest validate :validate_tag_alias validate :validate_forum_topic + def self.topic_title(antecedent_name, consequent_name) + "Tag alias: #{antecedent_name} -> #{consequent_name}" + end + + def self.command_string(antecedent_name, consequent_name) + "create alias [[#{antecedent_name}]] -> [[#{consequent_name}]]" + end + def initialize(attributes) @antecedent_name = attributes[:antecedent_name].strip.tr(" ", "_") @consequent_name = attributes[:consequent_name].strip.tr(" ", "_") @@ -23,7 +31,7 @@ class TagAliasRequest @forum_topic = build_forum_topic(@tag_alias.id) @forum_topic.save - @tag_alias.update_attribute(:forum_topic_id, @forum_topic.id) + @tag_alias.update_attributes(forum_topic_id: @forum_topic.id, forum_post_id: @forum_topic.posts.first.id) end end @@ -39,9 +47,9 @@ class TagAliasRequest def build_forum_topic(tag_alias_id) ForumTopic.new( - :title => "Tag alias: #{antecedent_name} -> #{consequent_name}", + :title => TagAliasRequest.topic_title(antecedent_name, consequent_name), :original_post_attributes => { - :body => "create alias [[#{antecedent_name}]] -> [[#{consequent_name}]]\n\n\"Link to alias\":/tag_aliases?search[id]=#{tag_alias_id}\n\n#{reason}" + :body => TagAliasRequest.command_string(antecedent_name, consequent_name) + "\n\n\"Link to alias\":/tag_aliases?search[id]=#{tag_alias_id}\n\n#{reason}" }, :category_id => 1 ) diff --git a/app/logical/tag_implication_request.rb b/app/logical/tag_implication_request.rb index a84ca15d3..0c29c3944 100644 --- a/app/logical/tag_implication_request.rb +++ b/app/logical/tag_implication_request.rb @@ -6,6 +6,14 @@ class TagImplicationRequest validate :validate_tag_implication validate :validate_forum_topic + def self.topic_title(antecedent_name, consequent_name) + "Tag implication: #{antecedent_name} -> #{consequent_name}" + end + + def self.command_string(antecedent_name, consequent_name) + "create implication [[#{antecedent_name}]] -> [[#{consequent_name}]]" + end + def initialize(attributes) @antecedent_name = attributes[:antecedent_name].strip.tr(" ", "_") @consequent_name = attributes[:consequent_name].strip.tr(" ", "_") @@ -23,7 +31,7 @@ class TagImplicationRequest @forum_topic = build_forum_topic(@tag_implication.id) @forum_topic.save - @tag_implication.update_attribute(:forum_topic_id, @forum_topic.id) + @tag_implication.update_attributes(:forum_topic_id => @forum_topic.id, :forum_post_id => @forum_topic.posts.first.id) end end @@ -39,9 +47,9 @@ class TagImplicationRequest def build_forum_topic(tag_implication_id) ForumTopic.new( - :title => "Tag implication: #{antecedent_name} -> #{consequent_name}", + :title => TagImplicationRequest.topic_title(antecedent_name, consequent_name), :original_post_attributes => { - :body => "create implication [[#{antecedent_name}]] -> [[#{consequent_name}]]\n\n\"Link to implication\":/tag_implications?search[id]=#{tag_implication_id}\n\n#{reason}" + :body => TagImplicationRequest.command_string(antecedent_name, consequent_name) + "\n\n\"Link to implication\":/tag_implications?search[id]=#{tag_implication_id}\n\n#{reason}" }, :category_id => 1 ) diff --git a/app/models/artist.rb b/app/models/artist.rb index 9c734f62e..d68e13ee5 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -323,7 +323,7 @@ class Artist < ActiveRecord::Base # potential race condition but unlikely unless TagImplication.where(:antecedent_name => name, :consequent_name => "banned_artist").exists? tag_implication = TagImplication.create!(:antecedent_name => name, :consequent_name => "banned_artist", :skip_secondary_validations => true) - tag_implication.approve!(CurrentUser.user) + tag_implication.approve!(approver: CurrentUser.user) end update_column(:is_banned, true) diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index e38c5cd9b..9731b0fcd 100644 --- a/app/models/bulk_update_request.rb +++ b/app/models/bulk_update_request.rb @@ -3,6 +3,7 @@ class BulkUpdateRequest < ActiveRecord::Base belongs_to :user belongs_to :forum_topic + belongs_to :forum_post belongs_to :approver, :class_name => "User" validates_presence_of :user @@ -31,63 +32,90 @@ class BulkUpdateRequest < ActiveRecord::Base end end + module ApprovalMethods + def forum_updater + @forum_updater ||= begin + post = if forum_topic + forum_post || forum_topic.posts.first + else + nil + end + ForumUpdater.new( + forum_topic, + forum_post: post, + expected_title: title + ) + end + end + + def approve!(approver) + 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("[i]UPDATE #{date_timestamp}[/i]: The \"bulk update request ##{id}\":/bulk_update_requests?search%5Bid%5D=#{id} has been approved.", "APPROVED") + end + + rescue Exception => x + self.approver = approver + CurrentUser.scoped(approver) do + forum_updater.update("[i]UPDATE #{date_timestamp}[/i]: \"Bulk update request ##{id}\":/bulk_update_requests?search%5Bid%5D=#{id} failed: #{x.to_s}", "FAILED") + end + end + + def date_timestamp + Time.now.strftime("%Y-%m-%d") + end + + def create_forum_topic + if forum_topic_id + update_attributes(:forum_post_id => forum_post.id) + else + forum_topic = ForumTopic.create(:title => title, :category_id => 1, :original_post_attributes => {:body => reason_with_link}) + update_attributes(:forum_topic_id => forum_topic.id, :forum_post_id => forum_topic.posts.first.id) + end + end + + def reject! + forum_updater.update("[i]UPDATE #{date_timestamp}[/i]: The \"bulk update request ##{id}\":/bulk_update_requests?search%5Bid%5D=#{id} has been rejected.", "REJECTED") + update_attribute(:status, "rejected") + end + end + + module ValidationMethods + def script_formatted_correctly + AliasAndImplicationImporter.tokenize(script) + return true + rescue StandardError => e + errors.add(:base, e.message) + return false + end + + def forum_topic_id_not_invalid + if forum_topic_id && !forum_topic + errors.add(:base, "Forum topic ID is invalid") + end + end + + def validate_script + begin + AliasAndImplicationImporter.new(script, forum_topic_id, "1", skip_secondary_validations).validate! + rescue RuntimeError => e + self.errors[:base] = e.message + return false + end + + errors.empty? + end + end + extend SearchMethods - - 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 - self.approver = approver - message_approver_on_failure(x) - update_topic_on_failure(x) - end - - def message_approver_on_failure(x) - msg = <<-EOS - Bulk Update Request ##{id} failed\n - Exception: #{x.class}\n - Message: #{x.to_s}\n - Stack trace:\n - EOS - - x.backtrace.each do |line| - msg += "#{line}\n" - end - - dmail = Dmail.new( - :from_id => approver.id, - :to_id => approver.id, - :owner_id => approver.id, - :title => "Bulk update request approval failed", - :body => msg - ) - dmail.owner_id = approver.id - dmail.save - end - - def update_topic_on_failure(x) - if forum_topic_id - body = "\"Bulk update request ##{id}\":/bulk_update_requests?search%5Bid%5D=#{id} failed: #{x.to_s}" - ForumPost.create(:body => body, :topic_id => forum_topic_id) - end - end + include ApprovalMethods + include ValidationMethods def editable?(user) user_id == user.id || user.is_builder? end - def create_forum_topic - if forum_topic_id - ForumPost.create(:body => reason_with_link, :topic_id => forum_topic_id) - else - forum_topic = ForumTopic.create(:title => "[bulk] #{title}", :category_id => 1, :original_post_attributes => {:body => reason_with_link}) - update_attribute(:forum_topic_id, forum_topic.id) - end - end - def reason_with_link "#{script_with_links}\n\n\"Link to request\":/bulk_update_requests?search[id]=#{id}\n\n#{reason}" end @@ -109,46 +137,11 @@ class BulkUpdateRequest < ActiveRecord::Base lines.join("\n") end - def reject! - update_forum_topic_for_reject - update_attribute(:status, "rejected") - end - def initialize_attributes self.user_id = CurrentUser.user.id unless self.user_id self.status = "pending" end - def script_formatted_correctly - AliasAndImplicationImporter.tokenize(script) - return true - rescue StandardError => e - errors.add(:base, e.message) - return false - end - - def forum_topic_id_not_invalid - if forum_topic_id && !forum_topic - errors.add(:base, "Forum topic ID is invalid") - end - end - - def update_forum_topic_for_approve - if forum_topic - forum_topic.posts.create( - :body => "The \"bulk update request ##{id}\":/bulk_update_requests?search%5Bid%5D=#{id} has been approved." - ) - end - end - - def update_forum_topic_for_reject - if forum_topic - forum_topic.posts.create( - :body => "The \"bulk update request ##{id}\":/bulk_update_requests?search%5Bid%5D=#{id} has been rejected." - ) - end - end - def normalize_text self.script = script.downcase end @@ -160,15 +153,4 @@ class BulkUpdateRequest < ActiveRecord::Base @skip_secondary_validations = false end end - - def validate_script - begin - AliasAndImplicationImporter.new(script, forum_topic_id, "1", skip_secondary_validations).validate! - rescue RuntimeError => e - self.errors[:base] = e.message - return false - end - - errors.empty? - end end diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index 7900894db..82d6aa4a8 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -20,6 +20,7 @@ class TagAlias < ActiveRecord::Base belongs_to :creator, :class_name => "User" belongs_to :approver, :class_name => "User" belongs_to :forum_topic + belongs_to :forum_post attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :skip_secondary_validations attr_accessible :status, :approver_id, :as => [:admin] @@ -77,8 +78,56 @@ class TagAlias < ActiveRecord::Base end end + module ApprovalMethods + def approve!(update_topic: true, approver: CurrentUser.user) + CurrentUser.scoped(approver) do + update({ :status => "queued", :approver_id => approver.id }, :as => CurrentUser.role) + delay(:queue => "default").process!(update_topic: update_topic) + end + end + + def approval_message + "[i]UPDATE #{date_timestamp}[/i]: The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) has been approved." + end + + def failure_message(e = nil) + "[i]UPDATE #{date_timestamp}[/i]: The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) failed during processing. Reason: #{e}" + end + + def reject_message + "[i]UPDATE #{date_timestamp}[/i]: The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) has been rejected." + end + + def conflict_message + "[i]UPDATE #{date_timestamp}[/i]: 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 + def forum_updater + @forum_updater ||= begin + post = if forum_topic + forum_post || forum_topic.posts.where("body like ?", TagAliasRequest.command_string(antecedent_name, consequent_name) + "%").last + else + nil + end + ForumUpdater.new( + forum_topic, + forum_post: post, + expected_title: TagAliasRequest.topic_title(antecedent_name, consequent_name) + ) + end + end + end + extend SearchMethods include CacheMethods + include ApprovalMethods + include ForumMethods def self.to_aliased(names) Cache.get_multi(Array(names), "ta") do |tag| @@ -86,30 +135,25 @@ class TagAlias < ActiveRecord::Base end.values end - 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) + def process!(update_topic: true) unless valid? raise errors.full_messages.join("; ") end tries = 0 - forum_message = [] + messages = [] begin CurrentUser.scoped(approver, CurrentUser.ip_addr) do - update({ :status => "processing" }, :as => approver.role) + update({ :status => "processing" }, :as => CurrentUser.role) move_aliases_and_implications move_saved_searches clear_all_cache ensure_category_consistency 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 => approver.role) + forum_updater.update(approval_message, "APPROVED") if update_topic + rename_wiki_and_artist + update({ :status => "active", :post_count => consequent_tag.post_count }, :as => CurrentUser.role) end rescue Exception => e if tries < 5 @@ -118,18 +162,14 @@ class TagAlias < ActiveRecord::Base retry end - forum_message << "The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) failed during processing. Reason: #{e}" - update({ :status => "error: #{e}" }, :as => approver.role) + CurrentUser.scoped(approver, CurrentUser.ip_addr) do + forum_updater.update(failure_message(e), "FAILED") if update_topic + update({ :status => "error: #{e}" }, :as => CurrentUser.role) + end if Rails.env.production? NewRelic::Agent.notice_error(e, :custom_params => {:tag_alias_id => id, :antecedent_name => antecedent_name, :consequent_name => consequent_name}) end - ensure - if update_topic && forum_topic.present? - CurrentUser.scoped(approver, CurrentUser.ip_addr) do - forum_topic.posts.create(:body => forum_message.join("\n\n")) - end - end end end @@ -248,8 +288,6 @@ class TagAlias < ActiveRecord::Base end def rename_wiki_and_artist - message = "" - antecedent_wiki = WikiPage.titled(antecedent_name).first if antecedent_wiki.present? if WikiPage.titled(consequent_name).blank? @@ -257,7 +295,7 @@ class TagAlias < ActiveRecord::Base antecedent_wiki.update(title: consequent_name, skip_secondary_validations: true) end else - 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." + forum_updater.update(conflict_message) end end @@ -271,8 +309,6 @@ class TagAlias < ActiveRecord::Base end end end - - message end def deletable_by?(user) @@ -286,18 +322,10 @@ class TagAlias < ActiveRecord::Base deletable_by?(user) end - def update_forum_topic_for_reject - if forum_topic - forum_topic.posts.create( - :body => "The tag alias [[#{antecedent_name}]] -> [[#{consequent_name}]] (alias ##{id}) has been rejected." - ) - end - end - def reject! - update({ :status => "deleted", }, :as => CurrentUser.role) + update({ :status => "deleted" }, :as => CurrentUser.role) clear_all_cache - update_forum_topic_for_reject + forum_updater.update(reject_message, "REJECTED") destroy end diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index cae1ff91b..44dc9c634 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -8,6 +8,7 @@ class TagImplication < ActiveRecord::Base belongs_to :creator, :class_name => "User" belongs_to :approver, :class_name => "User" belongs_to :forum_topic + belongs_to :forum_post before_validation :initialize_creator, :on => :create before_validation :normalize_names validates_format_of :status, :with => /\A(active|deleted|pending|processing|queued|error: .*)\Z/ @@ -127,91 +128,161 @@ class TagImplication < ActiveRecord::Base end end + module ValidationMethods + def absence_of_circular_relation + # We don't want a -> b && b -> a chains + if self.class.active.exists?(["antecedent_name = ? and consequent_name = ?", consequent_name, antecedent_name]) + self.errors[:base] << "Tag implication can not create a circular relation with another tag implication" + false + end + end + + def antecedent_is_not_aliased + # We don't want to implicate a -> b if a is already aliased to c + if TagAlias.active.exists?(["antecedent_name = ?", antecedent_name]) + self.errors[:base] << "Antecedent tag must not be aliased to another tag" + false + end + end + + def consequent_is_not_aliased + # We don't want to implicate a -> b if b is already aliased to c + if TagAlias.active.exists?(["antecedent_name = ?", consequent_name]) + self.errors[:base] << "Consequent tag must not be aliased to another tag" + false + end + end + + def antecedent_and_consequent_are_different + normalize_names + if antecedent_name == consequent_name + self.errors[:base] << "Cannot implicate a tag to itself" + false + end + end + + def wiki_pages_present + return if skip_secondary_validations + + unless WikiPage.titled(consequent_name).exists? + self.errors[:base] = "The #{consequent_name} tag needs a corresponding wiki page" + return false + end + + unless WikiPage.titled(antecedent_name).exists? + self.errors[:base] = "The #{antecedent_name} tag needs a corresponding wiki page" + return false + end + end + end + + module ApprovalMethods + def process!(update_topic: true) + unless valid? + raise errors.full_messages.join("; ") + end + + tries = 0 + + begin + CurrentUser.scoped(approver, CurrentUser.ip_addr) do + update({ :status => "processing" }, :as => CurrentUser.role) + update_posts + update({ :status => "active" }, :as => CurrentUser.role) + update_descendant_names_for_parents + forum_updater.update("[i]UPDATE #{date_timestamp}[/i]: The tag implication #{antecedent_name} -> #{consequent_name} has been approved.", "APPROVED") if update_topic + end + rescue Exception => e + if tries < 5 + tries += 1 + sleep 2 ** tries + retry + end + + forum_updater.update("[i]UPDATE #{date_timestamp}[/i]: The tag implication #{antecedent_name} -> #{consequent_name} failed during processing. Reason: #{e}", "FAILED") if update_topic + update({ :status => "error: #{e}" }, :as => CurrentUser.role) + + if Rails.env.production? + NewRelic::Agent.notice_error(e, :custom_params => {:tag_implication_id => id, :antecedent_name => antecedent_name, :consequent_name => consequent_name}) + end + end + end + + def update_posts + Post.without_timeout do + Post.raw_tag_match(antecedent_name).where("true /* TagImplication#update_posts */").find_each do |post| + fixed_tags = "#{post.tag_string} #{descendant_names}".strip + CurrentUser.scoped(creator, creator_ip_addr) do + post.update_attributes( + :tag_string => fixed_tags + ) + end + end + end + end + + def approve!(approver: CurrentUser.user, update_topic: true) + update({ :status => "queued", :approver_id => approver.id }, :as => approver.role) + delay(:queue => "default").process!(update_topic: update_topic) + end + + def reject! + update({ :status => "deleted", }, :as => CurrentUser.role) + forum_updater.update("[i]UPDATE #{date_timestamp}[/i]: The tag implication #{antecedent_name} -> #{consequent_name} has been rejected.", "REJECTED") + destroy + end + + def create_mod_action + implication = %Q("tag implication ##{id}":[#{Rails.application.routes.url_helpers.tag_implication_path(self)}]: [[#{antecedent_name}]] -> [[#{consequent_name}]]) + + if id_changed? + ModAction.log("created #{status} #{implication}") + else + # format the changes hash more nicely. + change_desc = changes.except(:updated_at).map do |attribute, values| + old, new = values[0], values[1] + if old.nil? + %Q(set #{attribute} to "#{new}") + else + %Q(changed #{attribute} from "#{old}" to "#{new}") + end + end.join(", ") + + ModAction.log("updated #{implication}\n#{change_desc}") + end + end + + def date_timestamp + Time.now.strftime("%Y-%m-%d") + end + + def forum_updater + @forum_updater ||= begin + post = if forum_topic + forum_post || forum_topic.posts.where("body like ?", TagImplicationRequest.command_string(antecedent_name, consequent_name) + "%").last + else + nil + end + ForumUpdater.new( + forum_topic, + forum_post: post, + expected_title: TagImplicationRequest.topic_title(antecedent_name, consequent_name) + ) + end + end + end + include DescendantMethods include ParentMethods extend SearchMethods + include ValidationMethods + include ApprovalMethods def initialize_creator self.creator_id = CurrentUser.user.id self.creator_ip_addr = CurrentUser.ip_addr end - def process!(update_topic=true) - unless valid? - raise errors.full_messages.join("; ") - end - - tries = 0 - - begin - CurrentUser.scoped(approver, CurrentUser.ip_addr) do - update({ :status => "processing" }, :as => approver.role) - update_posts - update({ :status => "active" }, :as => approver.role) - update_descendant_names_for_parents - update_forum_topic_for_approve if update_topic - end - rescue Exception => e - if tries < 5 - tries += 1 - sleep 2 ** tries - retry - end - - update_forum_topic_for_error(e) - update({ :status => "error: #{e}" }, :as => CurrentUser.role) - - if Rails.env.production? - NewRelic::Agent.notice_error(e, :custom_params => {:tag_implication_id => id, :antecedent_name => antecedent_name, :consequent_name => consequent_name}) - end - end - end - - def absence_of_circular_relation - # We don't want a -> b && b -> a chains - if self.class.active.exists?(["antecedent_name = ? and consequent_name = ?", consequent_name, antecedent_name]) - self.errors[:base] << "Tag implication can not create a circular relation with another tag implication" - false - end - end - - def antecedent_is_not_aliased - # We don't want to implicate a -> b if a is already aliased to c - if TagAlias.active.exists?(["antecedent_name = ?", antecedent_name]) - self.errors[:base] << "Antecedent tag must not be aliased to another tag" - false - end - end - - def consequent_is_not_aliased - # We don't want to implicate a -> b if b is already aliased to c - if TagAlias.active.exists?(["antecedent_name = ?", consequent_name]) - self.errors[:base] << "Consequent tag must not be aliased to another tag" - false - end - end - - def antecedent_and_consequent_are_different - normalize_names - if antecedent_name == consequent_name - self.errors[:base] << "Cannot implicate a tag to itself" - false - end - end - - def update_posts - Post.without_timeout do - Post.raw_tag_match(antecedent_name).where("true /* TagImplication#update_posts */").find_each do |post| - fixed_tags = "#{post.tag_string} #{descendant_names}".strip - CurrentUser.scoped(creator, creator_ip_addr) do - post.update_attributes( - :tag_string => fixed_tags - ) - end - end - end - end - def normalize_names self.antecedent_name = antecedent_name.downcase.tr(" ", "_") self.consequent_name = consequent_name.downcase.tr(" ", "_") @@ -249,73 +320,4 @@ class TagImplication < ActiveRecord::Base def editable_by?(user) deletable_by?(user) end - - def update_forum_topic_for_approve - if forum_topic - forum_topic.posts.create( - :body => "The tag implication #{antecedent_name} -> #{consequent_name} has been approved." - ) - end - end - - def update_forum_topic_for_reject - if forum_topic - forum_topic.posts.create( - :body => "The tag implication #{antecedent_name} -> #{consequent_name} has been rejected." - ) - end - end - - def update_forum_topic_for_error(e) - if forum_topic - forum_topic.posts.create( - :body => "The tag implication #{antecedent_name} -> #{consequent_name} failed during processing. Reason: #{e}" - ) - end - end - - def wiki_pages_present - return if skip_secondary_validations - - unless WikiPage.titled(consequent_name).exists? - self.errors[:base] = "The #{consequent_name} tag needs a corresponding wiki page" - return false - end - - unless WikiPage.titled(antecedent_name).exists? - self.errors[:base] = "The #{antecedent_name} tag needs a corresponding wiki page" - return false - end - end - - 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! - update({ :status => "deleted", }, :as => CurrentUser.role) - update_forum_topic_for_reject - destroy - end - - def create_mod_action - implication = %Q("tag implication ##{id}":[#{Rails.application.routes.url_helpers.tag_implication_path(self)}]: [[#{antecedent_name}]] -> [[#{consequent_name}]]) - - if id_changed? - ModAction.log("created #{status} #{implication}") - else - # format the changes hash more nicely. - change_desc = changes.except(:updated_at).map do |attribute, values| - old, new = values[0], values[1] - if old.nil? - %Q(set #{attribute} to "#{new}") - else - %Q(changed #{attribute} from "#{old}" to "#{new}") - end - end.join(", ") - - ModAction.log("updated #{implication}\n#{change_desc}") - end - end end diff --git a/db/migrate/20170330230231_add_forum_post_id_to_tag_requests.rb b/db/migrate/20170330230231_add_forum_post_id_to_tag_requests.rb new file mode 100644 index 000000000..7b1e3d343 --- /dev/null +++ b/db/migrate/20170330230231_add_forum_post_id_to_tag_requests.rb @@ -0,0 +1,9 @@ +class AddForumPostIdToTagRequests < ActiveRecord::Migration + def change + ActiveRecord::Base.without_timeout do + add_column :tag_aliases, :forum_post_id, :integer + add_column :tag_implications, :forum_post_id, :integer + add_column :bulk_update_requests, :forum_post_id, :integer + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 6789bdb88..f0e67fe41 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -891,7 +891,8 @@ CREATE TABLE bulk_update_requests ( status character varying DEFAULT 'pending'::character varying NOT NULL, created_at timestamp without time zone, updated_at timestamp without time zone, - approver_id integer + approver_id integer, + forum_post_id integer ); @@ -2952,7 +2953,8 @@ CREATE TABLE tag_aliases ( created_at timestamp without time zone, updated_at timestamp without time zone, post_count integer DEFAULT 0 NOT NULL, - approver_id integer + approver_id integer, + forum_post_id integer ); @@ -2990,7 +2992,8 @@ CREATE TABLE tag_implications ( status text DEFAULT 'pending'::text NOT NULL, created_at timestamp without time zone, updated_at timestamp without time zone, - approver_id integer + approver_id integer, + forum_post_id integer ); @@ -7217,13 +7220,6 @@ CREATE UNIQUE INDEX index_wiki_pages_on_title ON wiki_pages USING btree (title); CREATE INDEX index_wiki_pages_on_title_pattern ON wiki_pages USING btree (title text_pattern_ops); --- --- Name: index_wiki_pages_on_updated_at; Type: INDEX; Schema: public; Owner: - --- - -CREATE INDEX index_wiki_pages_on_updated_at ON wiki_pages USING btree (updated_at); - - -- -- Name: unique_schema_migrations; Type: INDEX; Schema: public; Owner: - -- @@ -7569,5 +7565,5 @@ INSERT INTO schema_migrations (version) VALUES ('20170316224630'); INSERT INTO schema_migrations (version) VALUES ('20170319000519'); -INSERT INTO schema_migrations (version) VALUES ('20170329185605'); +INSERT INTO schema_migrations (version) VALUES ('20170330230231'); diff --git a/test/factories/tag_alias.rb b/test/factories/tag_alias.rb index ee9e7715a..89f1ada42 100644 --- a/test/factories/tag_alias.rb +++ b/test/factories/tag_alias.rb @@ -8,7 +8,7 @@ FactoryGirl.define do after(:create) do |tag_alias| unless tag_alias.status == "pending" approver = FactoryGirl.create(:admin_user) unless approver.present? - tag_alias.approve!(approver) + tag_alias.approve!(approver: approver) end end end diff --git a/test/factories/tag_implication.rb b/test/factories/tag_implication.rb index 6bb91806e..9a4ef48a0 100644 --- a/test/factories/tag_implication.rb +++ b/test/factories/tag_implication.rb @@ -8,7 +8,7 @@ FactoryGirl.define do after(:create) do |tag_implication| unless tag_implication.status == "pending" approver = FactoryGirl.create(:admin_user) unless approver.present? - tag_implication.approve!(approver) + tag_implication.approve!(approver: approver) end end end diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index 2cd9609e1..b0f7a955a 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -64,17 +64,21 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase context "with an associated forum topic" do setup do - @topic = FactoryGirl.create(:forum_topic) - @req = FactoryGirl.create(:bulk_update_request, :script => "create alias AAA -> BBB", :forum_topic => @topic) + @topic = FactoryGirl.create(:forum_topic, :title => "[bulk] hoge") + @post = FactoryGirl.create(:forum_post, :topic_id => @topic.id) + @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_forum_topic_for_approve).raises(RuntimeError.new("blah")) - assert_difference("Dmail.count", 1) do + @req.stubs(:update).raises(RuntimeError.new("blah")) + assert_difference("ForumPost.count", 1) do @req.approve!(@admin) end - assert_match(/Exception: RuntimeError/, Dmail.last.body) - assert_match(/Message: blah/, Dmail.last.body) + + @topic.reload + @post.reload + assert_match(/\[FAILED\]/, @topic.title) + assert_match(/UPDATE/, @post.body) end should "downcase the text" do @@ -85,6 +89,11 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_difference("ForumPost.count") do @req.approve!(@admin) end + + @topic.reload + @post.reload + assert_match(/\[APPROVED\]/, @topic.title) + assert_match(/UPDATE/, @post.body) end should "update the topic when rejected" do @@ -93,6 +102,11 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_difference("ForumPost.count") do @req.reject! end + + @topic.reload + @post.reload + assert_match(/\[REJECTED\]/, @topic.title) + assert_match(/UPDATE/, @post.body) end end end diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index 91fa9dd94..6592b4709 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -74,7 +74,7 @@ class TagAliasTest < ActiveSupport::TestCase assert_nil(Cache.get("ta:aaa")) end - should "zzz move saved searches" do + should "move saved searches" do tag1 = FactoryGirl.create(:tag, :name => "...") tag2 = FactoryGirl.create(:tag, :name => "bbb") ss = FactoryGirl.create(:saved_search, :query => "123 ... 456", :user => CurrentUser.user) @@ -139,7 +139,8 @@ class TagAliasTest < ActiveSupport::TestCase context "with an associated forum topic" do setup do @admin = FactoryGirl.create(:admin_user) - @topic = FactoryGirl.create(:forum_topic) + @topic = FactoryGirl.create(:forum_topic, :title => TagAliasRequest.topic_title("aaa", "bbb")) + @post = FactoryGirl.create(:forum_post, :topic_id => @topic.id, :body => TagAliasRequest.command_string("aaa", "bbb")) @alias = FactoryGirl.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb", :forum_topic => @topic, :status => "pending") end @@ -147,26 +148,34 @@ class TagAliasTest < ActiveSupport::TestCase setup do @wiki1 = FactoryGirl.create(:wiki_page, :title => "aaa") @wiki2 = FactoryGirl.create(:wiki_page, :title => "bbb") - @alias.approve!(@admin) + @alias.approve!(approver: @admin) @admin.reload # reload to get the forum post the approval created. + @topic.reload end should "update the forum topic when approved" do - assert(@topic.posts.last, @admin.forum_posts.last) - assert_match(/The tag alias .* been approved/, @admin.forum_posts.last.body) + assert_equal("[APPROVED] Tag alias: aaa -> bbb", @topic.title) + assert_match(/The tag alias .* been approved/m, @admin.forum_posts[-2].body) end should "warn about conflicting wiki pages when approved" do - assert_match(/has conflicting wiki pages/, @admin.forum_posts.last.body) + assert_match(/has conflicting wiki pages/m, @admin.forum_posts[-1].body) end end should "update the topic when processed" do assert_difference("ForumPost.count") do - @alias.approve!(@admin) + @alias.approve!(approver: @admin) end end + should "update the parent post" do + previous = @post.body + @alias.approve!(approver: @admin) + @post.reload + assert_not_equal(previous, @post.body) + end + should "update the topic when rejected" do assert_difference("ForumPost.count") do @alias.reject! @@ -176,8 +185,10 @@ class TagAliasTest < ActiveSupport::TestCase should "update the topic when failed" do @alias.stubs(:sleep).returns(true) @alias.stubs(:update_posts).raises(Exception, "oh no") - @alias.approve!(@admin) + @alias.approve!(approver: @admin) + @topic.reload + assert_equal("[FAILED] Tag alias: aaa -> bbb", @topic.title) assert_match(/error: oh no/, @alias.status) assert_match(/The tag alias .* failed during processing/, @admin.forum_posts.last.body) end diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index e3e49c602..d1c32b366 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -186,20 +186,29 @@ class TagImplicationTest < ActiveSupport::TestCase context "with an associated forum topic" do setup do @admin = FactoryGirl.create(:admin_user) - @topic = FactoryGirl.create(:forum_topic) - @implication = FactoryGirl.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb", :forum_topic => @topic) + @topic = FactoryGirl.create(:forum_topic, :title => TagImplicationRequest.topic_title("aaa", "bbb")) + @post = FactoryGirl.create(:forum_post, topic_id: @topic.id, :body => TagImplicationRequest.command_string("aaa", "bbb")) + @implication = FactoryGirl.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb", :forum_topic => @topic, :status => "pending") end should "update the topic when processed" do assert_difference("ForumPost.count") do - @implication.process! + @implication.approve! end + @post.reload + @topic.reload + assert_match(/The tag implication .* has been approved/, @post.body) + assert_equal("[APPROVED] Tag implication: aaa -> bbb", @topic.title) end should "update the topic when rejected" do assert_difference("ForumPost.count") do @implication.reject! end + @post.reload + @topic.reload + assert_match(/The tag implication .* has been rejected/, @post.body) + assert_equal("[REJECTED] Tag implication: aaa -> bbb", @topic.title) end end end