From 8176ee6c0c9cd26281bac9b600556a17a6d31912 Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Fri, 27 Feb 2026 15:52:34 +0000
Subject: [PATCH 1/5] Skip the API call when there are no student ids

This might happen when loading a project that no users have started yet. It seems a waste to call the API in this case
---
 lib/concepts/school_student/list.rb       | 2 ++
 spec/concepts/school_student/list_spec.rb | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/lib/concepts/school_student/list.rb b/lib/concepts/school_student/list.rb
index 503bf7cda..ed04a205a 100644
--- a/lib/concepts/school_student/list.rb
+++ b/lib/concepts/school_student/list.rb
@@ -17,6 +17,8 @@ def call(school:, token:, student_ids: nil)
 
       def list_students(school, token, student_ids)
         student_ids ||= Role.student.where(school:).map(&:user_id)
+        return [] if student_ids.empty?
+
         students = ProfileApiClient.list_school_students(token:, school_id: school.id, student_ids:)
         students.map do |student|
           User.new(student.to_h.slice(:id, :username, :name, :email).merge(sso_providers: student.ssoProviders))
diff --git a/spec/concepts/school_student/list_spec.rb b/spec/concepts/school_student/list_spec.rb
index 20ad7a5a4..d32073226 100644
--- a/spec/concepts/school_student/list_spec.rb
+++ b/spec/concepts/school_student/list_spec.rb
@@ -84,6 +84,12 @@
         expect(response[:school_students]).to include(expected_user)
       end
     end
+
+    it 'skips the API call if student_ids is empty' do
+      response = described_class.call(school:, token:, student_ids: [])
+      expect(ProfileApiClient).not_to have_received(:list_school_students)
+      expect(response[:school_students].size).to eq(0)
+    end
   end
 
   context 'when listing fails' do

From 6ff5b971a54ae9c86b7b7f4ac720da7e859f086d Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Fri, 27 Feb 2026 16:10:33 +0000
Subject: [PATCH 2/5] Only rescue specific errors

I don't like this pattern of rescuing StandardError - it means that errors in development and test are hidden. Also, since we're reporting all the errors to sentry it creates a lot of noise.

In this case, it made an error that happened when listing students on a project that had none. This error happening as soon as a teacher creates a project and even in many tests but because we were rescuing it we didn't notice it. I will fix this problem in the next commit.

More generally, I think we should only handle expected errors - for example, a network error is an expected error. We shouldn't try to deal with unexpected errors as we can't predict what they are or how to handle them.

I've looked at sentry and the only other errors logged by this method in the last month are ones from Faraday - `BadRequestError`, `ForbiddenError`, `ServerError` and `UnauthorizedError`. I'd like to remove this rescue too eventually, but I would want to understand the Faraday errors first.
---
 lib/concepts/school_student/list.rb       | 2 +-
 spec/concepts/school_student/list_spec.rb | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/concepts/school_student/list.rb b/lib/concepts/school_student/list.rb
index ed04a205a..9b7d373bd 100644
--- a/lib/concepts/school_student/list.rb
+++ b/lib/concepts/school_student/list.rb
@@ -7,7 +7,7 @@ def call(school:, token:, student_ids: nil)
         response = OperationResponse.new
         response[:school_students] = list_students(school, token, student_ids)
         response
-      rescue StandardError => e
+      rescue Faraday::Error => e
         Sentry.capture_exception(e)
         response[:error] = "Error listing school students: #{e}"
         response
diff --git a/spec/concepts/school_student/list_spec.rb b/spec/concepts/school_student/list_spec.rb
index d32073226..5015cd99e 100644
--- a/spec/concepts/school_student/list_spec.rb
+++ b/spec/concepts/school_student/list_spec.rb
@@ -92,11 +92,11 @@
     end
   end
 
-  context 'when listing fails' do
+  context 'when listing fails due to a Faraday error' do
     let(:student_ids) { [123] }
 
     before do
-      allow(ProfileApiClient).to receive(:list_school_students).and_raise('Some API error')
+      allow(ProfileApiClient).to receive(:list_school_students).and_raise(Faraday::Error.new('Some API error'))
       allow(Sentry).to receive(:capture_exception)
     end
 
@@ -112,7 +112,7 @@
 
     it 'sent the exception to Sentry' do
       described_class.call(school:, token:, student_ids:)
