From 39bdb1baecef38bb124ab8219424dc631ca31399 Mon Sep 17 00:00:00 2001 From: Scott Date: Thu, 18 Jul 2024 14:16:24 +0100 Subject: [PATCH 01/13] class_member/create - update to allow student-ids array --- .../class_member/operations/create.rb | 19 ++- spec/concepts/class_member/create_spec.rb | 135 ++++++++++++++---- 2 files changed, 122 insertions(+), 32 deletions(-) diff --git a/lib/concepts/class_member/operations/create.rb b/lib/concepts/class_member/operations/create.rb index 0ba19259..9d859891 100644 --- a/lib/concepts/class_member/operations/create.rb +++ b/lib/concepts/class_member/operations/create.rb @@ -3,15 +3,24 @@ class ClassMember class Create class << self - def call(school_class:, class_member_params:) + def call(school_class:, student_ids:) response = OperationResponse.new - response[:class_member] = school_class.members.build(class_member_params) - response[:class_member].save! + response[:class_members] = [] + student_ids.each do |student_id| + class_member_params = { student_id: } + class_member = school_class.members.build(class_member_params) + class_member.save! + response[:class_members] << class_member + rescue StandardError => e + Sentry.capture_exception(e) + errors = class_member.errors.full_messages.join(',') + response[:errors] ||= {} + response[:errors][student_id] = "Error creating class member for student_id #{student_id}: #{errors}" + end 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" response end end diff --git a/spec/concepts/class_member/create_spec.rb b/spec/concepts/class_member/create_spec.rb index 2eecc9c1..eb2e0f01 100644 --- a/spec/concepts/class_member/create_spec.rb +++ b/spec/concepts/class_member/create_spec.rb @@ -5,61 +5,142 @@ 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(:failed_student_id) { 'i-am-not-an-id' } + 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:, student_ids:) 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:, student_ids:) }.to change(ClassMember, :count).by(3) + end + + it 'returns a class members JSON array' do + response = described_class.call(school_class:, student_ids:) + 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:, student_ids:) + 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:, student_ids:) + expect(response[:class_members]).to all(have_attributes(school_class: 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:, student_ids:) + expect(response[:class_members].map(&:student_id)).to match_array(student_ids) end - context 'when creation fails' do - let(:class_member_params) { {} } + context 'with an empty array of student_ids' do + it 'returns a successful operation response' do + response = described_class.call(school_class:, student_ids: []) + expect(response[:class_members]).to eq([]) + end + end + 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 student_ids' do + let(:student_ids) { nil } + + it 'does not create a class member' do + expect { described_class.call(school_class:, student_ids:) }.not_to change(ClassMember, :count) + end + + it 'returns a failed operation response' do + response = described_class.call(school_class:, student_ids:) + expect(response.failure?).to be(true) + end + + it 'returns the error message in the operation response' do + response = described_class.call(school_class:, student_ids:) + expect(response[:error]).to match(/Error creating class members/) + end - it 'returns a failed operation response' do - response = described_class.call(school_class:, class_member_params:) - expect(response.failure?).to be(true) + it 'sent the exception to Sentry' do + described_class.call(school_class:, student_ids:) + expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + end 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/) + context 'with non existent student_ids' do + let(:student_ids) { [failed_student_id] } + + it 'does not create a class member' do + expect { described_class.call(school_class:, student_ids:) }.not_to change(ClassMember, :count) + end + + it 'returns a successful operation response' do + response = described_class.call(school_class:, student_ids:) + expect(response.success?).to be(true) + end + + it 'returns a successful operation response' do + response = described_class.call(school_class:, student_ids:) + expect(response[:class_members]).to eq([]) + end + + it 'returns the error messages in the operation response' do + response = described_class.call(school_class:, student_ids:) + expect(response[:errors][failed_student_id]).to include("Error creating class member for student_id #{student_ids.first}: Student can't be blank") + end + + it 'sent the exception to Sentry' do + described_class.call(school_class:, student_ids:) + 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 'when one creation fails' do + let(:student_ids) { students.map(&:id) << failed_student_id } + + it 'returns a successful operation response' do + response = described_class.call(school_class:, student_ids:) + expect(response.success?).to be(true) + end + + it 'returns a class members JSON array' do + response = described_class.call(school_class:, student_ids:) + expect(response[:class_members].size).to eq(3) + end + + it 'returns class members in the operation response' do + response = described_class.call(school_class:, student_ids:) + expect(response[:class_members]).to all(be_a(ClassMember)) + end + + it 'assigns the school_class' do + response = described_class.call(school_class:, student_ids:) + expect(response[:class_members]).to all(have_attributes(school_class: school_class)) + end + + it 'assigns the successful student_ids' do + response = described_class.call(school_class:, student_ids:) + expected_student_ids = student_ids - [failed_student_id] + expect(response[:class_members].map(&:student_id)).to match_array(expected_student_ids) + end + + it 'returns the error messages in the operation response' do + response = described_class.call(school_class:, student_ids:) + expect(response[:errors][failed_student_id]).to eq("Error creating class member for student_id #{student_ids.last}: Student can't be blank") + end + + it 'sent the exception to Sentry' do + described_class.call(school_class:, student_ids:) + expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + end end end end From aeb4fce93e8e89743953689e2c3e9fb28bbcab96 Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 19 Jul 2024 21:16:09 +0100 Subject: [PATCH 02/13] class_members/create_batch endpoint --- .../api/class_members_controller.rb | 24 ++++++++++++-- app/models/ability.rb | 4 +-- app/models/class_member.rb | 21 +++--------- .../api/class_members/show.json.jbuilder | 32 ++++++++++++------- config/routes.rb | 4 ++- .../class_member/operations/create.rb | 14 ++++---- 6 files changed, 58 insertions(+), 41 deletions(-) diff --git a/app/controllers/api/class_members_controller.rb b/app/controllers/api/class_members_controller.rb index 45254d6f..ab53ac49 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_members = result[:class_members] + 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..36026181 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,7 @@ 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/show.json.jbuilder b/app/views/api/class_members/show.json.jbuilder index 4e8bc607..4e3d41aa 100644 --- a/app/views/api/class_members/show.json.jbuilder +++ b/app/views/api/class_members/show.json.jbuilder @@ -1,15 +1,23 @@ # frozen_string_literal: true -class_member, student = @class_member_with_student +json.array!(@class_members) do |class_member| + json.call( + class_member, + :id, + :school_class_id, + :student_id, + :created_at, + :updated_at + ) -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 class_member.student.present? + json.set! :student do + json.call( + class_member.student, + :id, + :username, + :name + ) + end + 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 9d859891..4e4dfca8 100644 --- a/lib/concepts/class_member/operations/create.rb +++ b/lib/concepts/class_member/operations/create.rb @@ -3,24 +3,26 @@ class ClassMember class Create class << self - def call(school_class:, student_ids:) + def call(school_class:, students:) response = OperationResponse.new response[:class_members] = [] - student_ids.each do |student_id| - class_member_params = { student_id: } - class_member = school_class.members.build(class_member_params) + raise ArgumentError, 'No valid students provided' if !students || students.empty? + students.each do |student| + params = { student_id: student.id } + class_member = school_class.members.build(params) + class_member.student = student class_member.save! response[:class_members] << class_member rescue StandardError => e Sentry.capture_exception(e) errors = class_member.errors.full_messages.join(',') response[:errors] ||= {} - response[:errors][student_id] = "Error creating class member for student_id #{student_id}: #{errors}" + response[:errors][student.id] = "Error creating class member for student_id #{student.id}: #{errors}" end response rescue StandardError => e Sentry.capture_exception(e) - response[:error] = "Error creating class members" + response[:error] = e.message || "Error creating class members" response end end From 2b016cc34732b1d3eb294df2a62600bb54f64e1c Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 19 Jul 2024 21:17:10 +0100 Subject: [PATCH 03/13] class_members_spec - remove student method tests --- spec/models/class_member_spec.rb | 74 +------------------------------- 1 file changed, 1 insertion(+), 73 deletions(-) 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 From 8564d6192f49cc0cbdc63b6542851e62fa514633 Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 19 Jul 2024 21:20:01 +0100 Subject: [PATCH 04/13] create_a_class_member_spec - updates to allow for profile api stubs and array response --- .../creating_a_class_member_spec.rb | 100 +++++++++++------- 1 file changed, 59 insertions(+), 41 deletions(-) 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..acd80d82 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,78 @@ } 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:) + 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:) } + student_ids = data.map { |class_member| class_member[:student_id] } + expect(student_ids).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_students = data.map { |class_member| 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_students).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_student_id) { SecureRandom.uuid } + + let(:invalid_params) { { class_member: { student_id: invalid_student_id } } } + + 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 From 8ad8e4c1a7e62bf10f5d6966f58f62e84dc5000e Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 19 Jul 2024 21:20:35 +0100 Subject: [PATCH 05/13] create_spec - edit to allow for student instances over id strings --- spec/concepts/class_member/create_spec.rb | 171 +++++++++++----------- 1 file changed, 85 insertions(+), 86 deletions(-) diff --git a/spec/concepts/class_member/create_spec.rb b/spec/concepts/class_member/create_spec.rb index eb2e0f01..d6ae35f6 100644 --- a/spec/concepts/class_member/create_spec.rb +++ b/spec/concepts/class_member/create_spec.rb @@ -8,138 +8,137 @@ let(:students) { create_list(:student, 3, school:) } let(:teacher) { create(:teacher, school:) } - let(:failed_student_id) { 'i-am-not-an-id' } let(:student_ids) { students.map(&:id) } it 'returns a successful operation response' do - response = described_class.call(school_class:, student_ids:) + 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:, student_ids:) }.to change(ClassMember, :count).by(3) + 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:, student_ids:) + response = described_class.call(school_class:, 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:, student_ids:) + 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:, student_ids:) + response = described_class.call(school_class:, students:) expect(response[:class_members]).to all(have_attributes(school_class: school_class)) end it 'assigns the student_id' do - response = described_class.call(school_class:, student_ids:) + response = described_class.call(school_class:, students:) expect(response[:class_members].map(&:student_id)).to match_array(student_ids) end - context 'with an empty array of student_ids' do - it 'returns a successful operation response' do - response = described_class.call(school_class:, student_ids: []) - expect(response[:class_members]).to eq([]) - end - end - context 'when creations fail' do before do allow(Sentry).to receive(:capture_exception) end - context 'with malformed student_ids' do - let(:student_ids) { nil } + context 'with malformed students' do + let(:students) { nil } it 'does not create a class member' do - expect { described_class.call(school_class:, student_ids:) }.not_to change(ClassMember, :count) + expect { described_class.call(school_class:, students:) }.not_to change(ClassMember, :count) end it 'returns a failed operation response' do - response = described_class.call(school_class:, student_ids:) + 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:, student_ids:) - expect(response[:error]).to match(/Error creating class members/) + 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:, student_ids:) + described_class.call(school_class:, students:) expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) end end - context 'with non existent student_ids' do - let(:student_ids) { [failed_student_id] } - - it 'does not create a class member' do - expect { described_class.call(school_class:, student_ids:) }.not_to change(ClassMember, :count) - end - - it 'returns a successful operation response' do - response = described_class.call(school_class:, student_ids:) - expect(response.success?).to be(true) - end - - it 'returns a successful operation response' do - response = described_class.call(school_class:, student_ids:) - expect(response[:class_members]).to eq([]) - end - - it 'returns the error messages in the operation response' do - response = described_class.call(school_class:, student_ids:) - expect(response[:errors][failed_student_id]).to include("Error creating class member for student_id #{student_ids.first}: Student can't be blank") - end - - it 'sent the exception to Sentry' do - described_class.call(school_class:, student_ids:) - expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) - end - end - - context 'when one creation fails' do - let(:student_ids) { students.map(&:id) << failed_student_id } - - it 'returns a successful operation response' do - response = described_class.call(school_class:, student_ids:) - expect(response.success?).to be(true) - end - - it 'returns a class members JSON array' do - response = described_class.call(school_class:, student_ids:) - expect(response[:class_members].size).to eq(3) - end - - it 'returns class members in the operation response' do - response = described_class.call(school_class:, student_ids:) - expect(response[:class_members]).to all(be_a(ClassMember)) - end - - it 'assigns the school_class' do - response = described_class.call(school_class:, student_ids:) - expect(response[:class_members]).to all(have_attributes(school_class: school_class)) - end - - it 'assigns the successful student_ids' do - response = described_class.call(school_class:, student_ids:) - expected_student_ids = student_ids - [failed_student_id] - expect(response[:class_members].map(&:student_id)).to match_array(expected_student_ids) - end - - it 'returns the error messages in the operation response' do - response = described_class.call(school_class:, student_ids:) - expect(response[:errors][failed_student_id]).to eq("Error creating class member for student_id #{student_ids.last}: Student can't be blank") - end - - it 'sent the exception to Sentry' do - described_class.call(school_class:, student_ids:) - 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(:new_student) { create(:student, school: different_school) } + let(:failed_student_id) { new_student.id } + + context 'with non existent students' do + let(:students) { [new_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 a successful operation response' 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][new_student.id]).to include("Error creating class member for student_id #{new_student.id}: Student '#{new_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 + [new_student] } + let(:student_ids) { students.map(&:id) << failed_student_id } + + 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: school_class)) + end + + it 'assigns the successful students' do + response = described_class.call(school_class:, students: new_students) + expected_student_ids = student_ids - [failed_student_id] + expect(response[:class_members].map(&:student_id)).to match_array(expected_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][failed_student_id]).to eq("Error creating class member for student_id #{new_student.id}: Student '#{new_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 From 3e8b5acc1538b5e12cb46af70c7e433414da55a0 Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 19 Jul 2024 21:21:05 +0100 Subject: [PATCH 06/13] class_members/create_batch spec --- .../creating_a_batch_of_class_members_spec.rb | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 spec/features/class_member/creating_a_batch_of_class_members_spec.rb 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..8ac9e64e --- /dev/null +++ b/spec/features/class_member/creating_a_batch_of_class_members_spec.rb @@ -0,0 +1,123 @@ +# 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.map { |class_member| class_member[: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.map { |class_member| class_member[: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 400 Bad Request 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 From 3ff8306d3270012ae75608d078f6d17ec335db4d Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 19 Jul 2024 21:30:08 +0100 Subject: [PATCH 07/13] rubocop linting --- app/models/ability.rb | 3 ++- lib/concepts/class_member/operations/create.rb | 5 +++-- spec/concepts/class_member/create_spec.rb | 8 ++++---- .../creating_a_batch_of_class_members_spec.rb | 8 +++----- .../features/class_member/creating_a_class_member_spec.rb | 4 ++-- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 36026181..0c16c5df 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -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 create_batch 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/lib/concepts/class_member/operations/create.rb b/lib/concepts/class_member/operations/create.rb index 4e4dfca8..fc12326e 100644 --- a/lib/concepts/class_member/operations/create.rb +++ b/lib/concepts/class_member/operations/create.rb @@ -6,7 +6,8 @@ class << self def call(school_class:, students:) response = OperationResponse.new response[:class_members] = [] - raise ArgumentError, 'No valid students provided' if !students || students.empty? + raise ArgumentError, 'No valid students provided' if students.blank? + students.each do |student| params = { student_id: student.id } class_member = school_class.members.build(params) @@ -22,7 +23,7 @@ def call(school_class:, students:) response rescue StandardError => e Sentry.capture_exception(e) - response[:error] = e.message || "Error creating class members" + response[:error] = e.message || 'Error creating class members' response end end diff --git a/spec/concepts/class_member/create_spec.rb b/spec/concepts/class_member/create_spec.rb index d6ae35f6..0c0a15ec 100644 --- a/spec/concepts/class_member/create_spec.rb +++ b/spec/concepts/class_member/create_spec.rb @@ -31,7 +31,7 @@ it 'assigns the school_class' do response = described_class.call(school_class:, students:) - expect(response[:class_members]).to all(have_attributes(school_class: school_class)) + expect(response[:class_members]).to all(have_attributes(school_class:)) end it 'assigns the student_id' do @@ -46,7 +46,7 @@ context 'with malformed students' do let(:students) { nil } - + it 'does not create a class member' do expect { described_class.call(school_class:, students:) }.not_to change(ClassMember, :count) end @@ -74,7 +74,7 @@ context 'with non existent students' do let(:students) { [new_student] } - + it 'does not create a class member' do expect { described_class.call(school_class:, students:) }.not_to change(ClassMember, :count) end @@ -121,7 +121,7 @@ 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: school_class)) + expect(response[:class_members]).to all(have_attributes(school_class:)) end it 'assigns the successful students' do 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 index 8ac9e64e..d51f2fbf 100644 --- 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 @@ -17,9 +17,8 @@ end context 'with valid params' do - let(:student_attributes) do - students.map do |student| + students.map do |student| { id: student.id, name: student.name, username: student.username } end end @@ -51,7 +50,7 @@ post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) data = JSON.parse(response.body, symbolize_names: true) - student_ids = data.map { |class_member| class_member[:student_id] } + student_ids = data.pluck(:student_id) expect(student_ids).to eq(params[:student_ids]) end @@ -59,13 +58,12 @@ post("/api/schools/#{school.id}/classes/#{school_class.id}/members/batch", headers:, params:) data = JSON.parse(response.body, symbolize_names: true) - response_students = data.map { |class_member| class_member[:student] } + 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: ' ' } } } 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 acd80d82..c58eca55 100644 --- a/spec/features/class_member/creating_a_class_member_spec.rb +++ b/spec/features/class_member/creating_a_class_member_spec.rb @@ -46,7 +46,7 @@ post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) data = JSON.parse(response.body, symbolize_names: true) - student_ids = data.map { |class_member| class_member[:student_id] } + student_ids = data.pluck(:student_id) expect(student_ids).to eq([student.id]) end @@ -54,7 +54,7 @@ post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) data = JSON.parse(response.body, symbolize_names: true) - response_students = data.map { |class_member| class_member[:student] } + response_students = data.pluck(:student) expect(response_students).to eq(student_attributes) end From 5544c0594be802fa553d9540df839445377906c6 Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 19 Jul 2024 21:35:38 +0100 Subject: [PATCH 08/13] rubocop test linting/refactor --- spec/concepts/class_member/create_spec.rb | 17 +++++++---------- .../creating_a_class_member_spec.rb | 4 +--- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/spec/concepts/class_member/create_spec.rb b/spec/concepts/class_member/create_spec.rb index 0c0a15ec..dbffe882 100644 --- a/spec/concepts/class_member/create_spec.rb +++ b/spec/concepts/class_member/create_spec.rb @@ -69,11 +69,10 @@ context 'with a student from a different school' do let(:different_school) { create(:school) } - let(:new_student) { create(:student, school: different_school) } - let(:failed_student_id) { new_student.id } + let(:different_school_student) { create(:student, school: different_school) } context 'with non existent students' do - let(:students) { [new_student] } + 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) @@ -84,14 +83,14 @@ expect(response.success?).to be(true) end - it 'returns a successful operation response' do + 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][new_student.id]).to include("Error creating class member for student_id #{new_student.id}: Student '#{new_student.id}' does not have the 'school-student' role for organisation '#{school.id}'") + 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 @@ -101,8 +100,7 @@ end context 'when one creation fails' do - let(:new_students) { students + [new_student] } - let(:student_ids) { students.map(&:id) << failed_student_id } + let(:new_students) { students + [different_school_student] } it 'returns a successful operation response' do response = described_class.call(school_class:, students: new_students) @@ -126,13 +124,12 @@ it 'assigns the successful students' do response = described_class.call(school_class:, students: new_students) - expected_student_ids = student_ids - [failed_student_id] - expect(response[:class_members].map(&:student_id)).to match_array(expected_student_ids) + 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][failed_student_id]).to eq("Error creating class member for student_id #{new_student.id}: Student '#{new_student.id}' does not have the 'school-student' role for organisation '#{school.id}'") + 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 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 c58eca55..de331c87 100644 --- a/spec/features/class_member/creating_a_class_member_spec.rb +++ b/spec/features/class_member/creating_a_class_member_spec.rb @@ -61,9 +61,7 @@ end context 'with invalid params' do - let(:invalid_student_id) { SecureRandom.uuid } - - let(:invalid_params) { { class_member: { student_id: invalid_student_id } } } + let(:invalid_params) { { class_member: { student_id: SecureRandom.uuid } } } before do stub_profile_api_list_school_students(school:, student_attributes: []) From bc2fdfb217e2d86e399b25c867ad6a98aa96212e Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 19 Jul 2024 22:02:30 +0100 Subject: [PATCH 09/13] ClassMember create concept rubocop refactor --- .../class_member/operations/create.rb | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/lib/concepts/class_member/operations/create.rb b/lib/concepts/class_member/operations/create.rb index fc12326e..080bded3 100644 --- a/lib/concepts/class_member/operations/create.rb +++ b/lib/concepts/class_member/operations/create.rb @@ -6,25 +6,35 @@ class << self def call(school_class:, students:) response = OperationResponse.new 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) + response[:error] = "Error creating class members: #{e.message}" + response + end + + private + + def create_class_members(school_class:, students:, response:) students.each do |student| - params = { student_id: student.id } - class_member = school_class.members.build(params) + 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 - Sentry.capture_exception(e) - errors = class_member.errors.full_messages.join(',') - response[:errors] ||= {} - response[:errors][student.id] = "Error creating class member for student_id #{student.id}: #{errors}" + handle_class_member_error(e, class_member, student, response) + response end - response - rescue StandardError => e - Sentry.capture_exception(e) - response[:error] = e.message || 'Error creating class members' - response + 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 From 2fce6339b155b5b53b5ee02d61d08e8e2eb575c8 Mon Sep 17 00:00:00 2001 From: Scott Date: Mon, 22 Jul 2024 16:58:53 +0100 Subject: [PATCH 10/13] add jbuilder partial for class_member --- .../class_members/_class_member.json.jbuilder | 19 ++++++++++++++ .../api/class_members/show.json.jbuilder | 26 +++++-------------- 2 files changed, 26 insertions(+), 19 deletions(-) create mode 100644 app/views/api/class_members/_class_member.json.jbuilder 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..2efda3a0 --- /dev/null +++ b/app/views/api/class_members/_class_member.json.jbuilder @@ -0,0 +1,19 @@ +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 4e3d41aa..e67f14e3 100644 --- a/app/views/api/class_members/show.json.jbuilder +++ b/app/views/api/class_members/show.json.jbuilder @@ -1,23 +1,11 @@ # frozen_string_literal: true -json.array!(@class_members) do |class_member| - 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 +if defined?(@class_members) + json.array! @class_members do |class_member| + json.partial! 'class_member', class_member: class_member + end +elsif defined?(@class_member) + json.class_member do + json.partial! 'class_member', class_member: @class_member end end From 1c30fe0d75547c0348f65b317917e99ac2affb7b Mon Sep 17 00:00:00 2001 From: Scott Date: Mon, 22 Jul 2024 16:59:16 +0100 Subject: [PATCH 11/13] fix test title for altered error response --- .../class_member/creating_a_batch_of_class_members_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index d51f2fbf..5e786d78 100644 --- 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 @@ -72,7 +72,7 @@ stub_profile_api_list_school_students(school:, student_attributes: []) end - it 'responds 400 Bad Request when params are missing' do + 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 From 248fcd76a5438bcb99a958ef81965420ac7022c3 Mon Sep 17 00:00:00 2001 From: Scott Date: Mon, 22 Jul 2024 17:00:06 +0100 Subject: [PATCH 12/13] update class member create to return an object of class_member using the new partial --- app/controllers/api/class_members_controller.rb | 2 +- .../class_member/creating_a_class_member_spec.rb | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/controllers/api/class_members_controller.rb b/app/controllers/api/class_members_controller.rb index ab53ac49..34454b2b 100644 --- a/app/controllers/api/class_members_controller.rb +++ b/app/controllers/api/class_members_controller.rb @@ -27,7 +27,7 @@ def create result = ClassMember::Create.call(school_class: @school_class, students: students[:school_students]) if result.success? - @class_members = result[:class_members] + @class_member = result[:class_members].first render :show, formats: [:json], status: :created else render json: { error: result[:error] }, status: :unprocessable_entity 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 de331c87..8ec6390c 100644 --- a/spec/features/class_member/creating_a_class_member_spec.rb +++ b/spec/features/class_member/creating_a_class_member_spec.rb @@ -24,10 +24,10 @@ end context 'with valid params' do - let(:student_attributes) { [{ id: student.id, name: student.name, username: student.username }] } + let(:student_attributes) { { id: student.id, name: student.name, username: student.username } } before do - stub_profile_api_list_school_students(school:, student_attributes:) + stub_profile_api_list_school_students(school:, student_attributes: [student_attributes]) end it 'responds 201 Created' do @@ -46,17 +46,16 @@ post("/api/schools/#{school.id}/classes/#{school_class.id}/members", headers:, params:) data = JSON.parse(response.body, symbolize_names: true) - student_ids = data.pluck(:student_id) - expect(student_ids).to eq([student.id]) + expect(data[:class_member][:student_id]).to eq(student.id) end 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) - response_students = data.pluck(:student) + response_student = data[:class_member][:student] - expect(response_students).to eq(student_attributes) + expect(response_student).to eq(student_attributes) end end From 1c3ddd9e567a7517b109e9465302956f7e6380a7 Mon Sep 17 00:00:00 2001 From: Scott Date: Mon, 22 Jul 2024 17:02:04 +0100 Subject: [PATCH 13/13] rubocop linting --- app/views/api/class_members/_class_member.json.jbuilder | 2 ++ app/views/api/class_members/show.json.jbuilder | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/views/api/class_members/_class_member.json.jbuilder b/app/views/api/class_members/_class_member.json.jbuilder index 2efda3a0..cba857c2 100644 --- a/app/views/api/class_members/_class_member.json.jbuilder +++ b/app/views/api/class_members/_class_member.json.jbuilder @@ -1,3 +1,5 @@ +# frozen_string_literal: true + json.call( class_member, :id, diff --git a/app/views/api/class_members/show.json.jbuilder b/app/views/api/class_members/show.json.jbuilder index e67f14e3..c4cbeeeb 100644 --- a/app/views/api/class_members/show.json.jbuilder +++ b/app/views/api/class_members/show.json.jbuilder @@ -2,7 +2,7 @@ if defined?(@class_members) json.array! @class_members do |class_member| - json.partial! 'class_member', class_member: class_member + json.partial! 'class_member', class_member: end elsif defined?(@class_member) json.class_member do