From 2e2b99f74b02240fafd720fcbb19849057af6254 Mon Sep 17 00:00:00 2001 From: miteruzo Date: Thu, 12 Mar 2026 00:46:21 +0900 Subject: [PATCH] #281 --- backend/app/models/tag.rb | 14 ++- backend/app/models/tag_name.rb | 4 + .../app/models/tag_name_sanitisation_rule.rb | 13 ++- backend/app/models/wiki_page.rb | 4 + ...311232100_add_discarded_at_to_tag_names.rb | 6 ++ ...11232300_add_discarded_at_to_wiki_pages.rb | 6 ++ backend/db/schema.rb | 6 +- .../models/tag_name_sanitisation_rule_spec.rb | 3 +- backend/spec/models/tag_spec.rb | 91 ++++++++++++------- 9 files changed, 104 insertions(+), 43 deletions(-) create mode 100644 backend/db/migrate/20260311232100_add_discarded_at_to_tag_names.rb create mode 100644 backend/db/migrate/20260311232300_add_discarded_at_to_wiki_pages.rb diff --git a/backend/app/models/tag.rb b/backend/app/models/tag.rb index 3a216b6..9f480cd 100644 --- a/backend/app/models/tag.rb +++ b/backend/app/models/tag.rb @@ -162,14 +162,18 @@ class Tag < ApplicationRecord end source_tag_name = source_tag.tag_name - nico_flg = source_tag.nico? + + if source_tag_name.wiki_page.present? + raise ActiveRecord::RecordInvalid.new(source_tag_name) + end + source_tag.discard! - source_tag_name.reload - if nico_flg - source_tag_name.destroy! + if source_tag.nico? + source_tag_name.discard! else - source_tag_name.update!(canonical: target_tag.tag_name) + source_tag_name.update_columns(canonical_id: target_tag.tag_name_id, + updated_at: Time.current) end end diff --git a/backend/app/models/tag_name.rb b/backend/app/models/tag_name.rb index 239f932..c353d2f 100644 --- a/backend/app/models/tag_name.rb +++ b/backend/app/models/tag_name.rb @@ -1,4 +1,8 @@ class TagName < ApplicationRecord + include Discard::Model + + default_scope -> { kept } + has_one :tag has_one :wiki_page diff --git a/backend/app/models/tag_name_sanitisation_rule.rb b/backend/app/models/tag_name_sanitisation_rule.rb index d0c8ea1..e37738a 100644 --- a/backend/app/models/tag_name_sanitisation_rule.rb +++ b/backend/app/models/tag_name_sanitisation_rule.rb @@ -24,12 +24,15 @@ class TagNameSanitisationRule < ApplicationRecord existing_tn = existing_tn.canonical || existing_tn next if existing_tn.id == tn.id - if existing_tn.tag - Tag.merge_tags!(existing_tn.tag, tn.tag) if tn.tag - elsif tn.tag - tn.tag.update_columns(tag_name_id: existing_tn.id, updated_at: Time.current) + existing_tag = Tag.find_by(tag_name_id: existing_tn.id) + source_tag = Tag.find_by(tag_name_id: tn.id) + + if existing_tag + Tag.merge_tags!(existing_tag, source_tag) if tn.tag + elsif source_tag + source_tag.update_columns(tag_name_id: existing_tn.id, updated_at: Time.current) end - tn.destroy! + tn.discard! next end diff --git a/backend/app/models/wiki_page.rb b/backend/app/models/wiki_page.rb index d795a8e..b1d1b02 100644 --- a/backend/app/models/wiki_page.rb +++ b/backend/app/models/wiki_page.rb @@ -2,6 +2,10 @@ require 'set' class WikiPage < ApplicationRecord + include Discard::Model + + default_scope -> { kept } + has_many :wiki_revisions, dependent: :destroy belongs_to :created_user, class_name: 'User' belongs_to :updated_user, class_name: 'User' diff --git a/backend/db/migrate/20260311232100_add_discarded_at_to_tag_names.rb b/backend/db/migrate/20260311232100_add_discarded_at_to_tag_names.rb new file mode 100644 index 0000000..b161873 --- /dev/null +++ b/backend/db/migrate/20260311232100_add_discarded_at_to_tag_names.rb @@ -0,0 +1,6 @@ +class AddDiscardedAtToTagNames < ActiveRecord::Migration[8.0] + def change + add_column :tag_names, :discarded_at, :datetime + add_index :tag_names, :discarded_at + end +end diff --git a/backend/db/migrate/20260311232300_add_discarded_at_to_wiki_pages.rb b/backend/db/migrate/20260311232300_add_discarded_at_to_wiki_pages.rb new file mode 100644 index 0000000..26435e0 --- /dev/null +++ b/backend/db/migrate/20260311232300_add_discarded_at_to_wiki_pages.rb @@ -0,0 +1,6 @@ +class AddDiscardedAtToWikiPages < ActiveRecord::Migration[8.0] + def change + add_column :wiki_pages, :discarded_at, :datetime + add_index :wiki_pages, :discarded_at + end +end diff --git a/backend/db/schema.rb b/backend/db/schema.rb index b6ffbd3..b6eb1f3 100644 --- a/backend/db/schema.rb +++ b/backend/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_03_11_123100) do +ActiveRecord::Schema[8.0].define(version: 2026_03_11_232300) do create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -142,7 +142,9 @@ ActiveRecord::Schema[8.0].define(version: 2026_03_11_123100) do t.bigint "canonical_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.datetime "discarded_at" t.index ["canonical_id"], name: "index_tag_names_on_canonical_id" + t.index ["discarded_at"], name: "index_tag_names_on_discarded_at" t.index ["name"], name: "index_tag_names_on_name", unique: true end @@ -204,7 +206,9 @@ ActiveRecord::Schema[8.0].define(version: 2026_03_11_123100) do t.bigint "updated_user_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.datetime "discarded_at" t.index ["created_user_id"], name: "index_wiki_pages_on_created_user_id" + t.index ["discarded_at"], name: "index_wiki_pages_on_discarded_at" t.index ["tag_name_id"], name: "index_wiki_pages_on_tag_name_id", unique: true t.index ["updated_user_id"], name: "index_wiki_pages_on_updated_user_id" end diff --git a/backend/spec/models/tag_name_sanitisation_rule_spec.rb b/backend/spec/models/tag_name_sanitisation_rule_spec.rb index bf90acb..654f85f 100644 --- a/backend/spec/models/tag_name_sanitisation_rule_spec.rb +++ b/backend/spec/models/tag_name_sanitisation_rule_spec.rb @@ -69,7 +69,8 @@ RSpec.describe TagNameSanitisationRule, type: :model do it 'moves the tag to the existing tag_name' do described_class.apply! - expect(source_tag.reload.tag_name_id).to eq(existing.id) + expected_tag_name_id = existing.canonical_id || existing.id + expect(source_tag.reload.tag_name_id).to eq(expected_tag_name_id) expect(TagName.exists?(source_tag_name_id)).to be(false) end end diff --git a/backend/spec/models/tag_spec.rb b/backend/spec/models/tag_spec.rb index 83583db..a9fc35e 100644 --- a/backend/spec/models/tag_spec.rb +++ b/backend/spec/models/tag_spec.rb @@ -13,14 +13,9 @@ RSpec.describe Tag, type: :model do context 'when merging a simple source tag' do let!(:source_post_tag) { PostTag.create!(post: post_record, tag: source_tag) } - it 'discards the source post_tag, creates an active target post_tag, ' + - 'discards the source tag, and aliases the source tag_name' do + it 'discards the source post_tag, creates an active target post_tag, discards the source tag, and aliases the source tag_name' do described_class.merge_tags!(target_tag, [source_tag]) - 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) - source_pt = PostTag.with_discarded.find(source_post_tag.id) active_target = PostTag.kept.find_by(post_id: post_record.id, tag_id: target_tag.id) @@ -29,7 +24,9 @@ RSpec.describe Tag, type: :model do expect(active_target).to be_present expect(Tag.with_discarded.find(source_tag.id)).to be_discarded - expect(source_tag.tag_name.reload.canonical_id).to eq(target_tag.tag_name_id) + expect(TagName.with_discarded.find(source_tag_name.id)).not_to be_discarded + expect(source_tag_name.reload.canonical_id).to eq(target_tag.tag_name_id) + expect(target_tag.reload.post_count).to eq(1) end end @@ -37,8 +34,7 @@ RSpec.describe Tag, type: :model do let!(:target_post_tag) { PostTag.create!(post: post_record, tag: target_tag) } let!(:source_post_tag) { PostTag.create!(post: post_record, tag: source_tag) } - it 'discards the source post_tag, keeps one active target post_tag, ' + - 'and aliases the source tag_name' do + it 'discards the source post_tag, keeps one active target post_tag, discards the source tag, and aliases the source tag_name' do described_class.merge_tags!(target_tag, [source_tag]) source_pt = PostTag.with_discarded.find(source_post_tag.id) @@ -47,17 +43,12 @@ RSpec.describe Tag, type: :model do expect(source_pt.discarded_at).to be_present expect(source_pt.tag_id).to eq(source_tag.id) expect(active.count).to eq(1) - - expect(source_pt.discarded_at).to be_present - expect(source_pt.tag_id).to eq(source_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) - expect(active.first.id).to eq(target_post_tag.id) expect(Tag.with_discarded.find(source_tag.id)).to be_discarded - expect(source_tag.tag_name.reload.canonical_id).to eq(target_tag.tag_name_id) + expect(TagName.with_discarded.find(source_tag_name.id)).not_to be_discarded + expect(source_tag_name.reload.canonical_id).to eq(target_tag.tag_name_id) + expect(target_tag.reload.post_count).to eq(1) end end @@ -67,10 +58,6 @@ RSpec.describe Tag, type: :model do it 'ignores the target in source_tags while still merging the source tag' do described_class.merge_tags!(target_tag, [source_tag, target_tag]) - expect(Tag.exists?(target_tag.id)).to be(true) - expect(Tag.exists?(source_tag.id)).to be(false) - expect(target_tag.reload.post_count).to eq(1) - source_pt = PostTag.with_discarded.find(source_post_tag.id) active_target = PostTag.kept.find_by(post_id: post_record.id, tag_id: target_tag.id) @@ -79,23 +66,65 @@ RSpec.describe Tag, type: :model do expect(source_pt.discarded_at).to be_present expect(source_pt.tag_id).to eq(source_tag.id) expect(active_target).to be_present + expect(source_tag_name.reload.canonical_id).to eq(target_tag.tag_name_id) + expect(target_tag.reload.post_count).to eq(1) + end + end + + context 'when the source tag_name is invalid under sanitisation rules' do + let!(:source_post_tag) { PostTag.create!(post: post_record, tag: source_tag) } + let!(:sanitisation_rule) do + TagNameSanitisationRule.create!( + priority: 99_999, + source_pattern: 'INVALIDTOKEN', + replacement: '' + ) + end + + before do + source_tag_name.update_columns( + name: "#{ source_tag_name.name }INVALIDTOKEN", + updated_at: Time.current + ) + end + + it 'still merges, but discards the source tag_name instead of aliasing it' do + described_class.merge_tags!(target_tag, [source_tag]) + + source_pt = PostTag.with_discarded.find(source_post_tag.id) + active_target = PostTag.kept.find_by(post_id: post_record.id, tag_id: target_tag.id) + discarded_source_tag_name = TagName.with_discarded.find(source_tag_name.id) + + expect(source_pt.discarded_at).to be_present + expect(source_pt.tag_id).to eq(source_tag.id) + expect(active_target).to be_present + + expect(Tag.with_discarded.find(source_tag.id)).to be_discarded + expect(target_tag.reload.post_count).to eq(1) end end - context 'when aliasing the source tag_name would be invalid under normal validation' do + context 'when the source tag_name has a wiki_page' do let!(:source_post_tag) { PostTag.create!(post: post_record, tag: source_tag) } let!(:wiki_page) do WikiPage.create!( - tag_name: source_tag.tag_name, + tag_name: source_tag_name, created_user: create_admin_user!, updated_user: create_admin_user! ) end - it 'still merges by bypassing validations' do + it 'rolls back the transaction' do expect { described_class.merge_tags!(target_tag, [source_tag]) }.to raise_error(ActiveRecord::RecordInvalid) + + expect(Tag.with_discarded.find(source_tag.id)).not_to be_discarded + expect(TagName.with_discarded.find(source_tag_name.id)).not_to be_discarded + expect(PostTag.kept.find(source_post_tag.id).tag_id).to eq(source_tag.id) + expect(PostTag.kept.find_by(post_id: post_record.id, tag_id: target_tag.id)).to be_nil + expect(source_tag_name.reload.canonical_id).to be_nil + expect(target_tag.reload.post_count).to eq(0) end end @@ -104,16 +133,16 @@ RSpec.describe Tag, type: :model do let!(:source_tag) { create(:tag, category: :nico, name: 'nico:bar') } let!(:source_tag_name_id) { source_tag.tag_name_id } - it 'deletes the source tag_name instead of aliasing it' do + it 'discards the source tag_name instead of aliasing it' do described_class.merge_tags!(target_tag, [source_tag]) - 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) + discarded_source_tag = Tag.with_discarded.find(source_tag.id) + discarded_source_tag_name = TagName.with_discarded.find(source_tag_name_id) - expect(Tag.with_discarded.find(source_tag.id)).not_to be_discarded - expect(PostTag.kept.find(source_post_tag.id).tag_id).to eq(source_tag.id) - expect(source_tag.tag_name.reload.canonical_id).to be_nil + expect(discarded_source_tag).to be_discarded + expect(discarded_source_tag_name).to be_discarded + expect(discarded_source_tag_name.canonical_id).to be_nil + expect(target_tag.reload.post_count).to eq(0) end end end