BURs: remove ability to skip secondary validations.

Remove the ability to skip secondary validations when creating a BUR.
The only skippable validation that still existed was the requirement
that both tags in an implication must have wiki pages. It's now
mandatory to write wiki pages for tags before you can request an
implication. This doesn't apply to empty tags.

app/logical/bulk_update_request_processor.rb

@@ -4,7 +4,7 @@ class BulkUpdateRequestProcessor
   class Error < StandardError; end
 
   attr_reader :bulk_update_request
-  delegate :script, :forum_topic_id, :skip_secondary_validations, to: :bulk_update_request
+  delegate :script, :forum_topic_id, to: :bulk_update_request
   validate :validate_script
 
   def initialize(bulk_update_request)
@@ -42,17 +42,25 @@ class BulkUpdateRequestProcessor
     commands.each do |command, *args|
       case command
       when :create_alias
-        tag_alias = TagAlias.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1], skip_secondary_validations: skip_secondary_validations)
+        tag_alias = TagAlias.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1])
         if tag_alias.invalid?
           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.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1], skip_secondary_validations: skip_secondary_validations, status: "active")
+        tag_implication = TagImplication.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1], status: "active")
         if tag_implication.invalid?
           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?
@@ -97,11 +105,11 @@ class BulkUpdateRequestProcessor
       commands.map do |command, *args|
         case command
         when :create_alias
-          tag_alias = TagAlias.create!(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: args[0], consequent_name: args[1], skip_secondary_validations: skip_secondary_validations)
+          tag_alias = TagAlias.create!(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: args[0], consequent_name: args[1])
           tag_alias.approve!(approver)
 
         when :create_implication
-          tag_implication = TagImplication.create!(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: args[0], consequent_name: args[1], skip_secondary_validations: skip_secondary_validations)
+          tag_implication = TagImplication.create!(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: args[0], consequent_name: args[1])
           tag_implication.approve!(approver)
 
         when :remove_alias

app/models/artist.rb

@@ -192,7 +192,7 @@ class Artist < ApplicationRecord
 
         # 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, creator: banner)
+          tag_implication = TagImplication.create!(antecedent_name: name, consequent_name: "banned_artist", creator: banner)
           tag_implication.approve!(banner)
         end
 

app/models/bulk_update_request.rb

@@ -1,7 +1,6 @@
 class BulkUpdateRequest < ApplicationRecord
   attr_accessor :title
   attr_accessor :reason
-  attr_reader :skip_secondary_validations
 
   belongs_to :user
   belongs_to :forum_topic, optional: true
@@ -75,7 +74,7 @@ class BulkUpdateRequest < ApplicationRecord
       transaction do
         CurrentUser.scoped(approver) do
           processor.process!(approver)
-          update!(status: "approved", approver: approver, skip_secondary_validations: true)
+          update!(status: "approved", approver: approver)
           forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has been approved by @#{approver.name}.")
         end
       end
@@ -120,10 +119,6 @@ class BulkUpdateRequest < ApplicationRecord
     self.tags = processor.affected_tags
   end
 
-  def skip_secondary_validations=(v)
-    @skip_secondary_validations = v.to_s.truthy?
-  end
-
   def processor
     @processor ||= BulkUpdateRequestProcessor.new(self)
   end

app/models/tag_implication.rb

@@ -8,7 +8,6 @@ class TagImplication < TagRelationship
   validate :absence_of_transitive_relation
   validate :antecedent_is_not_aliased
   validate :consequent_is_not_aliased
-  validate :wiki_pages_present, on: :create, unless: :skip_secondary_validations
 
   module DescendantMethods
     extend ActiveSupport::Concern
@@ -107,16 +106,6 @@ class TagImplication < TagRelationship
         errors[:base] << "Consequent tag must not be aliased to another tag"
       end
     end
-
-    def wiki_pages_present
-      if consequent_wiki.blank?
-        errors[:base] << "The #{consequent_name} tag needs a corresponding wiki page"
-      end
-
-      if antecedent_wiki.blank?
-        errors[:base] << "The #{antecedent_name} tag needs a corresponding wiki page"
-      end
-    end
   end
 
   module ApprovalMethods

app/models/tag_relationship.rb

@@ -4,8 +4,6 @@ class TagRelationship < ApplicationRecord
   EXPIRY = 60
   EXPIRY_WARNING = 55
 
-  attr_accessor :skip_secondary_validations
-
   belongs_to :creator, class_name: "User"
   belongs_to :approver, class_name: "User", optional: true
   belongs_to :forum_post, optional: true

app/policies/bulk_update_request_policy.rb

@@ -20,14 +20,14 @@ class BulkUpdateRequestPolicy < ApplicationPolicy
   end
 
   def permitted_attributes_for_create
-    [:script, :skip_secondary_validations, :title, :reason, :forum_topic_id]
+    [:script, :title, :reason, :forum_topic_id]
   end
 
   def permitted_attributes_for_update
     if can_update_forum?
-      [:script, :skip_secondary_validations, :forum_topic_id, :forum_post_id]
+      [:script, :forum_topic_id, :forum_post_id]
     else
