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)