-      expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError))
+      expect(Sentry).to have_received(:capture_exception).with(kind_of(Faraday::Error))
     end
   end
 end

From ef25800e3f555f06d7dc985c4b5a73b0383e0cbb Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Fri, 27 Feb 2026 16:35:05 +0000
Subject: [PATCH 3/5] Pass in school rather than inferring it

The .users and .with_users methods are intended to be called on associations, and infer the school by looking for the school in one of the projects in the association.

This can lead to errors (when there are no projects with a school)  or unexpected behaviour if trying to call on a scope containing different projects.

By requiring the school to be passed it, it makes clear to the caller that a school is required and it only works for a single school

As part of this I have fixed the related tests - some of these were passing in unexpected ways because an error was being rescued (see previous commit).
---
 .../api/projects/remixes_controller.rb        |  2 +-
 app/models/project.rb                         |  7 ++--
 spec/models/project_spec.rb                   | 36 +++++++++----------
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb
index 222279f7e..5affee788 100644
--- a/app/controllers/api/projects/remixes_controller.rb
+++ b/app/controllers/api/projects/remixes_controller.rb
@@ -9,7 +9,7 @@ class RemixesController < ApiController
 
       def index
         projects = Project.where(remixed_from_id: project.id).accessible_by(current_ability)
-        @projects_with_users = projects.includes(:school_project).with_users(current_user)
+        @projects_with_users = projects.includes(:school_project).with_users(school, current_user)
         render index: @projects_with_users, formats: [:json]
       end
 
diff --git a/app/models/project.rb b/app/models/project.rb
index dc692aa2d..589954dfc 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -42,13 +42,12 @@ module Types
     }
   )
 
-  def self.users(current_user)
-    school = School.find_by(id: pluck(:school_id))
+  def self.users(school, current_user)
     SchoolStudent::List.call(school:, token: current_user.token, student_ids: pluck(:user_id).uniq)[:school_students] || []
   end
 
-  def self.with_users(current_user)
-    by_id = users(current_user).index_by(&:id)
+  def self.with_users(school, current_user)
+    by_id = users(school, current_user).index_by(&:id)
     all.map { |instance| [instance, by_id[instance.user_id]] }
   end
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index f01b35224..eea25b9a0 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -195,23 +195,23 @@
     end
 
     it 'returns User instances for the current scope' do
-      create(:project, user_id: student.id, school_id: school.id)
-      user = described_class.all.users(teacher).first
+      create(:project, school:, user_id: student.id)
+      user = described_class.all.users(school, teacher).first
       expect(user.name).to eq('School Student')
     end
 
     it 'ignores members where no profile account exists' do
-      user_id = SecureRandom.uuid
-      create(:project, user_id:)
+      stub_profile_api_list_school_students(school:, student_attributes: [])
+      create(:project, school:, user_id: student.id)
 
-      user = described_class.all.users(teacher).first
+      user = described_class.all.users(school, teacher).first
       expect(user).to be_nil
     end
 
     it 'ignores members not included in the current scope' do
-      create(:project)
+      create(:project, school:, user_id: student.id)
 
-      user = described_class.none.users(teacher).first
+      user = described_class.none.users(school, teacher).first
       expect(user).to be_nil
     end
   end
@@ -231,24 +231,24 @@
     it 'returns an array of class members paired with their User instance' do
       project = create(:project, user_id: student.id)
 
-      pair = described_class.all.with_users(teacher).first
-      user = described_class.all.users(teacher).first
+      pair = described_class.all.with_users(school, teacher).first
+      user = described_class.all.users(school, teacher).first
 
       expect(pair).to eq([project, user])
     end
 
     it 'returns nil values for members where no profile account exists' do
-      user_id = SecureRandom.uuid
-      project = create(:project, user_id:)
+      stub_profile_api_list_school_students(school:, student_attributes: [])
+      project = create(:project, school:, user_id: student.id)
 
-      pair = described_class.all.with_users(teacher).first
+      pair = described_class.all.with_users(school, teacher).first
       expect(pair).to eq([project, nil])
     end
 
     it 'ignores members not included in the current scope' do
       create(:project)
 
-      pair = described_class.none.with_users(teacher).first
+      pair = described_class.none.with_users(school, teacher).first
       expect(pair).to be_nil
     end
   end
