diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb index 222279f7e..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(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 dc692aa2d..a9ab96068 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -42,21 +42,19 @@ module Types } ) - def self.users(current_user) - school = School.find_by(id: pluck(:school_id)) + 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(current_user) - by_id = users(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? - 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] 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/lib/concepts/school_student/list.rb b/lib/concepts/school_student/list.rb index 503bf7cda..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 @@ -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..5015cd99e 100644 --- a/spec/concepts/school_student/list_spec.rb +++ b/spec/concepts/school_student/list_spec.rb @@ -84,13 +84,19 @@ 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 + 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 @@ -106,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/models/project_spec.rb b/spec/models/project_spec.rb index f01b35224..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:) } @@ -195,28 +195,28 @@ 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.students(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 - 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) + create(:project, school:, user_id: student.id) - user = described_class.none.users(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(teacher).first - user = described_class.all.users(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 - 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_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(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:) } @@ -266,20 +266,20 @@ 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 + 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 - 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 - project.with_user(teacher) + project.with_student(teacher) expect(SchoolStudent::List).to have_received(:call).with( school:, @@ -289,10 +289,10 @@ 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) + 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)