From 672fe4a7ba233b28bff36e43257bd907914d35f5 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Mon, 15 Jun 2026 09:57:38 +0200 Subject: [PATCH 1/2] fix: validate sponsor seats and number_of_coaches presence Organisers could create sponsors without specifying seat/coach counts, saving nil to the database. The show page then crashed on coach_spots when evaluating nil / 2.0, manifesting as a 404. - Add model validations (presence + numericality >= 0) - Enable HTML5 browser validation on the form - Add fabricator defaults and fix test coverage Closes #1794. Supersedes #1803. --- app/models/sponsor.rb | 2 ++ app/views/admin/sponsors/_form.html.haml | 6 +++--- spec/controllers/admin/sponsors_controller_spec.rb | 8 ++++---- spec/fabricators/sponsor_fabricator.rb | 2 ++ spec/features/admin/manage_sponsor_spec.rb | 3 +++ spec/models/sponsor_spec.rb | 4 ++++ 6 files changed, 18 insertions(+), 7 deletions(-) diff --git a/app/models/sponsor.rb b/app/models/sponsor.rb index ad411e8c2..52b5b8ce3 100644 --- a/app/models/sponsor.rb +++ b/app/models/sponsor.rb @@ -32,6 +32,8 @@ class Sponsor < ApplicationRecord validates :level, inclusion: { in: Sponsor.levels.keys } validates :name, :address, :avatar, :website, :level, presence: true + validates :number_of_coaches, presence: true, numericality: { greater_than_or_equal_to: 0, only_integer: true } + validates :seats, presence: true, numericality: { greater_than_or_equal_to: 0, only_integer: true } validate :website_is_url, if: :website? default_scope -> { order('updated_at desc') } diff --git a/app/views/admin/sponsors/_form.html.haml b/app/views/admin/sponsors/_form.html.haml index 42e3c7d3c..4d2d6e451 100644 --- a/app/views/admin/sponsors/_form.html.haml +++ b/app/views/admin/sponsors/_form.html.haml @@ -1,4 +1,4 @@ -= simple_form_for [:admin, @sponsor], :html => {:multipart => true, novalidate: true } do |f| += simple_form_for [:admin, @sponsor], html: { multipart: true, novalidate: false } do |f| .row .col-12.col-md-6 = f.input :name @@ -27,9 +27,9 @@ %p Only required for hosts. .row .col-6 - = f.input :seats + = f.input :seats, input_html: { required: true, min: 0 } .col-6 - = f.input :number_of_coaches + = f.input :number_of_coaches, input_html: { required: true, min: 0 } = f.input :accessibility_info, input_html: { rows: 3 }, hint: raw(t('admin.shared.markdown_hint', link: link_to(t('admin.shared.markdown'), 'https://commonmark.org/help/'))) %p diff --git a/spec/controllers/admin/sponsors_controller_spec.rb b/spec/controllers/admin/sponsors_controller_spec.rb index a3061f45a..14f6174bc 100644 --- a/spec/controllers/admin/sponsors_controller_spec.rb +++ b/spec/controllers/admin/sponsors_controller_spec.rb @@ -39,7 +39,7 @@ expect do post :create, params: { sponsor: { - name: 'name', website: 'https://example.com', seats: 40, + name: 'name', website: 'https://example.com', seats: 40, number_of_coaches: 10, address: address, avatar: avatar } } @@ -53,7 +53,7 @@ expect do post :create, params: { sponsor: { - name: 'name', website: 'https://example.com', seats: 40, + name: 'name', website: 'https://example.com', seats: 40, number_of_coaches: 10, address: address, avatar: avatar, contact_ids: [member.id, member1.id] } } @@ -67,7 +67,7 @@ expect do post :create, params: { sponsor: { - name: 'name', website: 'https://example.com', seats: 40, + name: 'name', website: 'https://example.com', seats: 40, number_of_coaches: 10, address: address, avatar: avatar, contact_ids: [member.id, member1.id] } } @@ -81,7 +81,7 @@ expect do post :create, params: { sponsor: { - name: 'name', website: 'https://example.com', seats: 40, + name: 'name', website: 'https://example.com', seats: 40, number_of_coaches: 10, address: Fabricate(:address, latitude: '54.47474', longitude: '-0.12345'), avatar: avatar, members: [] } diff --git a/spec/fabricators/sponsor_fabricator.rb b/spec/fabricators/sponsor_fabricator.rb index 1c4327a93..7b3e25ce8 100644 --- a/spec/fabricators/sponsor_fabricator.rb +++ b/spec/fabricators/sponsor_fabricator.rb @@ -10,6 +10,8 @@ website { "http://#{Faker::Internet.domain_name}" } avatar { Rack::Test::UploadedFile.new(Rails.root.join('app', 'assets', 'images', 'sponsors', sponsor_image), 'image/png') } address + seats { Faker::Number.between(from: 1, to: 50) } + number_of_coaches { Faker::Number.between(from: 1, to: 50) } end Fabricator(:sponsor_full, from: :sponsor) do diff --git a/spec/features/admin/manage_sponsor_spec.rb b/spec/features/admin/manage_sponsor_spec.rb index 0e0e97720..6e3586903 100644 --- a/spec/features/admin/manage_sponsor_spec.rb +++ b/spec/features/admin/manage_sponsor_spec.rb @@ -16,6 +16,7 @@ fill_in 'Website', with: 'https://www.sponsorname.com/' attach_file('Avatar', Rails.root + 'spec/support/codebar-logo.png') fill_in 'Student spots', with: 20 + fill_in 'Coach spots', with: 10 select "Bronze", from: "Level" click_on 'Create sponsor' @@ -32,6 +33,7 @@ fill_in 'Website', with: 'https://www.sponsorname.com/' attach_file('Avatar', Rails.root + 'spec/support/codebar-logo.png') fill_in 'Student spots', with: 20 + fill_in 'Coach spots', with: 10 click_on 'Create sponsor' @@ -47,6 +49,7 @@ fill_in 'Website', with: 'https://www.sponsorname.com/' attach_file('Avatar', Rails.root + 'spec/support/codebar-logo.png') fill_in 'Student spots', with: 20 + fill_in 'Coach spots', with: 10 click_on 'Create sponsor' diff --git a/spec/models/sponsor_spec.rb b/spec/models/sponsor_spec.rb index b27f0583c..4feef1fb3 100644 --- a/spec/models/sponsor_spec.rb +++ b/spec/models/sponsor_spec.rb @@ -7,6 +7,10 @@ it { is_expected.to validate_presence_of(:avatar) } it { is_expected.to validate_presence_of(:website) } it { is_expected.to validate_presence_of(:level) } + it { is_expected.to validate_presence_of(:number_of_coaches) } + it { is_expected.to validate_presence_of(:seats) } + it { is_expected.to validate_numericality_of(:number_of_coaches).is_greater_than_or_equal_to(0).only_integer } + it { is_expected.to validate_numericality_of(:seats).is_greater_than_or_equal_to(0).only_integer } context 'scopes' do describe 'searching by_name' do From 10f531e2b4db3d9db85b46283839160cd977d74b Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Mon, 15 Jun 2026 10:17:47 +0200 Subject: [PATCH 2/2] fix: add migration to backfill nil sponsor number_of_coaches 89 production sponsors (11.9%) have nil number_of_coaches. Without backfill, editing them would fail the new presence validation. Uses the same formula as Sponsor#coach_spots: ROUND(seats / 2.0). --- ...615081352_backfill_sponsor_number_of_coaches.rb | 14 ++++++++++++++ db/schema.rb | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20260615081352_backfill_sponsor_number_of_coaches.rb diff --git a/db/migrate/20260615081352_backfill_sponsor_number_of_coaches.rb b/db/migrate/20260615081352_backfill_sponsor_number_of_coaches.rb new file mode 100644 index 000000000..14f664f11 --- /dev/null +++ b/db/migrate/20260615081352_backfill_sponsor_number_of_coaches.rb @@ -0,0 +1,14 @@ +class BackfillSponsorNumberOfCoaches < ActiveRecord::Migration[8.1] + # Backfill nil number_of_coaches using the same formula as Sponsor#coach_spots + # to ensure existing records pass the new presence validation when edited. + def up + Sponsor.where(number_of_coaches: nil) + .update_all("number_of_coaches = ROUND(seats / 2.0)") + end + + def down + # Intentionally irreversible: we can't distinguish original nils from + # intentionally-set values after backfill. + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index 1fc00de44..8fa211c5c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_04_10_174346) do +ActiveRecord::Schema[8.1].define(version: 2026_06_15_081352) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql"