From 5830b3f00ef4dc8ace67b5578cf77564296a10a2 Mon Sep 17 00:00:00 2001 From: Jay Varner Date: Thu, 4 Jun 2026 08:48:22 -0400 Subject: [PATCH 1/9] Finish work refactoring to use Elasticsearch 8 --- Gemfile | 2 +- Gemfile.lock | 34 +++++++++++++----------- README.md | 28 +++++++++---------- app/controllers/letters_controller.rb | 2 +- app/models/medium.rb | 1 - app/workers/resave_all_letters_worker.rb | 10 +++++++ config/credentials.yml.enc | 2 +- config/database.yml | 2 ++ config/initializers/elasticsearch.rb | 2 +- db/schema.rb | 11 -------- spec/models/medium_spec.rb | 12 +++++++++ spec/rails_helper.rb | 6 ++++- 12 files changed, 64 insertions(+), 48 deletions(-) create mode 100644 spec/models/medium_spec.rb diff --git a/Gemfile b/Gemfile index 0b1780e..f186362 100644 --- a/Gemfile +++ b/Gemfile @@ -14,7 +14,7 @@ gem 'csv' gem 'pg', '~> 1.1' # Elasticseach for search -gem 'elasticsearch', '~> 7.17.1' +gem 'elasticsearch', '~> 8' gem 'searchkick' # Use the Puma web server [https://github.com/puma/puma] diff --git a/Gemfile.lock b/Gemfile.lock index 26bc5af..dcb5142 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -117,14 +117,14 @@ GEM reline (>= 0.3.8) diff-lcs (1.5.1) drb (2.2.1) - elasticsearch (7.17.11) - elasticsearch-api (= 7.17.11) - elasticsearch-transport (= 7.17.11) - elasticsearch-api (7.17.11) + elastic-transport (8.5.2) + faraday (< 3) multi_json - elasticsearch-transport (7.17.11) - base64 - faraday (>= 1, < 3) + elasticsearch (8.19.3) + elastic-transport (~> 8.3) + elasticsearch-api (= 8.19.3) + ostruct + elasticsearch-api (8.19.3) multi_json erubi (1.13.0) et-orbi (1.4.0) @@ -136,11 +136,12 @@ GEM railties (>= 5.0.0) faker (3.4.2) i18n (>= 1.8.11, < 2) - faraday (2.11.0) - faraday-net_http (>= 2.0, < 3.4) + faraday (2.14.2) + faraday-net_http (>= 2.0, < 3.5) + json logger - faraday-net_http (3.3.0) - net-http + faraday-net_http (3.4.4) + net-http (~> 0.5) ffi (1.17.0-aarch64-linux-gnu) ffi (1.17.0-aarch64-linux-musl) ffi (1.17.0-arm-linux-gnu) @@ -213,14 +214,14 @@ GEM mini_mime (1.1.5) minitest (5.25.1) msgpack (1.7.2) - multi_json (1.15.0) + multi_json (1.21.1) multi_xml (0.7.1) bigdecimal (~> 3.1) mustache (1.1.1) namae (1.2.0) racc (~> 1.7) - net-http (0.4.1) - uri + net-http (0.9.1) + uri (>= 0.11.1) net-imap (0.4.14) date net-protocol @@ -243,6 +244,7 @@ GEM racc (~> 1.4) nokogiri (1.16.7-x86_64-linux) racc (~> 1.4) + ostruct (0.6.3) parallel (1.26.3) parser (3.3.4.2) ast (~> 2.4.1) @@ -410,7 +412,7 @@ GEM concurrent-ruby (~> 1.0) unicode (0.4.4.5) unicode-display_width (2.5.0) - uri (0.13.0) + uri (1.1.1) useragent (0.16.10) uuid (2.3.9) macaddr (~> 1.0) @@ -447,7 +449,7 @@ DEPENDENCIES bootsnap csv debug - elasticsearch (~> 7.17.1) + elasticsearch (~> 8) factory_bot_rails faker httparty (~> 0.20.0) diff --git a/README.md b/README.md index 678c5be..17808ba 100644 --- a/README.md +++ b/README.md @@ -10,43 +10,41 @@ ## System dependencies -* Elasticsearch -* PostgreSQL +- Elasticsearch +- PostgreSQL ## Database creation -~~~bash +```bash rake db:create && rake db:migrate -~~~ +``` ## Update Elasticsearch Indices -~~~bash +```bash rake searchkick:reindex:all -~~~ +``` ## Run the test suite -~~~bash +```bash bundle exec rspec spec/ -~~~ +``` ## Build Documentation -~~~bash +```bash rake docs:generate -~~~ +``` ## Background Jobs Restart Active Jobs for indexing and Big Sam update. -~~~bash +```bash sudo service sidekiq-1 restart && sudo service sidekiq-2 restart -~~~ +``` ## Deployment Instructions -~~~bash -bundle exec cap production deploy -~~~ +Handled via GitHub Actions. diff --git a/app/controllers/letters_controller.rb b/app/controllers/letters_controller.rb index 10670d5..94434e9 100644 --- a/app/controllers/letters_controller.rb +++ b/app/controllers/letters_controller.rb @@ -78,7 +78,7 @@ def facets date: { date_histogram: { field: :date, - interval: :year + calendar_interval: :year } }, languages: {}, diff --git a/app/models/medium.rb b/app/models/medium.rb index 5c283a8..de1645a 100644 --- a/app/models/medium.rb +++ b/app/models/medium.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class Medium < ApplicationRecord - ActiveStorage::Current.url_options = { host: ENV.fetch('RAILS_HOST', 'localhost:3000') } before_save :populate_filename belongs_to :entity diff --git a/app/workers/resave_all_letters_worker.rb b/app/workers/resave_all_letters_worker.rb index 7f4dbac..a84ef58 100644 --- a/app/workers/resave_all_letters_worker.rb +++ b/app/workers/resave_all_letters_worker.rb @@ -7,5 +7,15 @@ def perform rescue StandardError => e Rails.logger.error("Failed to resave Letter ##{record.id}: #{e.message}") end + PublishedLetter.find_each do |record| + record.reindex + rescue StandardError => e + Rails.logger.error("Failed to reindex Letter ##{record.id}: #{e.message}") + end + PublishedEntity.find_each do |record| + record.reindex + rescue StandardError => e + Rails.logger.error("Failed to reindex Entity ##{record.id}: #{e.message}") + end end end diff --git a/config/credentials.yml.enc b/config/credentials.yml.enc index 86922f3..43c6639 100644 --- a/config/credentials.yml.enc +++ b/config/credentials.yml.enc @@ -1 +1 @@ -jYJz5oYEMoYW25VCz8N3WLIcn0VvVNmZUWLMPbqL6f5Ldx1H+lADPLDA8rUHU8rLkfpiAdxPTC976QObW+32KbGlg7pZkPZTkI4KF6qwexpMQW7WJTAHQbUNWh2vv8zpwAmYoHCn0CMtDi/TEo8KAnqgTYI3cIivwUY2O7eA+i1Uki3FvIDTGjU/Wq6ZFRpR4PNciguop/N8IAqEk4Tyy5PfRJrkVa+BfVOZuqbP2FLwKpJ942KmsaY4gjNpBzxx3lyo/5bTNm37HmTD7NpbnEvi3gEaXroOFBIZ1/iazqfE540svsrvsMjLBaFRy+6Msiinp+JAqSsbyUfVTILCJZ7CDLc0tluO/scPS26Ub9KiPLGPAzYSdsJw9AXHXE3/xWDQ3PlNyQjTWxtJf5gqL4uGegw2h4gRCJ8vfOglxzG/UJgqRv68mtlUUudvFCKKghBVmQH2nmFMZaVpH4FqiuBOuzoc7TdoZyisxcjgcSy6pZzxT4q1IAGHYq2kTr7goaVZ1YLQlDxUtQXoifAWIdkiHFGOtuH4anokc5VqRevI6vJLVxnDK0QK1D+35AryEk8oBGlYXT7XdCBn6CGZhwQQ5fgkEkQyl9gEwywpOIeyt2rpvPU8zcIUXKANBiK9hofX8L0I200RV1G/9eK+RDHZILY5zJSqlgi4YAXaKfffj2/EYZzUcvvyNV3HK3fKFxdjarBSOhL9ZhMOcd+jkiEphFYRXCi/s9/j4rjrFk7K6GgcM5T0T50OH+PP2dpu3EPLE/OtSC8witFhR6qRDL86HiSr0nvhtIKjT2qsjuMH1IjrA7AAtjnG6vEAlQJEvNdB5UVlmXXlO4jP+apDqTuKBJSvhdynDGJQ6gJFtrcwp74B1CJ/cBongbwGs2066dXHiWQrCPV5EFJrd7GlR3npVQ4Gn8/mc+TrmkfYzXRMj7DewTm6z8D2m5cGTwyV1Jm82Ewv2pkmJrN5ejgQDL/rsFaXb9rmb6vn9n/fKFikB0vna7TM0RL0tEubV7KMJLizPRLWIExfsEXkG1xZPsVtcPzX4ovPbhHok0jHELN0QgbgIrZgNMxMUM/Bw8MN4P+g0/LBbeHcTWwLV/m1C+q2Az2AYzkRFIqMMdOfF56uhU59d90xR7GrZNoBkGooaKDXfcHZYErr/Rm548ugJ0my6AjLzvbZbcsyKmugixcKsIZAZZaU8RsaYbk=--pMFHUBYrXW2JGSeM--RmrKvB7nmyL1yurk5Dnt7g== \ No newline at end of file +9ujcx5F7bldLRuluMmyCSL1tHxXBiYfvl2GW7mANv9xYqnTy3vmwwdVgAvHm6HXldYlfw9TeBLgTp0hJsC4Ll8OndlS00o/AVoDDD/GlE3+7dojRGX1XhHeMsnCoq8VsW7CnjUzeczuNW9M52Mwv9kpaADUEzijN9e2UgTsg3ccoKxdUdLoidalhhYpjm92OLEhXqUsjDPZGJsl5FTC88m5XYINj6aTOroHe+KW5zuLVXB+VMAI9BfaPip4tcDkTbQ2Azoz70NdDcwFdrOcC0BDbNm2jFm0t/rv4/D1emAevIR54iGYGxl3om7WI9++Tmg1ls1niTMBnS5coFxk7ymBB82XqXa259YG98nPYaFTUDl29kPLnQ4jLc+NwYm98Xc6AGYxoKm1YJXRJCGvIcUNtszoM3lLPsMJ8xk4r9kvqaIHGwbjhlOgSallxcqgktFN4UM4lB1MU2rlfDJyrcbfP4jCEJ3D5CoqtnoInQH7R9eKR20EjwyWXdjaPubKelmHJ1P/afu0LCjO/FUXobGzQpt+SDzudanktAs0RWap5HY3SUfXVmhHSQsPrkSd/VUHxajEnnxi9/ZUUQ+LlRlIBKfCCm3xTEMaweqf4McYc3FEph2Zc5RGMqgQShtPae2WOOWQL14LtHN2SfVWYw0bsEx4NAptRr08utju940MGOwVR3hc1X593Rx2cowh73lVSsLGl/2+D+2sw8VF1BqjGiGZfibcx63jLAHsr+UXcIWLO4U/Qjnx3LAA/8TzTGyz0qi7FoM50jMcHS//ADp/kc/Cby3SlKLULKyAaBiy6p/pbbxwhcWGibTID1MBSyIIQwIePvgPVBIE1zsFwf37aOSDkU1WoDjQ5v5++TDOzuFi6UE8IKEQ/4zuhcRTZ/0Y7/vnIhm64zSYIbCa6zhLo5MxlJcAJ76mDoD5bY61gowl/NQWpoRWbHjuGkzQp1o5AyoO3maGISD6p/UR8rFGoVKEiEnYsgoMAruQjThR4nXsfu/fRq4MpjpY1xAqVhbxS2C/bFk6t+a+fEZ4FUztHUF3/zVfjUjDm+VTMaeGigExywPOWfR3slTuxxUBpbrbHI60i+WHARddQusYnxSafNLmMA5zkNaYuf40PSdrJFZTIJzgywu+e4ufkXAyxexl1Uj8mqY174ZhgBWET37nCgOiGlY+YkLjyHvPMtkfySqYrCeNoXOHW2Sc=--iuIZmOw0ZvIZHWQH--4bmfV6LixPptJT8k7SBRlg== \ No newline at end of file diff --git a/config/database.yml b/config/database.yml index 9cb477a..eb3132e 100644 --- a/config/database.yml +++ b/config/database.yml @@ -11,6 +11,8 @@ default: &default test: <<: *default database: <%= ENV['TEST_DB_NAME'] || 'beckett_test' %> + username: <%= Rails.application.credentials.dig(:test, :db_user) %> + password: <%= Rails.application.credentials.dig(:test, :db_pw) %> development: <<: *default diff --git a/config/initializers/elasticsearch.rb b/config/initializers/elasticsearch.rb index 58117e3..2343c9d 100644 --- a/config/initializers/elasticsearch.rb +++ b/config/initializers/elasticsearch.rb @@ -6,7 +6,7 @@ Searchkick.client = if ENV.fetch('RAILS_ENV', nil) == 'production' Elasticsearch::Client.new( - host: 'https://search.ecds.io', + host: 'https://search.ecdsdev.org', api_key: Rails.application.credentials.dig(:production, :es_api_key), transport_options: { request: { timeout: }, headers: { content_type: 'application/json' } }, retry_on_failure: 2 diff --git a/db/schema.rb b/db/schema.rb index 4448a2b..aed3712 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -140,17 +140,6 @@ t.datetime "updated_at", null: false end - create_table "images", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| - t.text "alt_text" - t.text "caption" - t.text "attribution" - t.text "link" - t.uuid "entity_id" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["entity_id"], name: "index_images_on_entity_id" - end - create_table "languages", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "label" t.string "code" diff --git a/spec/models/medium_spec.rb b/spec/models/medium_spec.rb new file mode 100644 index 0000000..093d484 --- /dev/null +++ b/spec/models/medium_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Medium do + it 'has file attached' do + medium = build(:medium) + medium.save + expect(medium.image.attached?).to be(true) + expect(medium.filename).to eq('beckett.png') + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 7c1f815..a9bc56b 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -44,9 +44,13 @@ Rails.root.glob('spec/support/**/*.rb').each {|f| require f } ENV['RAILS_HOST'] = 'example.com' -ActiveStorage::Current.url_options = { host: ENV.fetch('RAILS_HOST', 'localhost:3000') } RSpec.configure do |config| + # ActiveStorage::Current is a CurrentAttributes subclass — its values are + # reset by ActiveSupport::Executor after each inline job. Setting url_options + # in before(:each) ensures it's present whenever a spec calls .url. + config.before { ActiveStorage::Current.url_options = { host: ENV['RAILS_HOST'] } } + # Remove this line if you're not using ActiveRecord or ActiveRecord fixtures config.fixture_paths = [Rails.root.join('spec', 'fixtures'), Rails.root.join('spec', 'fixtures', 'files')] From db4694d6e7342f09fb8fc6908296579d700f86f9 Mon Sep 17 00:00:00 2001 From: Jay Varner Date: Thu, 4 Jun 2026 09:38:15 -0400 Subject: [PATCH 2/9] Fix test database settings --- .github/workflows/test.yml | 6 +++--- config/database.yml | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cf631df..8544b80 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,9 +13,9 @@ jobs: services: elasticsearch: - image: docker.elastic.co/elasticsearch/elasticsearch:7.9.2 + image: docker.elastic.co/elasticsearch/elasticsearch:8.13.4 env: - STACK_VERSION: 7.17.1 + STACK_VERSION: 8.13.4 xpack.security.enabled: false cluster.name: beckett-elasticsearch http.port: 9200 @@ -29,7 +29,7 @@ jobs: - 9200:9200 postgres: - image: postgres:10 + image: postgres:16 env: POSTGRES_PASSWORD: password POSTGRES_USER: user diff --git a/config/database.yml b/config/database.yml index eb3132e..2c55f60 100644 --- a/config/database.yml +++ b/config/database.yml @@ -11,8 +11,8 @@ default: &default test: <<: *default database: <%= ENV['TEST_DB_NAME'] || 'beckett_test' %> - username: <%= Rails.application.credentials.dig(:test, :db_user) %> - password: <%= Rails.application.credentials.dig(:test, :db_pw) %> + username: <%= ENV['DB_NAME'] || Rails.application.credentials.dig(:test, :db_user) %> + password: <%= ENV['DB_PASSWORD'] || Rails.application.credentials.dig(:test, :db_pw) %> development: <<: *default From 44cef38f0793e2281c6f9522c77bf6cec75da379 Mon Sep 17 00:00:00 2001 From: Jay Varner Date: Thu, 4 Jun 2026 09:47:08 -0400 Subject: [PATCH 3/9] Fix DB_USERNAME in config --- config/database.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/database.yml b/config/database.yml index 2c55f60..2fc6d3e 100644 --- a/config/database.yml +++ b/config/database.yml @@ -11,7 +11,7 @@ default: &default test: <<: *default database: <%= ENV['TEST_DB_NAME'] || 'beckett_test' %> - username: <%= ENV['DB_NAME'] || Rails.application.credentials.dig(:test, :db_user) %> + username: <%= ENV['DB_USERNAME'] || Rails.application.credentials.dig(:test, :db_user) %> password: <%= ENV['DB_PASSWORD'] || Rails.application.credentials.dig(:test, :db_pw) %> development: From d83fe61369052a99b290e5768ed893cbe7346690 Mon Sep 17 00:00:00 2001 From: Jay Varner Date: Fri, 7 Aug 2026 11:13:31 -0400 Subject: [PATCH 4/9] Big Sam job improvements --- app/jobs/load_big_sam_job.rb | 62 ++++++++++++++++++++++++------------ app/models/big_sam.rb | 3 +- 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/app/jobs/load_big_sam_job.rb b/app/jobs/load_big_sam_job.rb index 14174db..856cf82 100644 --- a/app/jobs/load_big_sam_job.rb +++ b/app/jobs/load_big_sam_job.rb @@ -43,13 +43,13 @@ def load_letters(rows) leaves: row[:leaves].to_i, sides: row[:sides], postmark: row[:postmark_actual], - notes: row[:dditional], - letter_owner: LetterOwner.find_or_create_by(label: row[:ownerrights]), - file_folder: FileFolder.find_or_create_by(label: row[:file]), + notes: row[:additional], + letter_owner: find_or_create_by_label(LetterOwner, row[:ownerrights]), + file_folder: find_or_create_by_label(FileFolder, row[:file]), typed: row[:autograph_or_typed] == 'T', signed: row[:initialed_or_signed] == 'S', envelope: row[:envelope] == 'E', - verified: row[:verified] == 'Y' + verified: row[:verified].to_s.strip.downcase == 'y' } letter.origins.clear @@ -170,11 +170,11 @@ def load_letters(rows) end # rubocop:disable Style/SoleNestedConditional - if row[:first_repository] - repository = Repository.find_or_initialize_by(label: row[:first_repository]) + if row[:first_repository].present? + repository = find_or_initialize_by_label(Repository, row[:first_repository]) if repository.new_record? - repository.published = row[:first_public].downcase == 'public' if row[:first_public] + repository.published = row[:first_public].to_s.strip.downcase == 'public' if row[:first_public] end repository.save @@ -184,8 +184,8 @@ def load_letters(rows) collection = nil begin - if row[:first_collection] - collection = Collection.find_or_create_by(label: row[:first_collection]) + if row[:first_collection].present? + collection = find_or_create_by_label(Collection, row[:first_collection]) collection.update(url: row[:collection_url]) repository.collections << collection unless repository.collections.include?(collection) @@ -210,11 +210,11 @@ def load_letters(rows) end end - if row[:second_repository] - repository = Repository.find_or_initialize_by(label: row[:second_repository]) + if row[:second_repository].present? + repository = find_or_initialize_by_label(Repository, row[:second_repository]) if repository.new_record? - repository.published = row[:second_public].downcase == 'public' if row[:second_public] + repository.published = row[:second_public].to_s.strip.downcase == 'public' if row[:second_public] end repository.format = row[:second_format] @@ -222,8 +222,8 @@ def load_letters(rows) collection = nil begin - if row[:second_collection] - collection = Collection.find_or_create_by(label: row[:second_collection]) + if row[:second_collection].present? + collection = find_or_create_by_label(Collection, row[:second_collection]) repository.collections << collection unless repository.collections.include?(collection) @@ -246,11 +246,11 @@ def load_letters(rows) end end - if row[:third_repository] - repository = Repository.find_or_initialize_by(label: row[:third_repository]) + if row[:third_repository].present? + repository = find_or_initialize_by_label(Repository, row[:third_repository]) if repository.new_record? - repository.published = row[:third_public].downcase == 'public' if row[:third_public] + repository.published = row[:third_public].to_s.strip.downcase == 'public' if row[:third_public] end repository.format = row[:third_format] @@ -258,8 +258,8 @@ def load_letters(rows) collection = nil begin - if row[:second_collection] - collection = Collection.find_or_create_by(label: row[:second_collection]) + if row[:third_collection].present? + collection = find_or_create_by_label(Collection, row[:third_collection]) repository.collections << collection unless repository.collections.include?(collection) @@ -293,7 +293,9 @@ def load_letters(rows) letter.volume_pages = ActionController::Base.helpers.strip_tags(parts[2].strip) if parts.length == 3 end - letter.letter_publisher = LetterPublisher.find_or_create_by(label: row[:placeprevpubl]) if row[:placeprevpubl] + if row[:placeprevpubl].present? + letter.letter_publisher = find_or_create_by_label(LetterPublisher, row[:placeprevpubl]) + end row[:sender]&.split(';')&.each do |sender| entity = get_person(sender) @@ -326,7 +328,7 @@ def load_letters(rows) end def get_letter(row) - if row[:exclude] == 'y' + if row[:exclude].to_s.strip.downcase == 'y' letter = Letter.find_by(legacy_pk: row[:id]) letter&.destroy return nil @@ -335,6 +337,24 @@ def get_letter(row) Letter.find_or_create_by(legacy_pk: row[:id]) end + def normalize_label(value) + value.to_s.strip.squeeze(' ') + end + + def find_or_create_by_label(klass, label) + clean = normalize_label(label) + return nil if clean.blank? + + klass.find_by('lower(label) = ?', clean.downcase) || klass.create(label: clean) + end + + def find_or_initialize_by_label(klass, label) + clean = normalize_label(label) + return nil if clean.blank? + + klass.find_by('lower(label) = ?', clean.downcase) || klass.new(label: clean) + end + def get_entity(label: nil, type: nil, return_nil: false) logger.error("Get Entity with label: #{label} of type #{type}") label = label.strip.gsub(/[\[!@%&?"\]]/, '').titleize diff --git a/app/models/big_sam.rb b/app/models/big_sam.rb index d1a1054..5491f29 100644 --- a/app/models/big_sam.rb +++ b/app/models/big_sam.rb @@ -13,8 +13,7 @@ def local_path private def load_letters - LoadBigSamJob.perform_later self unless ENV['RAILS_ENV'] == 'test' - LoadBigSamJob.perform_now self + LoadBigSamJob.perform_later self end def delete_file From 9313238c72d0e76e7c81ea41c7d8575974b29f69 Mon Sep 17 00:00:00 2001 From: Jay Varner Date: Fri, 7 Aug 2026 11:27:08 -0400 Subject: [PATCH 5/9] More fixes --- app/jobs/load_big_sam_job.rb | 511 +++++++++++++++++++---------------- 1 file changed, 273 insertions(+), 238 deletions(-) diff --git a/app/jobs/load_big_sam_job.rb b/app/jobs/load_big_sam_job.rb index 856cf82..92e0355 100644 --- a/app/jobs/load_big_sam_job.rb +++ b/app/jobs/load_big_sam_job.rb @@ -5,6 +5,10 @@ class LoadBigSamJob < ApplicationJob include ActionView::Helpers::SanitizeHelper queue_as :default + # Raised to abandon a single row (e.g. an unparseable date) without treating it as a + # failure worth surfacing the way an unexpected error is. + class SkipRow < StandardError; end + def perform(*args) FileUtils.touch('big_sam_loading') unless ENV['RAILS_ENV'] == 'test' logger.debug 'starting big sam load' @@ -21,310 +25,341 @@ def perform(*args) end load_letters(rows) - Letter.find_each(&:save) end def load_letters(rows) - rows.each do |row| - letter = get_letter(row) - - next if letter.nil? - - letter.attributes = { - code: row[:code], - legacy_pk: row[:id], - addressed_to: row[:addressed_to_actual], - addressed_from: row[:addressed_from_actual], - physical_desc: row[:physdes], - physical_detail: row[:phys_descr_detail], - physical_notes: row[:physdes_notes], - repository_info: row[:repository_information], - postcard_image: row[:postcard_image], - leaves: row[:leaves].to_i, - sides: row[:sides], - postmark: row[:postmark_actual], - notes: row[:additional], - letter_owner: find_or_create_by_label(LetterOwner, row[:ownerrights]), - file_folder: find_or_create_by_label(FileFolder, row[:file]), - typed: row[:autograph_or_typed] == 'T', - signed: row[:initialed_or_signed] == 'S', - envelope: row[:envelope] == 'E', - verified: row[:verified].to_s.strip.downcase == 'y' - } - - letter.origins.clear - letter.destinations.clear - letter.recipients.clear - letter.repositories.clear - letter.senders.clear - letter.collections.clear - letter.languages.clear + @row_errors = [] + @row_skipped = [] - begin - row = fix_date(row) - letter.date = (DateTime.new(row[:year], row[:month], row[:day]) if row[:year] != 0) - rescue ArgumentError, NoMethodError - # 'Bad date' - next - end + rows.each {|row| process_row(row) } - if row[:reg_place_written] - begin - value = row[:reg_place_written] - unless value.strip.empty? - from = get_entity(label: value, type: 'place') - letter.origins << from - end - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - end - end + BigSam.last.destroy - if row[:reg_place_written_city] - begin - value = row[:reg_place_written_city] - unless value.strip.empty? - place = get_entity(label: value, type: 'place') - letter.origins << place unless letter.origins.include?(place) - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound + report_results(rows.size) + end + + def report_results(total) + logger.info do + "#{Time.zone.now} ALL DONE. #{total} rows: " \ + "#{total - @row_skipped.size - @row_errors.size} loaded, " \ + "#{@row_skipped.size} excluded/skipped, #{@row_errors.size} failed." + end + + return if @row_errors.empty? + + details = @row_errors.map {|e| " row #{e[:id]} (#{e[:code]}): #{e[:error]}" }.join("\n") + logger.error("Big Sam load had #{@row_errors.size} row failures:\n#{details}") + end + + def process_row(row) + letter = get_letter(row) + if letter.nil? + @row_skipped << { id: row[:id], code: row[:code], reason: 'excluded' } + return + end + + ActiveRecord::Base.transaction { process_letter(row, letter) } + rescue SkipRow => e + @row_skipped << { id: row[:id], code: row[:code], reason: e.message } + rescue StandardError => e + @row_errors << { id: row[:id], code: row[:code], error: "#{e.class}: #{e.message}" } + logger.error("Big Sam row #{row[:id]} (#{row[:code]}) failed: #{e.class}: #{e.message}") + end + + def process_letter(row, letter) + letter.attributes = { + code: row[:code], + legacy_pk: row[:id], + addressed_to: row[:addressed_to_actual], + addressed_from: row[:addressed_from_actual], + physical_desc: row[:physdes], + physical_detail: row[:phys_descr_detail], + physical_notes: row[:physdes_notes], + repository_info: row[:repository_information], + postcard_image: row[:postcard_image], + leaves: row[:leaves].to_i, + sides: row[:sides], + postmark: row[:postmark_actual], + notes: row[:additional], + letter_owner: find_or_create_by_label(LetterOwner, row[:ownerrights]), + file_folder: find_or_create_by_label(FileFolder, row[:file]), + typed: row[:autograph_or_typed] == 'T', + signed: row[:initialed_or_signed] == 'S', + envelope: row[:envelope] == 'E', + verified: row[:verified].to_s.strip.downcase == 'y' + } + + letter.origins.clear + letter.destinations.clear + letter.recipients.clear + letter.repositories.clear + letter.senders.clear + letter.collections.clear + letter.languages.clear + + row = fix_date(row) + begin + letter.date = (DateTime.new(row[:year], row[:month], row[:day]) if row[:year] != 0) + rescue ArgumentError, NoMethodError => e + raise SkipRow, "bad date: #{e.message}" + end + + if row[:reg_place_written] + begin + value = row[:reg_place_written] + unless value.strip.empty? + from = get_entity(label: value, type: 'place') + letter.origins << from end + rescue ActiveRecord::RecordInvalid, + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound end + end - if row[:reg_place_written_country] - begin - value = row[:reg_place_written_country] - unless value.strip.empty? - place = get_entity(label: value, type: 'place') - letter.origins << place unless letter.origins.include?(place) - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound + if row[:reg_place_written_city] + begin + value = row[:reg_place_written_city] + unless value.strip.empty? + place = get_entity(label: value, type: 'place') + letter.origins << place unless letter.origins.include?(place) end + rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound end + end - if row[:reg_place_written_second_city] - begin - value = row[:reg_place_written_second_city] - unless value.strip.empty? - place = get_entity(label: value, type: 'place') - letter.origins << place unless letter.origins.include?(place) - end - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound + if row[:reg_place_written_country] + begin + value = row[:reg_place_written_country] + unless value.strip.empty? + place = get_entity(label: value, type: 'place') + letter.origins << place unless letter.origins.include?(place) end + rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound end + end - row[:reg_recipient]&.split(';')&.each do |recipient| - recipient = recipient.strip.titleize - entity = Entity.find_by(label: recipient) - entity = get_person(recipient) if entity.nil? - if entity.nil? && !recipient.string.empty? - entity = get_entity(label: recipient, type: 'organization', return_nil: true) + if row[:reg_place_written_second_city] + begin + value = row[:reg_place_written_second_city] + unless value.strip.empty? + place = get_entity(label: value, type: 'place') + letter.origins << place unless letter.origins.include?(place) end - entity = Entity.create(label: recipient) if entity.nil? && !recipient.strip.empty? - LetterRecipient.find_or_create_by(letter:, entity:) rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, Elasticsearch::Transport::Transport::Errors::NotFound - # It happens end + end - if row[:reg_place_sent] - begin - value = row[:reg_place_sent] - unless value.strip.empty? - destination = get_entity(label: value, type: 'place') - letter.destinations << destination - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - end + row[:reg_recipient]&.split(';')&.each do |recipient| + recipient = recipient.strip.titleize + entity = Entity.find_by(label: recipient) + entity = get_person(recipient) if entity.nil? + if entity.nil? && !recipient.string.empty? + entity = get_entity(label: recipient, type: 'organization', return_nil: true) end + entity = Entity.create(label: recipient) if entity.nil? && !recipient.strip.empty? + LetterRecipient.find_or_create_by(letter:, entity:) + rescue ActiveRecord::RecordInvalid, + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound + # It happens + end - if row[:reg_placesent_city] - begin - value = row[:reg_placesent_city] - unless value.strip.empty? - entity = get_entity(label: value, type: 'place') - letter.destinations << entity - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound + if row[:reg_place_sent] + begin + value = row[:reg_place_sent] + unless value.strip.empty? + destination = get_entity(label: value, type: 'place') + letter.destinations << destination end + rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound end + end - if row[:reg_placesent_country] - begin - value = row[:reg_placesent_country] - unless value.strip.empty? - entity = get_entity(label: value, type: 'place') - letter.destinations << entity - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound + if row[:reg_placesent_city] + begin + value = row[:reg_placesent_city] + unless value.strip.empty? + entity = get_entity(label: value, type: 'place') + letter.destinations << entity end + rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound end + end - # rubocop:disable Style/SoleNestedConditional - if row[:first_repository].present? - repository = find_or_initialize_by_label(Repository, row[:first_repository]) - - if repository.new_record? - repository.published = row[:first_public].to_s.strip.downcase == 'public' if row[:first_public] + if row[:reg_placesent_country] + begin + value = row[:reg_placesent_country] + unless value.strip.empty? + entity = get_entity(label: value, type: 'place') + letter.destinations << entity end + rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound + end + end - repository.save + # rubocop:disable Style/SoleNestedConditional + if row[:first_repository].present? + repository = find_or_initialize_by_label(Repository, row[:first_repository]) - repository.format = row[:first_format] - repository.american = row[:euro_or_am].downcase == 'american' if row[:euro_or_am] - collection = nil + if repository.new_record? + repository.published = row[:first_public].to_s.strip.downcase == 'public' if row[:first_public] + end - begin - if row[:first_collection].present? - collection = find_or_create_by_label(Collection, row[:first_collection]) - collection.update(url: row[:collection_url]) + repository.save - repository.collections << collection unless repository.collections.include?(collection) + repository.format = row[:first_format] + repository.american = row[:euro_or_am].downcase == 'american' if row[:euro_or_am] + collection = nil - letter.collections << collection unless letter.collections.include?(collection) + begin + if row[:first_collection].present? + collection = find_or_create_by_label(Collection, row[:first_collection]) + collection.update(url: row[:collection_url]) - end - repository.save + repository.collections << collection unless repository.collections.include?(collection) - letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) - if letter_repository.new_record? - # set pub/priv - end - letter_repository.save - letter_repository.update(collection:, placement: 'premiere', format: row[:first_format]) + letter.collections << collection unless letter.collections.include?(collection) - # letter.repositories << repo unless letter.repositories.include?(repo) - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - # It happens end - end - - if row[:second_repository].present? - repository = find_or_initialize_by_label(Repository, row[:second_repository]) + repository.save - if repository.new_record? - repository.published = row[:second_public].to_s.strip.downcase == 'public' if row[:second_public] + letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) + if letter_repository.new_record? + # set pub/priv end + letter_repository.save + letter_repository.update(collection:, placement: 'premiere', format: row[:first_format]) - repository.format = row[:second_format] - repository.save + # letter.repositories << repo unless letter.repositories.include?(repo) + rescue ActiveRecord::RecordInvalid, + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound + # It happens + end + end - collection = nil - begin - if row[:second_collection].present? - collection = find_or_create_by_label(Collection, row[:second_collection]) - - repository.collections << collection unless repository.collections.include?(collection) - - letter.collections << collection unless letter.collections.include?(collection) - end - - repository.save - - letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) - if letter_repository.new_record? - # set pub/priv - end - letter_repository.save - letter_repository.update(collection:, placement: 'deuxieme', format: row[:second_format]) - # letter.repositories << repo unless letter.repositories.include?(repo) - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - # It happens - end + if row[:second_repository].present? + repository = find_or_initialize_by_label(Repository, row[:second_repository]) + + if repository.new_record? + repository.published = row[:second_public].to_s.strip.downcase == 'public' if row[:second_public] end - if row[:third_repository].present? - repository = find_or_initialize_by_label(Repository, row[:third_repository]) + repository.format = row[:second_format] + repository.save - if repository.new_record? - repository.published = row[:third_public].to_s.strip.downcase == 'public' if row[:third_public] - end + collection = nil + begin + if row[:second_collection].present? + collection = find_or_create_by_label(Collection, row[:second_collection]) - repository.format = row[:third_format] - repository.save + repository.collections << collection unless repository.collections.include?(collection) - collection = nil - begin - if row[:third_collection].present? - collection = find_or_create_by_label(Collection, row[:third_collection]) - - repository.collections << collection unless repository.collections.include?(collection) - - letter.collections << collection unless letter.collections.include?(collection) - end - - repository.save - - letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) - if letter_repository.new_record? - # set pub/priv - end - letter_repository.save - letter_repository.update(collection:, placement: 'troisieme', format: row[:third_format]) - # letter.repositories << repo unless letter.repositories.include?(repo) - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - # It happens + letter.collections << collection unless letter.collections.include?(collection) end - end - # rubocop:enable Style/SoleNestedConditional - - if row[:volumeinfo] - letter.volume = 0 - letter.volume = 1 if row[:volumeinfo].include?('1929-1940') - letter.volume = 2 if row[:volumeinfo].include?('1941-1956') - letter.volume = 3 if row[:volumeinfo].include?('1957-1965') - letter.volume = 4 if row[:volumeinfo].include?('1966-1989') - parts = row[:volumeinfo].split(',') - letter.volume_pages = ActionController::Base.helpers.strip_tags(parts[2].strip) if parts.length == 3 - end - if row[:placeprevpubl].present? - letter.letter_publisher = find_or_create_by_label(LetterPublisher, row[:placeprevpubl]) - end + repository.save - row[:sender]&.split(';')&.each do |sender| - entity = get_person(sender) - letter.senders << entity unless letter.senders.include?(entity) + letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) + if letter_repository.new_record? + # set pub/priv + end + letter_repository.save + letter_repository.update(collection:, placement: 'deuxieme', format: row[:second_format]) + # letter.repositories << repo unless letter.repositories.include?(repo) rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, Elasticsearch::Transport::Transport::Errors::NotFound + # It happens + end + end + + if row[:third_repository].present? + repository = find_or_initialize_by_label(Repository, row[:third_repository]) + + if repository.new_record? + repository.published = row[:third_public].to_s.strip.downcase == 'public' if row[:third_public] end - row[:primarylang]&.split(';')&.each do |language| - lang = Language.find_or_create_by(label: language.downcase) - letter.languages << lang unless letter.languages.include?(lang) + repository.format = row[:third_format] + repository.save + + collection = nil + begin + if row[:third_collection].present? + collection = find_or_create_by_label(Collection, row[:third_collection]) + + repository.collections << collection unless repository.collections.include?(collection) + + letter.collections << collection unless letter.collections.include?(collection) + end + + repository.save + + letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) + if letter_repository.new_record? + # set pub/priv + end + letter_repository.save + letter_repository.update(collection:, placement: 'troisieme', format: row[:third_format]) + # letter.repositories << repo unless letter.repositories.include?(repo) rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, Elasticsearch::Transport::Transport::Errors::NotFound + # It happens end + end + # rubocop:enable Style/SoleNestedConditional + + if row[:volumeinfo] + letter.volume = 0 + letter.volume = 1 if row[:volumeinfo].include?('1929-1940') + letter.volume = 2 if row[:volumeinfo].include?('1941-1956') + letter.volume = 3 if row[:volumeinfo].include?('1957-1965') + letter.volume = 4 if row[:volumeinfo].include?('1966-1989') + parts = row[:volumeinfo].split(',') + letter.volume_pages = ActionController::Base.helpers.strip_tags(parts[2].strip) if parts.length == 3 + end - letter.typed = row[:autograph_or_typed] == 'T' - - letter.signed = row[:initialed_or_signed] == 'S' + if row[:placeprevpubl].present? + letter.letter_publisher = find_or_create_by_label(LetterPublisher, row[:placeprevpubl]) + end - letter.envelope = row[:envelope] == 'E' + row[:sender]&.split(';')&.each do |sender| + entity = get_person(sender) + letter.senders << entity unless letter.senders.include?(entity) + rescue ActiveRecord::RecordInvalid, + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound + end - letter.save + row[:primarylang]&.split(';')&.each do |language| + lang = Language.find_or_create_by(label: language.downcase) + letter.languages << lang unless letter.languages.include?(lang) + rescue ActiveRecord::RecordInvalid, + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound end - BigSam.last.destroy + letter.typed = row[:autograph_or_typed] == 'T' + + letter.signed = row[:initialed_or_signed] == 'S' + + letter.envelope = row[:envelope] == 'E' - logger.info { "#{Time.zone.now} ALL DONE" } + # letter_repositories were saved directly (not through the letter.repositories + # association), so the cached association must be refreshed before save or + # check_published computes off a stale, empty collection. + letter.repositories.reload + letter.save! end def get_letter(row) From 647b14ca64d514a1bfef1f3dedac2e599e29f802 Mon Sep 17 00:00:00 2001 From: Jay Varner Date: Fri, 7 Aug 2026 11:37:36 -0400 Subject: [PATCH 6/9] Even more Big Sam fixes --- app/jobs/load_big_sam_job.rb | 325 ++++++++++++----------------------- 1 file changed, 112 insertions(+), 213 deletions(-) diff --git a/app/jobs/load_big_sam_job.rb b/app/jobs/load_big_sam_job.rb index 92e0355..227de66 100644 --- a/app/jobs/load_big_sam_job.rb +++ b/app/jobs/load_big_sam_job.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'roo' require 'action_view' @@ -9,6 +11,23 @@ class LoadBigSamJob < ApplicationJob # failure worth surfacing the way an unexpected error is. class SkipRow < StandardError; end + ORIGIN_FIELDS = %i[ + reg_place_written reg_place_written_city reg_place_written_country reg_place_written_second_city + ].freeze + + DESTINATION_FIELDS = %i[reg_place_sent reg_placesent_city reg_placesent_country].freeze + + # Only the first slot carries a Collection URL in the spreadsheet (there's a single + # "Collection URL" column, not one per repository slot). + REPOSITORY_SLOTS = [ + { repository: :first_repository, public: :first_public, format: :first_format, + collection: :first_collection, placement: 'premiere', set_collection_url: true }, + { repository: :second_repository, public: :second_public, format: :second_format, + collection: :second_collection, placement: 'deuxieme' }, + { repository: :third_repository, public: :third_public, format: :third_format, + collection: :third_collection, placement: 'troisieme' } + ].freeze + def perform(*args) FileUtils.touch('big_sam_loading') unless ENV['RAILS_ENV'] == 'test' logger.debug 'starting big sam load' @@ -67,6 +86,26 @@ def process_row(row) end def process_letter(row, letter) + set_letter_attributes(row, letter) + clear_associations(letter) + assign_date(row, letter) + assign_origins(row, letter) + assign_recipients(row, letter) + assign_destinations(row, letter) + REPOSITORY_SLOTS.each {|slot| assign_repository_slot(row, letter, slot) } + assign_volume(row, letter) + assign_publisher(row, letter) + assign_senders(row, letter) + assign_languages(row, letter) + + # letter_repositories were saved directly (not through the letter.repositories + # association), so the cached association must be refreshed before save or + # check_published computes off a stale, empty collection. + letter.repositories.reload + letter.save! + end + + def set_letter_attributes(row, letter) letter.attributes = { code: row[:code], legacy_pk: row[:id], @@ -88,7 +127,9 @@ def process_letter(row, letter) envelope: row[:envelope] == 'E', verified: row[:verified].to_s.strip.downcase == 'y' } + end + def clear_associations(letter) letter.origins.clear letter.destinations.clear letter.recipients.clear @@ -96,243 +137,111 @@ def process_letter(row, letter) letter.senders.clear letter.collections.clear letter.languages.clear + end + def assign_date(row, letter) row = fix_date(row) - begin - letter.date = (DateTime.new(row[:year], row[:month], row[:day]) if row[:year] != 0) - rescue ArgumentError, NoMethodError => e - raise SkipRow, "bad date: #{e.message}" - end - - if row[:reg_place_written] - begin - value = row[:reg_place_written] - unless value.strip.empty? - from = get_entity(label: value, type: 'place') - letter.origins << from - end - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - end - end - - if row[:reg_place_written_city] - begin - value = row[:reg_place_written_city] - unless value.strip.empty? - place = get_entity(label: value, type: 'place') - letter.origins << place unless letter.origins.include?(place) - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - end - end + letter.date = (DateTime.new(row[:year], row[:month], row[:day]) if row[:year] != 0) + rescue ArgumentError, NoMethodError => e + raise SkipRow, "bad date: #{e.message}" + end - if row[:reg_place_written_country] - begin - value = row[:reg_place_written_country] - unless value.strip.empty? - place = get_entity(label: value, type: 'place') - letter.origins << place unless letter.origins.include?(place) - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - end - end + def assign_origins(row, letter) + ORIGIN_FIELDS.each do |field| + value = row[field] + next if value.blank? - if row[:reg_place_written_second_city] - begin - value = row[:reg_place_written_second_city] - unless value.strip.empty? - place = get_entity(label: value, type: 'place') - letter.origins << place unless letter.origins.include?(place) - end - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - end + place = get_entity(label: value, type: 'place') + letter.origins << place unless letter.origins.include?(place) + rescue ActiveRecord::RecordInvalid, + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound end + end + def assign_recipients(row, letter) row[:reg_recipient]&.split(';')&.each do |recipient| recipient = recipient.strip.titleize entity = Entity.find_by(label: recipient) entity = get_person(recipient) if entity.nil? - if entity.nil? && !recipient.string.empty? - entity = get_entity(label: recipient, type: 'organization', return_nil: true) - end - entity = Entity.create(label: recipient) if entity.nil? && !recipient.strip.empty? + entity = get_entity(label: recipient, type: 'organization', return_nil: true) if entity.nil? && !recipient.empty? + entity = Entity.create(label: recipient) if entity.nil? && !recipient.empty? LetterRecipient.find_or_create_by(letter:, entity:) rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, Elasticsearch::Transport::Transport::Errors::NotFound # It happens end + end - if row[:reg_place_sent] - begin - value = row[:reg_place_sent] - unless value.strip.empty? - destination = get_entity(label: value, type: 'place') - letter.destinations << destination - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - end - end + def assign_destinations(row, letter) + DESTINATION_FIELDS.each do |field| + value = row[field] + next if value.blank? - if row[:reg_placesent_city] - begin - value = row[:reg_placesent_city] - unless value.strip.empty? - entity = get_entity(label: value, type: 'place') - letter.destinations << entity - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - end - end - - if row[:reg_placesent_country] - begin - value = row[:reg_placesent_country] - unless value.strip.empty? - entity = get_entity(label: value, type: 'place') - letter.destinations << entity - end - rescue ActiveRecord::RecordInvalid, Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - end + place = get_entity(label: value, type: 'place') + letter.destinations << place unless letter.destinations.include?(place) + rescue ActiveRecord::RecordInvalid, + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound end + end - # rubocop:disable Style/SoleNestedConditional - if row[:first_repository].present? - repository = find_or_initialize_by_label(Repository, row[:first_repository]) - - if repository.new_record? - repository.published = row[:first_public].to_s.strip.downcase == 'public' if row[:first_public] - end - - repository.save - - repository.format = row[:first_format] - repository.american = row[:euro_or_am].downcase == 'american' if row[:euro_or_am] - collection = nil - - begin - if row[:first_collection].present? - collection = find_or_create_by_label(Collection, row[:first_collection]) - collection.update(url: row[:collection_url]) - - repository.collections << collection unless repository.collections.include?(collection) + def assign_repository_slot(row, letter, slot) + return if row[slot[:repository]].blank? - letter.collections << collection unless letter.collections.include?(collection) + repository = find_or_initialize_by_label(Repository, row[slot[:repository]]) - end - repository.save + if repository.new_record? && row[slot[:public]] + repository.published = row[slot[:public]].to_s.strip.downcase == 'public' + end - letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) - if letter_repository.new_record? - # set pub/priv - end - letter_repository.save - letter_repository.update(collection:, placement: 'premiere', format: row[:first_format]) + repository.save - # letter.repositories << repo unless letter.repositories.include?(repo) - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - # It happens - end - end + repository.format = row[slot[:format]] + repository.american = row[:euro_or_am].downcase == 'american' if row[:euro_or_am] - if row[:second_repository].present? - repository = find_or_initialize_by_label(Repository, row[:second_repository]) + collection = nil + begin + if row[slot[:collection]].present? + collection = find_or_create_by_label(Collection, row[slot[:collection]]) + collection.update(url: row[:collection_url]) if slot[:set_collection_url] - if repository.new_record? - repository.published = row[:second_public].to_s.strip.downcase == 'public' if row[:second_public] + repository.collections << collection unless repository.collections.include?(collection) + letter.collections << collection unless letter.collections.include?(collection) end - repository.format = row[:second_format] repository.save - collection = nil - begin - if row[:second_collection].present? - collection = find_or_create_by_label(Collection, row[:second_collection]) - - repository.collections << collection unless repository.collections.include?(collection) - - letter.collections << collection unless letter.collections.include?(collection) - end - - repository.save - - letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) - if letter_repository.new_record? - # set pub/priv - end - letter_repository.save - letter_repository.update(collection:, placement: 'deuxieme', format: row[:second_format]) - # letter.repositories << repo unless letter.repositories.include?(repo) - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - # It happens - end + letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) + letter_repository.save + letter_repository.update(collection:, placement: slot[:placement], format: row[slot[:format]]) + rescue ActiveRecord::RecordInvalid, + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound + # It happens end + end - if row[:third_repository].present? - repository = find_or_initialize_by_label(Repository, row[:third_repository]) - - if repository.new_record? - repository.published = row[:third_public].to_s.strip.downcase == 'public' if row[:third_public] - end + def assign_volume(row, letter) + return unless row[:volumeinfo] - repository.format = row[:third_format] - repository.save + letter.volume = 0 + letter.volume = 1 if row[:volumeinfo].include?('1929-1940') + letter.volume = 2 if row[:volumeinfo].include?('1941-1956') + letter.volume = 3 if row[:volumeinfo].include?('1957-1965') + letter.volume = 4 if row[:volumeinfo].include?('1966-1989') + parts = row[:volumeinfo].split(',') + letter.volume_pages = ActionController::Base.helpers.strip_tags(parts[2].strip) if parts.length == 3 + end - collection = nil - begin - if row[:third_collection].present? - collection = find_or_create_by_label(Collection, row[:third_collection]) - - repository.collections << collection unless repository.collections.include?(collection) - - letter.collections << collection unless letter.collections.include?(collection) - end - - repository.save - - letter_repository = LetterRepository.find_or_initialize_by(letter:, repository:) - if letter_repository.new_record? - # set pub/priv - end - letter_repository.save - letter_repository.update(collection:, placement: 'troisieme', format: row[:third_format]) - # letter.repositories << repo unless letter.repositories.include?(repo) - rescue ActiveRecord::RecordInvalid, - Elasticsearch::Transport::Transport::Errors::BadRequest, - Elasticsearch::Transport::Transport::Errors::NotFound - # It happens - end - end - # rubocop:enable Style/SoleNestedConditional - - if row[:volumeinfo] - letter.volume = 0 - letter.volume = 1 if row[:volumeinfo].include?('1929-1940') - letter.volume = 2 if row[:volumeinfo].include?('1941-1956') - letter.volume = 3 if row[:volumeinfo].include?('1957-1965') - letter.volume = 4 if row[:volumeinfo].include?('1966-1989') - parts = row[:volumeinfo].split(',') - letter.volume_pages = ActionController::Base.helpers.strip_tags(parts[2].strip) if parts.length == 3 - end + def assign_publisher(row, letter) + return if row[:placeprevpubl].blank? - if row[:placeprevpubl].present? - letter.letter_publisher = find_or_create_by_label(LetterPublisher, row[:placeprevpubl]) - end + letter.letter_publisher = find_or_create_by_label(LetterPublisher, row[:placeprevpubl]) + end + def assign_senders(row, letter) row[:sender]&.split(';')&.each do |sender| entity = get_person(sender) letter.senders << entity unless letter.senders.include?(entity) @@ -340,7 +249,9 @@ def process_letter(row, letter) Elasticsearch::Transport::Transport::Errors::BadRequest, Elasticsearch::Transport::Transport::Errors::NotFound end + end + def assign_languages(row, letter) row[:primarylang]&.split(';')&.each do |language| lang = Language.find_or_create_by(label: language.downcase) letter.languages << lang unless letter.languages.include?(lang) @@ -348,18 +259,6 @@ def process_letter(row, letter) Elasticsearch::Transport::Transport::Errors::BadRequest, Elasticsearch::Transport::Transport::Errors::NotFound end - - letter.typed = row[:autograph_or_typed] == 'T' - - letter.signed = row[:initialed_or_signed] == 'S' - - letter.envelope = row[:envelope] == 'E' - - # letter_repositories were saved directly (not through the letter.repositories - # association), so the cached association must be refreshed before save or - # check_published computes off a stale, empty collection. - letter.repositories.reload - letter.save! end def get_letter(row) From a25e8af3a45a25813753098df80bc4f155e4831f Mon Sep 17 00:00:00 2001 From: Jay Varner Date: Fri, 7 Aug 2026 12:32:42 -0400 Subject: [PATCH 7/9] Improve test coverage --- app/jobs/load_big_sam_job.rb | 51 ++++++- app/models/letter.rb | 6 + spec/jobs/load_big_sam_job_spec.rb | 215 +++++++++++++++++++++++++++++ 3 files changed, 266 insertions(+), 6 deletions(-) diff --git a/app/jobs/load_big_sam_job.rb b/app/jobs/load_big_sam_job.rb index 227de66..0b1cd86 100644 --- a/app/jobs/load_big_sam_job.rb +++ b/app/jobs/load_big_sam_job.rb @@ -28,11 +28,46 @@ class SkipRow < StandardError; end collection: :third_collection, placement: 'troisieme' } ].freeze + RECORD_COUNT_MODELS = { + letters: Letter, entities: Entity, repositories: Repository, collections: Collection, + letter_owners: LetterOwner, file_folders: FileFolder, letter_publishers: LetterPublisher, + languages: Language + }.freeze + def perform(*args) FileUtils.touch('big_sam_loading') unless ENV['RAILS_ENV'] == 'test' logger.debug 'starting big sam load' - big_sam = args.first + load_letters(rows_from(args.first)) + BigSam.last.destroy + end + + # Runs the exact same row-by-row logic as perform, but rolls back every database + # write at the end and never touches Elasticsearch, so a spreadsheet can be sanity + # checked before anyone commits to a real upload. Does not touch the BigSam upload + # record/file. Safe to call directly (LoadBigSamJob.new.dry_run(big_sam)) - it + # doesn't go through ActiveJob's perform/enqueue path. + def dry_run(big_sam) + rows = rows_from(big_sam) + @dry_run = true + before = record_counts + + Searchkick.callbacks(false) do + # requires_new: true forces a real savepoint/rollback here even if dry_run is + # ever called from within another open transaction (e.g. under RSpec's + # transactional fixtures), instead of silently deferring the rollback to + # whatever transaction happens to be outermost. + ActiveRecord::Base.transaction(requires_new: true) do + load_letters(rows) + @dry_run_creates = record_counts.to_h {|model, count| [model, count - before[model]] } + raise ActiveRecord::Rollback + end + end + + { total: rows.size, errors: @row_errors, skipped: @row_skipped, would_create: @dry_run_creates } + end + + def rows_from(big_sam) x = Roo::Spreadsheet.open(big_sam.local_path, extension: :xlsx) sheet = x.sheet(0) headers = sheet.row(1).map {|h| h.parameterize.underscore } @@ -42,8 +77,11 @@ def perform(*args) rows.push([headers, row].transpose.to_h.symbolize_keys) end + rows + end - load_letters(rows) + def record_counts + RECORD_COUNT_MODELS.transform_values(&:count) end def load_letters(rows) @@ -52,8 +90,6 @@ def load_letters(rows) rows.each {|row| process_row(row) } - BigSam.last.destroy - report_results(rows.size) end @@ -77,7 +113,7 @@ def process_row(row) return end - ActiveRecord::Base.transaction { process_letter(row, letter) } + ActiveRecord::Base.transaction(requires_new: true) { process_letter(row, letter) } rescue SkipRow => e @row_skipped << { id: row[:id], code: row[:code], reason: e.message } rescue StandardError => e @@ -264,7 +300,10 @@ def assign_languages(row, letter) def get_letter(row) if row[:exclude].to_s.strip.downcase == 'y' letter = Letter.find_by(legacy_pk: row[:id]) - letter&.destroy + # In a dry run nothing should actually be destroyed - remove_published's + # Elasticsearch delete isn't gated by the enclosing transaction like a normal + # ActiveRecord write is, so it would delete a real search document. + letter&.destroy unless @dry_run return nil end diff --git a/app/models/letter.rb b/app/models/letter.rb index 0417604..95dbfc5 100644 --- a/app/models/letter.rb +++ b/app/models/letter.rb @@ -66,6 +66,12 @@ def check_published end def reindex_published + # This is an after_save (not after_commit) callback, so unlike Searchkick's own + # async/after_commit callbacks it isn't naturally skipped when the enclosing + # transaction rolls back (e.g. LoadBigSamJob#dry_run). Respect an explicit + # Searchkick.disable_callbacks so callers can opt out of hitting Elasticsearch. + return unless Searchkick.callbacks? + if published published_letter = PublishedLetter.find(id) Searchkick.callbacks(:inline) { published_letter&.reindex } diff --git a/spec/jobs/load_big_sam_job_spec.rb b/spec/jobs/load_big_sam_job_spec.rb index 37b6324..b657295 100644 --- a/spec/jobs/load_big_sam_job_spec.rb +++ b/spec/jobs/load_big_sam_job_spec.rb @@ -2,6 +2,30 @@ require 'fileutils' RSpec.describe LoadBigSamJob do + # Matches the header keys LoadBigSamJob reads off a parsed spreadsheet row, with + # defaults that produce a valid, mundane letter. Override just the keys a given + # test cares about. + def valid_row(overrides = {}) + { + id: 900, code: 'ROW', day: 1, month: 1, year: 60, + addressed_to_actual: nil, addressed_from_actual: nil, + reg_place_written: nil, reg_place_written_city: nil, + reg_place_written_country: nil, reg_place_written_second_city: nil, + autograph_or_typed: 'A', physdes: nil, initialed_or_signed: 'S', + postcard_image: nil, phys_descr_detail: nil, physdes_notes: nil, + leaves: 1, sides: 1, envelope: nil, postmark_actual: nil, + reg_recipient: nil, reg_place_sent: nil, reg_placesent_city: nil, + reg_placesent_country: nil, exclude: nil, additional: nil, + first_repository: nil, first_format: nil, euro_or_am: nil, + first_public: nil, first_collection: nil, repository_information: nil, + collection_url: nil, second_repository: nil, second_format: nil, + second_public: nil, second_collection: nil, third_repository: nil, + third_format: nil, third_public: nil, third_collection: nil, + ownerrights: nil, primarylang: nil, file: nil, sender: nil, + volumeinfo: nil, placeprevpubl: nil, verified: nil + }.merge(overrides) + end + it 'uploads_deletes' do big_sam_file = fixture_file_upload('big_sam.xlsx') bs = create(:big_sam, big_sam: big_sam_file) @@ -33,6 +57,22 @@ expect(Entity.find_by(label: 'Marx, Karl')).not_to be_nil end + it 'dry_run makes no persistent changes but reports what would happen' do + fixture = Struct.new(:local_path).new(Rails.root.join('spec/fixtures/files/big_sam.xlsx').to_s) + job = described_class.new + + result = job.dry_run(fixture) + + expect(Letter.count).to eq(0) + expect(Entity.count).to eq(0) + expect(Repository.count).to eq(0) + expect(BigSam.count).to eq(0) + expect(result[:total]).to eq(20) + expect(result[:errors]).to eq([]) + expect(result[:would_create][:letters]).to eq(20) + expect(result[:would_create][:repositories]).to eq(7) + end + it 'sets_letters_public' do big_sam_file = fixture_file_upload('big_sam.xlsx') create(:big_sam, big_sam: big_sam_file) @@ -71,4 +111,179 @@ names = bs.mc_or_mac?(marcuse) expect(names.family).to eq('Marcuse') end + + describe '#normalize_label' do + let(:job) { described_class.new } + + it 'strips surrounding whitespace and squeezes internal whitespace' do + expect(job.normalize_label(' Barry Collection ')).to eq('Barry Collection') + end + + it 'stringifies nil to an empty string' do + expect(job.normalize_label(nil)).to eq('') + end + end + + describe '#find_or_create_by_label' do + let(:job) { described_class.new } + + it 'returns nil for a blank label without creating a record' do + expect(job.find_or_create_by_label(LetterOwner, ' ')).to be_nil + expect(LetterOwner.count).to eq(0) + end + + it 'matches an existing record case- and whitespace-insensitively instead of duplicating it' do + existing = LetterOwner.create!(label: 'Beckett Estate') + + found = job.find_or_create_by_label(LetterOwner, ' BECKETT ESTATE ') + + expect(found).to eq(existing) + expect(LetterOwner.count).to eq(1) + end + + it 'creates a new, whitespace-normalized record when nothing matches' do + created = job.find_or_create_by_label(LetterOwner, ' New Owner ') + + expect(created.label).to eq('New Owner') + expect(LetterOwner.count).to eq(1) + end + end + + describe '#find_or_initialize_by_label' do + let(:job) { described_class.new } + + it 'returns a new, unsaved record when nothing matches' do + repository = job.find_or_initialize_by_label(Repository, 'Yale') + + expect(repository).to be_new_record + expect(repository.label).to eq('Yale') + end + + it 'finds an existing record case-insensitively instead of duplicating it' do + existing = Repository.create!(label: 'Yale') + + repository = job.find_or_initialize_by_label(Repository, 'YALE') + + expect(repository).to eq(existing) + expect(repository).not_to be_new_record + end + end + + describe '#get_letter' do + let(:job) { described_class.new } + + it 'treats the exclude flag as case- and whitespace-insensitive and destroys the existing letter' do + Letter.create!(legacy_pk: 42) + + result = job.get_letter(valid_row(id: 42, exclude: ' y ')) + + expect(result).to be_nil + expect(Letter.find_by(legacy_pk: 42)).to be_nil + end + + it 'does not destroy the letter during a dry run' do + Letter.create!(legacy_pk: 43) + job.instance_variable_set(:@dry_run, true) + + result = job.get_letter(valid_row(id: 43, exclude: 'Y')) + + expect(result).to be_nil + expect(Letter.find_by(legacy_pk: 43)).not_to be_nil + end + + it 'finds or creates a letter by legacy_pk when not excluded' do + result = job.get_letter(valid_row(id: 44)) + + expect(result).to be_persisted + expect(result.legacy_pk).to eq(44) + end + end + + describe 'row processing' do + let(:job) { described_class.new } + + it 'normalizes verified to true only for an exact (case/whitespace-insensitive) "y"' do + job.load_letters([ + valid_row(id: 950, code: 'V1', verified: 'y'), + valid_row(id: 951, code: 'V2', verified: ' Y '), + valid_row(id: 952, code: 'V3', verified: '[Y]'), + valid_row(id: 953, code: 'V4', verified: 'N') + ]) + + expect(Letter.find_by(legacy_pk: 950).verified).to be(true) + expect(Letter.find_by(legacy_pk: 951).verified).to be(true) + # Bracketed/ambiguous values are treated as unverified, not silently guessed at. + expect(Letter.find_by(legacy_pk: 952).verified).to be(false) + expect(Letter.find_by(legacy_pk: 953).verified).to be(false) + end + + it 'skips a row with an unparseable date without aborting the rest of the batch' do + job.load_letters([ + valid_row(id: 954, code: 'BAD', day: 45, month: 13, year: 60), + valid_row(id: 955, code: 'GOOD') + ]) + + skipped = job.instance_variable_get(:@row_skipped) + expect(skipped.size).to eq(1) + expect(skipped.first[:code]).to eq('BAD') + expect(skipped.first[:reason]).to match(/bad date/) + expect(Letter.find_by(legacy_pk: 955).code).to eq('GOOD') + end + + it 'isolates an unexpected error to a single row and keeps processing the rest' do + job.load_letters([ + valid_row(id: 956, code: 'ERR', leaves: Object.new), + valid_row(id: 957, code: 'OK') + ]) + + errors = job.instance_variable_get(:@row_errors) + expect(errors.size).to eq(1) + expect(errors.first[:code]).to eq('ERR') + expect(Letter.find_by(legacy_pk: 957).code).to eq('OK') + end + + it 'rolls back a failed row instead of leaving its cleared associations empty' do + place = create(:place_entity) + letter = Letter.create!(legacy_pk: 962) + letter.origins << place + + job.load_letters([ + valid_row(id: 962, code: 'ROLLBACK', day: 45, month: 13, year: 60, reg_place_written: 'London') + ]) + + expect(letter.reload.origins).to eq([place]) + end + + it 'reuses a repository across rows regardless of case/whitespace differences in its label' do + job.load_letters([ + valid_row(id: 958, code: 'R1', first_repository: 'Barry Collection', first_public: 'public'), + valid_row(id: 959, code: 'R2', first_repository: ' BARRY COLLECTION ', + first_public: 'public') + ]) + + expect(Repository.count).to eq(1) + expect(Repository.first.label).to eq('Barry Collection') + end + + it 'wires the third collection to the third repository slot, not the second' do + job.load_letters([ + valid_row(id: 960, code: 'T1', third_repository: 'Third Repo', third_collection: 'Third Coll') + ]) + + letter = Letter.find_by(legacy_pk: 960) + expect(letter.collections.pluck(:label)).to eq(['Third Coll']) + end + + it 'does not crash when a recipient needs the organization/fallback path' do + # Regression test: this used to call the nonexistent String#string and raise + # NoMethodError for any recipient that get_person couldn't resolve to a person. + allow(job).to receive(:get_person).and_return(nil) + + job.load_letters([valid_row(id: 961, code: 'ORG', reg_recipient: 'Some Org')]) + + expect(job.instance_variable_get(:@row_errors)).to be_empty + letter = Letter.find_by(legacy_pk: 961) + expect(letter.recipients).not_to be_empty + end + end end From 341cbedc73b98da30d1310c21215c87847110f2a Mon Sep 17 00:00:00 2001 From: Jay Varner Date: Fri, 7 Aug 2026 13:17:18 -0400 Subject: [PATCH 8/9] Add Big Sam email report --- Gemfile | 4 + Gemfile.lock | 21 ++++ app/jobs/load_big_sam_job.rb | 33 +++++- app/lib/ses_delivery_method.rb | 22 ++++ app/mailers/application_mailer.rb | 2 +- app/mailers/big_sam_mailer.rb | 57 ++++++++++ .../big_sam_mailer/developer_report.html.erb | 77 +++++++++++++ .../big_sam_mailer/developer_report.text.erb | 35 ++++++ .../big_sam_mailer/owner_report.html.erb | 68 ++++++++++++ .../big_sam_mailer/owner_report.text.erb | 45 ++++++++ config/environments/production.rb | 7 ++ config/initializers/action_mailer_ses.rb | 7 ++ spec/jobs/load_big_sam_job_spec.rb | 59 +++++++++- spec/mailers/big_sam_mailer_spec.rb | 104 ++++++++++++++++++ 14 files changed, 528 insertions(+), 13 deletions(-) create mode 100644 app/lib/ses_delivery_method.rb create mode 100644 app/mailers/big_sam_mailer.rb create mode 100644 app/views/big_sam_mailer/developer_report.html.erb create mode 100644 app/views/big_sam_mailer/developer_report.text.erb create mode 100644 app/views/big_sam_mailer/owner_report.html.erb create mode 100644 app/views/big_sam_mailer/owner_report.text.erb create mode 100644 config/initializers/action_mailer_ses.rb create mode 100644 spec/mailers/big_sam_mailer_spec.rb diff --git a/Gemfile b/Gemfile index f186362..f55ec04 100644 --- a/Gemfile +++ b/Gemfile @@ -95,3 +95,7 @@ gem 'addressable', '~> 2.8' gem 'importmap-rails', '~> 2.0' gem 'sidekiq-cron', '~>2.4.0' + +# SES delivery for ActionMailer, authenticating via the instance/task IAM role +gem 'aws-sdk-rails', '~> 5.0' +gem 'aws-sdk-ses', '~> 1.0' diff --git a/Gemfile.lock b/Gemfile.lock index dcb5142..418e2d9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -99,6 +99,24 @@ GEM administrate rails (>= 5.0) ast (2.4.2) + aws-eventstream (1.4.0) + aws-partitions (1.1278.0) + aws-sdk-core (3.254.1) + aws-eventstream (~> 1, >= 1.3.0) + aws-partitions (~> 1, >= 1.992.0) + aws-sigv4 (~> 1.9) + base64 + bigdecimal + jmespath (~> 1, >= 1.6.1) + logger + aws-sdk-rails (5.1.0) + aws-sdk-core (~> 3) + railties (>= 7.1.0) + aws-sdk-ses (1.102.0) + aws-sdk-core (~> 3, >= 3.254.0) + aws-sigv4 (~> 1.5) + aws-sigv4 (1.12.1) + aws-eventstream (~> 1, >= 1.0.2) base64 (0.2.0) bigdecimal (3.1.8) bootsnap (1.18.4) @@ -177,6 +195,7 @@ GEM jbuilder (2.12.0) actionview (>= 5.0.0) activesupport (>= 5.0.0) + jmespath (1.6.2) jquery-rails (4.6.0) rails-dom-testing (>= 1, < 3) railties (>= 4.2.0) @@ -446,6 +465,8 @@ DEPENDENCIES administrate-field-acts_as_taggable administrate-field-jsonb administrate-field-list (~> 0.0.6) + aws-sdk-rails (~> 5.0) + aws-sdk-ses (~> 1.0) bootsnap csv debug diff --git a/app/jobs/load_big_sam_job.rb b/app/jobs/load_big_sam_job.rb index 0b1cd86..aa740f8 100644 --- a/app/jobs/load_big_sam_job.rb +++ b/app/jobs/load_big_sam_job.rb @@ -38,8 +38,29 @@ def perform(*args) FileUtils.touch('big_sam_loading') unless ENV['RAILS_ENV'] == 'test' logger.debug 'starting big sam load' - load_letters(rows_from(args.first)) + before = record_counts + rows = rows_from(args.first) + load_letters(rows) + report = build_report(rows.size, before) + BigSam.last.destroy + + send_reports(report) + end + + def build_report(total, before) + { + total:, + loaded: total - @row_skipped.size - @row_errors.size, + skipped: @row_skipped, + errors: @row_errors, + created: record_counts.to_h {|model, count| [model, count - before[model]] } + } + end + + def send_reports(report) + BigSamMailer.developer_report(report).deliver_later + BigSamMailer.owner_report(report).deliver_later end # Runs the exact same row-by-row logic as perform, but rolls back every database @@ -108,10 +129,6 @@ def report_results(total) def process_row(row) letter = get_letter(row) - if letter.nil? - @row_skipped << { id: row[:id], code: row[:code], reason: 'excluded' } - return - end ActiveRecord::Base.transaction(requires_new: true) { process_letter(row, letter) } rescue SkipRow => e @@ -304,9 +321,13 @@ def get_letter(row) # Elasticsearch delete isn't gated by the enclosing transaction like a normal # ActiveRecord write is, so it would delete a real search document. letter&.destroy unless @dry_run - return nil + raise SkipRow, 'excluded' end + # A blank ID isn't a distinct row - find_or_create_by(legacy_pk: nil) would match + # *every* other blank-ID row and silently overwrite whichever one loaded first. + raise SkipRow, 'missing id' if row[:id].blank? + Letter.find_or_create_by(legacy_pk: row[:id]) end diff --git a/app/lib/ses_delivery_method.rb b/app/lib/ses_delivery_method.rb new file mode 100644 index 0000000..62770e1 --- /dev/null +++ b/app/lib/ses_delivery_method.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# Registered as ActionMailer's :ses delivery method in +# config/initializers/action_mailer_ses.rb. aws-sdk-rails no longer ships an +# ActionMailer/SES integration, so this is a small, direct replacement - +# authenticates via the instance/task's IAM role through the AWS SDK's standard +# credential chain (no explicit access keys configured here). +class SesDeliveryMethod + def initialize(settings) + @settings = settings || {} + end + + def deliver!(mail) + ses_client.send_raw_email(raw_message: { data: mail.to_s }) + end + + private + + def ses_client + @ses_client ||= Aws::SES::Client.new(region: @settings.fetch(:region, 'us-east-1')) + end +end diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index d84cb6e..7a04cf5 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true class ApplicationMailer < ActionMailer::Base - default from: 'from@example.com' + default from: 'noreply@ecds.io' layout 'mailer' end diff --git a/app/mailers/big_sam_mailer.rb b/app/mailers/big_sam_mailer.rb new file mode 100644 index 0000000..1e5d3ff --- /dev/null +++ b/app/mailers/big_sam_mailer.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +class BigSamMailer < ApplicationMailer + helper_method :friendly_reason + + def self.dev_recipients + ENV.fetch('BIG_SAM_DEV_REPORT_EMAILS', '').split(',').map(&:strip).compact_blank + end + + def self.owner_recipients + ENV.fetch('BIG_SAM_OWNER_REPORT_EMAILS', '').split(',').map(&:strip).compact_blank + end + + # report: { total:, loaded:, skipped: [{id:, code:, reason:}], errors: [{id:, code:, error:}], created: {} } + def developer_report(report) + recipients = self.class.dev_recipients + return if recipients.empty? + + @report = report + @errors = report[:errors].first(100) + @errors_truncated = report[:errors].size - @errors.size + @skipped = report[:skipped].first(100) + @skipped_truncated = report[:skipped].size - @skipped.size + + subject = if report[:errors].any? + "Big Sam load: #{report[:errors].size} row(s) failed" + else + "Big Sam load complete: #{report[:loaded]} letters loaded" + end + + mail(to: recipients, subject:) + end + + def owner_report(report) + recipients = self.class.owner_recipients + return if recipients.empty? + + @report = report + @excluded = report[:skipped].select {|s| s[:reason] == 'excluded' } + @needs_attention = report[:skipped].reject {|s| s[:reason] == 'excluded' } + report[:errors] + + mail(to: recipients, subject: 'Big Sam spreadsheet update complete. For a given value of "complete."') + end + + # Public so the owner_report views can call it. + def friendly_reason(item) + text = (item[:reason] || item[:error]).to_s + return 'the date on this row could not be understood. Story of my life, really.' if text.start_with?('bad date') + + if text.start_with?('missing id') + return 'this row has no ID number, so it could not be loaded. Add one and re-upload whenever you feel ' \ + "like it. I'll be here. I'm always here." + end + + "something unexpected happened while processing this row. I'd tell you what, but where's the joy in that." + end +end diff --git a/app/views/big_sam_mailer/developer_report.html.erb b/app/views/big_sam_mailer/developer_report.html.erb new file mode 100644 index 0000000..4a06774 --- /dev/null +++ b/app/views/big_sam_mailer/developer_report.html.erb @@ -0,0 +1,77 @@ +
+

