From efed736d4ea5ed03623bab707909de4d6ad442a9 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 af9cffe245dd603a9e1acfdf7a7184f0177ac5b1 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 happened 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.

I've also included `ProfileApiClient::Error` as I saw `ProfileApiClient::Student422Error` errors being raised in school students controller.

The remix spec was failing after this change as it was swallowing an error before. Improve the test coverage and fix the failure.

Fixup error
---
 lib/concepts/school_student/list.rb       | 2 +-
 spec/concepts/school_student/list_spec.rb | 6 +++---
 spec/requests/projects/remix_spec.rb      | 7 +++++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/concepts/school_student/list.rb b/lib/concepts/school_student/list.rb
index ed04a205a..4080f40ed 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, ProfileApiClient::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
diff --git a/spec/requests/projects/remix_spec.rb b/spec/requests/projects/remix_spec.rb
index 1443deca5..50edde286 100644
--- a/spec/requests/projects/remix_spec.rb
+++ b/spec/requests/projects/remix_spec.rb
@@ -3,7 +3,6 @@
 require 'rails_helper'
 
 RSpec.describe 'Remix requests' do
-  let!(:original_project) { create(:project) }
   let(:project_params) do
     {
       name: original_project.name,
@@ -13,6 +12,7 @@
   end
   let(:school) { create(:school) }
   let(:owner) { create(:owner, school:) }
+  let!(:original_project) { create(:project, school:, user_id: owner.id) }
 
   before do
     mock_phrase_generation
@@ -32,6 +32,8 @@
 
     describe '#index' do
       before do
+        student_attributes = [{ id: authenticated_user.id, name: 'sally-student' }]
+        stub_profile_api_list_school_students(school:, student_attributes:)
         create_list(:project, 2, remixed_from_id: original_project.id, user_id: authenticated_user.id)
       end
 
@@ -40,9 +42,10 @@
         expect(response).to have_http_status(:ok)
       end
 
-      it 'returns the list of projects' do
+      it 'returns the list of projects with student names' do
         get("/api/projects/#{original_project.identifier}/remixes", headers:)
         expect(response.parsed_body.length).to eq(2)
+        expect(response.parsed_body.first['user_name']).to eq('sally-student')
       end
 
       it 'returns 404 response if invalid project' do

From 1ba40e364e78c2730685f9d03506bc8fa4ad676b 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        |  4 +--
 app/models/project.rb                         |  7 ++--
 spec/models/project_spec.rb                   | 36 +++++++++----------
 3 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb
index 222279f7e..e2f54b151 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(current_user)
-        render index: @projects_with_users, formats: [:json]
+        @projects_with_students = projects.includes(:school_project).with_users(project.school, current_user)
+        render index: @projects_with_students, formats: [:json]
       end
 
       def show
diff --git a/app/models/project.rb b/app/models/project.rb
index f74dbcdb4..a4d3df0b6 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -43,13 +43,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 ac3548d92a0d0d50439d638d62770e8e23c48ae0 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 a4d3df0b6..0a1cbcd99 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -56,7 +56,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 5e5e133ab053b501f85f1846f26e0908196e5c19 Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Fri, 6 Mar 2026 09:25:22 +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        |  2 +-
 app/controllers/api/projects_controller.rb    |  2 +-
 app/models/project.rb                         |  8 ++--
 .../api/projects/remixes/index.json.jbuilder  |  4 +-
 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 e2f54b151..18bdb3867 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_students = projects.includes(:school_project).with_users(project.school, current_user)
+        @projects_with_students = projects.includes(:school_project).with_students(project.school, current_user)
         render index: @projects_with_students, formats: [:json]
       end
 
diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb
index 272c2cf2a..d3937eca1 100644
--- a/app/controllers/api/projects_controller.rb
+++ b/app/controllers/api/projects_controller.rb
@@ -21,7 +21,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 0a1cbcd99..51334d6d7 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -43,16 +43,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..30a56995c 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, student|
   json.call(
     project,
     :identifier,
@@ -11,7 +11,7 @@ json.array!(@projects_with_users) do |project, user|
     :last_edited_at
   )
 
-  json.user_name(user&.name)
+  json.user_name(student&.name)
   json.finished(project.school_project&.finished)
   json.status(project.school_project&.status)
 end
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)