From 0dca538b0a2c1e465870ca7214d50c78aa235cfc Mon Sep 17 00:00:00 2001 From: Emi Matchu Date: Tue, 23 Jan 2024 05:10:43 -0800 Subject: [PATCH] Start migrating off `globalize` gem for species/color names Non-English languages haven't been supported on Neopets for a while, so I'd like to remove this extra cross-cutting complexity, especially since it's now inconsistent with the real site anyway! The main motivation is that I'd like to do this for items too, because I have a hunch that all the complexity of `globalize` to read `item.name` is part of what's making large user lists so slow to render: lots of little objects getting created down the stack, and needing to be garbage-collected later. I'm not sure that's why! But I figure removing this complexity is a simplicity win anyway, so let's do it! Note that this doesn't *finish* the migration, it just starts it! The `Species::Translation` and `Color::Translation` models still exist, and still have their data, and not all references to them are scrubbed yet. I especially don't want to delete the backing tables until both DTI and DTI 2020 are ready for it! So this change will someday be paired with another change to actually drop the tables - after backing up the data for future records just in case, of course! --- app/models/color.rb | 30 ++++++++----------- app/models/item/search/query.rb | 6 ++-- app/models/pet_type.rb | 6 ++-- app/models/species.rb | 20 ++++++------- ...123125509_add_name_to_species_and_color.rb | 16 ++++++++++ db/schema.rb | 4 ++- 6 files changed, 47 insertions(+), 35 deletions(-) create mode 100644 db/migrate/20240123125509_add_name_to_species_and_color.rb diff --git a/app/models/color.rb b/app/models/color.rb index d57dcedd..13575141 100644 --- a/app/models/color.rb +++ b/app/models/color.rb @@ -1,27 +1,23 @@ class Color < ApplicationRecord - translates :name + translates # TODO: Remove once we're all done with translations! has_many :pet_types - scope :alphabetical, -> { - ct = Color::Translation.arel_table - with_translations(I18n.locale).order(ct[:name].asc) - } - scope :basic, -> { where(:basic => true) } - scope :standard, -> { where(:standard => true) } - scope :nonstandard, -> { where(:standard => false) } + scope :alphabetical, -> { order(:name) } + scope :basic, -> { where(basic: true) } + scope :standard, -> { where(standard: true) } + scope :nonstandard, -> { where(standard: false) } scope :funny, -> { order(:prank) unless pranks_funny? } - scope :matching_name, ->(name, locale = I18n.locale) { - ct = Color::Translation.arel_table - joins(:translations).where(ct[:locale].eq(locale)). - where(ct[:name].matches(sanitize_sql_like(name))) - } validates :name, presence: true - # TODO: Should we consider replacing this at call sites? This used to be - # built into the globalize gem but isn't anymore! - def self.find_by_name(name) - matching_name(name).first + # Temporary writer to keep the English translation record updated, while + # primarily using the attribute on the model itself. + # + # Once this app and DTI 2020 are both comfortably off the translation system, + # we can remove this! + def name=(new_name) + globalize.write(:en, :name, new_name) + write_attribute(:name, new_name) end def as_json(options={}) diff --git a/app/models/item/search/query.rb b/app/models/item/search/query.rb index bf0045dd..4d0ed0e0 100644 --- a/app/models/item/search/query.rb +++ b/app/models/item/search/query.rb @@ -49,7 +49,7 @@ class Item when 'fits' color_name, species_name = value.split('-') begin - pet_type = PetType.matching_name(color_name, species_name, locale).first! + pet_type = PetType.matching_name(color_name, species_name).first! rescue ActiveRecord::RecordNotFound message = I18n.translate('items.search.errors.not_found.pet_type', name1: color_name.capitalize, name2: species_name.capitalize) @@ -60,8 +60,8 @@ class Item Filter.not_fits(pet_type.body_id, color_name, species_name)) when 'species' begin - species = Species.matching_name(value, locale).first! - color = Color.matching_name('blue', 'en').first! + species = Species.find_by_name!(value) + color = Color.find_by_name!('blue', 'en') pet_type = PetType.where(color_id: color.id, species_id: species.id).first! rescue ActiveRecord::RecordNotFound message = I18n.translate('items.search.errors.not_found.species', diff --git a/app/models/pet_type.rb b/app/models/pet_type.rb index 2d3cb48e..2ab62fc0 100644 --- a/app/models/pet_type.rb +++ b/app/models/pet_type.rb @@ -22,9 +22,9 @@ class PetType < ApplicationRecord scope :includes_child_translations, -> { includes({:color => :translations, :species => :translations}) } - scope :matching_name, ->(color_name, species_name, locale = I18n.locale) { - color = Color.matching_name(color_name, locale).first! - species = Species.matching_name(species_name, locale).first! + scope :matching_name, ->(color_name, species_name) { + color = Color.find_by_name!(color_name) + species = Species.find_by_name!(species_name) where(color_id: color.id, species_id: species.id) } diff --git a/app/models/species.rb b/app/models/species.rb index d20e1726..b38bd037 100644 --- a/app/models/species.rb +++ b/app/models/species.rb @@ -1,5 +1,5 @@ class Species < ApplicationRecord - translates :name + translates # TODO: Remove once we're all done with translations! has_many :pet_types scope :alphabetical, -> { @@ -7,21 +7,19 @@ class Species < ApplicationRecord with_translations(I18n.locale).order(st[:name].asc) } - scope :matching_name, ->(name, locale = I18n.locale) { - st = Species::Translation.arel_table - joins(:translations).where(st[:locale].eq(locale)). - where(st[:name].matches(sanitize_sql_like(name))) - } - scope :with_body_id, -> body_id { pt = PetType.arel_table joins(:pet_types).where(pt[:body_id].eq(body_id)).limit(1) } - # TODO: Should we consider replacing this at call sites? This used to be - # built into the globalize gem but isn't anymore! - def self.find_by_name(name) - matching_name(name).first + # Temporary writer to keep the English translation record updated, while + # primarily using the attribute on the model itself. + # + # Once this app and DTI 2020 are both comfortably off the translation system, + # we can remove this! + def name=(new_name) + globalize.write(:en, :name, new_name) + write_attribute(:name, new_name) end def as_json(options={}) diff --git a/db/migrate/20240123125509_add_name_to_species_and_color.rb b/db/migrate/20240123125509_add_name_to_species_and_color.rb new file mode 100644 index 00000000..d1c7a152 --- /dev/null +++ b/db/migrate/20240123125509_add_name_to_species_and_color.rb @@ -0,0 +1,16 @@ +class AddNameToSpeciesAndColor < ActiveRecord::Migration[7.1] + def change + add_column :species, :name, :string, null: false + add_column :colors, :name, :string, null: false + + Species.find_each do |species| + species.name = species.translation_for(:en).name + species.save! + end + + Color.find_each do |color| + color.name = color.translation_for(:en).name + color.save! + end + end +end diff --git a/db/schema.rb b/db/schema.rb index c2b7a6bc..9dfbc43f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_01_19_061745) do +ActiveRecord::Schema[7.1].define(version: 2024_01_23_125509) do create_table "auth_servers", id: :integer, charset: "latin1", collation: "latin1_swedish_ci", force: :cascade do |t| t.string "short_name", limit: 10, null: false t.string "name", limit: 40, null: false @@ -74,6 +74,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_01_19_061745) do t.boolean "basic" t.boolean "standard" t.boolean "prank", default: false, null: false + t.string "name", null: false end create_table "contributions", id: :integer, charset: "latin1", collation: "latin1_swedish_ci", force: :cascade do |t| @@ -233,6 +234,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_01_19_061745) do end create_table "species", id: :integer, charset: "latin1", collation: "latin1_swedish_ci", force: :cascade do |t| + t.string "name", null: false end create_table "species_translations", id: :integer, charset: "latin1", collation: "latin1_swedish_ci", force: :cascade do |t|