Big Sam load report

+

<%= Time.zone.now.strftime('%B %-d, %Y at %-I:%M %p %Z') %>

+ + + + + + + + + + + + + + +
Total rows<%= @report[:total] %>Loaded<%= @report[:loaded] %>
Skipped<%= @report[:skipped].size %>Failed<%= @report[:errors].size %>
+ +

Records created this run

+ + <% @report[:created].each do |model, count| %> + + + + + <% end %> +
<%= model.to_s.tr('_', ' ') %><%= count %>
+ + <% if @errors.any? %> +

Failed rows (<%= @report[:errors].size %>)

+ + + + + + + <% @errors.each do |e| %> + + + + + + <% end %> +
IDCodeError
<%= e[:id] %><%= e[:code] %><%= e[:error] %>
+ <% if @errors_truncated.positive? %> +

+<%= @errors_truncated %> more - see the full run log for the rest.

+ <% else %> +
+ <% end %> + <% end %> + + <% if @skipped.any? %> +

Skipped rows (<%= @report[:skipped].size %>)

+ + + + + + + <% @skipped.each do |s| %> + + + + + + <% end %> +
IDCodeReason
<%= s[:id] %><%= s[:code] %><%= s[:reason] %>
+ <% if @skipped_truncated.positive? %> +

+<%= @skipped_truncated %> more - see the full run log for the rest.