@@ -266,16 +266,16 @@
     end
 
     it 'returns the class member paired with their User instance' do
-      project = create(:project, user_id: student.id)
+      project = create(:project, school:, user_id: student.id)
 
       pair = project.with_user(teacher)
-      user = described_class.all.users(teacher).first
+      user = described_class.all.users(school, teacher).first
 
       expect(pair).to eq([project, user])
     end
 
     it 'calls the Profile API to fetch the user' do
-      project = create(:project, user_id: student.id, school_id: school.id)
+      project = create(:project, school:, user_id: student.id)
 
       allow(SchoolStudent::List).to receive(:call).and_call_original
 
@@ -289,8 +289,8 @@
     end
 
     it 'returns a nil value if the member has no profile account' do
-      user_id = SecureRandom.uuid
-      project = create(:project, user_id:)
+      stub_profile_api_list_school_students(school:, student_attributes: [])
+      project = create(:project, school:, user_id: student.id)
 
       pair = project.with_user(teacher)
       expect(pair).to eq([project, nil])

From c7b381dc98a32775f9a2651371f5ec2997f6d3fc Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Fri, 27 Feb 2026 16:40:46 +0000
Subject: [PATCH 4/5] Remove redundant line

school is a method available as an association so this isn't needed
---
 app/models/project.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index 589954dfc..b68f9ec27 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -55,7 +55,6 @@ def with_user(current_user)
     # students cannot call the Profile API so do not even try
     return [self, nil] if current_user&.student?
 
-    school = School.find_by(id: school_id)
     students = SchoolStudent::List.call(school:, token: current_user.token,
                                         student_ids: [user_id])[:school_students] || []
     [self, students.first]

From 8852dd267b8b3cff7f183d0dbbe310dedcded064 Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Fri, 27 Feb 2026 16:53:49 +0000
Subject: [PATCH 5/5] Rename from user -> student

When working with this code I found it confusing as specifically returns students, not teachers that may created projects.

I haven't changed the user variable in the projects controller as I think that might be other types of user.
---
 .../api/projects/remixes_controller.rb        |  4 +-
 app/controllers/api/projects_controller.rb    |  2 +-
 app/models/project.rb                         |  8 ++--
 .../api/projects/remixes/index.json.jbuilder  |  2 +-
 spec/models/project_spec.rb                   | 38 +++++++++----------
 5 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb
index 5affee788..b934b06ec 100644
--- a/app/controllers/api/projects/remixes_controller.rb
+++ b/app/controllers/api/projects/remixes_controller.rb
@@ -9,8 +9,8 @@ class RemixesController < ApiController
 
       def index
         projects = Project.where(remixed_from_id: project.id).accessible_by(current_ability)
-        @projects_with_users = projects.includes(:school_project).with_users(school, current_user)
-        render index: @projects_with_users, formats: [:json]
+        @projects_with_students = projects.includes(:school_project).with_students(school, current_user)
+        render index: @projects_with_students, formats: [:json]
       end
 
       def show
diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb
index 1ebb350f9..55f72987c 100644
--- a/app/controllers/api/projects_controller.rb
+++ b/app/controllers/api/projects_controller.rb
@@ -18,7 +18,7 @@ def index
 
     def show
       if !@project.school_id.nil? && @project.lesson_id.nil?
-        project_with_user = @project.with_user(current_user)
+        project_with_user = @project.with_student(current_user)
         @user = project_with_user[1]
       end
 
diff --git a/app/models/project.rb b/app/models/project.rb
index b68f9ec27..a9ab96068 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -42,16 +42,16 @@ module Types
     }
   )
 
-  def self.users(school, current_user)
+  def self.students(school, current_user)
     SchoolStudent::List.call(school:, token: current_user.token, student_ids: pluck(:user_id).uniq)[:school_students] || []
   end
 
-  def self.with_users(school, current_user)
-    by_id = users(school, current_user).index_by(&:id)
+  def self.with_students(school, current_user)
+    by_id = students(school, current_user).index_by(&:id)
     all.map { |instance| [instance, by_id[instance.user_id]] }
   end
 
-  def with_user(current_user)
+  def with_student(current_user)
     # students cannot call the Profile API so do not even try
     return [self, nil] if current_user&.student?
 
