From 41ff05c12196598f903e1c30d5d6b59911edb4ad Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 14 Nov 2018 15:54:51 -0600 Subject: [PATCH] artists: convert other_names to array (#3987). --- app/controllers/artists_controller.rb | 2 +- .../src/styles/specific/artists.scss | 2 +- app/models/artist.rb | 40 +++++++++---------- app/models/artist_version.rb | 2 +- app/views/artists/_form.html.erb | 2 +- app/views/artists/_summary.html.erb | 2 +- app/views/artists/index.html.erb | 2 +- ..._change_other_names_to_array_on_artists.rb | 23 +++++++++++ db/structure.sql | 24 +++-------- test/unit/artist_test.rb | 14 +++---- 10 files changed, 60 insertions(+), 53 deletions(-) create mode 100644 db/migrate/20181114202744_change_other_names_to_array_on_artists.rb diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index 21deaee0d..b06226798 100644 --- a/app/controllers/artists_controller.rb +++ b/app/controllers/artists_controller.rb @@ -105,7 +105,7 @@ private end def artist_params - permitted_params = %i[name other_names other_names_comma group_name url_string notes] + permitted_params = %i[name other_names other_names_string group_name url_string notes] permitted_params << :is_active if CurrentUser.is_builder? params.fetch(:artist, {}).permit(permitted_params) diff --git a/app/javascript/src/styles/specific/artists.scss b/app/javascript/src/styles/specific/artists.scss index 0d82f9466..1149ef5ce 100644 --- a/app/javascript/src/styles/specific/artists.scss +++ b/app/javascript/src/styles/specific/artists.scss @@ -14,7 +14,7 @@ div#c-artists, div#excerpt { textarea { height: 10em; - &#artist_other_names_comma { + &#artist_other_names_string { height: 3em; } } diff --git a/app/models/artist.rb b/app/models/artist.rb index 327049170..509d09c1b 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -3,8 +3,10 @@ class Artist < ApplicationRecord class RevertError < Exception ; end attr_accessor :url_string_changed + array_attribute :other_names before_validation :normalize_name + before_validation :normalize_other_names after_save :create_version after_save :categorize_tag after_save :update_wiki @@ -239,16 +241,8 @@ class Artist < ApplicationRecord name.tr("_", " ") end - def other_names_array - other_names.try(:split, /[[:space:]]+/) - end - - def other_names_comma - other_names_array.try(:join, ", ") - end - - def other_names_comma=(string) - self.other_names = string.split(/,/).map {|x| Artist.normalize_name(x)}.join(" ") + def normalize_other_names + self.other_names = other_names.map { |x| Artist.normalize_name(x) } end end @@ -308,7 +302,7 @@ class Artist < ApplicationRecord self.name = version.name self.url_string = version.urls.join("\n") self.is_active = version.is_active - self.other_names = version.other_names.join(" ") + self.other_names = version.other_names self.group_name = version.group_name save end @@ -466,13 +460,21 @@ class Artist < ApplicationRecord where(name: normalize_name(name)) end + def any_other_name_matches(regex) + where(id: Artist.from("unnest(other_names) AS other_name").where("other_name ~ ?", regex)) + end + + def any_other_name_like(name) + where(id: Artist.from("unnest(other_names) AS other_name").where("other_name LIKE ?", name.to_escaped_for_sql_like)) + end + def any_name_matches(query) if query =~ %r!\A/(.*)/\z! - where_regex(:name, $1).or(where_regex(:other_names, $1)).or(where_regex(:group_name, $1)) + where_regex(:name, $1).or(any_other_name_matches($1)).or(where_regex(:group_name, $1)) else normalized_name = normalize_name(query) normalized_name = "*#{normalized_name}*" unless normalized_name.include?("*") - where_like(:name, normalized_name).or(where_like(:other_names, normalized_name)).or(where_like(:group_name, normalized_name)) + where_like(:name, normalized_name).or(any_other_name_like(normalized_name)).or(where_like(:group_name, normalized_name)) end end @@ -492,9 +494,12 @@ class Artist < ApplicationRecord q = super q = q.search_text_attribute(:name, params) - q = q.search_text_attribute(:other_names, params) q = q.search_text_attribute(:group_name, params) + if params[:any_other_name_like] + q = q.any_other_name_like(params[:any_other_name_like]) + end + if params[:any_name_matches].present? q = q.any_name_matches(params[:any_name_matches]) end @@ -536,12 +541,6 @@ class Artist < ApplicationRecord end end - module ApiMethods - def hidden_attributes - super + [:other_names_index] - end - end - include UrlMethods include NameMethods include GroupMethods @@ -551,7 +550,6 @@ class Artist < ApplicationRecord include TagMethods include BanMethods extend SearchMethods - include ApiMethods def status if is_banned? && is_active? diff --git a/app/models/artist_version.rb b/app/models/artist_version.rb index 029b20546..2d116114a 100644 --- a/app/models/artist_version.rb +++ b/app/models/artist_version.rb @@ -68,7 +68,7 @@ class ArtistVersion < ApplicationRecord end def other_names_diff(version) - latest_names = artist.other_names_array || [] + latest_names = artist.other_names || [] new_names = other_names old_names = version.present? ? version.other_names : [] diff --git a/app/views/artists/_form.html.erb b/app/views/artists/_form.html.erb index 8dadd1fbe..7ae80753f 100644 --- a/app/views/artists/_form.html.erb +++ b/app/views/artists/_form.html.erb @@ -14,7 +14,7 @@

<%= @artist.name %>

<% end %> - <%= f.input :other_names_comma, :hint => "Separate with commas", :as => :text, :label => "Other names" %> + <%= f.input :other_names_string, :hint => "Separate with spaces", :as => :text, :label => "Other names" %> <%= f.input :group_name %> <%= f.input :url_string, :label => "URLs", :as => :text, :input_html => {:size => "50x5", :value => params.dig(:artist, :url_string) || @artist.urls.join("\n")}, :hint => "You can prefix a URL with - to mark it as dead." %> diff --git a/app/views/artists/_summary.html.erb b/app/views/artists/_summary.html.erb index e9eb006d3..9c50f7561 100644 --- a/app/views/artists/_summary.html.erb +++ b/app/views/artists/_summary.html.erb @@ -7,7 +7,7 @@
  • Tag Alias <%= artist.tag_alias_name %>
    • <% end %> <% if artist.other_names.present? %> -
  • Other Names <%= link_to_artists(artist.other_names.split(/ /)) %>
    • +
  • Other Names <%= link_to_artists(artist.other_names) %>
    • <% end %> <% if artist.group_name.present? %>
  • Group <%= link_to_artist(artist.group_name) %>
    • diff --git a/app/views/artists/index.html.erb b/app/views/artists/index.html.erb index 66c74fa14..2b8eca9ba 100644 --- a/app/views/artists/index.html.erb +++ b/app/views/artists/index.html.erb @@ -19,7 +19,7 @@ (group: <%= link_to(artist.group_name, artist_path(artist)) %>) <% end %> - <%= artist.other_names %> + <%= artist.other_names_string %> <%= artist.status %> <% end %> <% end %> diff --git a/db/migrate/20181114202744_change_other_names_to_array_on_artists.rb b/db/migrate/20181114202744_change_other_names_to_array_on_artists.rb new file mode 100644 index 000000000..a56183c4f --- /dev/null +++ b/db/migrate/20181114202744_change_other_names_to_array_on_artists.rb @@ -0,0 +1,23 @@ +class ChangeOtherNamesToArrayOnArtists < ActiveRecord::Migration[5.2] + def up + Artist.without_timeout do + remove_index :artists, name: "index_artists_on_other_names_trgm" + change_column :artists, :other_names, "text[]", using: "array_remove(regexp_split_to_array(other_names, '\\s+'), '')", default: "{}" + add_index :artists, :other_names, using: :gin + + remove_column :artists, :other_names_index + execute "DROP TRIGGER trigger_artists_on_update ON artists" + end + end + + def down + Artist.without_timeout do + remove_index :artists, :other_names + change_column :artists, :other_names, "text", using: "array_to_string(other_names, ' ')", default: nil + add_index :artists, :other_names, name: "index_artists_on_other_names_trgm", using: :gin, opclass: :gin_trgm_ops + + add_column :artists, :other_names_index, :tsvector + execute "CREATE TRIGGER trigger_artists_on_update BEFORE INSERT OR UPDATE ON artists FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger('other_names_index', 'public.danbooru', 'other_names')" + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 97d96fbaf..81291c3c1 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -765,8 +765,7 @@ CREATE TABLE public.artists ( creator_id integer NOT NULL, is_active boolean DEFAULT true NOT NULL, is_banned boolean DEFAULT false NOT NULL, - other_names text, - other_names_index tsvector, + other_names text[] DEFAULT '{}'::text[], group_name character varying, created_at timestamp without time zone, updated_at timestamp without time zone @@ -5078,17 +5077,10 @@ CREATE INDEX index_artists_on_name_trgm ON public.artists USING gin (name public -- --- Name: index_artists_on_other_names_index; Type: INDEX; Schema: public; Owner: - +-- Name: index_artists_on_other_names; Type: INDEX; Schema: public; Owner: - -- -CREATE INDEX index_artists_on_other_names_index ON public.artists USING gin (other_names_index); - - --- --- Name: index_artists_on_other_names_trgm; Type: INDEX; Schema: public; Owner: - --- - -CREATE INDEX index_artists_on_other_names_trgm ON public.artists USING gin (other_names public.gin_trgm_ops); +CREATE INDEX index_artists_on_other_names ON public.artists USING gin (other_names); -- @@ -7345,13 +7337,6 @@ CREATE INDEX index_wiki_pages_on_updated_at ON public.wiki_pages USING btree (up CREATE TRIGGER insert_favorites_trigger BEFORE INSERT ON public.favorites FOR EACH ROW EXECUTE PROCEDURE public.favorites_insert_trigger(); --- --- Name: artists trigger_artists_on_update; Type: TRIGGER; Schema: public; Owner: - --- - -CREATE TRIGGER trigger_artists_on_update BEFORE INSERT OR UPDATE ON public.artists FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger('other_names_index', 'public.danbooru', 'other_names'); - - -- -- Name: comments trigger_comments_on_update; Type: TRIGGER; Schema: public; Owner: - -- @@ -7574,6 +7559,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20181108205842'), ('20181113174914'), ('20181114180205'), -('20181114185032'); +('20181114185032'), +('20181114202744'); diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index bd40c45ae..76117910d 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -407,8 +407,8 @@ class ArtistTest < ActiveSupport::TestCase end should "normalize its other names" do - artist = FactoryBot.create(:artist, :name => "a1", :other_names_comma => "aaa, bbb, ccc ddd") - assert_equal("aaa, bbb, ccc_ddd", artist.other_names_comma) + artist = FactoryBot.create(:artist, :name => "a1", :other_names => "aaa bbb ccc_ddd") + assert_equal("aaa bbb ccc_ddd", artist.other_names_string) end should "search on its name should return results" do @@ -421,11 +421,11 @@ class ArtistTest < ActiveSupport::TestCase end should "search on other names should return matches" do - artist = FactoryBot.create(:artist, :name => "artist", :other_names_comma => "aaa, ccc ddd") + artist = FactoryBot.create(:artist, :name => "artist", :other_names_string => "aaa ccc_ddd") - assert_nil(Artist.search(other_names_like: "*artist*").first) - assert_not_nil(Artist.search(other_names_like: "*aaa*").first) - assert_not_nil(Artist.search(other_names_like: "*ccc_ddd*").first) + assert_nil(Artist.search(any_other_name_like: "*artist*").first) + assert_not_nil(Artist.search(any_other_name_like: "*aaa*").first) + assert_not_nil(Artist.search(any_other_name_like: "*ccc_ddd*").first) assert_not_nil(Artist.search(name: "artist").first) assert_not_nil(Artist.search(:any_name_matches => "aaa").first) assert_not_nil(Artist.search(:any_name_matches => "/a/").first) @@ -478,7 +478,7 @@ class ArtistTest < ActiveSupport::TestCase assert_equal(%w[yyy], first_version.other_names) artist.revert_to!(first_version) artist.reload - assert_equal("yyy", artist.other_names) + assert_equal(%w[yyy], artist.other_names) end should "update the category of the tag when created" do