diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index 1afdb43ac..4070e61e0 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -1,5 +1,7 @@ class TagAlias < TagRelationship - validates_uniqueness_of :antecedent_name, scope: :status, conditions: -> { active } + # Validate that the alias doesn't exist yet when it's created or when a BUR + # is requested, but not when a BUR is approved (to allow failed BURs to be reapproved) + validates_uniqueness_of :antecedent_name, scope: :status, conditions: -> { active }, on: %i[create update request] validate :absence_of_transitive_relation before_create :delete_conflicting_relationships diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index 2c7fecfe8..58dfd17e4 100644 --- a/app/models/tag_relationship.rb +++ b/app/models/tag_relationship.rb @@ -115,8 +115,21 @@ class TagRelationship < ApplicationRecord end end + # Create a new alias or implication, set it to active, and update the posts. + # This method is idempotent; if the alias or implication already exists, keep + # it and update the posts again. + # + # @param antecedent_name [String] the left-hand side of the alias or implication + # @param consequent_name [String] the right-hand side of the alias or implication + # @param approver [User] the user approving the alias or implication + # @param forum_topic [ForumTopic] the forum topic where the alias or implication was requested. def self.approve!(antecedent_name:, consequent_name:, approver:, forum_topic: nil) - tag_relationship = create!(creator: approver, approver: approver, antecedent_name: antecedent_name, consequent_name: consequent_name, forum_topic: forum_topic) + tag_relationship = find_or_create_by!(antecedent_name: antecedent_name, consequent_name: consequent_name, status: "active") do |relationship| + relationship.creator = approver + relationship.approver = approver + relationship.forum_topic = forum_topic + end + tag_relationship.process! end diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index 0930e0876..c2f3ba57e 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -539,6 +539,42 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end end + context "when a bulk update request fails" do + should "allow it to be approved again" do + @post = create(:post, tag_string: "foo aaa") + @bur = create(:bulk_update_request, script: "alias foo -> bar") + + TagAlias.any_instance.stubs(:process!).raises(RuntimeError.new("oh no")) + @bur.approve!(@admin) + assert_raises(RuntimeError) { perform_enqueued_jobs } + + assert_equal("aaa foo", @post.reload.tag_string) + + assert_equal("failed", @bur.reload.status) + assert_not_nil(@bur.forum_topic) + assert_equal(@admin, @bur.approver) + + @ta = TagAlias.find_by!(antecedent_name: "foo", consequent_name: "bar") + assert_equal("active", @ta.status) + assert_equal(@admin, @ta.approver) + assert_equal(@bur.forum_topic, @ta.forum_topic) + + TagAlias.any_instance.unstub(:process!) + @bur.approve!(@admin) + perform_enqueued_jobs + + assert_equal("aaa bar", @post.reload.tag_string) + + assert_equal("approved", @bur.reload.status) + assert_not_nil(@bur.forum_topic) + assert_equal(@admin, @bur.approver) + + assert_equal("active", @ta.reload.status) + assert_equal(@admin, @ta.approver) + assert_equal(@bur.forum_topic, @ta.forum_topic) + end + end + should "create a forum topic" do bur = create(:bulk_update_request, reason: "zzz", script: "create alias aaa -> bbb")