Remove default scopes; rewrite migrations to be more robust against model changes

dmolesUC · dmolesUC · commit 10d7795f9e2c · 2022-09-22T12:20:15.000-07:00
diff --git a/app/controllers/facets_controller.rb b/app/controllers/facets_controller.rb
@@ -3,7 +3,7 @@ class FacetsController < ApplicationController
 
   def index
     # TODO: Is there a way to do this with fewer SQL queries?
-    @facets = Facet.includes(terms: %i[parent children]).all
+    @facets = Facet.order(:ord).includes(terms: %i[parent children]).all
     render jsonapi: @facets
   end
 end
diff --git a/app/controllers/items_controller.rb b/app/controllers/items_controller.rb
@@ -93,6 +93,9 @@ def filtered_items
       items = items.where(suppressed: false)
       items = items.with_all_keywords(keywords) if keywords
       items.includes(:terms)
+      # TODO: add sorting to API
+      # TODO: allow sorting by facet values
+      items.order(:artist, :title, :date)
     end
   end
 end
diff --git a/app/models/facet.rb b/app/models/facet.rb
@@ -11,9 +11,4 @@ class Facet < ApplicationRecord
 
   validates :name, presence: true
   validates :name, uniqueness: true
-
-  # ------------------------------------------------------------
-  # Scopes
-
-  default_scope { order(:ord) }
 end
diff --git a/app/models/item.rb b/app/models/item.rb
@@ -30,10 +30,6 @@ class Item < ApplicationRecord
   # ------------------------------------------------------------
   # Scopes
 
-  # TODO: add sorting to API
-  # TODO: allow sorting by facet values
-  default_scope { order(:artist, :title, :date) }
-
   pg_search_scope(
     :with_all_keywords,
     against: { title: 'A', artist: 'B', description: 'C', date: 'D' },
diff --git a/app/models/term.rb b/app/models/term.rb
@@ -19,8 +19,6 @@ class Term < ApplicationRecord
   # ------------------------------------------------------------
   # Scopes
 
-  default_scope { order(:facet_id, :parent_id, :ord, :value) }
-
   scope :find_by_self_or_parent, ->(**conditions) {
     sql = <<~SQL.squish
       INNER JOIN (#{Term.where(**conditions).to_sql})#{' '}
diff --git a/app/serializers/facet_serializer.rb b/app/serializers/facet_serializer.rb
@@ -3,5 +3,9 @@ class FacetSerializer
 
   attributes(:name)
 
-  has_many :terms
+  has_many :terms do |facet|
+    # NOTE: We don't do this with a default_scope on Term because that
+    #       will break migrations written before :ord was added
+    facet.terms.order(:ord, :value)
+  end
 end
diff --git a/config/environments/production.rb b/config/environments/production.rb
@@ -86,8 +86,8 @@
   }
   config.action_mailer.smtp_settings = smtp_settings
   config.after_initialize do
-    settings_cleaned = smtp_settings.merge(password: mail_smtp_password.gsub(/./, '*'))
-    Rails.logger.info('SMTP configured', smtp_settings: settings_cleaned)
+    settings_cleaned = smtp_settings.merge(password: mail_smtp_password&.gsub(/./, '*'))
+    Rails.logger.info('SMTP configured', smtp_settings: settings_cleaned.inspect)
   end
 
   if ENV['INTERCEPT_EMAILS'].present?
diff --git a/db/migrate/20220311190417_populate_items_table.rb b/db/migrate/20220311190417_populate_items_table.rb
@@ -1,6 +1,3 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Item < ActiveRecord::Base; end) unless defined?(Item)
-
 class PopulateItemsTable < ActiveRecord::Migration[7.0]
   DATA = [
     { id: 1, image: 'anonymous (apples).jpg', image_base: 'anonymous (apples)', title: 'Apples', artist: 'Unknown', artist_url: nil, date: '1876-1885', decade: 'Before 1900', description: '(Plate xxix) Published by H. Bull & R. Hogg for the Herefordshire Pomona, London.', medium: 'Lithograph', colors: 'Color', genre: 'Still Life', dimensions: '16.5 x 20.5"', size: 'Small', series: nil, bib_number: 'b16390480', mms_id: '991051103819706532', barcode: 'C093330688', circulation: nil, location: nil, value: '100', appraisal_date: '2006', notes: nil, reserve_date: '2019-08-29' },
diff --git a/db/migrate/20220406202724_drop_image_base.rb b/db/migrate/20220406202724_drop_image_base.rb
@@ -1,13 +1,12 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Item < ActiveRecord::Base; end) unless defined?(Item)
-
 class DropImageBase < ActiveRecord::Migration[7.0]
   def up
     remove_column(:items, :image_base)
   end
 
   def down
     add_column(:items, :image_base, :string)
+    Item.reset_column_information
+
     Item.update_all("image_base = REGEXP_REPLACE(image, '\\.jpg$', '', 'i')")
   end
 end
diff --git a/db/migrate/20220406203912_drop_bib_number.rb b/db/migrate/20220406203912_drop_bib_number.rb
@@ -5,6 +5,7 @@ def up
 
   def down
     add_column(:items, :bib_number, :string)
+    Item.reset_column_information
 
     stmt = <<~SQL.strip
       UPDATE items
diff --git a/db/migrate/20220414191138_populate_facets.rb b/db/migrate/20220414191138_populate_facets.rb
@@ -1,8 +1,3 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Facet < ActiveRecord::Base; end) unless defined?(Facet)
-(class Term < ActiveRecord::Base; end) unless defined?(Term)
-(class ItemsTerm < ActiveRecord::Base; end) unless defined?(ItemsTerm)
-
 # noinspection RubyLiteralArrayInspection
 class PopulateFacets < ActiveRecord::Migration[7.0]
 
