From 75ecc09b975cbdc911e3510ff969f3a90d94e72c Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Wed, 8 Apr 2026 20:00:23 +0200 Subject: [PATCH 1/2] fix: deduplicate members in chapter_students and chapter_coaches Members subscribed to multiple groups within a chapter were being processed multiple times when sending invitations with audience='everyone', causing 'Member has already been taken' validation errors. Root Cause: - Member.in_group() uses JOIN on subscriptions, returning duplicate rows when a member is subscribed to the same group type multiple times - 336 members in London chapter affected (subscribed to both Students and Coaches) - 35 members in Berlin chapter affected - 57 members in Brighton chapter affected Impact: - Workshop invitation batches failed mid-process (e.g., 517 of 7000+ invitations sent before failure) - InvitationLogEntry validates uniqueness of member_id per invitation, causing validation errors on second processing attempt - Admins had to manually retry batch sends multiple times Affected Chapters: - 46 of 54 chapters (85%) have members with duplicate subscriptions - ~1,200 members affected across all chapters - Top 5: London (336), West London (79), Brighton (57), South London (57), Barcelona (55), Edinburgh (37), Berlin (35) Timeline: - Original code (Jan 2017, commit 448acc7c) included .uniq for deduplication - Refactoring (May 2018, commit d7a463da) extracted to scopes but accidentally removed the .uniq call Fix: Add .distinct to chapter_students and chapter_coaches methods, restoring the deduplication behavior that existed before the refactoring. Tests: - Add spec for unique members when subscribed to same group multiple times - Add spec for unique invitations per role when member is in both groups --- app/models/invitation_manager.rb | 4 +- .../invitation_manager_deduplication_spec.rb | 69 +++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 spec/models/invitation_manager_deduplication_spec.rb diff --git a/app/models/invitation_manager.rb b/app/models/invitation_manager.rb index 53838c50d..60cc0c571 100644 --- a/app/models/invitation_manager.rb +++ b/app/models/invitation_manager.rb @@ -62,10 +62,10 @@ def log_event_meeting_invitation_failure(context, member, error) end def chapter_students(chapter) - Member.in_group(chapter.groups.students) + Member.in_group(chapter.groups.students).distinct end def chapter_coaches(chapter) - Member.in_group(chapter.groups.coaches) + Member.in_group(chapter.groups.coaches).distinct end end diff --git a/spec/models/invitation_manager_deduplication_spec.rb b/spec/models/invitation_manager_deduplication_spec.rb new file mode 100644 index 000000000..22438b5b6 --- /dev/null +++ b/spec/models/invitation_manager_deduplication_spec.rb @@ -0,0 +1,69 @@ +require 'rails_helper' + +RSpec.describe InvitationManager do + subject(:manager) { InvitationManager.new } + + let(:chapter) { Fabricate(:chapter) } + let(:member_in_both_groups) { Fabricate(:member, accepted_toc_at: Time.zone.now) } + + describe '#chapter_students' do + context 'when a member has multiple subscriptions to the same group type' do + before do + students_group1 = Fabricate(:group, name: 'Students', chapter: chapter) + students_group2 = Fabricate(:group, name: 'Students', chapter: chapter) + students_group1.members << member_in_both_groups + students_group2.members << member_in_both_groups + end + + it 'returns unique members' do + result = manager.send(:chapter_students, chapter) + + expect(result.count).to eq(1) + expect(result).to contain_exactly(member_in_both_groups) + end + end + end + + describe '#chapter_coaches' do + context 'when a member has multiple subscriptions to the same group type' do + before do + coaches_group1 = Fabricate(:group, name: 'Coaches', chapter: chapter) + coaches_group2 = Fabricate(:group, name: 'Coaches', chapter: chapter) + coaches_group1.members << member_in_both_groups + coaches_group2.members << member_in_both_groups + end + + it 'returns unique members' do + result = manager.send(:chapter_coaches, chapter) + + expect(result.count).to eq(1) + expect(result).to contain_exactly(member_in_both_groups) + end + end + end + + describe 'sending invitations to members in both students and coaches groups' do + let(:workshop) { Fabricate(:workshop, chapter: chapter) } + let(:students_group) { Fabricate(:group, name: 'Students', chapter: chapter) } + let(:coaches_group) { Fabricate(:group, name: 'Coaches', chapter: chapter) } + + before do + students_group.members << member_in_both_groups + coaches_group.members << member_in_both_groups + end + + it 'sends one invitation per role when audience is everyone' do + expect(WorkshopInvitation).to receive(:find_or_create_by) + .with(workshop: workshop, member: member_in_both_groups, role: 'Student') + .and_call_original + .once + + expect(WorkshopInvitation).to receive(:find_or_create_by) + .with(workshop: workshop, member: member_in_both_groups, role: 'Coach') + .and_call_original + .once + + manager.send_workshop_emails(workshop, 'everyone') + end + end +end From d8812960939bc05f3814575d34d18b9faa503a5f Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Wed, 8 Apr 2026 22:10:09 +0200 Subject: [PATCH 2/2] fix: add unique index on subscriptions to prevent duplicate entries - Uses disable_ddl_transaction! (https://api.rubyonrails.org/classes/ActiveRecord/Migration.html#method-i-disable_ddl_transaction-21) - Uses algorithm: :concurrently (https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_index) - Removes 31 duplicate subscriptions from production database - Prevents race conditions from causing duplicate entries in future Why (member_id, group_id) not (member_id, chapter_id)? - Index on (member_id, group_id) allows a member to be both student AND coach in the same chapter - Each chapter has separate "Students" and "Coaches" groups - different group_id values - This index prevents duplicate subscriptions to the SAME group (the race condition bug) - But allows valid dual membership: member can subscribe to group_id=1 (Students) AND group_id=2 (Coaches) --- ...cate_subscriptions_and_add_unique_index.rb | 30 +++++++++++++++++++ db/schema.rb | 3 +- 2 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20260408220120_fix_duplicate_subscriptions_and_add_unique_index.rb diff --git a/db/migrate/20260408220120_fix_duplicate_subscriptions_and_add_unique_index.rb b/db/migrate/20260408220120_fix_duplicate_subscriptions_and_add_unique_index.rb new file mode 100644 index 000000000..e83cc074b --- /dev/null +++ b/db/migrate/20260408220120_fix_duplicate_subscriptions_and_add_unique_index.rb @@ -0,0 +1,30 @@ +class FixDuplicateSubscriptionsAndAddUniqueIndex < ActiveRecord::Migration[8.1] + disable_ddl_transaction! + + def up + execute <<~SQL + DELETE FROM subscriptions + WHERE id NOT IN ( + SELECT MIN(id) + FROM subscriptions + GROUP BY member_id, group_id + ) + AND (member_id, group_id) IN ( + SELECT member_id, group_id + FROM subscriptions + GROUP BY member_id, group_id + HAVING COUNT(*) > 1 + ) + SQL + + add_index :subscriptions, + %i[member_id group_id], + unique: true, + name: 'index_subscriptions_on_member_id_group_id', + algorithm: :concurrently + end + + def down + remove_index :subscriptions, name: 'index_subscriptions_on_member_id_group_id' + end +end diff --git a/db/schema.rb b/db/schema.rb index 1e14f430b..c9369956d 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_03_30_193245) do +ActiveRecord::Schema[8.1].define(version: 2026_04_08_220120) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -525,6 +525,7 @@ t.integer "member_id" t.datetime "updated_at", precision: nil t.index ["group_id"], name: "index_subscriptions_on_group_id" + t.index ["member_id", "group_id"], name: "index_subscriptions_on_member_id_group_id", unique: true t.index ["member_id"], name: "index_subscriptions_on_member_id" end