From b70d6835cd7d481efbe93f47c39db6cc8858ec68 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 15:36:35 +0000 Subject: [PATCH] Paginate member newsletter list MailDelivery loads Members::NewsletterDeliveriesController#index loaded every processed newsletter delivery, including HTML content, before applying the page limit, then queried Newsletter once per row. Load one page without content, includes(:source_newsletter), and add (member_id, mailable_type, created_at) so the list query can seek and sort. Drop unused Newsletter.preload_sources!. Tests keep the query count bounded, reject SELECT *, and cover offset pagination. Co-authored-by: Thibaud Guillaume-Gentil --- .../newsletter_deliveries_controller.rb | 12 +- app/models/mail_delivery.rb | 5 +- app/models/newsletter/delivery.rb | 16 +-- ...ewsletter_list_index_to_mail_deliveries.rb | 8 ++ db/schema.rb | 3 +- .../newsletter_deliveries_controller_test.rb | 129 ++++++++++++++++++ test/models/newsletter/delivery_test.rb | 24 ++++ 7 files changed, 181 insertions(+), 16 deletions(-) create mode 100644 db/migrate/20260907120000_add_member_newsletter_list_index_to_mail_deliveries.rb diff --git a/app/controllers/members/newsletter_deliveries_controller.rb b/app/controllers/members/newsletter_deliveries_controller.rb index 230f41318..960a3c708 100644 --- a/app/controllers/members/newsletter_deliveries_controller.rb +++ b/app/controllers/members/newsletter_deliveries_controller.rb @@ -7,10 +7,14 @@ class Members::NewsletterDeliveriesController < Members::BaseController def index offset = params[:offset].to_i - @deliveries = Newsletter.deliveries_for(current_member) - @next_offset = offset + PER_PAGE - @next_offset = nil unless @deliveries.count > @next_offset - @deliveries = @deliveries.offset(offset).first(PER_PAGE) + page = Newsletter.deliveries_for(current_member) + .without_content + .offset(offset) + .limit(PER_PAGE + 1) + .to_a + + @next_offset = offset + PER_PAGE if page.size > PER_PAGE + @deliveries = page.first(PER_PAGE) end def show diff --git a/app/models/mail_delivery.rb b/app/models/mail_delivery.rb index 0067d482f..a1d02e29f 100644 --- a/app/models/mail_delivery.rb +++ b/app/models/mail_delivery.rb @@ -11,11 +11,14 @@ class MailDelivery < ApplicationRecord has_states :draft, :processing, :delivered, :partially_delivered, :not_delivered belongs_to :member + belongs_to :source_newsletter, class_name: "Newsletter", + foreign_key: :mailable_id, optional: true has_many :emails, class_name: "MailDelivery::Email", dependent: :destroy scope :processed, -> { where.not(state: [ :draft, :processing ]) } scope :newsletters, -> { where(mailable_type: "Newsletter") } scope :mail_templates, -> { where.not(mailable_type: "Newsletter") } + scope :without_content, -> { select(*(column_names - [ "content" ])) } scope :with_email, ->(email) { joins(:emails).merge(Email.with_email(email)) } @@ -120,7 +123,7 @@ def mailables def source @source ||= if newsletter? - mailables.first + source_newsletter else MailTemplate.find_by!(title: mail_template_title) end diff --git a/app/models/newsletter/delivery.rb b/app/models/newsletter/delivery.rb index a1aa87ab3..0b7ce38a2 100644 --- a/app/models/newsletter/delivery.rb +++ b/app/models/newsletter/delivery.rb @@ -17,16 +17,12 @@ module Delivery class_methods do def deliveries_for(member) - deliveries = - MailDelivery - .newsletters - .processed - .where(member: member) - .order(created_at: :desc) - newsletter_ids = deliveries.flat_map(&:mailable_ids).uniq - newsletters = Newsletter.where(id: newsletter_ids).index_by(&:id) - deliveries.each { |d| d.preload_source!(newsletters[d.mailable_ids.first]) } - deliveries + MailDelivery + .newsletters + .processed + .where(member: member) + .includes(:source_newsletter) + .order(created_at: :desc) end end diff --git a/db/migrate/20260907120000_add_member_newsletter_list_index_to_mail_deliveries.rb b/db/migrate/20260907120000_add_member_newsletter_list_index_to_mail_deliveries.rb new file mode 100644 index 000000000..b0285a6dd --- /dev/null +++ b/db/migrate/20260907120000_add_member_newsletter_list_index_to_mail_deliveries.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddMemberNewsletterListIndexToMailDeliveries < ActiveRecord::Migration[8.1] + def change + add_index :mail_deliveries, [ :member_id, :mailable_type, :created_at ], + name: "idx_mail_deliveries_on_member_mailable_created" + end +end diff --git a/db/schema.rb b/db/schema.rb index e891bb095..fb8545cfe 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_09_02_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_09_07_120000) do create_table "absences", force: :cascade do |t| t.datetime "admins_notified_at" t.datetime "created_at" @@ -631,6 +631,7 @@ t.index ["mailable_type", "mailable_id"], name: "idx_mail_deliveries_on_mailable_type_id" t.index ["mailable_type", "mailable_ids", "member_id"], name: "idx_mail_deliveries_on_mailable_member" t.index ["member_id", "created_at"], name: "idx_mail_deliveries_on_member_created" + t.index ["member_id", "mailable_type", "created_at"], name: "idx_mail_deliveries_on_member_mailable_created" t.index ["member_id"], name: "index_mail_deliveries_on_member_id" t.index ["state"], name: "index_mail_deliveries_on_state" t.check_constraint "JSON_TYPE(mailable_ids) = 'array'", name: "mail_deliveries_mailable_ids_is_array" diff --git a/test/controllers/members/newsletter_deliveries_controller_test.rb b/test/controllers/members/newsletter_deliveries_controller_test.rb index 856d45ae3..a012a1ba1 100644 --- a/test/controllers/members/newsletter_deliveries_controller_test.rb +++ b/test/controllers/members/newsletter_deliveries_controller_test.rb @@ -7,6 +7,16 @@ class Members::NewsletterDeliveriesControllerTest < ActionDispatch::IntegrationT host! "members.acme.test" end + def login(member) + session = Session.create!( + member: member, + email: member.emails_array.first, + remote_addr: "127.0.0.1", + user_agent: "Test Browser") + get "/sessions/#{session.generate_token_for(:redeem)}" + session + end + def login_as_admin_originated(member, admin: admins(:ultra)) session = Session.create!( admin: admin, @@ -18,6 +28,56 @@ def login_as_admin_originated(member, admin: admins(:ultra)) session end + test "index lists the latest processed newsletter deliveries" do + login(members(:john)) + + get members_newsletter_deliveries_path + + assert_response :success + assert_select ".newsletter-title", text: "Subject John Doe" + assert_select ".newsletter-date", text: /1 April 2024/ + end + + test "index query count stays bounded as newsletter deliveries grow" do + member = members(:john) + login(member) + + create_newsletter_deliveries!(member, 25) + few_queries = collect_sql_queries { get members_newsletter_deliveries_path } + assert_response :success + assert_bounded_newsletter_index_queries(few_queries) + + create_newsletter_deliveries!(member, 40, start_at: 1.hour.ago) + many_queries = collect_sql_queries { get members_newsletter_deliveries_path } + assert_response :success + assert_bounded_newsletter_index_queries(many_queries) + + assert_equal query_signatures(few_queries).size, query_signatures(many_queries).size + assert_equal newsletter_loads(few_queries).size, newsletter_loads(many_queries).size + assert_equal mail_delivery_loads(few_queries).size, mail_delivery_loads(many_queries).size + end + + test "index paginates without loading every delivery" do + member = members(:john) + login(member) + create_newsletter_deliveries!( + member, + Members::NewsletterDeliveriesController::PER_PAGE, + start_at: 1.hour.from_now) + + get members_newsletter_deliveries_path + + assert_response :success + assert_select ".newsletter-title", text: "Extra 0" + assert_select ".newsletter-title", text: "Subject John Doe", count: 0 + assert_select "#show-more a[href*='offset=20']" + + get members_newsletter_deliveries_path(offset: 20, format: :turbo_stream) + + assert_response :success + assert_includes response.body, "Subject John Doe" + end + test "index shows the member email in the unsubscribe banner for an admin-originated session" do member = members(:john) EmailSuppression.suppress!(member.emails_array.first, @@ -52,4 +112,73 @@ def login_as_admin_originated(member, admin: admins(:ultra)) assert_redirected_to members_newsletter_deliveries_path assert_equal I18n.t("members.read_only_sessions.alert"), flash[:alert] end + + private + + def create_newsletter_deliveries!(member, count, start_at: Time.current) + newsletter = newsletters(:sent) + rows = count.times.map { |i| + { + mailable_type: "Newsletter", + mailable_ids: [ newsletter.id ], + action: "newsletter", + member_id: member.id, + subject: "Extra #{i}", + state: "delivered", + content: "#{"x" * 2_000}", + created_at: start_at - i.minutes, + updated_at: start_at - i.minutes + } + } + MailDelivery.insert_all(rows) + end + + 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 mail_delivery_loads(queries) + queries.select { |sql| + sql.match?(/SELECT .+FROM ["`]mail_deliveries["`]/i) && + !sql.match?(/SELECT 1 AS one/i) && + !sql.match?(/COUNT\(/i) + } + end + + def newsletter_loads(queries) + queries.select { |sql| sql.match?(/SELECT .+FROM ["`]newsletters["`]/i) } + end + + def query_signatures(queries) + (mail_delivery_loads(queries) + newsletter_loads(queries)).map { |sql| + sql.gsub(/\d+/, "N") + } + end + + def assert_bounded_newsletter_index_queries(queries) + loads = mail_delivery_loads(queries) + assert_operator loads.size, :<=, 2, + "expected bounded MailDelivery loads, got:\n#{loads.join("\n")}" + loads.each do |sql| + assert_match(/LIMIT/i, sql) + end + + page_loads = loads.select { |sql| sql.match?(/OFFSET/i) } + assert_equal 1, page_loads.size, "expected one paginated MailDelivery load, got:\n#{loads.join("\n")}" + page_loads.each do |sql| + assert_no_match(/\bSELECT\s+(?:["`]?\w+["`]?\.)?\*/i, sql) + assert_match(/["`]subject["`]/, sql) + assert_no_match(/["`]content["`]/, sql) + end + + newsletter_sql = newsletter_loads(queries) + assert_operator newsletter_sql.size, :<=, 2, + "expected bounded Newsletter loads, got:\n#{newsletter_sql.join("\n")}" + end end diff --git a/test/models/newsletter/delivery_test.rb b/test/models/newsletter/delivery_test.rb index 0409f48ff..bb4f3b88b 100644 --- a/test/models/newsletter/delivery_test.rb +++ b/test/models/newsletter/delivery_test.rb @@ -162,6 +162,30 @@ class NewsletterDeliveryTest < ActiveSupport::TestCase end end + test "deliveries_for uses the member newsletter list index" do + columns = ActiveRecord::Base.connection.indexes(:mail_deliveries).map(&:columns) + assert_includes columns, %w[member_id mailable_type created_at] + + sql = Newsletter.deliveries_for(members(:john)).limit(21).to_sql + plan = ActiveRecord::Base.connection.exec_query("EXPLAIN QUERY PLAN #{sql}").rows.flatten.join(" ") + assert_match(/idx_mail_deliveries_on_member_mailable_created/, plan) + end + + test "deliveries_for is an unloaded relation of processed newsletter deliveries" do + draft = MailDelivery.create!( + mailable_type: "Newsletter", + mailable_ids: [ newsletters(:sent).id ], + action: "newsletter", + member: members(:john), + subject: "Draft", + state: :draft) + relation = Newsletter.deliveries_for(members(:john)) + + assert_not relation.loaded? + assert_includes relation, mail_deliveries(:sent_john) + assert_not_includes relation, draft + end + test "deliveries_with_missing_emails" do travel_to "2024-01-01" newsletter = build_newsletter(