diff --git a/db/migrate/20220415173214_drop_facet_attributes.rb b/db/migrate/20220415173214_drop_facet_attributes.rb
@@ -9,6 +9,7 @@ def down
     add_column :items, :decade, :string
     add_column :items, :genre, :string
     add_column :items, :medium, :string
+    Item.reset_column_information
 
     facets_by_id = Facet.eager_load(:terms).each_with_object({}) do |f, ff|
       ff[f.id] = f
diff --git a/db/migrate/20220531202541_rename_colors_facet.rb b/db/migrate/20220531202541_rename_colors_facet.rb
@@ -1,6 +1,3 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Facet < ActiveRecord::Base; end) unless defined?(Facet)
-
 class RenameColorsFacet < ActiveRecord::Migration[7.0]
 
   FROM_NAME, TO_NAME = %w[Colors Appearance]
diff --git a/db/migrate/20220609204903_add_thumbnail_to_items.rb b/db/migrate/20220609204903_add_thumbnail_to_items.rb
@@ -1,9 +1,7 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Item < ActiveRecord::Base; end) unless defined?(Item)
-
 class AddThumbnailToItems < ActiveRecord::Migration[7.0]
   def change
     add_column :items, :thumbnail, :string, null: :false
+    Item.reset_column_information
 
     Item.update_all("thumbnail = REGEXP_REPLACE(image, '(?i)\\.jpg$', '_360px.jpg')")
   end
diff --git a/db/migrate/20220809165539_drop_duplicate_item.rb b/db/migrate/20220809165539_drop_duplicate_item.rb
@@ -1,8 +1,3 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Facet < ActiveRecord::Base; end) unless defined?(Facet)
-(class Item < ActiveRecord::Base; end) unless defined?(Item)
-(class Term < ActiveRecord::Base; end) unless defined?(Term)
-
 class DropDuplicateItem < ActiveRecord::Migration[7.0]
 
   ITEM_ATTRS = {
diff --git a/db/migrate/20220809173101_fix_incorrect_mms_id.rb b/db/migrate/20220809173101_fix_incorrect_mms_id.rb
@@ -1,6 +1,3 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Item < ActiveRecord::Base; end) unless defined?(Item)
-
 class FixIncorrectMMSId < ActiveRecord::Migration[7.0]
 
   OLD_ID = '991051112729706532'
diff --git a/db/migrate/20220809180904_add_suppressed_to_items.rb b/db/migrate/20220809180904_add_suppressed_to_items.rb
@@ -1,5 +1,6 @@
 class AddSuppressedToItems < ActiveRecord::Migration[7.0]
   def change
     add_column :items, :suppressed, :boolean, default: false, null: false
+    Item.reset_column_information
   end
 end
diff --git a/db/migrate/20220810184802_drop_test_item.rb b/db/migrate/20220810184802_drop_test_item.rb
@@ -1,8 +1,3 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Facet < ActiveRecord::Base; end) unless defined?(Facet)
-(class Item < ActiveRecord::Base; end) unless defined?(Item)
-(class Term < ActiveRecord::Base; end) unless defined?(Term)
-
 class DropTestItem < ActiveRecord::Migration[7.0]
 
   ITEM_ATTRS = {
diff --git a/db/migrate/20220908164857_suppress_additional_items.rb b/db/migrate/20220908164857_suppress_additional_items.rb
@@ -1,6 +1,3 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Item < ActiveRecord::Base; end) unless defined?(Item)
-
 class SuppressAdditionalItems < ActiveRecord::Migration[7.0]
 
   ITEM_ATTRS = [
diff --git a/db/migrate/20220914235036_add_risograph_to_medium.rb b/db/migrate/20220914235036_add_risograph_to_medium.rb
@@ -1,7 +1,3 @@
-# Ensure migration can run without error even if we delete/rename the models
-(class Facet < ActiveRecord::Base; end) unless defined?(Facet)
-(class Term < ActiveRecord::Base; end) unless defined?(Term)
-
 class AddRisographToMedium < ActiveRecord::Migration[7.0]
   def up
     return if Rails.env.test?
diff --git a/db/migrate/20220920203301_drop_incomplete_items.rb b/db/migrate/20220920203301_drop_incomplete_items.rb
@@ -1,7 +1,3 @@
-# Ensure migration can run without error even if we delete/rename the models
-(
-  class Item < ActiveRecord::Base; end) unless defined?(Item)
-
 class DropIncompleteItems < ActiveRecord::Migration[7.0]
 
   DATA = [
diff --git a/db/migrate/20220921205344_support_ordered_terms.rb b/db/migrate/20220921205344_support_ordered_terms.rb
@@ -24,27 +24,46 @@ class SupportOrderedTerms < ActiveRecord::Migration[7.0]
     ].freeze
   }.freeze
 
