From d136a12a6542d84498e8445e0f3cae20a892526a Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 11 May 2020 00:20:21 -0500 Subject: [PATCH] Fix #4359: Allow builders to move small (artist) tags manually. Allow builders to approve artist alias BURs. The BUR must contain only artist aliases or mass updates and each artist must have less than 100 posts. --- app/logical/bulk_update_request_processor.rb | 26 +++++++++++++++++ app/models/bulk_update_request.rb | 4 +++ app/policies/bulk_update_request_policy.rb | 2 +- .../_bur_edit_links.html.erb | 4 +-- .../bulk_update_requests_controller_test.rb | 28 +++++++++++++++++++ 5 files changed, 60 insertions(+), 4 deletions(-) diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb index 951495048..96212a72e 100644 --- a/app/logical/bulk_update_request_processor.rb +++ b/app/logical/bulk_update_request_processor.rb @@ -117,6 +117,22 @@ class BulkUpdateRequestProcessor [] end + def is_tag_move_allowed? + tokens.all? do |type, *args| + case type + when :create_alias + BulkUpdateRequestProcessor.is_tag_move_allowed?(args[0], args[1]) + when :mass_update + lhs = PostQueryBuilder.new(args[0]) + rhs = PostQueryBuilder.new(args[1]) + + lhs.is_simple_tag? && rhs.is_simple_tag? && BulkUpdateRequestProcessor.is_tag_move_allowed?(args[0], args[1]) + else + false + end + end + end + def to_dtext tokens.map do |token| case token[0] @@ -132,5 +148,15 @@ class BulkUpdateRequestProcessor end.join("\n") end + private + + def self.is_tag_move_allowed?(antecedent_name, consequent_name) + antecedent_tag = Tag.find_by_name(Tag.normalize_name(antecedent_name)) + consequent_tag = Tag.find_by_name(Tag.normalize_name(consequent_name)) + + (antecedent_tag.blank? || antecedent_tag.empty? || (antecedent_tag.artist? && antecedent_tag.post_count <= 100)) && + (consequent_tag.blank? || consequent_tag.empty? || (consequent_tag.artist? && consequent_tag.post_count <= 100)) + end + memoize :tokens end diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index 96b095a74..6909728a6 100644 --- a/app/models/bulk_update_request.rb +++ b/app/models/bulk_update_request.rb @@ -136,6 +136,10 @@ class BulkUpdateRequest < ApplicationRecord @processor ||= BulkUpdateRequestProcessor.new(script, forum_topic_id: forum_topic_id, skip_secondary_validations: skip_secondary_validations) end + def is_tag_move_allowed? + processor.is_tag_move_allowed? + end + def is_pending? status == "pending" end diff --git a/app/policies/bulk_update_request_policy.rb b/app/policies/bulk_update_request_policy.rb index 5c9201d93..7f0700636 100644 --- a/app/policies/bulk_update_request_policy.rb +++ b/app/policies/bulk_update_request_policy.rb @@ -8,7 +8,7 @@ class BulkUpdateRequestPolicy < ApplicationPolicy end def approve? - user.is_admin? && !record.is_approved? + unbanned? && !record.is_approved? && (user.is_admin? || (user.is_builder? && record.is_tag_move_allowed?)) end def destroy? diff --git a/app/views/bulk_update_requests/_bur_edit_links.html.erb b/app/views/bulk_update_requests/_bur_edit_links.html.erb index 644938b27..def972e05 100644 --- a/app/views/bulk_update_requests/_bur_edit_links.html.erb +++ b/app/views/bulk_update_requests/_bur_edit_links.html.erb @@ -1,8 +1,6 @@ <%# bur %> -<% if policy(bur).approve? %> - <%= link_to "Approve", approve_bulk_update_request_path(bur), remote: true, method: :post, "data-confirm": "Are you sure you want to approve this bulk update request?" %> | -<% end %> +<%= link_to_if policy(bur).approve?, "Approve", approve_bulk_update_request_path(bur), remote: true, method: :post, "data-confirm": "Are you sure you want to approve this bulk update request?" %> | <% if policy(bur).destroy? %> <%= link_to "Reject", bur, remote: true, method: :delete, "data-confirm": "Are you sure you want to reject this bulk update request?" %> | <% end %> diff --git a/test/functional/bulk_update_requests_controller_test.rb b/test/functional/bulk_update_requests_controller_test.rb index f6b551448..b91a8e04c 100644 --- a/test/functional/bulk_update_requests_controller_test.rb +++ b/test/functional/bulk_update_requests_controller_test.rb @@ -4,6 +4,7 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest context "BulkUpdateRequestsController" do setup do @user = create(:user) + @builder = create(:builder_user) @admin = create(:admin_user) @bulk_update_request = create(:bulk_update_request, user: @user) end @@ -121,6 +122,33 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest end end + context "for a builder" do + should "fail for a large artist move" do + create(:tag, name: "artist1", category: Tag.categories.artist, post_count: 1000) + @bulk_update_request = create(:bulk_update_request, script: "create alias artist1 -> artist2") + + post_auth approve_bulk_update_request_path(@bulk_update_request), @builder + + assert_response 403 + assert_equal("pending", @bulk_update_request.reload.status) + assert_equal(false, TagAlias.where(antecedent_name: "artist1", consequent_name: "artist2").exists?) + end + + should "succeed for a small artist move" do + create(:tag, name: "artist1a", category: Tag.categories.artist, post_count: 10) + create(:tag, name: "artist1b", category: Tag.categories.general, post_count: 0) + create(:tag, name: "artist2a", category: Tag.categories.artist, post_count: 20) + @bulk_update_request = create(:bulk_update_request, script: "mass update artist1a -> artist1b\ncreate alias artist2a -> artist2b") + + post_auth approve_bulk_update_request_path(@bulk_update_request), @builder + + assert_redirected_to(bulk_update_requests_path) + assert_equal("approved", @bulk_update_request.reload.status) + assert_equal(@builder, @bulk_update_request.approver) + assert_equal(true, TagAlias.where(antecedent_name: "artist2a", consequent_name: "artist2b").exists?) + end + end + context "for an admin" do should "succeed" do post_auth approve_bulk_update_request_path(@bulk_update_request), @admin