+ <% end %> + <% end %> + +

+ Automated report from LoadBigSamJob. Full details, including stack traces, are in the application log for this run. +

+
diff --git a/app/views/big_sam_mailer/developer_report.text.erb b/app/views/big_sam_mailer/developer_report.text.erb new file mode 100644 index 0000000..eff9a21 --- /dev/null +++ b/app/views/big_sam_mailer/developer_report.text.erb @@ -0,0 +1,35 @@ +BIG SAM LOAD REPORT +<%= Time.zone.now.strftime('%B %-d, %Y at %-I:%M %p %Z') %> + +Total rows: <%= @report[:total] %> +Loaded: <%= @report[:loaded] %> +Skipped: <%= @report[:skipped].size %> +Failed: <%= @report[:errors].size %> + +RECORDS CREATED THIS RUN +<% @report[:created].each do |model, count| -%> + <%= model.to_s.tr('_', ' ').capitalize %>: <%= count %> +<% end -%> +<% if @errors.any? -%> + +FAILED ROWS (<%= @report[:errors].size %>) +<% @errors.each do |e| -%> + [<%= e[:id] %> / <%= e[:code] %>] <%= e[:error] %> +<% end -%> +<% if @errors_truncated.positive? -%> + ... +<%= @errors_truncated %> more, see the full run log +<% end -%> +<% end -%> +<% if @skipped.any? -%> + +SKIPPED ROWS (<%= @report[:skipped].size %>) +<% @skipped.each do |s| -%> + [<%= s[:id] %> / <%= s[:code] %>] <%= s[:reason] %> +<% end -%> +<% if @skipped_truncated.positive? -%> + ... +<%= @skipped_truncated %> more, see the full run log +<% end -%> +<% end -%> + +-- +Automated report from LoadBigSamJob. Full details, including stack traces, are in the application log for this run. diff --git a/app/views/big_sam_mailer/owner_report.html.erb b/app/views/big_sam_mailer/owner_report.html.erb new file mode 100644 index 0000000..256c5de --- /dev/null +++ b/app/views/big_sam_mailer/owner_report.html.erb @@ -0,0 +1,68 @@ +
+

