From 0835fcd1e57afbbf8270dc545fb774203db18ba1 Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Tue, 12 May 2026 09:14:58 +0100 Subject: [PATCH 1/4] Add bulk lesson creation via lesson_projects Expand the LessonsController#create to accept a lesson_projects array in the request body and create multiple lessons in one request. Add bulk_create_params to the LessonsController, permitting origin_identifier on each entry so the caller can correlate responses with the original projects. Add Lesson::CreateBulk to create individual Lessons and echo origin_identifier on each result. --- app/controllers/api/lessons_controller.rb | 75 ++++++-- .../api/lessons/create_bulk.json.jbuilder | 11 ++ lib/concepts/lesson/operations/create_bulk.rb | 20 +++ spec/concepts/lesson/create_bulk_spec.rb | 79 +++++++++ .../features/lesson/creating_a_lesson_spec.rb | 164 ++++++++++++++++++ 5 files changed, 338 insertions(+), 11 deletions(-) create mode 100644 app/views/api/lessons/create_bulk.json.jbuilder create mode 100644 lib/concepts/lesson/operations/create_bulk.rb create mode 100644 spec/concepts/lesson/create_bulk_spec.rb diff --git a/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index 22cfad672..1e79da925 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/app/controllers/api/lessons_controller.rb @@ -30,13 +30,20 @@ def show end def create - result = Lesson::Create.call(lesson_params: create_params) - - if result.success? - @lesson_with_user = result[:lesson].with_user - render :show, formats: [:json], status: :created + if params[:lesson_projects].present? + @results = Lesson::CreateBulk.call( + lessons_params: params[:lesson_projects].map { |entry| bulk_create_params(entry) } + ) + @user = current_user + render :create_bulk, formats: [:json], status: :created else - render json: { error: result[:error] }, status: :unprocessable_content + result = Lesson::Create.call(lesson_params: create_params) + if result.success? + @lesson_with_user = result[:lesson].with_user + render :show, formats: [:json], status: :created + else + render json: { error: result[:error] }, status: :unprocessable_content + end end end @@ -78,14 +85,39 @@ def filtered_lessons_scope end def verify_school_class_belongs_to_school - return if create_params[:school_class_id].blank? - return if school&.classes&.pluck(:id)&.include?(create_params[:school_class_id]) + if params[:lesson_projects].present? + params[:lesson_projects].each { |lesson_params| verify_lesson_school_class!(lesson_params) } + else + verify_lesson_school_class!(create_params) + end + end + + def verify_lesson_school_class!(lesson_params) + school_class_id = lesson_params[:school_class_id] + return if school_class_id.blank? + + school = School.find_by(id: lesson_params[:school_id]) + return if school&.classes&.exists?(id: school_class_id) raise ParameterError, 'school_class_id does not correspond to school_id' end def verify_can_create_scratch_projects - return unless scratch_project? && !school.scratch_enabled? + if params[:lesson_projects].present? + scratch_project_params = params[:lesson_projects].find { |lesson_params| scratch_project?(lesson_params) } + return unless scratch_project_params + + verify_lesson_scratch!(scratch_project_params) + else + verify_lesson_scratch!(create_params) + end + end + + def verify_lesson_scratch!(lesson_params) + return unless scratch_project?(lesson_params) + + school = School.find_by(id: lesson_params[:school_id]) + return if school&.scratch_enabled? render json: { error: 'Forbidden' }, status: :forbidden end @@ -104,8 +136,8 @@ def user_remix(lesson) ) end - def scratch_project? - create_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH + def scratch_project?(lesson_params) + lesson_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH end def update_params @@ -118,6 +150,27 @@ def update_params ) end + def bulk_create_params(lesson_project) + lesson_project.permit( + :school_id, + :school_class_id, + :name, + :description, + :visibility, + :due_date, + :origin_identifier, + { + project_attributes: [ + :name, + :project_type, + :locale, + { components: %i[id name extension content index default] }, + { scratch_component: {} } + ] + } + ).merge(user_id: current_user.id) + end + def create_params params.fetch(:lesson, {}).permit( :school_id, diff --git a/app/views/api/lessons/create_bulk.json.jbuilder b/app/views/api/lessons/create_bulk.json.jbuilder new file mode 100644 index 000000000..c1a76d41e --- /dev/null +++ b/app/views/api/lessons/create_bulk.json.jbuilder @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +json.array!(@results) do |result| + if result.success? + json.partial! 'lesson', lesson: result[:lesson], user: @user + else + json.error result[:error] + end + + json.origin_identifier result[:origin_identifier] if result[:origin_identifier].present? +end diff --git a/lib/concepts/lesson/operations/create_bulk.rb b/lib/concepts/lesson/operations/create_bulk.rb new file mode 100644 index 000000000..ba9ce7ff8 --- /dev/null +++ b/lib/concepts/lesson/operations/create_bulk.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class Lesson + class CreateBulk + class << self + def call(lessons_params:) + lessons_params.map { |lesson| create_one(lesson) } + end + + private + + def create_one(lesson_params) + origin_identifier = lesson_params[:origin_identifier] + Lesson::Create.call(lesson_params: lesson_params.except(:origin_identifier)).tap do |result| + result[:origin_identifier] = origin_identifier if origin_identifier.present? + end + end + end + end +end diff --git a/spec/concepts/lesson/create_bulk_spec.rb b/spec/concepts/lesson/create_bulk_spec.rb new file mode 100644 index 000000000..9a9645b6e --- /dev/null +++ b/spec/concepts/lesson/create_bulk_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Lesson::CreateBulk, type: :unit do + let(:school) { create(:school) } + let(:teacher) { create(:teacher, school:) } + + let(:lessons_params) do + [ + { + name: 'Test Lesson', + user_id: teacher.id, + school_id: school.id, + origin_identifier: 'test-lesson-identifier-one', + project_attributes: { + name: 'Hello world project', + project_type: Project::Types::PYTHON, + components: [ + { name: 'main.py', extension: 'py', content: 'print("Hello, world!")' } + ] + } + }, + { + name: 'Test Lesson 2', + user_id: teacher.id, + school_id: school.id, + origin_identifier: 'test-lesson-identifier-two', + project_attributes: { + name: 'Hello world project', + project_type: Project::Types::PYTHON, + components: [ + { name: 'main.py', extension: 'py', content: 'print("Hello, world!")' } + ] + } + } + ] + end + + context 'with a teacher' do + let(:result) { described_class.call(lessons_params:) } + + before do + allow(User).to receive(:from_userinfo).with(ids: teacher.id).and_return([teacher]) + end + + it 'returns a successful operation response for the first lesson' do + expect(result.first.success?).to be(true) + end + + it 'returns a successful operation response for the second lesson' do + expect(result.second.success?).to be(true) + end + + it 'creates multiple lessons' do + expect { described_class.call(lessons_params:) }.to change(Lesson, :count).by(2) + end + + it 'does not pass origin_identifier to lesson creation' do + received_params = [] + allow(Lesson::Create).to receive(:call).and_wrap_original do |method, lesson_params:| + received_params << lesson_params + method.call(lesson_params:) + end + + described_class.call(lessons_params:) + + expect(received_params).to all(satisfy { |params| !params.key?(:origin_identifier) }) + end + + it 'appends the origin_identifier to the first created lesson' do + expect(result.first[:origin_identifier]).to eq('test-lesson-identifier-one') + end + + it 'appends the origin_identifier to the second created lesson' do + expect(result.second[:origin_identifier]).to eq('test-lesson-identifier-two') + end + end +end diff --git a/spec/features/lesson/creating_a_lesson_spec.rb b/spec/features/lesson/creating_a_lesson_spec.rb index 3679c672c..d40aa0212 100644 --- a/spec/features/lesson/creating_a_lesson_spec.rb +++ b/spec/features/lesson/creating_a_lesson_spec.rb @@ -118,6 +118,170 @@ end end + context 'when bulk creating lessons via lesson_projects' do + before { school.update!(scratch_enabled: true) } + + let(:lesson_project_params) do + [ + { + name: 'Lesson 1', + school_id: school.id, + project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } + }, + { + name: 'Lesson 2', + school_id: school.id, + project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } + } + ] + end + + it 'responds 201 Created' do + post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) + expect(response).to have_http_status(:created) + end + + it 'creates one lesson per entry' do + expect do + post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) + end.to change(Lesson, :count).by(2) + end + + it 'responds with the same lesson JSON shape as a single create' do + post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data).to all(include(:id, :name, :user_name)) + expect(data.pluck(:name)).to contain_exactly('Lesson 1', 'Lesson 2') + end + + it 'omits origin_identifier when not supplied' do + post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data).to all(satisfy { |entry| !entry.key?(:origin_identifier) }) + end + + context 'when origin_identifier is supplied' do + let(:lesson_project_params) do + [ + { + name: 'Lesson 1', + school_id: school.id, + origin_identifier: 'curriculum-project-one', + project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } + }, + { + name: 'Lesson 2', + school_id: school.id, + origin_identifier: 'curriculum-project-two', + project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } + } + ] + end + + it 'echoes origin_identifier on each successful entry' do + post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) + data = JSON.parse(response.body, symbolize_names: true) + expect(data.pluck(:origin_identifier)).to contain_exactly('curriculum-project-one', 'curriculum-project-two') + end + end + + context 'when some entries are invalid' do + let(:invalid_lesson_project_params) do + lesson_project_params + [{ + name: ' ', + school_id: school.id, + origin_identifier: 'curriculum-project-three', + project_attributes: { name: 'Project 3', project_type: Project::Types::CODE_EDITOR_SCRATCH } + }] + end + + it 'responds 201 Created' do + post('/api/lessons', headers:, params: { lesson_projects: invalid_lesson_project_params }) + expect(response).to have_http_status(:created) + end + + it 'includes an error entry for the failed lesson' do + post('/api/lessons', headers:, params: { lesson_projects: invalid_lesson_project_params }) + expect(response.parsed_body.any? { |entry| entry['error'].present? }).to be true + end + + it 'still creates the valid lessons' do + expect do + post('/api/lessons', headers:, params: { lesson_projects: invalid_lesson_project_params }) + end.to change(Lesson, :count).by(2) + end + + it 'echoes origin_identifier on failed entries' do + post('/api/lessons', headers:, params: { lesson_projects: invalid_lesson_project_params }) + error_entry = response.parsed_body.find { |entry| entry['error'].present? } + + expect(error_entry['origin_identifier']).to eq('curriculum-project-three') + end + end + + context 'when entries are associated with a school class' do + let(:school_class) { create(:school_class, teacher_ids: [teacher.id], school:) } + let(:lesson_project_params) do + [ + { + name: 'Lesson 1', + school_id: school.id, + school_class_id: school_class.id, + project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } + }, + { + name: 'Lesson 2', + school_id: school.id, + school_class_id: school_class.id, + project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } + } + ] + end + + before do + authenticated_in_hydra_as(teacher) + school_class.update!(teachers: [ClassTeacher.new({ teacher_id: teacher.id })]) + end + + it 'responds 201 Created' do + post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) + + expect(response).to have_http_status(:created) + end + + it 'responds 422 Unprocessable if school_class_id does not correspond to school_id' do + mismatched_params = lesson_project_params.map { |entry| entry.merge(school_id: SecureRandom.uuid) } + + post('/api/lessons', headers:, params: { lesson_projects: mismatched_params }) + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'does not create any lessons when school_class_id does not correspond to school_id' do + mismatched_params = lesson_project_params.map { |entry| entry.merge(school_id: SecureRandom.uuid) } + + expect do + post('/api/lessons', headers:, params: { lesson_projects: mismatched_params }) + end.not_to change(Lesson, :count) + end + + it 'rejects the request when only one entry has a mismatched school_id' do + mismatched_params = [ + lesson_project_params.first, + lesson_project_params.last.merge(school_id: SecureRandom.uuid) + ] + + expect do + post('/api/lessons', headers:, params: { lesson_projects: mismatched_params }) + end.not_to change(Lesson, :count) + + expect(response).to have_http_status(:unprocessable_content) + end + end + end + context 'when the lesson is associated with a school class' do let(:school_class) { create(:school_class, teacher_ids: [teacher.id], school:) } let(:school) { create(:school) } From cd618567539a336adc78c6c76cb91e96426183fe Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Mon, 8 Jun 2026 10:17:01 +0100 Subject: [PATCH 2/4] DRY up shared create params in LessonsController --- app/controllers/api/lessons_controller.rb | 58 +++++++++-------------- 1 file changed, 23 insertions(+), 35 deletions(-) diff --git a/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index 1e79da925..219dc0991 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/app/controllers/api/lessons_controller.rb @@ -4,6 +4,23 @@ module Api class LessonsController < ApiController include RemixSelection + LESSON_ATTRIBUTES = %i[ + school_id + school_class_id + name + description + visibility + due_date + ].freeze + + PROJECT_ATTRIBUTES = [ + :name, + :project_type, + :locale, + { components: %i[id name extension content index default] }, + { scratch_component: {} } + ].freeze + before_action :authorize_user, except: %i[index show] before_action :verify_school_class_belongs_to_school, only: :create before_action :verify_can_create_scratch_projects, only: %i[create create_copy] @@ -151,44 +168,15 @@ def update_params end def bulk_create_params(lesson_project) - lesson_project.permit( - :school_id, - :school_class_id, - :name, - :description, - :visibility, - :due_date, - :origin_identifier, - { - project_attributes: [ - :name, - :project_type, - :locale, - { components: %i[id name extension content index default] }, - { scratch_component: {} } - ] - } - ).merge(user_id: current_user.id) + permit_lesson_params(lesson_project, :origin_identifier).merge(user_id: current_user.id) end def create_params - params.fetch(:lesson, {}).permit( - :school_id, - :school_class_id, - :name, - :description, - :visibility, - :due_date, - { - project_attributes: [ - :name, - :project_type, - :locale, - { components: %i[id name extension content index default] }, - { scratch_component: {} } - ] - } - ).merge(user_id: current_user.id) + permit_lesson_params(params.fetch(:lesson, {})).merge(user_id: current_user.id) + end + + def permit_lesson_params(source, *extra) + source.permit(*LESSON_ATTRIBUTES, *extra, project_attributes: PROJECT_ATTRIBUTES) end def school_owner? From 80eb0fc1d47ff48c9227e57652a303207e1b141e Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Wed, 10 Jun 2026 10:38:23 +0100 Subject: [PATCH 3/4] Create lessons BatchController Separate batch lesson creation from single-lesson creation Add LessonCreation concern to handle shared params and logic --- .../api/lessons/batch_controller.rb | 53 +++++ app/controllers/api/lessons_controller.rb | 85 +------ app/controllers/concerns/lesson_creation.rb | 47 ++++ .../create_batch.json.jbuilder} | 2 +- config/routes.rb | 1 + .../{create_bulk.rb => create_batch.rb} | 2 +- ...eate_bulk_spec.rb => create_batch_spec.rb} | 2 +- .../creating_a_batch_of_lessons_spec.rb | 207 ++++++++++++++++++ .../features/lesson/creating_a_lesson_spec.rb | 164 -------------- 9 files changed, 321 insertions(+), 242 deletions(-) create mode 100644 app/controllers/api/lessons/batch_controller.rb create mode 100644 app/controllers/concerns/lesson_creation.rb rename app/views/api/lessons/{create_bulk.json.jbuilder => batch/create_batch.json.jbuilder} (74%) rename lib/concepts/lesson/operations/{create_bulk.rb => create_batch.rb} (96%) rename spec/concepts/lesson/{create_bulk_spec.rb => create_batch_spec.rb} (97%) create mode 100644 spec/features/lesson/creating_a_batch_of_lessons_spec.rb diff --git a/app/controllers/api/lessons/batch_controller.rb b/app/controllers/api/lessons/batch_controller.rb new file mode 100644 index 000000000..73a902c37 --- /dev/null +++ b/app/controllers/api/lessons/batch_controller.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module Api + module Lessons + class BatchController < ApiController + include RemixSelection + include LessonCreation + + before_action :authorize_user + before_action :verify_school_class_belongs_to_school + before_action :verify_can_create_scratch_projects + load_and_authorize_resource :lesson + + def create_batch + raise ParameterError, 'lesson_projects cannot be blank' unless lesson_projects? + + @results = Lesson::CreateBatch.call( + lessons_params: params[:lesson_projects].map { |entry| create_batch_params(entry) } + ) + @user = current_user + render :create_batch, formats: [:json], status: :created + end + + private + + def verify_school_class_belongs_to_school + return unless lesson_projects? + + params[:lesson_projects].each { |lesson_params| verify_lesson_school_class!(lesson_params) } + end + + def verify_can_create_scratch_projects + return unless lesson_projects? + + scratch_project_params = params[:lesson_projects].find { |lesson_params| scratch_project?(lesson_params) } + return unless scratch_project_params + + verify_lesson_scratch!(scratch_project_params) + end + + def create_batch_params(lesson_project) + lesson_project.permit(*LESSON_ATTRIBUTES, :origin_identifier, project_attributes: PROJECT_ATTRIBUTES).merge(user_id: current_user.id) + end + + def lesson_projects? + projects = params[:lesson_projects] + return false unless projects.is_a?(Array) + + projects.any?(&:present?) + end + end + end +end diff --git a/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index 219dc0991..675548196 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/app/controllers/api/lessons_controller.rb @@ -3,23 +3,7 @@ module Api class LessonsController < ApiController include RemixSelection - - LESSON_ATTRIBUTES = %i[ - school_id - school_class_id - name - description - visibility - due_date - ].freeze - - PROJECT_ATTRIBUTES = [ - :name, - :project_type, - :locale, - { components: %i[id name extension content index default] }, - { scratch_component: {} } - ].freeze + include LessonCreation before_action :authorize_user, except: %i[index show] before_action :verify_school_class_belongs_to_school, only: :create @@ -47,20 +31,12 @@ def show end def create - if params[:lesson_projects].present? - @results = Lesson::CreateBulk.call( - lessons_params: params[:lesson_projects].map { |entry| bulk_create_params(entry) } - ) - @user = current_user - render :create_bulk, formats: [:json], status: :created + result = Lesson::Create.call(lesson_params: create_params) + if result.success? + @lesson_with_user = result[:lesson].with_user + render :show, formats: [:json], status: :created else - result = Lesson::Create.call(lesson_params: create_params) - if result.success? - @lesson_with_user = result[:lesson].with_user - render :show, formats: [:json], status: :created - else - render json: { error: result[:error] }, status: :unprocessable_content - end + render json: { error: result[:error] }, status: :unprocessable_content end end @@ -102,41 +78,11 @@ def filtered_lessons_scope end def verify_school_class_belongs_to_school - if params[:lesson_projects].present? - params[:lesson_projects].each { |lesson_params| verify_lesson_school_class!(lesson_params) } - else - verify_lesson_school_class!(create_params) - end - end - - def verify_lesson_school_class!(lesson_params) - school_class_id = lesson_params[:school_class_id] - return if school_class_id.blank? - - school = School.find_by(id: lesson_params[:school_id]) - return if school&.classes&.exists?(id: school_class_id) - - raise ParameterError, 'school_class_id does not correspond to school_id' + verify_lesson_school_class!(create_params) end def verify_can_create_scratch_projects - if params[:lesson_projects].present? - scratch_project_params = params[:lesson_projects].find { |lesson_params| scratch_project?(lesson_params) } - return unless scratch_project_params - - verify_lesson_scratch!(scratch_project_params) - else - verify_lesson_scratch!(create_params) - end - end - - def verify_lesson_scratch!(lesson_params) - return unless scratch_project?(lesson_params) - - school = School.find_by(id: lesson_params[:school_id]) - return if school&.scratch_enabled? - - render json: { error: 'Forbidden' }, status: :forbidden + verify_lesson_scratch!(create_params) end def user_remixes(lessons) @@ -153,10 +99,6 @@ def user_remix(lesson) ) end - def scratch_project?(lesson_params) - lesson_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH - end - def update_params params.fetch(:lesson, {}).permit( :name, @@ -167,16 +109,9 @@ def update_params ) end - def bulk_create_params(lesson_project) - permit_lesson_params(lesson_project, :origin_identifier).merge(user_id: current_user.id) - end - def create_params - permit_lesson_params(params.fetch(:lesson, {})).merge(user_id: current_user.id) - end - - def permit_lesson_params(source, *extra) - source.permit(*LESSON_ATTRIBUTES, *extra, project_attributes: PROJECT_ATTRIBUTES) + source = params.fetch(:lesson, {}) + source.permit(*LESSON_ATTRIBUTES, project_attributes: PROJECT_ATTRIBUTES).merge(user_id: current_user.id) end def school_owner? diff --git a/app/controllers/concerns/lesson_creation.rb b/app/controllers/concerns/lesson_creation.rb new file mode 100644 index 000000000..80563dd1c --- /dev/null +++ b/app/controllers/concerns/lesson_creation.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module LessonCreation + extend ActiveSupport::Concern + + LESSON_ATTRIBUTES = %i[ + school_id + school_class_id + name + description + visibility + due_date + ].freeze + + PROJECT_ATTRIBUTES = [ + :name, + :project_type, + :locale, + { components: %i[id name extension content index default] }, + { scratch_component: {} } + ].freeze + + private + + def verify_lesson_school_class!(lesson_params) + school_class_id = lesson_params[:school_class_id] + return if school_class_id.blank? + + school = School.find_by(id: lesson_params[:school_id]) + return if school&.classes&.exists?(id: school_class_id) + + raise ParameterError, 'school_class_id does not correspond to school_id' + end + + def verify_lesson_scratch!(lesson_params) + return unless scratch_project?(lesson_params) + + school = School.find_by(id: lesson_params[:school_id]) + return if school&.scratch_enabled? + + render json: { error: 'Forbidden' }, status: :forbidden + end + + def scratch_project?(lesson_params) + lesson_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH + end +end diff --git a/app/views/api/lessons/create_bulk.json.jbuilder b/app/views/api/lessons/batch/create_batch.json.jbuilder similarity index 74% rename from app/views/api/lessons/create_bulk.json.jbuilder rename to app/views/api/lessons/batch/create_batch.json.jbuilder index c1a76d41e..c3bcec865 100644 --- a/app/views/api/lessons/create_bulk.json.jbuilder +++ b/app/views/api/lessons/batch/create_batch.json.jbuilder @@ -2,7 +2,7 @@ json.array!(@results) do |result| if result.success? - json.partial! 'lesson', lesson: result[:lesson], user: @user + json.partial! 'api/lessons/lesson', lesson: result[:lesson], user: @user else json.error result[:error] end diff --git a/config/routes.rb b/config/routes.rb index 23d279083..99bb7a685 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -86,6 +86,7 @@ resources :lessons, only: %i[index create show update destroy] do post :copy, on: :member, to: 'lessons#create_copy' + post :batch, on: :collection, to: 'lessons/batch#create_batch' end resources :teacher_invitations, param: :token, only: :show do diff --git a/lib/concepts/lesson/operations/create_bulk.rb b/lib/concepts/lesson/operations/create_batch.rb similarity index 96% rename from lib/concepts/lesson/operations/create_bulk.rb rename to lib/concepts/lesson/operations/create_batch.rb index ba9ce7ff8..d1bb50f80 100644 --- a/lib/concepts/lesson/operations/create_bulk.rb +++ b/lib/concepts/lesson/operations/create_batch.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Lesson - class CreateBulk + class CreateBatch class << self def call(lessons_params:) lessons_params.map { |lesson| create_one(lesson) } diff --git a/spec/concepts/lesson/create_bulk_spec.rb b/spec/concepts/lesson/create_batch_spec.rb similarity index 97% rename from spec/concepts/lesson/create_bulk_spec.rb rename to spec/concepts/lesson/create_batch_spec.rb index 9a9645b6e..ff8523aef 100644 --- a/spec/concepts/lesson/create_bulk_spec.rb +++ b/spec/concepts/lesson/create_batch_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' -RSpec.describe Lesson::CreateBulk, type: :unit do +RSpec.describe Lesson::CreateBatch, type: :unit do let(:school) { create(:school) } let(:teacher) { create(:teacher, school:) } diff --git a/spec/features/lesson/creating_a_batch_of_lessons_spec.rb b/spec/features/lesson/creating_a_batch_of_lessons_spec.rb new file mode 100644 index 000000000..f46dc97f3 --- /dev/null +++ b/spec/features/lesson/creating_a_batch_of_lessons_spec.rb @@ -0,0 +1,207 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Creating a batch of lessons', type: :request do + let(:headers) { { Authorization: UserProfileMock::TOKEN } } + let(:lesson_project_params) do + [ + { + name: 'Lesson 1', + school_id: school.id, + project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } + }, + { + name: 'Lesson 2', + school_id: school.id, + project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } + } + ] + end + + let(:teacher) { create(:teacher, school:) } + let(:school) { create(:school, scratch_enabled:) } + let(:scratch_enabled) { true } + let(:batch_path) { '/api/lessons/batch' } + let(:lesson_projects) { lesson_project_params } + + before do + authenticated_in_hydra_as(teacher) + stub_user_info_api_for(teacher) + post(batch_path, headers:, params: { lesson_projects: }) + end + + it 'responds 201 Created' do + expect(response).to have_http_status(:created) + end + + it 'creates the lessons' do + expect(Lesson.count).to eq(2) + end + + it 'responds with the same lesson JSON shape as a single create' do + data = JSON.parse(response.body, symbolize_names: true) + + expect(data).to all(include(:id, :name, :user_name)) + expect(data.pluck(:name)).to contain_exactly('Lesson 1', 'Lesson 2') + end + + it 'omits origin_identifier when not supplied' do + data = JSON.parse(response.body, symbolize_names: true) + expect(data).to all(satisfy { |entry| !entry.key?(:origin_identifier) }) + end + + context 'when origin_identifier is supplied' do + let(:lesson_project_params) do + [ + { + name: 'Lesson 1', + school_id: school.id, + origin_identifier: 'curriculum-project-one', + project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } + }, + { + name: 'Lesson 2', + school_id: school.id, + origin_identifier: 'curriculum-project-two', + project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } + } + ] + end + + it 'echoes origin_identifier on each successful entry' do + data = JSON.parse(response.body, symbolize_names: true) + expect(data.pluck(:origin_identifier)).to contain_exactly('curriculum-project-one', 'curriculum-project-two') + end + end + + context 'when some entries are invalid' do + let(:lesson_projects) do + lesson_project_params + [{ + name: ' ', + school_id: school.id, + origin_identifier: 'curriculum-project-three', + project_attributes: { name: 'Project 3', project_type: Project::Types::CODE_EDITOR_SCRATCH } + }] + end + + it 'responds 201 Created' do + expect(response).to have_http_status(:created) + end + + it 'still creates the valid lessons' do + expect(Lesson.count).to eq(2) + end + + it 'echoes origin_identifier on failed entries' do + error_entry = response.parsed_body.find { |entry| entry['error'].present? } + expect(error_entry['origin_identifier']).to eq('curriculum-project-three') + end + end + + context 'when entries are associated with a school class' do + let(:school_class) { create(:school_class, teacher_ids: [teacher.id], school:) } + let(:lesson_project_params) do + [ + { + name: 'Lesson 1', + school_id: school.id, + school_class_id: school_class.id, + project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } + }, + { + name: 'Lesson 2', + school_id: school.id, + school_class_id: school_class.id, + project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } + } + ] + end + + before do + authenticated_in_hydra_as(teacher) + school_class.update!(teachers: [ClassTeacher.new({ teacher_id: teacher.id })]) + end + + it 'responds 201 Created' do + expect(response).to have_http_status(:created) + end + + context 'when school_class_id does not correspond to school_id' do + let(:lesson_projects) { lesson_project_params.map { |entry| entry.merge(school_id: SecureRandom.uuid) } } + + it 'responds 422 Unprocessable' do + expect(response).to have_http_status(:unprocessable_content) + end + + it 'does not create any lessons' do + expect(Lesson.count).to eq(0) + end + end + + context 'when only one entry has a mismatched school_id' do + let(:lesson_projects) do + [ + lesson_project_params.first, + lesson_project_params.last.merge(school_id: SecureRandom.uuid) + ] + end + + it 'rejects the entire request' do + expect(response).to have_http_status(:unprocessable_content) + end + + it 'does not create any lessons' do + expect(Lesson.count).to eq(0) + end + end + end + + context 'when the school does not have Scratch enabled' do + let(:scratch_enabled) { false } + + it 'returns forbidden' do + expect(response).to have_http_status(:forbidden) + end + + it 'does not create any lessons' do + expect(Lesson.count).to eq(0) + end + end + + context 'when there lesson projects is an empty array' do + let(:lesson_project_params) { [] } + + it 'responds 422 Unprocessable' do + expect(response).to have_http_status(:unprocessable_content) + end + + it 'does not create any lessons' do + expect(Lesson.count).to eq(0) + end + end + + context 'when lesson projects is an array with an empty project' do + let(:lesson_project_params) { [{}] } + + it 'responds 422 Unprocessable' do + expect(response).to have_http_status(:unprocessable_content) + end + + it 'does not create any lessons' do + expect(Lesson.count).to eq(0) + end + end + + context 'when lesson projects is nil' do + let(:lesson_project_params) { nil } + + it 'responds 422 Unprocessable' do + expect(response).to have_http_status(:unprocessable_content) + end + + it 'does not create any lessons' do + expect(Lesson.count).to eq(0) + end + end +end diff --git a/spec/features/lesson/creating_a_lesson_spec.rb b/spec/features/lesson/creating_a_lesson_spec.rb index d40aa0212..3679c672c 100644 --- a/spec/features/lesson/creating_a_lesson_spec.rb +++ b/spec/features/lesson/creating_a_lesson_spec.rb @@ -118,170 +118,6 @@ end end - context 'when bulk creating lessons via lesson_projects' do - before { school.update!(scratch_enabled: true) } - - let(:lesson_project_params) do - [ - { - name: 'Lesson 1', - school_id: school.id, - project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } - }, - { - name: 'Lesson 2', - school_id: school.id, - project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } - } - ] - end - - it 'responds 201 Created' do - post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) - expect(response).to have_http_status(:created) - end - - it 'creates one lesson per entry' do - expect do - post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) - end.to change(Lesson, :count).by(2) - end - - it 'responds with the same lesson JSON shape as a single create' do - post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) - data = JSON.parse(response.body, symbolize_names: true) - - expect(data).to all(include(:id, :name, :user_name)) - expect(data.pluck(:name)).to contain_exactly('Lesson 1', 'Lesson 2') - end - - it 'omits origin_identifier when not supplied' do - post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) - data = JSON.parse(response.body, symbolize_names: true) - - expect(data).to all(satisfy { |entry| !entry.key?(:origin_identifier) }) - end - - context 'when origin_identifier is supplied' do - let(:lesson_project_params) do - [ - { - name: 'Lesson 1', - school_id: school.id, - origin_identifier: 'curriculum-project-one', - project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } - }, - { - name: 'Lesson 2', - school_id: school.id, - origin_identifier: 'curriculum-project-two', - project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } - } - ] - end - - it 'echoes origin_identifier on each successful entry' do - post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) - data = JSON.parse(response.body, symbolize_names: true) - expect(data.pluck(:origin_identifier)).to contain_exactly('curriculum-project-one', 'curriculum-project-two') - end - end - - context 'when some entries are invalid' do - let(:invalid_lesson_project_params) do - lesson_project_params + [{ - name: ' ', - school_id: school.id, - origin_identifier: 'curriculum-project-three', - project_attributes: { name: 'Project 3', project_type: Project::Types::CODE_EDITOR_SCRATCH } - }] - end - - it 'responds 201 Created' do - post('/api/lessons', headers:, params: { lesson_projects: invalid_lesson_project_params }) - expect(response).to have_http_status(:created) - end - - it 'includes an error entry for the failed lesson' do - post('/api/lessons', headers:, params: { lesson_projects: invalid_lesson_project_params }) - expect(response.parsed_body.any? { |entry| entry['error'].present? }).to be true - end - - it 'still creates the valid lessons' do - expect do - post('/api/lessons', headers:, params: { lesson_projects: invalid_lesson_project_params }) - end.to change(Lesson, :count).by(2) - end - - it 'echoes origin_identifier on failed entries' do - post('/api/lessons', headers:, params: { lesson_projects: invalid_lesson_project_params }) - error_entry = response.parsed_body.find { |entry| entry['error'].present? } - - expect(error_entry['origin_identifier']).to eq('curriculum-project-three') - end - end - - context 'when entries are associated with a school class' do - let(:school_class) { create(:school_class, teacher_ids: [teacher.id], school:) } - let(:lesson_project_params) do - [ - { - name: 'Lesson 1', - school_id: school.id, - school_class_id: school_class.id, - project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } - }, - { - name: 'Lesson 2', - school_id: school.id, - school_class_id: school_class.id, - project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } - } - ] - end - - before do - authenticated_in_hydra_as(teacher) - school_class.update!(teachers: [ClassTeacher.new({ teacher_id: teacher.id })]) - end - - it 'responds 201 Created' do - post('/api/lessons', headers:, params: { lesson_projects: lesson_project_params }) - - expect(response).to have_http_status(:created) - end - - it 'responds 422 Unprocessable if school_class_id does not correspond to school_id' do - mismatched_params = lesson_project_params.map { |entry| entry.merge(school_id: SecureRandom.uuid) } - - post('/api/lessons', headers:, params: { lesson_projects: mismatched_params }) - - expect(response).to have_http_status(:unprocessable_content) - end - - it 'does not create any lessons when school_class_id does not correspond to school_id' do - mismatched_params = lesson_project_params.map { |entry| entry.merge(school_id: SecureRandom.uuid) } - - expect do - post('/api/lessons', headers:, params: { lesson_projects: mismatched_params }) - end.not_to change(Lesson, :count) - end - - it 'rejects the request when only one entry has a mismatched school_id' do - mismatched_params = [ - lesson_project_params.first, - lesson_project_params.last.merge(school_id: SecureRandom.uuid) - ] - - expect do - post('/api/lessons', headers:, params: { lesson_projects: mismatched_params }) - end.not_to change(Lesson, :count) - - expect(response).to have_http_status(:unprocessable_content) - end - end - end - context 'when the lesson is associated with a school class' do let(:school_class) { create(:school_class, teacher_ids: [teacher.id], school:) } let(:school) { create(:school) } From aba356b8d8898b139ef47cf68b53cc5d636498ee Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Wed, 10 Jun 2026 12:23:38 +0100 Subject: [PATCH 4/4] Authorize each lesson in batch create with CanCan's authorize! --- .../api/lessons/batch_controller.rb | 16 ++++++++++-- .../creating_a_batch_of_lessons_spec.rb | 26 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/lessons/batch_controller.rb b/app/controllers/api/lessons/batch_controller.rb index 73a902c37..43732eb0a 100644 --- a/app/controllers/api/lessons/batch_controller.rb +++ b/app/controllers/api/lessons/batch_controller.rb @@ -9,13 +9,13 @@ class BatchController < ApiController before_action :authorize_user before_action :verify_school_class_belongs_to_school before_action :verify_can_create_scratch_projects - load_and_authorize_resource :lesson + before_action :authorize_lesson_projects! def create_batch raise ParameterError, 'lesson_projects cannot be blank' unless lesson_projects? @results = Lesson::CreateBatch.call( - lessons_params: params[:lesson_projects].map { |entry| create_batch_params(entry) } + lessons_params: batch_lessons_params ) @user = current_user render :create_batch, formats: [:json], status: :created @@ -38,6 +38,10 @@ def verify_can_create_scratch_projects verify_lesson_scratch!(scratch_project_params) end + def batch_lessons_params + @batch_lessons_params ||= params[:lesson_projects].map { |lesson_params| create_batch_params(lesson_params) } + end + def create_batch_params(lesson_project) lesson_project.permit(*LESSON_ATTRIBUTES, :origin_identifier, project_attributes: PROJECT_ATTRIBUTES).merge(user_id: current_user.id) end @@ -48,6 +52,14 @@ def lesson_projects? projects.any?(&:present?) end + + def authorize_lesson_projects! + return unless lesson_projects? + + batch_lessons_params.each do |lesson_params| + authorize! :create, Lesson.new(lesson_params.slice(:school_id, :school_class_id, :user_id)) + end + end end end end diff --git a/spec/features/lesson/creating_a_batch_of_lessons_spec.rb b/spec/features/lesson/creating_a_batch_of_lessons_spec.rb index f46dc97f3..7a7e84f86 100644 --- a/spec/features/lesson/creating_a_batch_of_lessons_spec.rb +++ b/spec/features/lesson/creating_a_batch_of_lessons_spec.rb @@ -157,6 +157,32 @@ end end + context 'when the user does not belong to the school' do + let(:other_school) { create(:school, scratch_enabled: true) } + let(:lesson_project_params) do + [ + { + name: 'Lesson 1', + school_id: other_school.id, + project_attributes: { name: 'Project 1', project_type: Project::Types::CODE_EDITOR_SCRATCH } + }, + { + name: 'Lesson 2', + school_id: other_school.id, + project_attributes: { name: 'Project 2', project_type: Project::Types::CODE_EDITOR_SCRATCH } + } + ] + end + + it 'responds 403 Forbidden' do + expect(response).to have_http_status(:forbidden) + end + + it 'does not create any lessons' do + expect(Lesson.count).to eq(0) + end + end + context 'when the school does not have Scratch enabled' do let(:scratch_enabled) { false }