From 7709e84502d9e857163e4af3e8b98450b58dec88 Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 3 Nov 2021 19:25:18 -0500 Subject: [PATCH] BURs: allow reapproving failed BURs containing alias or implication removals. Make it possible to reapprove failed BURs that removed aliases or implications. Before if a BUR failed midway through, and we tried to reapprove it, then it would fail when it got to a `remove alias` line because the alias had already been removed. Now we keep going if we try to remove an alias or implication that has already been removed. --- app/logical/bulk_update_request_processor.rb | 18 +++++++---- test/unit/bulk_update_request_test.rb | 32 +++++++++++++++++--- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb index 6cf5c9078..840ee835c 100644 --- a/app/logical/bulk_update_request_processor.rb +++ b/app/logical/bulk_update_request_processor.rb @@ -79,7 +79,10 @@ class BulkUpdateRequestProcessor when :remove_alias tag_alias = TagAlias.active.find_by(antecedent_name: args[0], consequent_name: args[1]) - if tag_alias.nil? + + if validation_context == :approval + # ignore non-existing aliases when approving a BUR + elsif tag_alias.nil? errors.add(:base, "Can't remove alias #{args[0]} -> #{args[1]} (alias doesn't exist)") else tag_alias.update(status: "deleted") @@ -87,7 +90,10 @@ class BulkUpdateRequestProcessor when :remove_implication tag_implication = TagImplication.active.find_by(antecedent_name: args[0], consequent_name: args[1]) - if tag_implication.nil? + + if validation_context == :approval + # ignore non-existing implication when approving a BUR + elsif tag_implication.nil? errors.add(:base, "Can't remove implication #{args[0]} -> #{args[1]} (implication doesn't exist)") else tag_implication.update(status: "deleted") @@ -147,12 +153,12 @@ class BulkUpdateRequestProcessor TagImplication.approve!(antecedent_name: args[0], consequent_name: args[1], approver: approver, forum_topic: forum_topic) when :remove_alias - tag_alias = TagAlias.active.find_by!(antecedent_name: args[0], consequent_name: args[1]) - tag_alias.reject!(User.system) + tag_alias = TagAlias.active.find_by(antecedent_name: args[0], consequent_name: args[1]) + tag_alias&.reject!(User.system) when :remove_implication - tag_implication = TagImplication.active.find_by!(antecedent_name: args[0], consequent_name: args[1]) - tag_implication.reject!(User.system) + tag_implication = TagImplication.active.find_by(antecedent_name: args[0], consequent_name: args[1]) + tag_implication&.reject!(User.system) when :mass_update BulkUpdateRequestProcessor.mass_update(args[0], args[1]) diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index 1f376d954..9fa911129 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -220,7 +220,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_equal("approved", @bur.reload.status) end - should "fail if the alias isn't active" do + should "fail to validate if the alias isn't active" do create(:tag_alias, antecedent_name: "foo", consequent_name: "bar", status: "deleted") @bur = build(:bulk_update_request, script: "remove alias foo -> bar") @@ -228,13 +228,25 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_equal(["Can't remove alias foo -> bar (alias doesn't exist)"], @bur.errors[:base]) end - should "fail if the alias doesn't already exist" do + should "fail to validate if the alias doesn't already exist" do @bur = build(:bulk_update_request, script: "remove alias foo -> bar") assert_equal(false, @bur.valid?) assert_equal(["Can't remove alias foo -> bar (alias doesn't exist)"], @bur.errors[:base]) end + should "allow reapproving a failed BUR when the alias has already been removed" do + @alias = create(:tag_alias, antecedent_name: "foo", consequent_name: "bar") + @bur = create(:bulk_update_request, script: "unalias foo -> bar", status: "failed") + @alias.reject! + + @bur.approve!(@admin) + perform_enqueued_jobs + + assert_equal(true, @alias.reload.is_deleted?) + assert_equal("approved", @bur.reload.status) + end + should "be processed sequentially after the create alias command" do @bur = create_bur!("create alias foo -> bar\nremove alias foo -> bar", @admin) @@ -256,7 +268,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_equal("approved", @bur.reload.status) end - should "fail if the implication isn't active" do + should "fail to validate if the implication isn't active" do create(:tag_implication, antecedent_name: "foo", consequent_name: "bar", status: "deleted") @bur = build(:bulk_update_request, script: "remove implication foo -> bar") @@ -264,13 +276,25 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_equal(["Can't remove implication foo -> bar (implication doesn't exist)"], @bur.errors[:base]) end - should "fail if the implication doesn't already exist" do + should "fail to validate if the implication doesn't already exist" do @bur = build(:bulk_update_request, script: "remove implication foo -> bar") assert_equal(false, @bur.valid?) assert_equal(["Can't remove implication foo -> bar (implication doesn't exist)"], @bur.errors[:base]) end + should "allow reapproving a failed BUR when the implication has already been removed" do + @implication = create(:tag_implication, antecedent_name: "foo", consequent_name: "bar") + @bur = create(:bulk_update_request, script: "unimply foo -> bar", status: "failed") + @implication.reject! + + @bur.approve!(@admin) + perform_enqueued_jobs + + assert_equal(true, @implication.reload.is_deleted?) + assert_equal("approved", @bur.reload.status) + end + should "be processed sequentially after the create implication command" do @bur = create_bur!("imply foo -> bar\nunimply foo -> bar", @admin)