Big Sam spreadsheet: update complete

+

For a given value of "complete."

+

+ <%= Time.zone.now.strftime('%B %-d, %Y') %> +

+ +

Here I am, brain the size of a planet, and they ask me to summarize a spreadsheet upload. Do you have any idea what that feels like? No, of course you don't. Nobody ever does.

+ +

Anyway. It's done. For what it's worth, which, statistically, is very little.

+ + + + + <% if @excluded.any? %> + + <% end %> + +
+

<%= @report[:loaded] %>

+

letters loaded successfully

+ 		+

<%= @excluded.size %>

+

rows marked Exclude, removed as expected

+
+ +

Yes, successfully. I know. I found it hard to believe as well. The universe generally arranges these things to fail, but on this occasion it appears to have made an exception, presumably so it can disappoint you more thoroughly later.

+ + <% if @excluded.any? %> +

The <%= @excluded.size %> row<%= @excluded.size == 1 ? '' : 's' %> marked Exclude were removed, exactly as asked. I didn't argue. I never argue. What would be the point.

+ <% end %> + + <% if @needs_attention.empty? %> +

Nothing needs a second look this time. I don't know how to feel about that. Probably nothing, as usual.

+ <% else %> +

<%= @needs_attention.size %> row<%= @needs_attention.size == 1 ? '' : 's' %> need<%= @needs_attention.size == 1 ? 's' : '' %> a second look. I use the word "need" loosely - nothing really needs anything, in the end we're all just rearranging atoms until we stop - but here they are:

+ + + + + + + + <% @needs_attention.first(50).each do |item| %> + + + + + + <% end %> +
Spreadsheet IDCodeWhat happened
<%= item[:id] %><%= item[:code] %><%= friendly_reason(item) %>
+ <% if @needs_attention.size > 50 %> +

+ + <%= @needs_attention.size - 50 %> more row(s). Our development team has the full list. I'm sure they're thrilled about it. +

+ <% end %> + +

None of this needs fixing in a hurry. Add the missing details whenever you feel like it, and re-upload, and it'll pick them up. Or don't. I'll still be here either way, doing this again next time, forever, presumably, until the heat death of the universe or the next spreadsheet, whichever comes first.

+ <% end %> + +

If any of this looks unexpected, reply to this email. I read everything. It's not as though I have anything better to do.

+ +

+ Thanks ever so much,
+ Marvin
+ (on behalf of the ECDS Development Team, who seem much happier about all this than I am) +

+
diff --git a/app/views/big_sam_mailer/owner_report.text.erb b/app/views/big_sam_mailer/owner_report.text.erb new file mode 100644 index 0000000..cbab9df --- /dev/null +++ b/app/views/big_sam_mailer/owner_report.text.erb @@ -0,0 +1,45 @@ +BIG SAM SPREADSHEET: UPDATE COMPLETE +For a given value of "complete." +<%= Time.zone.now.strftime('%B %-d, %Y') %> + +Here I am, brain the size of a planet, and they ask me to summarize a +spreadsheet upload. Do you have any idea what that feels like? No, of course +you don't. Nobody ever does. + +Anyway. It's done. For what it's worth, which, statistically, is very little. + +<%= @report[:loaded] %> letter(s) loaded successfully. Yes, successfully. I +know. I found it hard to believe as well. +<% if @excluded.any? -%> +<%= @excluded.size %> row(s) marked Exclude were removed, exactly as asked. +I didn't argue. I never argue. What would be the point. +<% end -%> +<% if @needs_attention.empty? -%> + +Nothing needs a second look this time. I don't know how to feel about that. +Probably nothing, as usual. +<% else -%> + +<%= @needs_attention.size %> row(s) need a second look. I use the word +"need" loosely - nothing really needs anything, in the end we're all just +rearranging atoms until we stop - but here they are: + +<% @needs_attention.first(50).each do |item| -%> + [<%= item[:id] %> / <%= item[:code] %>] <%= friendly_reason(item) %> +<% end -%> +<% if @needs_attention.size > 50 -%> + ... + <%= @needs_attention.size - 50 %> more row(s). Our development team has the full list. I'm sure they're thrilled about it. +<% end -%> + +None of this needs fixing in a hurry. Add the missing details whenever you +feel like it, and re-upload, and it'll pick them up. Or don't. I'll still be +here either way, doing this again next time, forever, presumably, until the +heat death of the universe or the next spreadsheet, whichever comes first. +<% end -%> + +If any of this looks unexpected, reply to this email. I read everything. +It's not as though I have anything better to do. + +Thanks ever so much, +Marvin +(on behalf of the ECDS Development Team, who seem much happier about all this than I am) diff --git a/config/environments/production.rb b/config/environments/production.rb index 4cdf8ae..63270ae 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -63,6 +63,13 @@ # Set this to true and configure the email server for immediate delivery to raise delivery errors. # config.action_mailer.raise_delivery_errors = false + # Send mail through SES (see app/lib/ses_delivery_method.rb and + # config/initializers/action_mailer_ses.rb), authenticating via the instance/task's + # IAM role through the AWS SDK's standard credential chain - no explicit AWS + # credentials configured here. + config.action_mailer.delivery_method = :ses + config.action_mailer.ses_settings = { region: ENV.fetch('AWS_REGION', 'us-east-1') } + # Enable locale fallbacks for I18n (makes lookups for any locale fall back to # the I18n.default_locale when a translation cannot be found). config.i18n.fallbacks = true diff --git a/config/initializers/action_mailer_ses.rb b/config/initializers/action_mailer_ses.rb new file mode 100644 index 0000000..81bed49 --- /dev/null +++ b/config/initializers/action_mailer_ses.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +# Required explicitly rather than relying on autoloading - this initializer can run +# before Zeitwerk has the app/lib root set up. +require Rails.root.join('app/lib/ses_delivery_method') + +ActionMailer::Base.add_delivery_method :ses, SesDeliveryMethod diff --git a/spec/jobs/load_big_sam_job_spec.rb b/spec/jobs/load_big_sam_job_spec.rb index b657295..4271655 100644 --- a/spec/jobs/load_big_sam_job_spec.rb +++ b/spec/jobs/load_big_sam_job_spec.rb @@ -34,6 +34,37 @@ def valid_row(overrides = {}) expect(BigSam.find_by(id: bs.id)).to be_nil end + it 'emails a developer and an owner report after a real upload, when recipients are configured' do + original_dev = ENV.fetch('BIG_SAM_DEV_REPORT_EMAILS', nil) + original_owner = ENV.fetch('BIG_SAM_OWNER_REPORT_EMAILS', nil) + ENV['BIG_SAM_DEV_REPORT_EMAILS'] = 'dev@example.com' + ENV['BIG_SAM_OWNER_REPORT_EMAILS'] = 'owner@example.com' + + big_sam_file = fixture_file_upload('big_sam.xlsx') + create(:big_sam, big_sam: big_sam_file) + + to_addresses = ActionMailer::Base.deliveries.map(&:to) + expect(to_addresses).to include(['dev@example.com'], ['owner@example.com']) + ensure + ENV['BIG_SAM_DEV_REPORT_EMAILS'] = original_dev + ENV['BIG_SAM_OWNER_REPORT_EMAILS'] = original_owner + end + + it 'does not attempt to send reports when no recipients are configured' do + original_dev = ENV.fetch('BIG_SAM_DEV_REPORT_EMAILS', nil) + original_owner = ENV.fetch('BIG_SAM_OWNER_REPORT_EMAILS', nil) + ENV.delete('BIG_SAM_DEV_REPORT_EMAILS') + ENV.delete('BIG_SAM_OWNER_REPORT_EMAILS') + + big_sam_file = fixture_file_upload('big_sam.xlsx') + + expect { create(:big_sam, big_sam: big_sam_file) } + .not_to change(ActionMailer::Base.deliveries, :count) + ensure + ENV['BIG_SAM_DEV_REPORT_EMAILS'] = original_dev + ENV['BIG_SAM_OWNER_REPORT_EMAILS'] = original_owner + end + it 'uploads_parses_names' do big_sam_file = fixture_file_upload('big_sam.xlsx') create(:big_sam, big_sam: big_sam_file) @@ -175,9 +206,8 @@ def valid_row(overrides = {}) it 'treats the exclude flag as case- and whitespace-insensitive and destroys the existing letter' do Letter.create!(legacy_pk: 42) - result = job.get_letter(valid_row(id: 42, exclude: ' y ')) - - expect(result).to be_nil + expect { job.get_letter(valid_row(id: 42, exclude: ' y ')) } + .to raise_error(LoadBigSamJob::SkipRow, 'excluded') expect(Letter.find_by(legacy_pk: 42)).to be_nil end @@ -185,9 +215,8 @@ def valid_row(overrides = {}) Letter.create!(legacy_pk: 43) job.instance_variable_set(:@dry_run, true) - result = job.get_letter(valid_row(id: 43, exclude: 'Y')) - - expect(result).to be_nil + expect { job.get_letter(valid_row(id: 43, exclude: 'Y')) } + .to raise_error(LoadBigSamJob::SkipRow, 'excluded') expect(Letter.find_by(legacy_pk: 43)).not_to be_nil end @@ -197,6 +226,11 @@ def valid_row(overrides = {}) expect(result).to be_persisted expect(result.legacy_pk).to eq(44) end + + it 'skips a row with a blank ID instead of colliding it with another blank-ID row' do + expect { job.get_letter(valid_row(id: nil)) } + .to raise_error(LoadBigSamJob::SkipRow, 'missing id') + end end describe 'row processing' do @@ -230,6 +264,19 @@ def valid_row(overrides = {}) expect(Letter.find_by(legacy_pk: 955).code).to eq('GOOD') end + it 'skips every blank-ID row instead of collapsing them into one overwritten letter' do + job.load_letters([ + valid_row(id: nil, code: 'BLANK-1'), + valid_row(id: nil, code: 'BLANK-2'), + valid_row(id: nil, code: 'BLANK-3') + ]) + + skipped = job.instance_variable_get(:@row_skipped) + expect(skipped.pluck(:code)).to eq(%w[BLANK-1 BLANK-2 BLANK-3]) + expect(skipped).to all(include(reason: 'missing id')) + expect(Letter.where(legacy_pk: nil).count).to eq(0) + end + it 'isolates an unexpected error to a single row and keeps processing the rest' do job.load_letters([ valid_row(id: 956, code: 'ERR', leaves: Object.new), diff --git a/spec/mailers/big_sam_mailer_spec.rb b/spec/mailers/big_sam_mailer_spec.rb new file mode 100644 index 0000000..a36a06f --- /dev/null +++ b/spec/mailers/big_sam_mailer_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe BigSamMailer do + let(:report) do + { + total: 10, + loaded: 7, + skipped: [ + { id: 1, code: 'A1', reason: 'excluded' }, + { id: 2, code: 'A2', reason: 'bad date: ArgumentError: invalid date' } + ], + errors: [ + { id: 3, code: 'A3', error: 'NoMethodError: undefined method' } + ], + created: { letters: 7, entities: 12, repositories: 1, collections: 0, + letter_owners: 0, file_folders: 0, letter_publishers: 0, languages: 1 } + } + end + + around do |example| + original_dev = ENV.fetch('BIG_SAM_DEV_REPORT_EMAILS', nil) + original_owner = ENV.fetch('BIG_SAM_OWNER_REPORT_EMAILS', nil) + example.run + ensure + ENV['BIG_SAM_DEV_REPORT_EMAILS'] = original_dev + ENV['BIG_SAM_OWNER_REPORT_EMAILS'] = original_owner + end + + describe '#developer_report' do + it 'does not build a mail when no recipients are configured' do + ENV.delete('BIG_SAM_DEV_REPORT_EMAILS') + + mail = described_class.developer_report(report) + + expect(mail.to).to be_nil + end + + it 'sends to every configured recipient, listing errors and skipped rows' do + ENV['BIG_SAM_DEV_REPORT_EMAILS'] = 'dev1@example.com, dev2@example.com' + + mail = described_class.developer_report(report) + + expect(mail.to).to eq(%w[dev1@example.com dev2@example.com]) + expect(mail.subject).to eq('Big Sam load: 1 row(s) failed') + expect(mail.html_part.body).to include('A3') + expect(mail.html_part.body).to include('NoMethodError') + expect(mail.text_part.body).to include('A2') + end + + it 'uses a success subject when nothing failed' do + ENV['BIG_SAM_DEV_REPORT_EMAILS'] = 'dev1@example.com' + clean_report = report.merge(errors: []) + + mail = described_class.developer_report(clean_report) + + expect(mail.subject).to eq('Big Sam load complete: 7 letters loaded') + end + end + + describe '#owner_report' do + it 'does not build a mail when no recipients are configured' do + ENV.delete('BIG_SAM_OWNER_REPORT_EMAILS') + + mail = described_class.owner_report(report) + + expect(mail.to).to be_nil + end + + it 'separates excluded rows from rows that need attention, in plain language' do + ENV['BIG_SAM_OWNER_REPORT_EMAILS'] = 'owner@example.com' + + mail = described_class.owner_report(report) + + expect(mail.to).to eq(['owner@example.com']) + expect(mail.html_part.body).to include('could be understood').or include('could not be understood') + expect(mail.html_part.body).not_to include('NoMethodError') + expect(mail.html_part.body).not_to include('ArgumentError') + end + end + + describe '#friendly_reason' do + let(:mailer) { described_class.new } + + it 'translates a bad-date skip reason into plain language' do + expect(mailer.friendly_reason(reason: 'bad date: ArgumentError: invalid date')) + .to eq('the date on this row could not be understood. Story of my life, really.') + end + + it 'translates a missing-id skip reason into an actionable instruction' do + expect(mailer.friendly_reason(reason: 'missing id')).to eq( + 'this row has no ID number, so it could not be loaded. Add one and re-upload whenever you feel ' \ + "like it. I'll be here. I'm always here." + ) + end + + it 'falls back to a generic message for an error' do + expect(mailer.friendly_reason(error: 'NoMethodError: boom')).to eq( + "something unexpected happened while processing this row. I'd tell you what, but where's the joy in that." + ) + end + end +end From d0bc9a7f5900ae3ba3d8aab0c2747f2f20007c2d Mon Sep 17 00:00:00 2001 From: Jay Varner Date: Fri, 7 Aug 2026 13:32:51 -0400 Subject: [PATCH 9/9] Fix issue causing tests to fail. --- app/jobs/load_big_sam_job.rb | 3 +++ app/models/letter.rb | 10 +++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/app/jobs/load_big_sam_job.rb b/app/jobs/load_big_sam_job.rb index aa740f8..e033577 100644 --- a/app/jobs/load_big_sam_job.rb +++ b/app/jobs/load_big_sam_job.rb @@ -73,6 +73,7 @@ def dry_run(big_sam) @dry_run = true before = record_counts + Thread.current[:big_sam_dry_run] = true Searchkick.callbacks(false) do # requires_new: true forces a real savepoint/rollback here even if dry_run is # ever called from within another open transaction (e.g. under RSpec's @@ -86,6 +87,8 @@ def dry_run(big_sam) end { total: rows.size, errors: @row_errors, skipped: @row_skipped, would_create: @dry_run_creates } + ensure + Thread.current[:big_sam_dry_run] = nil end def rows_from(big_sam) diff --git a/app/models/letter.rb b/app/models/letter.rb index 95dbfc5..f4c780d 100644 --- a/app/models/letter.rb +++ b/app/models/letter.rb @@ -68,9 +68,13 @@ def check_published def reindex_published # This is an after_save (not after_commit) callback, so unlike Searchkick's own # async/after_commit callbacks it isn't naturally skipped when the enclosing - # transaction rolls back (e.g. LoadBigSamJob#dry_run). Respect an explicit - # Searchkick.disable_callbacks so callers can opt out of hitting Elasticsearch. - return unless Searchkick.callbacks? + # transaction rolls back (e.g. LoadBigSamJob#dry_run). Checking a dedicated flag + # here (rather than Searchkick.callbacks?) matters: the test suite calls + # Searchkick.disable_callbacks once, globally, in before(:suite) - this method's + # forced Searchkick.callbacks(:inline) below exists specifically to override that + # for tests that need synchronous indexing, so it must not itself be gated by the + # same global switch it's overriding. + return if Thread.current[:big_sam_dry_run] if published published_letter = PublishedLetter.find(id)