diff --git a/app/models/artist.rb b/app/models/artist.rb index 56f1baee8..a9e1e3c4e 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -3,11 +3,13 @@ class Artist < ApplicationRecord class RevertError < StandardError; end attr_accessor :url_string_changed - array_attribute :other_names + deletable - before_validation :normalize_name - before_validation :normalize_other_names + normalize :name, :normalize_name + normalize :other_names, :normalize_other_names + array_attribute :other_names # XXX must come after `normalize :other_names` + validate :validate_tag_category validates :name, tag_name: true, uniqueness: true before_save :update_tag_category @@ -55,27 +57,26 @@ class Artist < ApplicationRecord end end - module NameMethods - extend ActiveSupport::Concern - - module ClassMethods + concerning :NameMethods do + class_methods do def normalize_name(name) name.to_s.mb_chars.downcase.strip.gsub(/ /, '_').to_s end - end - def normalize_name - self.name = Artist.normalize_name(name) + def normalize_other_names(other_names) + other_names.map { |name| normalize_other_name(name) }.uniq.reject(&:blank?) + end + + # XXX Differences from wiki page other names: allow uppercase, use NFC + # instead of NFKC, and allow repeated, leading, and trailing underscores. + def normalize_other_name(other_name) + other_name.to_s.unicode_normalize(:nfc).normalize_whitespace.squish.tr(" ", "_") + end end def pretty_name name.tr("_", " ") end - - def normalize_other_names - self.other_names = other_names.map { |x| Artist.normalize_name(x) }.uniq - self.other_names -= [name] - end end module VersionMethods @@ -145,8 +146,6 @@ class Artist < ApplicationRecord artist = Artist.new(params) end - artist.normalize_name - artist.normalize_other_names artist end end @@ -284,7 +283,6 @@ class Artist < ApplicationRecord end include UrlMethods - include NameMethods include VersionMethods extend FactoryMethods include TagMethods diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 2f0fceb7d..d9da10bd4 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -3,12 +3,12 @@ class WikiPage < ApplicationRecord META_WIKIS = ["list_of_", "tag_group:", "pool_group:", "howto:", "about:", "help:", "template:"] - before_validation :normalize_other_names before_save :update_dtext_links, if: :dtext_links_changed? after_save :create_version normalize :title, :normalize_title normalize :body, :normalize_text + normalize :other_names, :normalize_other_names validates :title, tag_name: true, presence: true, uniqueness: true, if: :title_changed? validates :body, presence: true, unless: -> { is_deleted? || other_names.present? } @@ -153,12 +153,12 @@ class WikiPage < ApplicationRecord title.to_s.downcase.delete_prefix("~").gsub(/[[:space:]]+/, "_").squeeze("_").gsub(/\A_|_\z/, "") end - def normalize_other_names - self.other_names = other_names.map { |name| WikiPage.normalize_other_name(name) }.uniq + def self.normalize_other_names(other_names) + other_names.map { |name| normalize_other_name(name) }.uniq.reject(&:blank?) end def self.normalize_other_name(name) - name.unicode_normalize(:nfkc).gsub(/[[:space:]]+/, " ").strip.tr(" ", "_") + name.to_s.unicode_normalize(:nfkc).normalize_whitespace.gsub(/[[:space:]]+/, "_").squeeze("_").gsub(/\A_|_\z/, "") end def category_name diff --git a/script/fixes/071_renormalize_other_names_in_wiki_pages.rb b/script/fixes/071_renormalize_other_names_in_wiki_pages.rb new file mode 100755 index 000000000..2fa7611c5 --- /dev/null +++ b/script/fixes/071_renormalize_other_names_in_wiki_pages.rb @@ -0,0 +1,17 @@ +#!/usr/bin/env ruby + +require_relative "../../config/environment" + +CurrentUser.scoped(User.system) do + WikiPage.transaction do + WikiPage.find_each do |wp| + old_other_names = wp.other_names + wp.other_names = old_other_names + + if wp.changed? + p [wp.id, wp.changes] + wp.save! + end + end + end +end diff --git a/script/fixes/072_renormalize_other_names_in_artists.rb b/script/fixes/072_renormalize_other_names_in_artists.rb new file mode 100755 index 000000000..6c9e1fc88 --- /dev/null +++ b/script/fixes/072_renormalize_other_names_in_artists.rb @@ -0,0 +1,17 @@ +#!/usr/bin/env ruby + +require_relative "../../config/environment" + +CurrentUser.scoped(User.system) do + Artist.transaction do + Artist.find_each do |artist| + old_other_names = artist.other_names + artist.other_names = old_other_names + + if artist.changed? + p [artist.id, artist.changes] + artist.save! + end + end + end +end diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index a7fefeb7e..7fdaa3471 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -384,9 +384,27 @@ class ArtistTest < ActiveSupport::TestCase end end - should "normalize its other names" do - artist = FactoryBot.create(:artist, name: "a1", other_names: "a1 aaa aaa AAA bbb ccc_ddd") - assert_equal("aaa bbb ccc_ddd", artist.other_names_string) + context "the #normalize_other_names method" do + subject { build(:artist) } + + should normalize_attribute(:other_names).from([" foo"]).to(["foo"]) + should normalize_attribute(:other_names).from(["foo "]).to(["foo"]) + should normalize_attribute(:other_names).from(["___foo"]).to(["___foo"]) + should normalize_attribute(:other_names).from(["foo___"]).to(["foo___"]) + should normalize_attribute(:other_names).from(["foo\n"]).to(["foo"]) + should normalize_attribute(:other_names).from(["foo bar"]).to(["foo_bar"]) + should normalize_attribute(:other_names).from(["foo bar"]).to(["foo_bar"]) + should normalize_attribute(:other_names).from(["foo___bar"]).to(["foo___bar"]) + should normalize_attribute(:other_names).from([" _Foo Bar_ "]).to(["_Foo_Bar_"]) + should normalize_attribute(:other_names).from(["foo 1", "bar 2"]).to(["foo_1", "bar_2"]) + should normalize_attribute(:other_names).from(["foo", nil, "", " ", "bar"]).to(["foo", "bar"]) + should normalize_attribute(:other_names).from([nil, "", " "]).to([]) + should normalize_attribute(:other_names).from(["pokémon".unicode_normalize(:nfd)]).to(["pokémon".unicode_normalize(:nfkc)]) + should normalize_attribute(:other_names).from(["foo", "foo"]).to(["foo"]) + + should normalize_attribute(:other_names).from("foo foo").to(["foo"]) + should normalize_attribute(:other_names).from("foo bar").to(["foo", "bar"]) + should normalize_attribute(:other_names).from("_foo_ Bar").to(["_foo_", "Bar"]) end should "search on its name should return results" do @@ -550,7 +568,7 @@ class ArtistTest < ActiveSupport::TestCase artist = Artist.new_with_defaults(source: source) assert_equal("niceandcool", artist.name) - assert_equal("nice_and_cool", artist.other_names_string) + assert_equal("Nice_and_Cool niceandcool", artist.other_names_string) assert_includes(artist.urls.map(&:url), "https://www.pixiv.net/users/906442") assert_includes(artist.urls.map(&:url), "https://www.pixiv.net/stacc/niceandcool") end @@ -561,7 +579,7 @@ class ArtistTest < ActiveSupport::TestCase artist = Artist.new_with_defaults(name: "test_artist") assert_equal("test_artist", artist.name) - assert_equal("nice_and_cool niceandcool", artist.other_names_string) + assert_equal("Nice_and_Cool niceandcool", artist.other_names_string) assert_includes(artist.urls.map(&:url), "https://www.pixiv.net/users/906442") assert_includes(artist.urls.map(&:url), "https://www.pixiv.net/stacc/niceandcool") end diff --git a/test/unit/wiki_page_test.rb b/test/unit/wiki_page_test.rb index 6f0f557bf..9b37af9f3 100644 --- a/test/unit/wiki_page_test.rb +++ b/test/unit/wiki_page_test.rb @@ -18,15 +18,6 @@ class WikiPageTest < ActiveSupport::TestCase @wiki_page = FactoryBot.create(:wiki_page, :title => "HOT POTATO", :other_names => "foo*bar baz") end - should "normalize its title" do - assert_equal("hot_potato", @wiki_page.title) - end - - should "normalize its other names" do - @wiki_page.update(:other_names => "foo*bar baz baz 加賀（艦これ）") - assert_equal(%w[foo*bar baz 加賀(艦これ)], @wiki_page.other_names) - end - should "search by title" do matches = WikiPage.titled("hot potato") assert_equal(1, matches.count) @@ -91,6 +82,27 @@ class WikiPageTest < ActiveSupport::TestCase end end + context "the #normalize_other_names method" do + subject { build(:wiki_page) } + + should normalize_attribute(:other_names).from([" foo"]).to(["foo"]) + should normalize_attribute(:other_names).from(["foo "]).to(["foo"]) + should normalize_attribute(:other_names).from(["___foo"]).to(["foo"]) + should normalize_attribute(:other_names).from(["foo___"]).to(["foo"]) + should normalize_attribute(:other_names).from(["foo\n"]).to(["foo"]) + should normalize_attribute(:other_names).from(["foo bar"]).to(["foo_bar"]) + should normalize_attribute(:other_names).from(["foo bar"]).to(["foo_bar"]) + should normalize_attribute(:other_names).from(["foo___bar"]).to(["foo_bar"]) + should normalize_attribute(:other_names).from([" _Foo Bar_ "]).to(["Foo_Bar"]) + should normalize_attribute(:other_names).from(["foo 1", "bar 2"]).to(["foo_1", "bar_2"]) + should normalize_attribute(:other_names).from(["foo", nil, "", " ", "bar"]).to(["foo", "bar"]) + should normalize_attribute(:other_names).from([nil, "", " "]).to([]) + should normalize_attribute(:other_names).from(["pokémon".unicode_normalize(:nfd)]).to(["pokémon".unicode_normalize(:nfkc)]) + should normalize_attribute(:other_names).from(["ＡＢＣ"]).to(["ABC"]) + should normalize_attribute(:other_names).from(["foo", "foo"]).to(["foo"]) + should normalize_attribute(:other_names).from(%w[foo*bar baz baz 加賀（艦これ）]).to(%w[foo*bar baz 加賀(艦これ)]) + end + context "during title validation" do should normalize_attribute(:title).from(" foo ").to("foo") should normalize_attribute(:title).from("~foo").to("foo")