Class-member-create-allow-multiple-student-ids (#392)

## What's changed?

- Additional class_members endpoint for batch creation
- both create and create_batch use the same create concept
- class members create methods first check for the students in profile
via `SchoolStudent::List`
- the create concept utilises the `student` instance over ID strings
- the `student` attributes are nested in the `class_members` response
- failed student class member creations are added to an `errors` object
which is returned along with the successful creations

Author: sra405

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

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)

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

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

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

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'

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

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