-  def change
+  def up
     add_column(:facets, :ordered, :boolean, default: false, null: false)
     add_column(:terms, :ord, :integer)
+    Facet.reset_column_information
+    Term.reset_column_information
 
     set_order!
 
     add_index(:terms, [:facet_id, :ord], unique: true)
   end
 
+  def down
+    remove_column(:terms, :ord)
+    remove_column(:facets, :ordered)
+  end
+
   private
 
   def set_order!
-    return if Rails.env.test?
+    sql = 'UPDATE facets SET ordered = true where facets.name IN (?)'
+    stmt = ActiveRecord::Base.sanitize_sql([sql, ORDERED_FACET_VALUES.keys])
+    exec_update(stmt)
 
     ORDERED_FACET_VALUES.each do |facet_name, values|
-      facet = Facet.find_by!(name: facet_name)
-      facet.update(ordered: true)
-
-      values.each_with_index do |value, i|
-        term = Term.find_by!(facet: facet, value: value)
-        term.update(ord: i)
+      values.each_with_index do |value, ord|
+        sql = <<~SQL
+          UPDATE terms
+             SET ord = :ord
+            FROM facets
+           WHERE facets.id = terms.facet_id
+             AND facets.name = :facet_name
+             AND terms.value = :value
+        SQL
+        stmt = ActiveRecord::Base.sanitize_sql(
+          [
+            sql,
+            { ord: ord, facet_name: facet_name, value: value}
+          ]
+        )
+        exec_update(stmt)
       end
     end
   end
diff --git a/db/migrate/20220921220637_support_ordered_facets.rb b/db/migrate/20220921220637_support_ordered_facets.rb
@@ -7,20 +7,25 @@ class SupportOrderedFacets < ActiveRecord::Migration[7.0]
     Appearance
   ].freeze
 
-  def change
+  def up
     add_column(:facets, :ord, :integer, default: 0, null: false)
+    Facet.reset_column_information
+
     set_order!
     add_index(:facets, :ord, unique: true)
   end
 
+  def down
+    remove_column(:facets, :ord)
+  end
+
   private
 
   def set_order!
-    return if Rails.env.test?
-
-    ORDERED_FACETS.each_with_index do |name, index|
-      facet = Facet.find_by!(name: name)
-      facet.update(ord: index)
+    ORDERED_FACETS.each_with_index do |name, ord|
+      sql = 'UPDATE facets SET ord = :ord WHERE name = :name'
+      stmt = ActiveRecord::Base.sanitize_sql([sql, { ord: ord, name: name }])
+      exec_update(stmt)
     end
   end
 end
diff --git a/spec/serializers/facet_serializer_spec.rb b/spec/serializers/facet_serializer_spec.rb
@@ -26,12 +26,25 @@
     expect(actual_hash).to deep_eq_hash(expected_hash)
   end
 
-  it 'can include the terms' do
+  it 'sorts included terms by value' do
     facet = create(:facet_medium)
     serializer = FacetSerializer.new(facet, { include: [:terms] })
     actual_hash = serializer.serializable_hash
 
-    expected_includes = facet.terms.map { |t| TermSerializer.new(t).serializable_hash[:data] }
+    expected_includes = facet.terms.order(:value).map { |t| TermSerializer.new(t).serializable_hash[:data] }
+    actual_includes = actual_hash[:included]
+    expect(actual_includes.size).to eq(expected_includes.size)
+    expected_includes.each_with_index do |incl, i|
+      expect(actual_includes[i]).to deep_eq_hash(incl)
+    end
+  end
+
+  it 'sorts included terms by ord if present' do
+    facet = create(:facet_decade)
+
+    serializer = FacetSerializer.new(facet, { include: [:terms] })
+    actual_hash = serializer.serializable_hash
+    expected_includes = facet.terms.order(:ord, :value).map { |t| TermSerializer.new(t).serializable_hash[:data] }
     actual_includes = actual_hash[:included]
     expect(actual_includes.size).to eq(expected_includes.size)
     expected_includes.each_with_index do |incl, i|