diff --git a/app/views/api/projects/remixes/index.json.jbuilder b/app/views/api/projects/remixes/index.json.jbuilder
index d69c0c880..a22092434 100644
--- a/app/views/api/projects/remixes/index.json.jbuilder
+++ b/app/views/api/projects/remixes/index.json.jbuilder
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-json.array!(@projects_with_users) do |project, user|
+json.array!(@projects_with_students) do |project, user|
   json.call(
     project,
     :identifier,
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index eea25b9a0..371f4e044 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -182,7 +182,7 @@
     end
   end
 
-  describe '.users' do
+  describe '.students' do
     let(:student) { create(:student, school:, name: 'School Student') }
     let(:teacher) { create(:teacher, school:) }
 
@@ -196,7 +196,7 @@
 
     it 'returns User instances for the current scope' do
       create(:project, school:, user_id: student.id)
-      user = described_class.all.users(school, teacher).first
+      user = described_class.all.students(school, teacher).first
       expect(user.name).to eq('School Student')
     end
 
@@ -204,19 +204,19 @@
       stub_profile_api_list_school_students(school:, student_attributes: [])
       create(:project, school:, user_id: student.id)
 
-      user = described_class.all.users(school, teacher).first
-      expect(user).to be_nil
+      student = described_class.all.students(school, teacher).first
+      expect(student).to be_nil
     end
 
     it 'ignores members not included in the current scope' do
       create(:project, school:, user_id: student.id)
 
-      user = described_class.none.users(school, teacher).first
-      expect(user).to be_nil
+      student = described_class.none.students(school, teacher).first
+      expect(student).to be_nil
     end
   end
 
-  describe '.with_users' do
+  describe '.with_students' do
     let(:student) { create(:student, school:) }
     let(:teacher) { create(:teacher, school:) }
 
@@ -231,29 +231,29 @@
     it 'returns an array of class members paired with their User instance' do
       project = create(:project, user_id: student.id)
 
-      pair = described_class.all.with_users(school, teacher).first
-      user = described_class.all.users(school, teacher).first
+      pair = described_class.all.with_students(school, teacher).first
+      students = described_class.all.students(school, teacher).first
 
-      expect(pair).to eq([project, user])
+      expect(pair).to eq([project, students])
     end
 
     it 'returns nil values for members where no profile account exists' do
       stub_profile_api_list_school_students(school:, student_attributes: [])
       project = create(:project, school:, user_id: student.id)
 
-      pair = described_class.all.with_users(school, teacher).first
+      pair = described_class.all.with_students(school, teacher).first
       expect(pair).to eq([project, nil])
     end
 
     it 'ignores members not included in the current scope' do
       create(:project)
 
-      pair = described_class.none.with_users(school, teacher).first
+      pair = described_class.none.with_students(school, teacher).first
       expect(pair).to be_nil
     end
   end
 
-  describe '#with_user' do
+  describe '#with_student' do
     let(:student) { create(:student, school:) }
     let(:teacher) { create(:teacher, school:) }
 
@@ -268,10 +268,10 @@
     it 'returns the class member paired with their User instance' do
       project = create(:project, school:, user_id: student.id)
 
-      pair = project.with_user(teacher)
-      user = described_class.all.users(school, teacher).first
+      pair = project.with_student(teacher)
+      student = described_class.all.students(school, teacher).first
 
-      expect(pair).to eq([project, user])
+      expect(pair).to eq([project, student])
     end
 
     it 'calls the Profile API to fetch the user' do
@@ -279,7 +279,7 @@
 
       allow(SchoolStudent::List).to receive(:call).and_call_original
 
-      project.with_user(teacher)
+      project.with_student(teacher)
 
       expect(SchoolStudent::List).to have_received(:call).with(
         school:,
@@ -292,7 +292,7 @@
       stub_profile_api_list_school_students(school:, student_attributes: [])
       project = create(:project, school:, user_id: student.id)
 
-      pair = project.with_user(teacher)
+      pair = project.with_student(teacher)
       expect(pair).to eq([project, nil])
     end
 
@@ -302,7 +302,7 @@
 
         allow(SchoolStudent::List).to receive(:call)
 
-        pair = project.with_user(student)
+        pair = project.with_student(student)
 
         expect(pair).to eq([project, nil])
         expect(SchoolStudent::List).not_to have_received(:call)