From 2e24655b12714ad06ec4142fee89dbff7d28eb35 Mon Sep 17 00:00:00 2001
From: "jonathan.kerr" <3410350+jonodrew@users.noreply.github.com>
Date: Fri, 17 Apr 2026 16:58:53 +0100
Subject: [PATCH] fix: #students and #coaches return Chapter-specific students
 and coaches

This fixes a bug where the methods returned _all_ of the students and coaches that the user could access

Signed-off-by: jonathan.kerr <3410350+jonodrew@users.noreply.github.com>
---
 app/models/README.md        |  7 +++++++
 app/models/chapter.rb       | 18 ++++++++++++------
 spec/models/chapter_spec.rb | 28 ++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/app/models/README.md b/app/models/README.md
index ef547f54f..ad21602b7 100644
--- a/app/models/README.md
+++ b/app/models/README.md
@@ -40,6 +40,13 @@ is only accessed via the `Permission` class and Rolify.
 A `chapter` represents a local organisation. It primarily serves to
 collate things by location and organisers.
 
+`Chapter` exposes two tiers of member access methods. The raw methods
+(`#students`, `#coaches`) return all members subscribed to that group
+regardless of eligibility. The filtered methods (`#eligible_students`,
+`#eligible_coaches`) exclude banned members and those who have not
+accepted the terms of conduct, and should be preferred when building
+workshop invitation lists.
+
 ## Sponsor
 
 A `sponsor` represents a location and organisation. Sponsors have a
diff --git a/app/models/chapter.rb b/app/models/chapter.rb
index d28f497a6..a3f744a91 100644
--- a/app/models/chapter.rb
+++ b/app/models/chapter.rb
@@ -36,28 +36,34 @@ def organisers
     @organisers ||= Member.with_role(:organiser, self)
   end
 
+  # All members subscribed to this chapter's Students group, regardless of ban or TOC status.
+  # Use #eligible_students when building invitation lists.
   def students
-    Member.joins(:groups)
-          .merge(Group.students)
-          .distinct
+    members_for_group('Students')
   end
 
+  # All members subscribed to this chapter's Coaches group, regardless of ban or TOC status.
+  # Use #eligible_coaches when building invitation lists.
   def coaches
-    Member.joins(:groups)
-          .merge(Group.coaches)
-          .distinct
+    members_for_group('Coaches')
   end
 
+  # Chapter students who are not banned and have accepted the TOC — safe to invite to workshops.
   def eligible_students
     Member.in_group(groups.students).distinct
   end
 
+  # Chapter coaches who are not banned and have accepted the TOC — safe to invite to workshops.
   def eligible_coaches
     Member.in_group(groups.coaches).distinct
   end
 
   private
 
+  def members_for_group(name)
+    members.where(groups: { name: name }).distinct
+  end
+
   def expire_chapters_sidebar_cache
     Rails.cache.delete('chapters-sidebar')
   end
diff --git a/spec/models/chapter_spec.rb b/spec/models/chapter_spec.rb
index a20445d2a..bdab259da 100644
--- a/spec/models/chapter_spec.rb
+++ b/spec/models/chapter_spec.rb
@@ -66,6 +66,34 @@
     end
   end
 
+  describe '#students' do
+    let(:chapter) { Fabricate(:chapter) }
+    let(:student_group) { Fabricate(:group, chapter: chapter, name: 'Students') }
+
+    it 'returns only students from this chapter' do
+      other_chapter = Fabricate(:chapter)
+      other_group = Fabricate(:group, chapter: other_chapter, name: 'Students')
+      this_student = Fabricate(:member, groups: [student_group])
+      _other_student = Fabricate(:member, groups: [other_group])
+
+      expect(chapter.students).to contain_exactly(this_student)
+    end
+  end
+
+  describe '#coaches' do
+    let(:chapter) { Fabricate(:chapter) }
+    let(:coach_group) { Fabricate(:group, chapter: chapter, name: 'Coaches') }
+
+    it 'returns only coaches from this chapter' do
+      other_chapter = Fabricate(:chapter)
+      other_group = Fabricate(:group, chapter: other_chapter, name: 'Coaches')
+      this_coach = Fabricate(:member, groups: [coach_group])
+      _other_coach = Fabricate(:member, groups: [other_group])
+
+      expect(chapter.coaches).to contain_exactly(this_coach)
+    end
+  end
+
   describe '#eligible_students' do
     let(:chapter) { Fabricate(:chapter) }
     let(:student_group) { Fabricate(:group, chapter: chapter, name: 'Students') }