From 6275e851483742b2e9d4921f9134b60fe752845f Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Wed, 2 Dec 2020 14:30:18 -0600
Subject: [PATCH] BURs: refactor implication wiki page validations.

Move the validation that the tags in an implication must have wiki pages
back into the TagImplication model. Use validation contexts to only run
the validation when the BUR is created, not when the BUR is approved.
---
 app/logical/bulk_update_request_processor.rb | 23 ++++++++------------
 app/models/bulk_update_request.rb            |  3 ++-
 app/models/tag_implication.rb                | 11 ++++++++++
 test/unit/bulk_update_request_test.rb        |  2 +-
 4 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb
index 9601e95ef..af8536637 100644
--- a/app/logical/bulk_update_request_processor.rb
+++ b/app/logical/bulk_update_request_processor.rb
@@ -40,30 +40,27 @@ class BulkUpdateRequestProcessor
     end
   end
 
+  # validation_context will be either :request (when the BUR is first created
+  # or edited) or :approval (when the BUR is approved). Certain validations
+  # only run when the BUR is requested, not when it's approved.
   def validate_script
     BulkUpdateRequest.transaction(requires_new: true) do
       commands.each do |command, *args|
         case command
         when :create_alias
-          tag_alias = TagAlias.create(creator: User.system, antecedent_name: args[0], consequent_name: args[1])
-          if tag_alias.invalid?
+          tag_alias = TagAlias.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1])
+          tag_alias.save(context: validation_context)
+          if tag_alias.errors.present?
             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.create(creator: User.system, antecedent_name: args[0], consequent_name: args[1], status: "active")
-          if tag_implication.invalid?
+          tag_implication = TagImplication.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1], status: "active")
+          tag_implication.save(context: validation_context)
+          if tag_implication.errors.present?
             errors[:base] << "Can't create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name} (#{tag_implication.errors.full_messages.join("; ")})"
           end
 
-          if !tag_implication.antecedent_tag.empty? && tag_implication.antecedent_wiki.blank?
-            errors[:base] << "'#{tag_implication.antecedent_tag.name}' must have a wiki page"
-          end
-
-          if !tag_implication.consequent_tag.empty? && tag_implication.consequent_wiki.blank?
-            errors[:base] << "'#{tag_implication.consequent_tag.name}' must have a wiki page"
-          end
-
         when :remove_alias
           tag_alias = TagAlias.active.find_by(antecedent_name: args[0], consequent_name: args[1])
           if tag_alias.nil?
@@ -110,8 +107,6 @@ class BulkUpdateRequestProcessor
 
   def process!(approver)
     ActiveRecord::Base.transaction do
-      validate!
-
       commands.map do |command, *args|
         case command
         when :create_alias
diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb
index 9e0784851..3b2e9a0df 100644
--- a/app/models/bulk_update_request.rb
+++ b/app/models/bulk_update_request.rb
@@ -73,6 +73,7 @@ class BulkUpdateRequest < ApplicationRecord
     def approve!(approver)
       transaction do
         CurrentUser.scoped(approver) do
+          processor.validate!(:approval)
           processor.process!(approver)
           update!(status: "approved", approver: approver)
           forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has been approved by @#{approver.name}.")
@@ -107,7 +108,7 @@ class BulkUpdateRequest < ApplicationRecord
   end
 
   def validate_script
-    if processor.invalid?
+    if processor.invalid?(:request)
       errors[:base] << processor.errors.full_messages.join("; ")
     end
   end
diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb
index 6c5da1d4f..aa583c87d 100644
--- a/app/models/tag_implication.rb
+++ b/app/models/tag_implication.rb
@@ -7,6 +7,7 @@ class TagImplication < TagRelationship
   validate :absence_of_transitive_relation
   validate :antecedent_is_not_aliased
   validate :consequent_is_not_aliased
+  validate :has_wiki_page, on: :request
 
   concerning :HierarchyMethods do
     class_methods do
@@ -91,6 +92,16 @@ class TagImplication < TagRelationship
         errors[:base] << "Consequent tag must not be aliased to another tag"
       end
     end
+
+    def has_wiki_page
+      if !antecedent_tag.empty? && antecedent_wiki.blank?
+        errors[:base] << "'#{antecedent_name}' must have a wiki page"
+      end
+
+      if !consequent_tag.empty? && consequent_wiki.blank?
+        errors[:base] << "'#{consequent_name}' must have a wiki page"
+      end
+    end
   end
 
   concerning :ApprovalMethods do
diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb
index 77031626f..4bcba51a1 100644
--- a/test/unit/bulk_update_request_test.rb
+++ b/test/unit/bulk_update_request_test.rb
@@ -335,7 +335,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @bur = build(:bulk_update_request, script: "imply a -> b")
 
           assert_equal(false, @bur.valid?)
-          assert_equal(["'a' must have a wiki page; 'b' must have a wiki page"], @bur.errors.full_messages)
+          assert_equal(["Can't create implication a -> b ('a' must have a wiki page; 'b' must have a wiki page)"], @bur.errors.full_messages)
         end
       end
     end