#281

backend/app/models/tag.rb

@@ -152,11 +152,9 @@ class Tag < ApplicationRecord
         st.post_tags.find_each do |pt|
           if PostTag.kept.exists?(post_id: pt.post_id, tag_id: target_tag.id)
             pt.discard_by!(nil)
-            # discard 後の update! は禁止なので DB を直に更新
-            pt.update_columns(tag_id: target_tag.id, updated_at: Time.current)
-          else
-            pt.update!(tag: target_tag)
           end
+          # discard 後の update! は禁止なので DB を直に更新
+          pt.update_columns(tag_id: target_tag.id, updated_at: Time.current)
         end
 
         tag_name = st.tag_name
@@ -167,7 +165,8 @@ class Tag < ApplicationRecord
         if nico_flg
           tag_name.destroy!
         else
-          tag_name.update!(canonical: target_tag.tag_name)
+          tag_name.update_columns(canonical_id: target_tag.tag_name&.id,
+                                  updated_at: Time.current)
         end
       end
 

backend/app/models/tag_name.rb

@@ -1,6 +1,4 @@
 class TagName < ApplicationRecord
-  before_validation :sanitise_name
-
   has_one :tag
   has_one :wiki_page
 
@@ -12,6 +10,7 @@ class TagName < ApplicationRecord
   validate :canonical_must_be_canonical
   validate :alias_name_must_not_have_prefix
   validate :canonical_must_not_be_present_with_tag_or_wiki_page
+  validate :name_must_be_sanitised
 
   def self.canonicalise names
     names = Array(names).map { |n| n.to_s.strip }.reject(&:blank?)
@@ -45,4 +44,10 @@ class TagName < ApplicationRecord
       errors.add :canonical, 'タグもしくは Wiki の参照がある名前はエーリアスになれません．'
     end
   end
+
+  def name_must_be_sanitised
+    if name != TagNameSanitisationRule.sanitise(name)
+      errors.add :name, '名前に使用できない文字が含まれてゐます．'
+    end
+  end
 end

backend/spec/factories/tags.rb

@@ -1,12 +1,22 @@
 FactoryBot.define do
   factory :tag do
+    transient do
+      name { nil }
+    end
+
     category { :general }
     post_count { 0 }
     association :tag_name
 
+    after(:build) do |tag, evaluator|
+      tag.name = evaluator.name if evaluator.name.present?
+    end
+
     trait :nico do
       category { :nico }
-      tag_name { association(:tag_name, name: "nico:#{ SecureRandom.hex(4) }") }
+      transient do
+        name { "nico:#{ SecureRandom.hex(4) }" }
+      end
     end
   end
 end

backend/spec/models/tag_name_sanitisation_rule_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe TagNameSanitisationRule, type: :model do
       described_class.create!(priority: 20, source_pattern: 'ABC', replacement: 'abc')
     end
 
-    it 'applies rules in priority order' do
+    it 'applies sanitisation rules sequentially in priority order' do
       expect(described_class.sanitise('A_B_C')).to eq('abc')
     end
 
@@ -31,7 +31,11 @@ RSpec.describe TagNameSanitisationRule, type: :model do
     end
 
     context 'when no conflicting tag_name exists' do
-      let!(:tag_name) { TagName.create!(name: 'foo_bar') }
+      let!(:tag_name) do
+        TagName.create!(name: 'tmp').tap do |tn|
+          tn.update_columns(name: 'foo_bar', updated_at: Time.current)
+        end
+      end
 
       it 'renames the tag_name' do
         described_class.apply!
@@ -41,7 +45,11 @@ RSpec.describe TagNameSanitisationRule, type: :model do
 
     context 'when a conflicting canonical tag_name exists' do
       let!(:existing) { TagName.create!(name: 'foobar') }
-      let!(:source)   { TagName.create!(name: 'foo_bar') }
+      let!(:source) do
+        TagName.create!(name: 'tmp').tap do |tn|
+          tn.update_columns(name: 'foo_bar', updated_at: Time.current)
+        end
+      end
 
       it 'deletes the source tag_name' do
         described_class.apply!
@@ -52,11 +60,40 @@ RSpec.describe TagNameSanitisationRule, type: :model do
 
     context 'when the source tag_name has a tag and the existing one has no tag' do
       let!(:existing) { TagName.create!(name: 'foobar') }
-      let!(:source_tag) { create(:tag, name: 'foo_bar', category: :general) }
+      let!(:source_tag) { create(:tag, name: 'tmp', category: :general) }
+      let!(:source_tag_name_id) { source_tag.tag_name_id }
+
+      before do
+        source_tag.tag_name.update_columns(name: 'foo_bar', updated_at: Time.current)
+      end
 
       it 'moves the tag to the existing tag_name' do
         described_class.apply!
         expect(source_tag.reload.tag_name_id).to eq(existing.id)
