From cefbd8b44a1fd8a22565a7df91e50c7e332e177b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 14 Sep 2026 07:54:17 +0000 Subject: [PATCH 1/2] Bound Activity collection on participation admin forms Stop loading every past Activity into the ActiveAdmin new/edit select. The form now queries a column-narrowed, LIMIT 200 coming and past collection and always keeps the assigned activity. Co-authored-by: Thibaud Guillaume-Gentil --- app/admin/activity_participation.rb | 2 +- app/helpers/admin_helper.rb | 8 ++ app/models/activity.rb | 20 +++ ...activity_participations_controller_test.rb | 129 ++++++++++++++++++ test/models/activity_test.rb | 45 ++++++ test/support/activities_helper.rb | 19 +++ 6 files changed, 222 insertions(+), 1 deletion(-) create mode 100644 test/controllers/activity_participations_controller_test.rb diff --git a/app/admin/activity_participation.rb b/app/admin/activity_participation.rb index 169f0c6ca..fe794b895 100644 --- a/app/admin/activity_participation.rb +++ b/app/admin/activity_participation.rb @@ -198,7 +198,7 @@ form do |f| f.inputs t(".details"), icon: "notebook-text" do f.input :activity, - collection: grouped_by_date(Activity), + collection: activity_participation_form_activities_collection(f.object), prompt: true f.input :member, collection: members_collection, diff --git a/app/helpers/admin_helper.rb b/app/helpers/admin_helper.rb index 3aadb725c..197ec6cb9 100644 --- a/app/helpers/admin_helper.rb +++ b/app/helpers/admin_helper.rb @@ -79,6 +79,14 @@ def member_cities_collection Member.pluck(:city).uniq.map(&:presence).compact.sort end + def activity_participation_form_activities_collection(participation = nil) + collection = Activity.admin_form_collection(selected: participation&.activity) + [ + [ t("active_admin.scopes.coming"), option_for_select(collection[:coming]) ], + [ t("active_admin.scopes.past"), option_for_select(collection[:past]) ] + ] + end + def grouped_by_date(relation, past: :last) if fy_year = params.dig(:q, :during_year) relation = relation.during_year(fy_year) diff --git a/app/models/activity.rb b/app/models/activity.rb index 2a1cab850..3b28fa96a 100644 --- a/app/models/activity.rb +++ b/app/models/activity.rb @@ -8,6 +8,9 @@ class Activity < ApplicationRecord include BulkDatesInsert include Availability, Presetable + ADMIN_FORM_COLLECTION_LIMIT = 200 + ADMIN_FORM_COLLECTION_COLUMNS = %i[id date start_time end_time places].freeze + attribute :start_time, :time_only attribute :end_time, :time_only @@ -26,6 +29,23 @@ class Activity < ApplicationRecord validate :period_duration_must_one_hour scope :ordered, ->(order) { order(date: order, start_time: :asc) } + scope :admin_form_select, -> { select(*ADMIN_FORM_COLLECTION_COLUMNS) } + + def self.admin_form_collection(selected: nil) + scope = admin_form_select + coming = scope.coming.order(:date).limit(ADMIN_FORM_COLLECTION_LIMIT).to_a + past = scope.past.reorder(date: :desc).limit(ADMIN_FORM_COLLECTION_LIMIT).to_a + + if selected + if selected.coming? + coming << selected unless coming.any? { |activity| activity.id == selected.id } + elsif selected.past? + past.unshift(selected) unless past.any? { |activity| activity.id == selected.id } + end + end + + { coming: coming, past: past } + end def display_name name diff --git a/test/controllers/activity_participations_controller_test.rb b/test/controllers/activity_participations_controller_test.rb new file mode 100644 index 000000000..20a7613ed --- /dev/null +++ b/test/controllers/activity_participations_controller_test.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +require "test_helper" + +class ActivityParticipationsControllerTest < ActionDispatch::IntegrationTest + setup do + host! "admin.acme.test" + travel_to "2024-09-11" + login admins(:super) + end + + def login(admin) + session = Session.create!( + admin_email: admin.email, + remote_addr: "127.0.0.1", + user_agent: "Test Browser") + get "/sessions/#{session.generate_token_for(:redeem)}" + end + + test "new form uses coming and past activity optgroups" do + coming = create_activity(date: Date.current + 1.week) + + get new_activity_participation_path + + assert_response :success + assert_select "select#activity_participation_activity_id" do + assert_select "optgroup[label=?]", I18n.t("active_admin.scopes.coming") do + assert_select "option[value=?]", coming.id + end + assert_select "optgroup[label=?]", I18n.t("active_admin.scopes.past") do + assert_select "option[value=?]", activities(:harvest).id + end + end + end + + test "edit form keeps the assigned activity selected" do + participation = activity_participations(:john_harvest) + + get edit_activity_participation_path(participation) + + assert_response :success + assert_select "select#activity_participation_activity_id option[value=?][selected]", + participation.activity_id + end + + test "edit form keeps an assigned activity that is older than the collection limit" do + extras = insert_admin_form_activities!( + (0...(Activity::ADMIN_FORM_COLLECTION_LIMIT + 5)).map { |i| Date.new(2019, 1, 1) + i.days }) + oldest = extras.min_by(&:date) + dropped = extras.sort_by(&:date)[1] + participation = ActivityParticipation.create!( + member: members(:martha), + activity: oldest, + participants_count: 1) + + get edit_activity_participation_path(participation) + + assert_response :success + assert_select "select#activity_participation_activity_id option[value=?][selected]", oldest.id + assert_select "select#activity_participation_activity_id option[value=?]", dropped.id, count: 0 + end + + test "new and edit form activity queries stay bounded as activity count grows" do + participation = activity_participations(:john_harvest) + insert_admin_form_activities!( + (0...Activity::ADMIN_FORM_COLLECTION_LIMIT).map { |i| Date.new(2020, 1, 1) + i.days }) + + few_new = collect_sql_queries { get new_activity_participation_path } + assert_response :success + assert_bounded_activity_form_queries(few_new) + + few_edit = collect_sql_queries { get edit_activity_participation_path(participation) } + assert_response :success + assert_bounded_activity_form_queries(few_edit) + + insert_admin_form_activities!( + (0...Activity::ADMIN_FORM_COLLECTION_LIMIT).map { |i| Date.new(2018, 1, 1) + i.days }) + + many_new = collect_sql_queries { get new_activity_participation_path } + assert_response :success + assert_bounded_activity_form_queries(many_new) + + many_edit = collect_sql_queries { get edit_activity_participation_path(participation) } + assert_response :success + assert_bounded_activity_form_queries(many_edit) + + assert_equal activity_form_load_signatures(few_new), activity_form_load_signatures(many_new) + assert_equal activity_form_load_signatures(few_edit), activity_form_load_signatures(many_edit) + end + + private + + def collect_sql_queries + queries = [] + callback = ->(_name, _start, _finish, _id, payload) { + sql = payload[:sql] + queries << sql unless payload[:name] == "SCHEMA" || sql.match?(/\A(?:BEGIN|COMMIT|SAVEPOINT|RELEASE)/i) + } + ActiveSupport::Notifications.subscribed(callback, "sql.active_record") { yield } + queries + end + + def activity_collection_loads(queries) + queries.select { |sql| + sql.match?(/SELECT .+FROM ["`]activities["`]/i) && + sql.match?(/["`]activities["`]\.["`]date["`]/) && + !sql.match?(/SELECT 1 AS one/i) && + !sql.match?(/COUNT\(/i) + } + end + + def activity_form_load_signatures(queries) + activity_collection_loads(queries).map { |sql| sql.gsub(/\d+/, "N") } + end + + def assert_bounded_activity_form_queries(queries) + loads = activity_collection_loads(queries) + assert_equal 2, loads.size, "expected coming and past Activity collection loads, got:\n#{loads.join("\n")}" + loads.each do |sql| + assert_match(/LIMIT #{Activity::ADMIN_FORM_COLLECTION_LIMIT}\b/i, sql) + assert_no_match(/\bSELECT\s+(?:["`]?\w+["`]?\.)?\*/i, sql) + assert_match(/["`]date["`]/, sql) + assert_match(/["`]places["`]/, sql) + assert_no_match(/["`]descriptions["`]/, sql) + assert_no_match(/["`]titles["`]/, sql) + assert_no_match(/["`]place_urls["`]/, sql) + end + end +end diff --git a/test/models/activity_test.rb b/test/models/activity_test.rb index 9f772ce8e..7e5f5f3fe 100644 --- a/test/models/activity_test.rb +++ b/test/models/activity_test.rb @@ -51,4 +51,49 @@ def setup assert_equal "8:30-12:00", activity.period end + + test "admin_form_collection limits past and coming activities and keeps the selected one" do + travel_to "2024-09-11" + overflow = Activity::ADMIN_FORM_COLLECTION_LIMIT + 2 + extras = insert_admin_form_activities!( + (0...overflow).map { |i| Date.new(2019, 1, 1) + i.days } + + (1..overflow).map { |i| Date.current + i.days }) + oldest = extras.min_by(&:date) + dropped_past = extras.sort_by(&:date).second + nearest_coming = extras.select { |activity| activity.date.future? }.min_by(&:date) + farthest_coming = extras.max_by(&:date) + + collection = Activity.admin_form_collection(selected: oldest) + + assert_equal Activity::ADMIN_FORM_COLLECTION_LIMIT, collection[:coming].size + assert_equal Activity::ADMIN_FORM_COLLECTION_LIMIT + 1, collection[:past].size + assert_includes collection[:past].map(&:id), oldest.id + assert_not_includes collection[:past].map(&:id), dropped_past.id + assert_includes collection[:coming].map(&:id), nearest_coming.id + assert_not_includes collection[:coming].map(&:id), farthest_coming.id + end + + test "admin_form_collection queries are column-narrowed and limited" do + travel_to "2024-09-11" + insert_admin_form_activities!((0..5).map { |i| Date.current - (i + 1).weeks }) + + queries = [] + callback = ->(_name, _start, _finish, _id, payload) { queries << payload[:sql] } + ActiveSupport::Notifications.subscribed(callback, "sql.active_record") do + Activity.admin_form_collection + end + + collection_loads = queries.select { |sql| + sql.match?(/FROM ["`]activities["`]/i) && sql.match?(/["`]date["`]/) + } + assert_equal 2, collection_loads.size + collection_loads.each do |sql| + assert_match(/LIMIT #{Activity::ADMIN_FORM_COLLECTION_LIMIT}\b/i, sql) + assert_no_match(/\bSELECT\s+(?:["`]?\w+["`]?\.)?\*/i, sql) + assert_match(/["`]places["`]/, sql) + assert_no_match(/["`]descriptions["`]/, sql) + assert_no_match(/["`]titles["`]/, sql) + assert_no_match(/["`]place_urls["`]/, sql) + end + end end diff --git a/test/support/activities_helper.rb b/test/support/activities_helper.rb index 18af97d47..b4490e831 100644 --- a/test/support/activities_helper.rb +++ b/test/support/activities_helper.rb @@ -10,4 +10,23 @@ def create_activity(attributes = {}) }.merge(attributes)) Activity.find(activity.id) # hard reload to get preset end + + def insert_admin_form_activities!(dates) + now = Time.current + Activity.insert_all(dates.map { |date| + { + date: date, + start_time: "08:00", + end_time: "10:00", + places: { "en" => "Farm" }, + titles: { "en" => "Extra" }, + descriptions: {}, + place_urls: {}, + visible: true, + created_at: now, + updated_at: now + } + }) + Activity.where(date: dates).order(:date) + end end From 9a2791b056994695045d5b0511cefd20b07bc5e8 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 14 Sep 2026 07:58:13 +0000 Subject: [PATCH 2/2] Match SQLite parameterized LIMIT in form query tests Activity collection loads bind LIMIT as ?, so assert the clause instead of a literal 200. Co-authored-by: Thibaud Guillaume-Gentil --- test/controllers/activity_participations_controller_test.rb | 2 +- test/models/activity_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/controllers/activity_participations_controller_test.rb b/test/controllers/activity_participations_controller_test.rb index 20a7613ed..de9e3552a 100644 --- a/test/controllers/activity_participations_controller_test.rb +++ b/test/controllers/activity_participations_controller_test.rb @@ -117,7 +117,7 @@ def assert_bounded_activity_form_queries(queries) loads = activity_collection_loads(queries) assert_equal 2, loads.size, "expected coming and past Activity collection loads, got:\n#{loads.join("\n")}" loads.each do |sql| - assert_match(/LIMIT #{Activity::ADMIN_FORM_COLLECTION_LIMIT}\b/i, sql) + assert_match(/LIMIT/i, sql) assert_no_match(/\bSELECT\s+(?:["`]?\w+["`]?\.)?\*/i, sql) assert_match(/["`]date["`]/, sql) assert_match(/["`]places["`]/, sql) diff --git a/test/models/activity_test.rb b/test/models/activity_test.rb index 7e5f5f3fe..c94d30330 100644 --- a/test/models/activity_test.rb +++ b/test/models/activity_test.rb @@ -88,7 +88,7 @@ def setup } assert_equal 2, collection_loads.size collection_loads.each do |sql| - assert_match(/LIMIT #{Activity::ADMIN_FORM_COLLECTION_LIMIT}\b/i, sql) + assert_match(/LIMIT/i, sql) assert_no_match(/\bSELECT\s+(?:["`]?\w+["`]?\.)?\*/i, sql) assert_match(/["`]places["`]/, sql) assert_no_match(/["`]descriptions["`]/, sql)