Add admin test coverage and fix two letters-form crashes

Gemfile

@@ -62,6 +62,13 @@ group :development, :test do
   gem 'shoulda-matchers'
 end
 
+group :test do
+  # For system specs (spec/system). selenium-webdriver >= 4.6 manages the
+  # chromedriver binary itself (Selenium Manager) - no separate webdriver gem needed.
+  gem 'capybara'
+  gem 'selenium-webdriver'
+end
+
 gem 'pundit', '~> 2.2'
 
 gem 'kaminari', '~> 1.2'

Gemfile.lock

@@ -122,6 +122,15 @@ GEM
     bootsnap (1.18.4)
       msgpack (~> 1.2)
     builder (3.3.0)
+    capybara (3.40.0)
+      addressable
+      matrix
+      mini_mime (>= 0.1.3)
+      nokogiri (~> 1.11)
+      rack (>= 1.6.0)
+      rack-test (>= 0.6.3)
+      regexp_parser (>= 1.5, < 3.0)
+      xpath (~> 3.2)
     concurrent-ruby (1.3.4)
     connection_pool (2.4.1)
     crass (1.0.6)
@@ -226,6 +235,7 @@ GEM
       net-pop
       net-smtp
     marcel (1.0.4)
+    matrix (0.4.3)
     mime-types (3.5.2)
       mime-types-data (~> 3.2015)
     mime-types-data (3.2024.0820)
@@ -399,6 +409,12 @@ GEM
       hashie
     securerandom (0.3.1)
     selectize-rails (0.12.6)
+    selenium-webdriver (4.46.0)
+      base64 (~> 0.2)
+      logger (~> 1.4)
+      rexml (~> 3.2, >= 3.2.5)
+      rubyzip (>= 1.2.2, < 4.0)
+      websocket (~> 1.0)
     shoulda-matchers (6.4.0)
       activesupport (>= 5.2.0)
     sidekiq (7.3.8)
@@ -436,9 +452,12 @@ GEM
     uuid (2.3.9)
       macaddr (~> 1.0)
     webrick (1.8.1)
+    websocket (1.2.11)
     websocket-driver (0.7.6)
       websocket-extensions (>= 0.1.0)
     websocket-extensions (0.1.5)
+    xpath (3.2.0)
+      nokogiri (~> 1.8)
     zeitwerk (2.6.17)
 
 PLATFORMS
@@ -468,6 +487,7 @@ DEPENDENCIES
   aws-sdk-rails (~> 5.0)
   aws-sdk-ses (~> 1.0)
   bootsnap
+  capybara
   csv
   debug
   elasticsearch (~> 8)
@@ -493,6 +513,7 @@ DEPENDENCIES
   rubocop-rspec
   rubocop-rspec_rails
   searchkick
+  selenium-webdriver
   shoulda-matchers
   sidekiq (>= 7.2.2, < 8)
   sidekiq-cron (~> 2.4.0)

app/fields/has_many_through_field.rb

@@ -8,21 +8,19 @@ def to_s
   end
 
   def associated_resource_options
-    # is_entities = resource['_index']&.include?('entities')
-    # where = {}
-    # order = {}
-    # where[options[:type]] = options[:type] if options[:type]
-    # order[options[:order_by]] = :acs if options[:order_by]
-    # associated_class.search('*', load: false, order:, where:).map do |resource|
-    #   if options[:verbose_option] && is_entities
-    #     ["#{resource.e_type.titleize} #{resource.legacy_pk}: #{resource.clean_label}", resource.id]
-    #   else
-    #     [resource.clean_label, resource.id]
-    #   end
-    # end
     where = {}
     where[:e_type] = options[:type] if options[:type]
-    associated_class.search('*', load: false, order: { e_type: :asc }, where:).map do |resource|
+
+    # Sorting here in Ruby, rather than passing `order:` to .search, is deliberate:
+    # an ES-level sort requires the field to have a keyword/sortable mapping, which
+    # e_type doesn't (Searchable, app/models/concerns/searchable.rb, never declares
+    # one) - that was raising a Searchkick::InvalidQueryError for every letter's
+    # entity picker (order: { e_type: :asc } was hardcoded here regardless of the
+    # order_by option below actually being requested).
+    results = associated_class.search('*', load: false, where:)
+    results = results.sort_by {|resource| resource.public_send(options[:order_by]).to_s } if options[:order_by]
+
+    results.map do |resource|
       if options[:verbose_option] && resource['_index'].include?('entities')
         ["#{resource.e_type.titleize} #{resource.legacy_pk}: #{resource.clean_label}", resource.id]
       else

app/views/admin/letters/_form.html.erb

@@ -188,10 +188,6 @@ and renders all form fields for a resource's editable attributes.
   }
 
   defer(() => {
-    let authenticity_token = document.querySelector("[name='authenticity_token']").value;
-    let headers = {
-      "content-type": "application/x-www-form-urlencoded"
-    };
     // let letter = "<%= page.resource.id %>"
     const input = document.createElement("input");
     input.type = "hidden";

config/environments/production.rb

@@ -63,12 +63,12 @@
   # 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.
+  # Send mail through SES. Registration (and the region setting) lives in
+  # config/initializers/action_mailer_ses.rb / app/lib/ses_delivery_method.rb, not
+  # here - see the comment there for why. Authenticates 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/initializers/action_mailer_ses.rb

@@ -4,4 +4,10 @@
 # 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
+# Passed here, as add_delivery_method's default_options, rather than via
+# config.action_mailer.ses_settings in config/environments/*.rb: referencing
+# ActionMailer::Base below is what triggers its first load in this app, and Rails
+# applies config.action_mailer.* settings via a load hook that fires on that same
+# first load - so a `ses_settings=` assigned in production.rb would run before this
+# add_delivery_method call has defined that setter, raising NoMethodError.
+ActionMailer::Base.add_delivery_method :ses, SesDeliveryMethod, region: ENV.fetch('AWS_REGION', 'us-east-1')

spec/dashboards/about_page_dashboard_spec.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe AboutPageDashboard do
+  it_behaves_like 'a dashboard with display_resource',
+                  factory: :about_page,
+                  expected: ->(about_page) { "AboutPage ##{about_page.title}" }
+end

spec/dashboards/faq_dashboard_spec.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe FaqDashboard do
+  it_behaves_like 'a dashboard with display_resource',
+                  factory: :faq,
+                  expected: ->(faq) { "Faq ##{faq.question}" }
+end

spec/dashboards/letter_dashboard_spec.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe LetterDashboard do
+  it_behaves_like 'a dashboard with display_resource',
+                  factory: :letter,
+                  expected: ->(letter) { "Letter ##{letter.legacy_pk}" }
+
+  describe '#permitted_attributes' do
+    it 'adds start_date on top of the default FORM_ATTRIBUTES-derived list' do
+      # FORM_ATTRIBUTES is %i[entities content] - `entities` (a has-many field)
+      # permits as {entity_ids: []}, `content` permits as itself, and this override
+      # adds start_date and content again (already present via FORM_ATTRIBUTES, so
+      # a harmless duplicate rather than a second distinct attribute).
+      expect(described_class.new.permitted_attributes).to eq(
+        [{ entity_ids: [] }, :content, :start_date, :content]
+      )
+    end
+  end
+end

spec/dashboards/letter_owner_dashboard_spec.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe LetterOwnerDashboard do
+  it_behaves_like 'a dashboard with display_resource',
+                  factory: :letter_owner,
+                  expected: ->(letter_owner) { letter_owner.label }
+end

spec/dashboards/letter_publisher_dashboard_spec.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe LetterPublisherDashboard do
+  it_behaves_like 'a dashboard with display_resource',
+                  factory: :letter_publisher,
+                  expected: ->(letter_publisher) { letter_publisher.label }
+end

spec/dashboards/repository_dashboard_spec.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe RepositoryDashboard do
+  it_behaves_like 'a dashboard with display_resource',
+                  factory: :repository,
+                  expected: ->(repository) { "Repository #{repository.label}" }
+end

spec/fields/contenteditable_field_spec.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+require 'administrate/field/base'
+
+RSpec.describe ContenteditableField do
+  describe '#to_s' do
+    it 'returns the raw data unchanged, with no sanitization' do
+      html = '<p>raw <script>alert(1)</script> html</p>'
+      field = described_class.new(:content, html, nil)
+
+      expect(field.to_s).to eq(html)
+    end
+  end
+end

spec/fields/has_many_through_field_spec.rb

@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+# In a full app boot Administrate's engine loads its own Field classes before any
+# app/fields/*.rb file is ever referenced, but running this spec in isolation can
+# autoload HasManyThroughField (which inherits from Administrate::Field::HasMany)
+# before that happens.
+require 'administrate/field/has_many'
+
+RSpec.describe HasManyThroughField do
+  let(:letter) { Letter.new }
+
+  def field_for(options)
+    described_class.new(:entities, nil, nil, options.merge(resource: letter))
+  end
+
+  describe '#to_s' do
+    it 'returns the raw data' do
+      field = described_class.new(:entities, %w[a b], nil, resource: letter)
+      expect(field.to_s).to eq(%w[a b])
+    end
+  end
+
+  describe '#associated_resource_options' do
+    it 'scopes the search to options[:type] when given' do
+      field = field_for(type: 'person')
+      allow(Entity).to receive(:search).and_return([])
+
+      field.associated_resource_options
+
+      expect(Entity).to have_received(:search).with('*', load: false, where: { e_type: 'person' })
+    end
+
+    it 'does not scope the search when no type option is given' do
+      field = field_for({})
+      allow(Entity).to receive(:search).and_return([])
+
+      field.associated_resource_options
+
+      expect(Entity).to have_received(:search).with('*', load: false, where: {})
+    end
+
+    it 'does not ask Elasticsearch to sort - e_type has no sortable mapping' do
+      # Regression test: this field used to hardcode order: { e_type: :asc } in the
+      # Searchkick query regardless of any order_by option, which raised
+      # Searchkick::InvalidQueryError for every letter's entity picker (e_type isn't
+      # mapped as sortable - see app/models/concerns/searchable.rb). Sorting now
+      # happens in Ruby, after the results come back - see the next example.
+      field = field_for(order_by: 'e_type')
+      allow(Entity).to receive(:search).and_return([])
+
+      field.associated_resource_options
+
+      expect(Entity).to have_received(:search).with('*', load: false, where: {})
+    end
+
+    it 'sorts the results in Ruby by options[:order_by] when given' do
+      person = instance_double(Entity, e_type: 'person', clean_label: 'B Person', id: '1')
+      allow(person).to receive(:[]).with('_index').and_return('beckett_entities_test')
+      place = instance_double(Entity, e_type: 'place', clean_label: 'A Place', id: '2')
+      allow(place).to receive(:[]).with('_index').and_return('beckett_entities_test')
+      field = field_for(order_by: 'e_type')
+      allow(Entity).to receive(:search).and_return([person, place])
+
+      # "person" sorts before "place" alphabetically (e < l at the second character)
+      expect(field.associated_resource_options).to eq([['B Person', '1'], ['A Place', '2']])
+    end
+
+    it 'formats verbose entity results as "Type PK: Label"' do
+      result = instance_double(Entity, e_type: 'person', legacy_pk: 42, clean_label: 'Beckett, Samuel', id: 'abc')
+      allow(result).to receive(:[]).with('_index').and_return('beckett_entities_test')
+      field = field_for(verbose_option: true)
+      allow(Entity).to receive(:search).and_return([result])
+
+      expect(field.associated_resource_options).to eq([['Person 42: Beckett, Samuel', 'abc']])
+    end
+
+    it 'formats non-verbose results as [label, id]' do
+      result = instance_double(Entity, clean_label: 'Paris', id: 'xyz')
+      allow(result).to receive(:[]).with('_index').and_return('beckett_places_test')
+      field = field_for({})
+      allow(Entity).to receive(:search).and_return([result])
+
+      expect(field.associated_resource_options).to eq([%w[Paris xyz]])
+    end
+
+    it 'formats verbose results as [label, id] when the result is not from the entities index' do
+      result = instance_double(Entity, clean_label: 'Some Repository', id: 'def')
+      allow(result).to receive(:[]).with('_index').and_return('beckett_repositories_test')
+      field = field_for(verbose_option: true)
+      allow(Entity).to receive(:search).and_return([result])
+
+      expect(field.associated_resource_options).to eq([['Some Repository', 'def']])
+    end
+  end
+end

spec/fields/rich_text_field_spec.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+require 'administrate/field/base'
+
+RSpec.describe RichTextField do
+  describe '#to_s' do
+    it 'permits the allowed tags and attributes' do
+      html = '<p><strong>Bold</strong> <a href="/x" class="link">text</a></p>'
+      field = described_class.new(:description, html, nil)
+
+      expect(field.to_s).to eq(html)
+    end
+
+    it 'strips the script tag itself (its text content survives as inert text, same as Rails::Html::Sanitizer)' do
+      field = described_class.new(:description, '<p>safe</p><script>alert(1)</script>', nil)
+
+      result = field.to_s
+
+      expect(result).not_to include('<script')
+      expect(result).to include('safe')
+    end
+
+    it 'strips event handler attributes like onclick' do
+      field = described_class.new(:description, '<p onclick="alert(1)">click me</p>', nil)
+
+      result = field.to_s
+
+      expect(result).not_to include('onclick')
+      expect(result).not_to include('alert(1)')
+      expect(result).to include('click me')
+    end
+
+    it 'strips disallowed tags but keeps their text content' do
+      field = described_class.new(:description, '<iframe src="https://evil.example"></iframe>plain text', nil)
+
+      result = field.to_s
+
+      expect(result).not_to include('<iframe')
+      expect(result).not_to include('evil.example')
+      expect(result).to include('plain text')
+    end
+
+    it 'strips javascript: hrefs' do
+      field = described_class.new(:description, '<a href="javascript:alert(1)">click</a>', nil)
+
+      result = field.to_s
+
+      expect(result).not_to include('javascript:')
+    end
+  end
+end