+        expect(TagName.exists?(source_tag_name_id)).to be(false)
+      end
+    end
+
+    context 'when both source and existing tag_names have tags' do
+      let!(:existing_tn) { TagName.create!(name: 'foobar') }
+      let!(:existing_tag) { Tag.create!(tag_name: existing_tn, category: :general) }
+
+      let!(:source_tn) { TagName.create!(name: 'tmp') }
+      let!(:source_tag) { Tag.create!(tag_name: source_tn, category: :general) }
+      let!(:source_tag_name_id) { source_tn.id }
+
+      before do
+        source_tn.update_columns(name: 'foo_bar', updated_at: Time.current)
+      end
+
+      it 'merges the source tag into the existing tag and deletes the source tag_name' do
+        expect(TagName.find_by(name: 'foobar')&.tag&.id).to eq(existing_tag.id)
+        expect(TagName.find_by(name: 'foo_bar')&.tag&.id).to eq(source_tag.id)
+
+        described_class.apply!
+
+        expect(Tag.exists?(source_tag.id)).to be(false)
+        expect(TagName.exists?(source_tag.tag_name_id)).to be(false)
       end
     end
   end

backend/spec/models/tag_name_spec.rb

@@ -1,28 +0,0 @@
-require 'rails_helper'
-
-RSpec.describe TagName, type: :model do
-  describe 'before_validation :sanitise_name' do
-    before do
-      TagNameSanitisationRule.create!(priority: 10, source_pattern: '_', replacement: '')
-    end
-
-    it 'sanitises name on create' do
-      tag_name = TagName.create!(name: 'foo_bar')
-      expect(tag_name.reload.name).to eq('foobar')
-    end
-
-    it 'sanitises name on update' do
-      tag_name = TagName.create!(name: 'foobar')
-      tag_name.update!(name: 'foo_bar')
-      expect(tag_name.reload.name).to eq('foobar')
-    end
-
-    it 'becomes invalid when sanitised name conflicts with another tag_name' do
-      TagName.create!(name: 'foobar')
-      tag_name = TagName.new(name: 'foo_bar')
-
-      expect(tag_name).to be_invalid
-      expect(tag_name.errors[:name]).to be_present
-    end
-  end
-end

backend/spec/models/tag_spec.rb

@@ -35,7 +35,7 @@ RSpec.describe Tag, type: :model do
 
         expect(active.count).to eq(1)
         expect(discarded_source.discarded_at).to be_present
-        expect(discarded_source.tag_id).to eq(source_tag.id)
+        expect(discarded_source.tag_id).to eq(target_tag.id)
         expect(Tag.exists?(source_tag.id)).to be(false)
         expect(source_tag_name.reload.canonical_id).to eq(target_tag.tag_name_id)
         expect(target_tag.reload.post_count).to eq(1)
@@ -55,7 +55,7 @@ RSpec.describe Tag, type: :model do
       end
     end
 
-    context 'when aliasing the source tag_name is invalid' do
+    context 'when aliasing the source tag_name would be invalid under normal validation' do
       let!(:source_post_tag) { PostTag.create!(post: post_record, tag: source_tag) }
       let!(:wiki_page) do
         WikiPage.create!(
@@ -65,14 +65,13 @@ RSpec.describe Tag, type: :model do
         )
       end
 
-      it 'rolls back the transaction' do
-        expect {
-          described_class.merge_tags!(target_tag, [source_tag])
-        }.to raise_error(ActiveRecord::RecordInvalid)
+      it 'still merges by bypassing validations' do
+        described_class.merge_tags!(target_tag, [source_tag])
 
-        expect(Tag.exists?(source_tag.id)).to be(true)
-        expect(source_post_tag.reload.tag_id).to eq(source_tag.id)
-        expect(source_tag_name.reload.canonical_id).to be_nil
+        expect(Tag.exists?(source_tag.id)).to be(false)
+        expect(source_post_tag.reload.tag_id).to eq(target_tag.id)
+        expect(source_tag_name.reload.canonical_id).to eq(target_tag.tag_name_id)
+        expect(source_tag_name.reload).not_to be_valid
       end
     end
 
@@ -86,6 +85,7 @@ RSpec.describe Tag, type: :model do
 
         expect(Tag.exists?(source_tag.id)).to be(false)
         expect(TagName.exists?(source_tag_name_id)).to be(false)
+        expect(target_tag.reload.post_count).to eq(0)
       end
     end
   end