-      [:script, :skip_secondary_validations]
+      [:script]
     end
   end
 end

app/views/bulk_update_requests/_form.html.erb

@@ -28,16 +28,6 @@
     </div>
   <% end %>
 
-  <% if @bulk_update_request.errors.any? %>
-    <div class="input">
-      <label class="checkbox optional" for="bulk_update_request_skip_secondary_validations">
-        <%= check_box "bulk_update_request", "skip_secondary_validations" %>
-        Skip validations
-      </label>
-      <p class="hint">You can ignore the wiki page and minimum count requirements</p>
-    </div>
-  <% end %>
-
   <% if @bulk_update_request.persisted? && policy(@bulk_update_request).can_update_forum? %>
     <%= f.input :forum_topic_id %>
     <%= f.input :forum_post_id %>

test/factories/bulk_update_request.rb

@@ -4,6 +4,5 @@ FactoryBot.define do
     title {"xxx"}
     script {"create alias aaa -> bbb"}
     reason { FFaker::Lorem.sentences.join(" ") }
-    skip_secondary_validations {true}
   end
 end

test/factories/tag_alias.rb

@@ -4,6 +4,5 @@ FactoryBot.define do
     antecedent_name {"#{FFaker::Name.first_name.downcase}#{rand(1000)}"}
     consequent_name {"#{FFaker::Name.first_name.downcase}#{rand(1000)}"}
     status {"active"}
-    skip_secondary_validations {true}
   end
 end

test/factories/tag_implication.rb

@@ -4,6 +4,5 @@ FactoryBot.define do
     antecedent_name {"#{FFaker::Name.first_name.downcase}#{rand(1000)}"}
     consequent_name {"#{FFaker::Name.first_name.downcase}#{rand(1000)}"}
     status {"active"}
-    skip_secondary_validations {true}
   end
 end

test/functional/bulk_update_requests_controller_test.rb

@@ -48,26 +48,14 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest
     end
 
     context "#update" do
-      should "still handle enabled secondary validations correctly" do
-        put_auth bulk_update_request_path(@bulk_update_request.id), @user, params: {bulk_update_request: {script: "create alias zzz -> 222", skip_secondary_validations: "0"}}
-        assert_response :redirect
-        assert_equal("create alias zzz -> 222", @bulk_update_request.reload.script)
-      end
-
-      should "still handle disabled secondary validations correctly" do
-        put_auth bulk_update_request_path(@bulk_update_request.id), @user, params: {bulk_update_request: {script: "create alias zzz -> 222", skip_secondary_validations: "1"}}
-        assert_response :redirect
-        assert_equal("create alias zzz -> 222", @bulk_update_request.reload.script)
-      end
-
       should "allow builders to update other people's requests" do
-        put_auth bulk_update_request_path(@bulk_update_request.id), create(:builder_user), params: {bulk_update_request: {script: "create alias zzz -> 222", skip_secondary_validations: "0"}}
+        put_auth bulk_update_request_path(@bulk_update_request.id), create(:builder_user), params: {bulk_update_request: {script: "create alias zzz -> 222" }}
         assert_response :redirect
         assert_equal("create alias zzz -> 222", @bulk_update_request.reload.script)
       end
 
       should "not allow members to update other people's requests" do
-        put_auth bulk_update_request_path(@bulk_update_request.id), create(:user), params: {bulk_update_request: {script: "create alias zzz -> 222", skip_secondary_validations: "0"}}
+        put_auth bulk_update_request_path(@bulk_update_request.id), create(:user), params: {bulk_update_request: {script: "create alias zzz -> 222" }}
         assert_response 403
         assert_equal("create alias aaa -> bbb", @bulk_update_request.reload.script)
       end

test/unit/bulk_update_request_test.rb

@@ -309,6 +309,24 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           assert_equal(true, @bur.valid?)
         end
       end
+
+      context "requesting an implication for an empty tag without a wiki" do
+        should "succeed" do
+          @bur = create(:bulk_update_request, script: "imply a -> b")
+          assert_equal(true, @bur.valid?)
+        end
+      end
+
+      context "requesting an implication for a populated tag without a wiki" do
+        should "fail" do
+          create(:tag, name: "a", post_count: 1)
+          create(:tag, name: "b", post_count: 1)
+          @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)
+        end
+      end
     end
 
     context "when the script is updated" do

test/unit/tag_implication_test.rb

@@ -61,16 +61,6 @@ class TagImplicationTest < ActiveSupport::TestCase
       end
     end
 
-    context "on secondary validation" do
-      should "warn if either tag is missing a wiki" do
-        ti = FactoryBot.build(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", skip_secondary_validations: false)
-
-        assert(ti.invalid?)
-        assert_includes(ti.errors[:base], "The aaa tag needs a corresponding wiki page")
-        assert_includes(ti.errors[:base], "The bbb tag needs a corresponding wiki page")
-      end
-    end
-
     should "populate the creator information" do
       ti = create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", creator: CurrentUser.user)
       assert_equal(CurrentUser.user.id, ti.creator_id)