diff --git a/app/controllers/api/class_members_controller.rb b/app/controllers/api/class_members_controller.rb index 45254d6f..34454b2b 100644 --- a/app/controllers/api/class_members_controller.rb +++ b/app/controllers/api/class_members_controller.rb @@ -22,10 +22,24 @@ def index end def create - result = ClassMember::Create.call(school_class: @school_class, class_member_params:) + student_ids = [class_member_params[:student_id]] + students = SchoolStudent::List.call(school: @school, token: current_user.token, student_ids:) + result = ClassMember::Create.call(school_class: @school_class, students: students[:school_students]) if result.success? - @class_member_with_student = result[:class_member].with_student + @class_member = result[:class_members].first + render :show, formats: [:json], status: :created + else + render json: { error: result[:error] }, status: :unprocessable_entity + end + end + + def create_batch + students = SchoolStudent::List.call(school: @school, token: current_user.token, student_ids: create_batch_params) + result = ClassMember::Create.call(school_class: @school_class, students: students[:school_students]) + + if result.success? + @class_members = result[:class_members] render :show, formats: [:json], status: :created else render json: { error: result[:error] }, status: :unprocessable_entity @@ -45,7 +59,11 @@ def destroy private def class_member_params - params.require(:class_member).permit(:student_id, student_ids: []) + params.require(:class_member).permit(:student_id) + end + + def create_batch_params + params.permit(student_ids: []) end end end diff --git a/app/models/ability.rb b/app/models/ability.rb index ce86b82a..0c16c5df 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -51,7 +51,7 @@ def define_school_owner_abilities(school:) can(%i[read update destroy], School, id: school.id) can(%i[read create update destroy], SchoolClass, school: { id: school.id }) can(%i[read], Project, school_id: school.id, lesson: { visibility: %w[teachers students] }) - can(%i[read create destroy], ClassMember, school_class: { school: { id: school.id } }) + can(%i[read create create_batch destroy], ClassMember, school_class: { school: { id: school.id } }) can(%i[read create destroy], :school_owner) can(%i[read create destroy], :school_teacher) can(%i[read create create_batch update destroy], :school_student) @@ -66,7 +66,8 @@ def define_school_teacher_abilities(user:, school:) can(%i[create], SchoolClass, school: { id: school.id }) can(%i[read update destroy], SchoolClass, school: { id: school.id }, teacher_id: user.id) can(%i[read], Project, school_id: school.id, lesson: { visibility: %w[teachers students] }) - can(%i[read create destroy], ClassMember, school_class: { school: { id: school.id }, teacher_id: user.id }) + can(%i[read create create_batch destroy], ClassMember, + school_class: { school: { id: school.id }, teacher_id: user.id }) can(%i[read], :school_owner) can(%i[read], :school_teacher) can(%i[read create create_batch update], :school_student) diff --git a/app/models/class_member.rb b/app/models/class_member.rb index a8158aff..8beecb84 100644 --- a/app/models/class_member.rb +++ b/app/models/class_member.rb @@ -3,6 +3,7 @@ class ClassMember < ApplicationRecord belongs_to :school_class delegate :school, to: :school_class + attr_accessor :student validates :student_id, presence: true, uniqueness: { scope: :school_class_id, @@ -11,28 +12,14 @@ class ClassMember < ApplicationRecord validate :student_has_the_school_student_role_for_the_school - def self.students - User.from_userinfo(ids: pluck(:student_id)) - end - - def self.with_students - by_id = students.index_by(&:id) - all.map { |instance| [instance, by_id[instance.student_id]] } - end - - def with_student - [self, User.from_userinfo(ids: student_id).first] - end - private def student_has_the_school_student_role_for_the_school - return unless student_id_changed? && errors.blank? + return unless student_id_changed? && errors.blank? && student.present? - user = User.new(id: student_id) - return if user.school_student?(school) + return if student.school_student?(school) - msg = "'#{student_id}' does not have the 'school-student' role for organisation '#{school.id}'" + msg = "'#{student.id}' does not have the 'school-student' role for organisation '#{school.id}'" errors.add(:student, msg) end end diff --git a/app/views/api/class_members/_class_member.json.jbuilder b/app/views/api/class_members/_class_member.json.jbuilder new file mode 100644 index 00000000..cba857c2 --- /dev/null +++ b/app/views/api/class_members/_class_member.json.jbuilder @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +json.call( + class_member, + :id, + :school_class_id, + :student_id, + :created_at, + :updated_at +) + +if class_member.student.present? + json.set! :student do + json.call( + class_member.student, + :id, + :username, + :name + ) + end +end diff --git a/app/views/api/class_members/show.json.jbuilder b/app/views/api/class_members/show.json.jbuilder index 4e8bc607..c4cbeeeb 100644 --- a/app/views/api/class_members/show.json.jbuilder +++ b/app/views/api/class_members/show.json.jbuilder @@ -1,15 +1,11 @@ # frozen_string_literal: true -class_member, student = @class_member_with_student - -json.call( - class_member, - :id, - :school_class_id, - :student_id, - :created_at, - :updated_at -) - -json.student_username(student&.username) -json.student_name(student&.name) +if defined?(@class_members) + json.array! @class_members do |class_member| + json.partial! 'class_member', class_member: + end +elsif defined?(@class_member) + json.class_member do + json.partial! 'class_member', class_member: @class_member + end +end diff --git a/config/routes.rb b/config/routes.rb index 33829a18..559c1525 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -42,7 +42,9 @@ resource :school, only: [:show], controller: 'my_school' resources :schools, only: %i[index show create update destroy] do resources :classes, only: %i[index show create update destroy], controller: 'school_classes' do - resources :members, only: %i[index create destroy], controller: 'class_members' + resources :members, only: %i[index create destroy], controller: 'class_members' do + post :batch, on: :collection, to: 'class_members#create_batch' + end end resources :owners, only: %i[index create destroy], controller: 'school_owners' diff --git a/lib/concepts/class_member/operations/create.rb b/lib/concepts/class_member/operations/create.rb index 0ba19259..080bded3 100644 --- a/lib/concepts/class_member/operations/create.rb +++ b/lib/concepts/class_member/operations/create.rb @@ -3,17 +3,39 @@ class ClassMember class Create class << self - def call(school_class:, class_member_params:) + def call(school_class:, students:) response = OperationResponse.new - response[:class_member] = school_class.members.build(class_member_params) - response[:class_member].save! + response[:class_members] = [] + response[:errors] = {} + raise ArgumentError, 'No valid students provided' if students.blank? + + create_class_members(school_class:, students:, response:) response rescue StandardError => e Sentry.capture_exception(e) - errors = response[:class_member].errors.full_messages.join(',') - response[:error] = "Error creating class member: #{errors}" + response[:error] = "Error creating class members: #{e.message}" response end + + private + + def create_class_members(school_class:, students:, response:) + students.each do |student| + class_member = school_class.members.build({ student_id: student.id }) + class_member.student = student + class_member.save! + response[:class_members] << class_member + rescue StandardError => e + handle_class_member_error(e, class_member, student, response) + response + end + end + + def handle_class_member_error(exception, class_member, student, response) + Sentry.capture_exception(exception) + errors = class_member.errors.full_messages.join(',') + response[:errors][student.id] = "Error creating class member for student_id #{student.id}: #{errors}" + end end end end diff --git a/spec/concepts/class_member/create_spec.rb b/spec/concepts/class_member/create_spec.rb index 2eecc9c1..dbffe882 100644 --- a/spec/concepts/class_member/create_spec.rb +++ b/spec/concepts/class_member/create_spec.rb @@ -5,61 +5,138 @@ RSpec.describe ClassMember::Create, type: :unit do let!(:school_class) { create(:school_class, teacher_id: teacher.id, school:) } let(:school) { create(:school) } - let(:student) { create(:student, school:) } + let(:students) { create_list(:student, 3, school:) } let(:teacher) { create(:teacher, school:) } - let(:class_member_params) do - { student_id: student.id } - end + let(:student_ids) { students.map(&:id) } it 'returns a successful operation response' do - response = described_class.call(school_class:, class_member_params:) + response = described_class.call(school_class:, students:) expect(response.success?).to be(true) end it 'creates a school class' do - expect { described_class.call(school_class:, class_member_params:) }.to change(ClassMember, :count).by(1) + expect { described_class.call(school_class:, students:) }.to change(ClassMember, :count).by(3) + end + + it 'returns a class members JSON array' do + response = described_class.call(school_class:, students:) + expect(response[:class_members].size).to eq(3) end - it 'returns the class member in the operation response' do - response = described_class.call(school_class:, class_member_params:) - expect(response[:class_member]).to be_a(ClassMember) + it 'returns class members in the operation response' do + response = described_class.call(school_class:, students:) + expect(response[:class_members]).to all(be_a(ClassMember)) end it 'assigns the school_class' do - response = described_class.call(school_class:, class_member_params:) - expect(response[:class_member].school_class).to eq(school_class) + response = described_class.call(school_class:, students:) + expect(response[:class_members]).to all(have_attributes(school_class:)) end it 'assigns the student_id' do - response = described_class.call(school_class:, class_member_params:) - expect(response[:class_member].student_id).to eq(student.id) + response = described_class.call(school_class:, students:) + expect(response[:class_members].map(&:student_id)).to match_array(student_ids) end - context 'when creation fails' do - let(:class_member_params) { {} } - + context 'when creations fail' do before do allow(Sentry).to receive(:capture_exception) end - it 'does not create a class member' do - expect { described_class.call(school_class:, class_member_params:) }.not_to change(ClassMember, :count) - end + context 'with malformed students' do + let(:students) { nil } - it 'returns a failed operation response' do - response = described_class.call(school_class:, class_member_params:) - expect(response.failure?).to be(true) - end + it 'does not create a class member' do + expect { described_class.call(school_class:, students:) }.not_to change(ClassMember, :count) + end - it 'returns the error message in the operation response' do - response = described_class.call(school_class:, class_member_params:) - expect(response[:error]).to match(/Error creating class member/) + it 'returns a failed operation response' do + response = described_class.call(school_class:, students:) + expect(response.failure?).to be(true) + end + + it 'returns the error message in the operation response' do + response = described_class.call(school_class:, students:) + expect(response[:error]).to match(/No valid students provided/) + end + + it 'sent the exception to Sentry' do + described_class.call(school_class:, students:) + expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + end end - it 'sent the exception to Sentry' do - described_class.call(school_class:, class_member_params:) - expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + context 'with a student from a different school' do + let(:different_school) { create(:school) } + let(:different_school_student) { create(:student, school: different_school) } + + context 'with non existent students' do + let(:students) { [different_school_student] } + + it 'does not create a class member' do + expect { described_class.call(school_class:, students:) }.not_to change(ClassMember, :count) + end + + it 'returns a successful operation response' do + response = described_class.call(school_class:, students:) + expect(response.success?).to be(true) + end + + it 'returns an empty class members array' do + response = described_class.call(school_class:, students:) + expect(response[:class_members]).to eq([]) + end + + it 'returns the error messages in the operation response' do + response = described_class.call(school_class:, students:) + expect(response[:errors][different_school_student.id]).to include("Error creating class member for student_id #{different_school_student.id}: Student '#{different_school_student.id}' does not have the 'school-student' role for organisation '#{school.id}'") + end + + it 'sent the exception to Sentry' do + described_class.call(school_class:, students:) + expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + end + end + + context 'when one creation fails' do + let(:new_students) { students + [different_school_student] } + + it 'returns a successful operation response' do + response = described_class.call(school_class:, students: new_students) + expect(response.success?).to be(true) + end + + it 'returns a class members JSON array' do + response = described_class.call(school_class:, students: new_students) + expect(response[:class_members].size).to eq(3) + end + + it 'returns class members in the operation response' do + response = described_class.call(school_class:, students: new_students) + expect(response[:class_members]).to all(be_a(ClassMember)) + end + + it 'assigns the school_class' do + response = described_class.call(school_class:, students: new_students) + expect(response[:class_members]).to all(have_attributes(school_class:)) + end + + it 'assigns the successful students' do + response = described_class.call(school_class:, students: new_students) + expect(response[:class_members].map(&:student_id)).to match_array(student_ids) + end + + it 'returns the error messages in the operation response' do + response = described_class.call(school_class:, students: new_students) + expect(response[:errors][different_school_student.id]).to eq("Error creating class member for student_id #{different_school_student.id}: Student '#{different_school_student.id}' does not have the 'school-student' role for organisation '#{school.id}'") + end + + it 'sent the exception to Sentry' do + described_class.call(school_class:, students: new_students) + expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + end + end end end end diff --git a/spec/features/class_member/creating_a_batch_of_class_members_spec.rb b/spec/features/class_member/creating_a_batch_of_class_members_spec.rb new file mode 100644 index 00000000..5e786d78 --- /dev/null +++ b/spec/features/class_member/creating_a_batch_of_class_members_spec.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Creating a class member', type: :request do + let(:headers) { { Authorization: UserProfileMock::TOKEN } } + let!(:school_class) { create(:school_class, teacher_id: teacher.id, school:) } + let(:school) { create(:school) } + let(:students) { create_list(:student, 3, school:) } + let(:teacher) { create(:teacher, school:) } + let(:owner) { create(:owner, school:) } + + let(:params) do + { + student_ids: students.map(&:id) + } + end + + context 'with valid params' do + let(:student_attributes) do + students.map do |student| + { id: student.id, name: student.name, username: student.username } + end + end + + before do + authenticated_in_hydra_as(owner) + stub_profile_api_list_school_students(school:, student_attributes:) + end + + it 'responds 201 Created' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) + expect(response).to have_http_status(:created) + end + + it 'responds 201 Created when the user is a school-teacher' do + authenticated_in_hydra_as(teacher) + + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) + expect(response).to have_http_status(:created) + end + + it 'responds with the class members JSON array' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + expect(data.size).to eq(3) + end + + it 'responds with the class member JSON' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + student_ids = data.pluck(:student_id) + expect(student_ids).to eq(params[:student_ids]) + end + + it 'responds with the student JSON' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + response_students = data.pluck(:student) + + expect(response_students).to eq(student_attributes) + end + end + + context 'with invalid params' do + let(:invalid_params) { { class_member: { student_ids: ' ' } } } + + before do + authenticated_in_hydra_as(owner) + stub_profile_api_list_school_students(school:, student_attributes: []) + end + + it 'responds 422 Unprocessable Entity when params are missing' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:) + expect(response).to have_http_status(:unprocessable_entity) + end + + it 'responds 422 Unprocessable Entity when params are invalid' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params: invalid_params) + expect(response).to have_http_status(:unprocessable_entity) + end + + it 'returns the error message in the operation response' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params: invalid_params) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:error]).to match(/No valid students provided/) + end + + it 'responds 401 Unauthorized when no token is given' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", params:) + expect(response).to have_http_status(:unauthorized) + end + + it 'responds 403 Forbidden when the user is a school-owner for a different school' do + school = create(:school, id: SecureRandom.uuid) + school_class.update!(school_id: school.id) + + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the user is not the school-teacher for the class' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the user is a school-student' do + student = create(:student, school:) + authenticated_in_hydra_as(student) + + post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) + expect(response).to have_http_status(:forbidden) + end + end +end diff --git a/spec/features/class_member/creating_a_class_member_spec.rb b/spec/features/class_member/creating_a_class_member_spec.rb index 1273ca26..8ec6390c 100644 --- a/spec/features/class_member/creating_a_class_member_spec.rb +++ b/spec/features/class_member/creating_a_class_member_spec.rb @@ -3,15 +3,10 @@ require 'rails_helper' RSpec.describe 'Creating a class member', type: :request do - before do - authenticated_in_hydra_as(owner) - stub_user_info_api_for(student) - end - let(:headers) { { Authorization: UserProfileMock::TOKEN } } let!(:school_class) { create(:school_class, teacher_id: teacher.id, school:) } let(:school) { create(:school) } - let(:student) { create(:student, school:, name: 'School Student') } + let(:student) { create(:student, school:, name: 'School Student', username: 'school-student') } let(:teacher) { create(:teacher, school:) } let(:owner) { create(:owner, school:) } @@ -23,55 +18,75 @@ } end - it 'responds 201 Created' do - post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) - expect(response).to have_http_status(:created) + before do + authenticated_in_hydra_as(owner) + stub_user_info_api_for(student) end - it 'responds 201 Created when the user is a school-teacher' do - authenticated_in_hydra_as(teacher) + context 'with valid params' do + let(:student_attributes) { { id: student.id, name: student.name, username: student.username } } - post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) - expect(response).to have_http_status(:created) - end + before do + stub_profile_api_list_school_students(school:, student_attributes: [student_attributes]) + end - it 'responds with the class member JSON' do - post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) - data = JSON.parse(response.body, symbolize_names: true) + it 'responds 201 Created' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) + expect(response).to have_http_status(:created) + end - expect(data[:student_id]).to eq(student.id) - end + it 'responds 201 Created when the user is a school-teacher' do + authenticated_in_hydra_as(teacher) - it 'responds with the student JSON' do - post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) - data = JSON.parse(response.body, symbolize_names: true) + post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) + expect(response).to have_http_status(:created) + end - expect(data[:student_name]).to eq('School Student') - end + it 'responds with the class member JSON' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) - it "responds with nil attributes for the student if their user profile doesn't exist" do - student_id = SecureRandom.uuid - new_params = { class_member: params[:class_member].merge(student_id:) } + expect(data[:class_member][:student_id]).to eq(student.id) + end - post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params: new_params) - data = JSON.parse(response.body, symbolize_names: true) + it 'responds with the student JSON' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) - expect(data[:student_name]).to be_nil - end + response_student = data[:class_member][:student] - it 'responds 400 Bad Request when params are missing' do - post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:) - expect(response).to have_http_status(:bad_request) + expect(response_student).to eq(student_attributes) + end end - it 'responds 422 Unprocessable Entity when params are invalid' do - post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params: { class_member: { student_id: ' ' } }) - expect(response).to have_http_status(:unprocessable_entity) - end + context 'with invalid params' do + let(:invalid_params) { { class_member: { student_id: SecureRandom.uuid } } } + + before do + stub_profile_api_list_school_students(school:, student_attributes: []) + end + + it 'responds 400 Bad Request when params are missing' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:) + expect(response).to have_http_status(:bad_request) + end + + it 'responds 422 Unprocessable Entity when params are invalid' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params: invalid_params) + expect(response).to have_http_status(:unprocessable_entity) + end + + it 'returns the error message in the operation response' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params: invalid_params) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:error]).to match(/No valid students provided/) + end - it 'responds 401 Unauthorized when no token is given' do - post("/api/schools/#{school.id}/classes/#{school_class.id}/members", params:) - expect(response).to have_http_status(:unauthorized) + it 'responds 401 Unauthorized when no token is given' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/members", params:) + expect(response).to have_http_status(:unauthorized) + end end it 'responds 403 Forbidden when the user is a school-owner for a different school' do diff --git a/spec/models/class_member_spec.rb b/spec/models/class_member_spec.rb index f214b99e..43ea184a 100644 --- a/spec/models/class_member_spec.rb +++ b/spec/models/class_member_spec.rb @@ -51,7 +51,7 @@ end it 'requires a student that has the school-student role for the school' do - class_member.student_id = teacher.id + class_member.student = teacher expect(class_member).to be_invalid end @@ -61,76 +61,4 @@ expect(duplicate).to be_invalid end end - - describe '.students' do - it 'returns User instances for the current scope' do - create(:class_member, student_id: student.id, school_class:) - - student = described_class.all.students.first - expect(student.name).to eq('School Student') - end - - it 'ignores members where no profile account exists' do - student = create(:student, school:) - stub_user_info_api_for_unknown_users(user_id: student.id) - create(:class_member, student_id: student.id, school_class:) - - student = described_class.all.students.first - expect(student).to be_nil - end - - it 'ignores members not included in the current scope' do - create(:class_member, student_id: student.id, school_class:) - - student = described_class.none.students.first - expect(student).to be_nil - end - end - - describe '.with_students' do - it 'returns an array of class members paired with their User instance' do - class_member = create(:class_member, student_id: student.id, school_class:) - - pair = described_class.all.with_students.first - student = described_class.all.students.first - - expect(pair).to eq([class_member, student]) - end - - it 'returns nil values for members where no profile account exists' do - student = create(:student, school:) - stub_user_info_api_for_unknown_users(user_id: student.id) - class_member = create(:class_member, student_id: student.id, school_class:) - - pair = described_class.all.with_students.first - expect(pair).to eq([class_member, nil]) - end - - it 'ignores members not included in the current scope' do - create(:class_member, student_id: student.id, school_class:) - - pair = described_class.none.with_students.first - expect(pair).to be_nil - end - end - - describe '#with_student' do - it 'returns the class member paired with their User instance' do - class_member = create(:class_member, student_id: student.id, school_class:) - - pair = class_member.with_student - student = described_class.all.students.first - - expect(pair).to eq([class_member, student]) - end - - it 'returns a nil value if the member has no profile account' do - student = create(:student, school:) - stub_user_info_api_for_unknown_users(user_id: student.id) - class_member = create(:class_member, student_id: student.id, school_class:) - - pair = class_member.with_student - expect(pair).to eq([class_member, nil]) - end - end end