diff --git a/app/controllers/api/lessons/batch_controller.rb b/app/controllers/api/lessons/batch_controller.rb new file mode 100644 index 000000000..43732eb0a --- /dev/null +++ b/app/controllers/api/lessons/batch_controller.rb @@ -0,0 +1,65 @@ +# 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 + before_action :authorize_lesson_projects! + + def create_batch + raise ParameterError, 'lesson_projects cannot be blank' unless lesson_projects? + + @results = Lesson::CreateBatch.call( + lessons_params: batch_lessons_params + ) + @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 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 + + def lesson_projects? + projects = params[:lesson_projects] + return false unless projects.is_a?(Array) + + 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/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index 22cfad672..675548196 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/app/controllers/api/lessons_controller.rb @@ -3,6 +3,7 @@ module Api class LessonsController < ApiController include RemixSelection + include LessonCreation before_action :authorize_user, except: %i[index show] before_action :verify_school_class_belongs_to_school, only: :create @@ -31,7 +32,6 @@ def show 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 @@ -78,16 +78,11 @@ 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]) - - raise ParameterError, 'school_class_id does not correspond to school_id' + verify_lesson_school_class!(create_params) end def verify_can_create_scratch_projects - return unless scratch_project? && !school.scratch_enabled? - - render json: { error: 'Forbidden' }, status: :forbidden + verify_lesson_scratch!(create_params) end def user_remixes(lessons) @@ -104,10 +99,6 @@ def user_remix(lesson) ) end - def scratch_project? - create_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH - end - def update_params params.fetch(:lesson, {}).permit( :name, @@ -119,23 +110,8 @@ def update_params 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) + 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/batch/create_batch.json.jbuilder b/app/views/api/lessons/batch/create_batch.json.jbuilder new file mode 100644 index 000000000..c3bcec865 --- /dev/null +++ b/app/views/api/lessons/batch/create_batch.json.jbuilder @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +json.array!(@results) do |result| + if result.success? + json.partial! 'api/lessons/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/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_batch.rb b/lib/concepts/lesson/operations/create_batch.rb new file mode 100644 index 000000000..d1bb50f80 --- /dev/null +++ b/lib/concepts/lesson/operations/create_batch.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class Lesson + class CreateBatch + 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_batch_spec.rb b/spec/concepts/lesson/create_batch_spec.rb new file mode 100644 index 000000000..ff8523aef --- /dev/null +++ b/spec/concepts/lesson/create_batch_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Lesson::CreateBatch, 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_batch_of_lessons_spec.rb b/spec/features/lesson/creating_a_batch_of_lessons_spec.rb new file mode 100644 index 000000000..7a7e84f86 --- /dev/null +++ b/spec/features/lesson/creating_a_batch_of_lessons_spec.rb @@ -0,0 +1,233 @@ +# 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 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 } + + 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