Fix issue when reordering tags and @@remove_unused_tags is true

When reordering taggings, the taggings get deleted and  recreated. However, by the time the taggings are recreated, the tags have already been removed.

Fixed by wrapping the delete + create in a transaction, and only removing unused tags on transaction commit.

Author: james-reading

CHANGELOG.md

@@ -17,6 +17,7 @@ As such, _Breaking Changes_ are major. _Features_ would map to either major or m
   * Reduce packaged gem size
 * Fixes
   * [@iainbeeston Fix mismatched indentations warning message](https://github.com/mbleigh/acts-as-taggable-on/pull/1150)
+  * [@james-reading Fix issue when reordering tags and @@remove_unused_tags is true](https://github.com/mbleigh/acts-as-taggable-on/pull/1154)
 
 ### [v12.0.0) / 2024-11-09](https://github.com/mbleigh/acts-as-taggable-on/compare/v11.0.0...v12.0.0)
 

lib/acts-as-taggable-on/taggable/core.rb

@@ -272,15 +272,17 @@ def save_tags
             # Delete discarded tags and create new tags
           end
 
-          # Destroy old taggings:
-          taggings.not_owned.by_context(context).where(tag_id: old_tags).destroy_all if old_tags.present?
-
-          # Create new taggings:
-          new_tags.each do |tag|
-            if taggable_tenant
-              taggings.create!(tag_id: tag.id, context: context.to_s, taggable: self, tenant: taggable_tenant)
-            else
-              taggings.create!(tag_id: tag.id, context: context.to_s, taggable: self)
+          transaction do
+            # Destroy old taggings:
+            taggings.not_owned.by_context(context).where(tag_id: old_tags).destroy_all if old_tags.present?
+
+            # Create new taggings:
+            new_tags.each do |tag|
+              if taggable_tenant
+                taggings.create!(tag_id: tag.id, context: context.to_s, taggable: self, tenant: taggable_tenant)
+              else
+                taggings.create!(tag_id: tag.id, context: context.to_s, taggable: self)
+              end
             end
           end
         end

lib/acts-as-taggable-on/tagging.rb

@@ -23,7 +23,7 @@ class Tagging < ActsAsTaggableOn.base_class.constantize # :nodoc:
 
     validates_uniqueness_of :tag_id, scope: %i[taggable_type taggable_id context tagger_id tagger_type]
 
-    after_destroy :remove_unused_tags
+    after_destroy_commit :remove_unused_tags
 
     private
 

spec/acts_as_taggable_on/taggable_spec.rb

@@ -92,6 +92,23 @@
     ids = @taggable.tags.map { |t| t.taggings.first.id }
     expect(ids).to eq(ids.sort)
   end
+
+  it 'can reorder tags successfully when @@remove_unused_tags is true' do
+    previous_setting = ActsAsTaggableOn.remove_unused_tags
+    ActsAsTaggableOn.remove_unused_tags = true
+
+    @taggable.tag_list = 'pow, ruby, rails'
+    @taggable.save!
+    @taggable.reload
+    expect(@taggable.tag_list).to eq(%w(pow ruby rails))
+
+    @taggable.tag_list = 'rails, pow, ruby'
+    @taggable.save!
+    @taggable.reload
+    expect(@taggable.tag_list).to eq(%w(rails pow ruby))
+  ensure
+    ActsAsTaggableOn.remove_unused_tags = previous_setting
+  end
 end
 
 RSpec.describe 'Taggable' do