From 7f90bc42160b96a1f67cf07c5807c314ed046f24 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 12 Nov 2020 20:08:13 -0600 Subject: [PATCH] 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 | 18 +++++++++++++----- app/models/artist.rb | 2 +- app/models/bulk_update_request.rb | 7 +------ app/models/tag_implication.rb | 11 ----------- app/models/tag_relationship.rb | 2 -- app/policies/bulk_update_request_policy.rb | 6 +++--- app/views/bulk_update_requests/_form.html.erb | 10 ---------- test/factories/bulk_update_request.rb | 1 - test/factories/tag_alias.rb | 1 - test/factories/tag_implication.rb | 1 - .../bulk_update_requests_controller_test.rb | 16 ++-------------- test/unit/bulk_update_request_test.rb | 18 ++++++++++++++++++ test/unit/tag_implication_test.rb | 10 ---------- 13 files changed, 38 insertions(+), 65 deletions(-) diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb index 1e369bb84..f9604f1f0 100644 --- a/app/logical/bulk_update_request_processor.rb +++ b/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 diff --git a/app/models/artist.rb b/app/models/artist.rb index dfee85f2d..d059a8367 100644 --- a/app/models/artist.rb +++ b/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 diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index 996fde440..9e0784851 100644 --- a/app/models/bulk_update_request.rb +++ b/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 diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 69f09781c..c5be0ac91 100644 --- a/app/models/tag_implication.rb +++ b/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 diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index 921394458..7eb6c020c 100644 --- a/app/models/tag_relationship.rb +++ b/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 diff --git a/app/policies/bulk_update_request_policy.rb b/app/policies/bulk_update_request_policy.rb index 7f0700636..bc25fa886 100644 --- a/app/policies/bulk_update_request_policy.rb +++ b/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 diff --git a/app/views/bulk_update_requests/_form.html.erb b/app/views/bulk_update_requests/_form.html.erb index acf886b59..41cef73d0 100644 --- a/app/views/bulk_update_requests/_form.html.erb +++ b/app/views/bulk_update_requests/_form.html.erb @@ -28,16 +28,6 @@ <% end %> - <% if @bulk_update_request.errors.any? %> -
- -

You can ignore the wiki page and minimum count requirements

-
- <% end %> - <% if @bulk_update_request.persisted? && policy(@bulk_update_request).can_update_forum? %> <%= f.input :forum_topic_id %> <%= f.input :forum_post_id %> diff --git a/test/factories/bulk_update_request.rb b/test/factories/bulk_update_request.rb index 1e3dd0740..bf7396300 100644 --- a/test/factories/bulk_update_request.rb +++ b/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 diff --git a/test/factories/tag_alias.rb b/test/factories/tag_alias.rb index c280d7dab..88dc7e04b 100644 --- a/test/factories/tag_alias.rb +++ b/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 diff --git a/test/factories/tag_implication.rb b/test/factories/tag_implication.rb index 08677a9d2..6769b7a55 100644 --- a/test/factories/tag_implication.rb +++ b/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 diff --git a/test/functional/bulk_update_requests_controller_test.rb b/test/functional/bulk_update_requests_controller_test.rb index 5303719ca..f4c47f79d 100644 --- a/test/functional/bulk_update_requests_controller_test.rb +++ b/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 diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index 27ba2b842..03f5deefc 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/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 diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index 263529cb5..a71852814 100644 --- a/test/unit/tag_implication_test.rb +++ b/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)