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/Gemfile b/Gemfile index 0b1780e..f55ec04 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] @@ -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 26bc5af..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) @@ -117,14 +135,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 +154,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) @@ -176,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) @@ -213,14 +233,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 +263,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 +431,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) @@ -444,10 +465,12 @@ 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 - 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/jobs/load_big_sam_job.rb b/app/jobs/load_big_sam_job.rb index 14174db..e033577 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' @@ -5,11 +7,91 @@ 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 + + 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 + + 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 + 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 + # 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 + + 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 + # 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 } + ensure + Thread.current[:big_sam_dry_run] = nil + 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 } @@ -19,320 +101,255 @@ def perform(*args) rows.push([headers, row].transpose.to_h.symbolize_keys) end + rows + end - load_letters(rows) - Letter.find_each(&:save) + def record_counts + RECORD_COUNT_MODELS.transform_values(&:count) 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[:dditional], - letter_owner: LetterOwner.find_or_create_by(label: row[:ownerrights]), - file_folder: FileFolder.find_or_create_by(label: row[:file]), - typed: row[:autograph_or_typed] == 'T', - signed: row[:initialed_or_signed] == 'S', - envelope: row[:envelope] == 'E', - verified: row[:verified] == 'Y' - } - - letter.origins.clear - letter.destinations.clear - letter.recipients.clear - letter.repositories.clear - letter.senders.clear - letter.collections.clear - letter.languages.clear - - 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 + @row_errors = [] + @row_skipped = [] - 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 + rows.each {|row| process_row(row) } - 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_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 - - 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 - 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 + report_results(rows.size) + 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 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 - 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 + return if @row_errors.empty? - 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 + 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 - # rubocop:disable Style/SoleNestedConditional - if row[:first_repository] - repository = Repository.find_or_initialize_by(label: row[:first_repository]) + def process_row(row) + letter = get_letter(row) - if repository.new_record? - repository.published = row[:first_public].downcase == 'public' if row[:first_public] - end + 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 + @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 - repository.save + 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 - repository.format = row[:first_format] - repository.american = row[:euro_or_am].downcase == 'american' if row[:euro_or_am] - collection = nil + def set_letter_attributes(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' + } + end - begin - if row[:first_collection] - collection = Collection.find_or_create_by(label: row[:first_collection]) - collection.update(url: row[:collection_url]) + def clear_associations(letter) + letter.origins.clear + letter.destinations.clear + letter.recipients.clear + letter.repositories.clear + letter.senders.clear + letter.collections.clear + letter.languages.clear + end - repository.collections << collection unless repository.collections.include?(collection) + def assign_date(row, letter) + row = fix_date(row) + 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 - letter.collections << collection unless letter.collections.include?(collection) + def assign_origins(row, letter) + ORIGIN_FIELDS.each do |field| + value = row[field] + next if value.blank? - end - repository.save + 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 - 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]) + 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? + 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 - # 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 + def assign_destinations(row, letter) + DESTINATION_FIELDS.each do |field| + value = row[field] + next if value.blank? - if row[:second_repository] - repository = Repository.find_or_initialize_by(label: row[:second_repository]) + 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 - if repository.new_record? - repository.published = row[:second_public].downcase == 'public' if row[:second_public] - end + def assign_repository_slot(row, letter, slot) + return if row[slot[:repository]].blank? - repository.format = row[:second_format] - repository.save + repository = find_or_initialize_by_label(Repository, row[slot[:repository]]) - collection = nil - begin - if row[:second_collection] - collection = Collection.find_or_create_by(label: row[:second_collection]) + if repository.new_record? && row[slot[:public]] + repository.published = row[slot[:public]].to_s.strip.downcase == 'public' + end - repository.collections << collection unless repository.collections.include?(collection) + repository.save - letter.collections << collection unless letter.collections.include?(collection) - end + repository.format = row[slot[:format]] + repository.american = row[:euro_or_am].downcase == 'american' if row[:euro_or_am] - repository.save + 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] - 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 + repository.collections << collection unless repository.collections.include?(collection) + letter.collections << collection unless letter.collections.include?(collection) end - if row[:third_repository] - repository = Repository.find_or_initialize_by(label: row[:third_repository]) + repository.save - if repository.new_record? - repository.published = row[:third_public].downcase == 'public' if row[:third_public] - end - - repository.format = row[:third_format] - repository.save - - collection = nil - begin - if row[:second_collection] - collection = Collection.find_or_create_by(label: row[:second_collection]) - - repository.collections << collection unless repository.collections.include?(collection) + 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 - letter.collections << collection unless letter.collections.include?(collection) - end + def assign_volume(row, letter) + return unless row[:volumeinfo] - 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 - 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? - letter.letter_publisher = LetterPublisher.find_or_create_by(label: row[:placeprevpubl]) if row[:placeprevpubl] + letter.letter_publisher = find_or_create_by_label(LetterPublisher, row[:placeprevpubl]) + end - 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 + def assign_senders(row, letter) + 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 + end - 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 + 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) + rescue ActiveRecord::RecordInvalid, + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound + end + end - letter.typed = row[:autograph_or_typed] == 'T' + def get_letter(row) + if row[:exclude].to_s.strip.downcase == 'y' + letter = Letter.find_by(legacy_pk: row[:id]) + # 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 + raise SkipRow, 'excluded' + end - letter.signed = row[:initialed_or_signed] == 'S' + # 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.envelope = row[:envelope] == 'E' + Letter.find_or_create_by(legacy_pk: row[:id]) + end - letter.save - end + def normalize_label(value) + value.to_s.strip.squeeze(' ') + end - BigSam.last.destroy + def find_or_create_by_label(klass, label) + clean = normalize_label(label) + return nil if clean.blank? - logger.info { "#{Time.zone.now} ALL DONE" } + klass.find_by('lower(label) = ?', clean.downcase) || klass.create(label: clean) end - def get_letter(row) - if row[:exclude] == 'y' - letter = Letter.find_by(legacy_pk: row[:id]) - letter&.destroy - return nil - end + def find_or_initialize_by_label(klass, label) + clean = normalize_label(label) + return nil if clean.blank? - Letter.find_or_create_by(legacy_pk: row[:id]) + klass.find_by('lower(label) = ?', clean.downcase) || klass.new(label: clean) end def get_entity(label: nil, type: nil, return_nil: false) 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/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 diff --git a/app/models/letter.rb b/app/models/letter.rb index 0417604..f4c780d 100644 --- a/app/models/letter.rb +++ b/app/models/letter.rb @@ -66,6 +66,16 @@ 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). 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) Searchkick.callbacks(:inline) { published_letter&.reindex } 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/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/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..2fc6d3e 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: <%= ENV['DB_USERNAME'] || Rails.application.credentials.dig(:test, :db_user) %> + password: <%= ENV['DB_PASSWORD'] || Rails.application.credentials.dig(:test, :db_pw) %> development: <<: *default 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/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/jobs/load_big_sam_job_spec.rb b/spec/jobs/load_big_sam_job_spec.rb index 37b6324..4271655 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) @@ -10,6 +34,37 @@ 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) @@ -33,6 +88,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 +142,195 @@ 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) + + 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 + + it 'does not destroy the letter during a dry run' do + Letter.create!(legacy_pk: 43) + job.instance_variable_set(:@dry_run, true) + + 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 + + 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 + + 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 + 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 '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), + 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 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 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')]