diff --git a/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index a1071b654..0c1385ccb 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/app/controllers/api/lessons_controller.rb @@ -31,7 +31,9 @@ def show end def create - result = Lesson::Create.call(lesson_params: create_params) + authorize! :show, source_project if source_project + + result = Lesson::Create.call(lesson_params: create_params, source_project:) if result.success? track_project_event('Project - Created', result[:lesson].project) @lesson_with_user = result[:lesson].with_user @@ -85,7 +87,7 @@ def verify_school_class_belongs_to_school end def verify_can_create_scratch_projects - verify_lesson_scratch!(create_params) + verify_lesson_scratch!(create_params, source_project:) end def user_remixes(lessons) @@ -124,5 +126,29 @@ def school_owner? def school @school ||= @lesson&.school || School.find_by(id: create_params[:school_id]) || SchoolClass.find_by(id: params[:school_class_id])&.school end + + def source_project + return @source_project if defined?(@source_project) + + @source_project = find_source_project!(source_project_identifier, create_params.dig(:project_attributes, :locale)) + end + + def source_project_identifier + params.dig(:lesson, :source_project_identifier) + end + + def find_source_project!(identifier, locale) + return nil if identifier.blank? + + project = ProjectLoader.new(identifier, [locale]).load + raise ParameterError, "source project '#{identifier}' not found" if project.nil? + + # Only ExCS 'code editor' projects are remixed here; legacy scratch projects keep the stub path. + return nil unless project.scratch_project? + + raise ParameterError, 'source project must be an Experience CS project' unless project.origin == Project::Origins::EXPERIENCE_CS + + project + end end end diff --git a/app/controllers/concerns/lesson_creation.rb b/app/controllers/concerns/lesson_creation.rb index 80563dd1c..438f461fb 100644 --- a/app/controllers/concerns/lesson_creation.rb +++ b/app/controllers/concerns/lesson_creation.rb @@ -32,8 +32,8 @@ def verify_lesson_school_class!(lesson_params) raise ParameterError, 'school_class_id does not correspond to school_id' end - def verify_lesson_scratch!(lesson_params) - return unless scratch_project?(lesson_params) + def verify_lesson_scratch!(lesson_params, source_project: nil) + return unless scratch_project?(lesson_params) || source_project&.scratch_project? school = School.find_by(id: lesson_params[:school_id]) return if school&.scratch_enabled? diff --git a/lib/concepts/lesson/operations/create.rb b/lib/concepts/lesson/operations/create.rb index 77ac1d55d..9c7e32ede 100644 --- a/lib/concepts/lesson/operations/create.rb +++ b/lib/concepts/lesson/operations/create.rb @@ -3,9 +3,10 @@ class Lesson class Create class << self - def call(lesson_params:) + # When source_project is given, the lesson's project is built as a remix of it instead of a stub. + def call(lesson_params:, source_project: nil) response = OperationResponse.new - response[:lesson] = build_lesson(lesson_params) + response[:lesson] = build_lesson(lesson_params, source_project) response[:lesson].save! response rescue StandardError => e @@ -21,14 +22,30 @@ def call(lesson_params:) private - def build_lesson(lesson_hash) + def build_lesson(lesson_hash, source_project) new_lesson = Lesson.new(lesson_hash.except(:project_attributes)) - project_params = lesson_hash[:project_attributes].merge({ user_id: lesson_hash[:user_id], - school_id: lesson_hash[:school_id], - lesson_id: new_lesson.id }) - new_lesson.project = Project.new(project_params) + project_params = (lesson_hash[:project_attributes] || {}).merge({ user_id: lesson_hash[:user_id], + school_id: lesson_hash[:school_id], + lesson_id: new_lesson.id }) + new_lesson.project = source_project ? build_copy(source_project, project_params) : Project.new(project_params) new_lesson end + + def build_copy(source_project, project_params) + Project::Copying.copy_project(source_project, + attributes: attributes_for_copy(source_project, project_params), + media: %i[images videos audio]) + end + + def attributes_for_copy(source_project, project_params) + { + locale: nil, + name: project_params[:name].presence || source_project.name, + user_id: project_params[:user_id], + school_id: project_params[:school_id], + lesson_id: project_params[:lesson_id] + } + end end end end diff --git a/lib/concepts/lesson/operations/create_copy.rb b/lib/concepts/lesson/operations/create_copy.rb index 72a8f0aad..866ccbff5 100644 --- a/lib/concepts/lesson/operations/create_copy.rb +++ b/lib/concepts/lesson/operations/create_copy.rb @@ -27,34 +27,11 @@ def build_copy(lesson, lesson_params) lesson_copy.assign_attributes(lesson_params) project_params = { name: lesson_copy.name, user_id: lesson_params[:user_id], lesson_id: lesson_copy.id } - lesson_copy.project = build_project_copy(lesson.project, project_params) + lesson_copy.project = Project::Copying.copy_project(lesson.project, attributes: project_params) lesson_copy end - def build_project_copy(project, project_params) - project_attributes = project.attributes.except('id', 'identifier', 'created_at', 'updated_at').merge(project_params) - project_copy = Project.new(project_attributes) - - project.images.each do |image| - project_copy.images.attach(image.blob) - end - - project.components.each do |component| - project_copy.components.build({ name: component.name, extension: component.extension, content: component.content }) - end - - copy_scratch_component(project, project_copy) - - project_copy - end - - def copy_scratch_component(project, project_copy) - return unless project.scratch_project? && project.scratch_component - - project_copy.build_scratch_component(content: project.scratch_component.content.deep_dup) - end - def copy_scratch_assets(project, project_copy) return unless project.scratch_project? diff --git a/lib/concepts/project/copying.rb b/lib/concepts/project/copying.rb new file mode 100644 index 000000000..408719cf5 --- /dev/null +++ b/lib/concepts/project/copying.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +class Project + class Copying + class << self + def copy_project(source_project, attributes: {}, media: %i[images]) + source_project.dup.tap do |project_copy| + project_copy.identifier = nil + project_copy.assign_attributes(attributes) + + copy_components(source_project, project_copy) + copy_scratch_component(source_project, project_copy) + copy_media(source_project, project_copy, media) + end + end + + def copy_media(source_project, project_copy, media) + media.each do |collection| + source_project.public_send(collection).each do |attachment| + project_copy.public_send(collection).attach(attachment.blob) + end + end + end + + private + + def copy_components(source_project, project_copy) + source_project.components.each do |component| + project_copy.components.build(component.attributes.slice('name', 'extension', 'content')) + end + end + + def copy_scratch_component(source_project, project_copy) + return unless source_project.scratch_project? && source_project.scratch_component + + project_copy.build_scratch_component(content: source_project.scratch_component.content.deep_dup) + end + end + end +end diff --git a/lib/concepts/project/operations/create_remix.rb b/lib/concepts/project/operations/create_remix.rb index 81f70714b..a306576dd 100644 --- a/lib/concepts/project/operations/create_remix.rb +++ b/lib/concepts/project/operations/create_remix.rb @@ -35,17 +35,7 @@ def remix_project(response, params, user_id, original_project, remix_origin) def create_remix(original_project, params, user_id, remix_origin) remix = format_project(original_project, params, user_id, remix_origin) - original_project.images.each do |image| - remix.images.attach(image.blob) - end - - original_project.videos.each do |video| - remix.videos.attach(video.blob) - end - - original_project.audio.each do |audio_file| - remix.audio.attach(audio_file.blob) - end + Project::Copying.copy_media(original_project, remix, %i[images videos audio]) params[:components].each do |x| remix.components.build(x.slice(:name, :extension, :content)) diff --git a/spec/concepts/lesson/create_spec.rb b/spec/concepts/lesson/create_spec.rb index 253aff556..d8dc75470 100644 --- a/spec/concepts/lesson/create_spec.rb +++ b/spec/concepts/lesson/create_spec.rb @@ -159,4 +159,179 @@ expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) end end + + # Lesson::Create builds the lesson's project as a remix of + # an Experience CS project instead of a stub. + context 'when a source project is given' do + let(:english_content) { { 'targets' => [{ 'name' => 'Stage' }], 'monitors' => [], 'extensions' => [], 'meta' => {} } } + let(:french_content) { { 'targets' => [{ 'name' => 'Scène' }], 'monitors' => [], 'extensions' => [], 'meta' => {} } } + + let(:source_project_en) do + create(:scratch_project, identifier: 'my-digital-canvas', locale: 'en', user_id: nil, + name: 'My digital canvas', origin: Project::Origins::EXPERIENCE_CS, + instructions: 'English instructions') + .tap { |project| project.scratch_component.update!(content: english_content) } + end + + let(:source_project_fr) do + create(:scratch_project, identifier: source_project_en.identifier, locale: 'fr-FR', user_id: nil, + name: 'Ma toile numérique', origin: Project::Origins::EXPERIENCE_CS, + instructions: 'Instructions en français') + .tap { |project| project.scratch_component.update!(content: french_content) } + end + + # Eager, so the count matchers below only see what the operation itself creates. + let!(:source_project) { source_project_fr } + + let(:lesson_params) do + { + name: 'Test Lesson', + user_id: teacher.id, + school_id: school.id, + project_attributes: { name: 'My digital canvas' } + } + end + + let(:response) { described_class.call(lesson_params:, source_project:) } + + let(:lesson_project) { response[:lesson].project } + + before do + allow(User).to receive(:from_userinfo).with(ids: teacher.id).and_return([teacher]) + end + + it 'returns a successful operation response' do + expect(response.success?).to be(true) + end + + it 'creates a lesson' do + expect { described_class.call(lesson_params:, source_project:) }.to change(Lesson, :count).by(1) + end + + it 'creates one project for the lesson' do + expect { response }.to change(Project, :count).by(1) + end + + it 'generates a new identifier rather than reusing the source identifier' do + expect(lesson_project.identifier).not_to eq(source_project.identifier) + end + + it 'copies the scratch component content from the source project' do + expect(lesson_project.scratch_component.content).to eq(french_content) + end + + it 'copies the project_type from the source project' do + expect(lesson_project.project_type).to eq(Project::Types::CODE_EDITOR_SCRATCH) + end + + it 'copies the instructions from the source project' do + expect(lesson_project.instructions).to eq('Instructions en français') + end + + # remix of the requested locale row, but locale is null + it 'copies content from the requested locale, not the en locale' do + expect(lesson_project.scratch_component.content).not_to eq(english_content) + end + + it 'does not copy the instructions from the en locale' do + expect(lesson_project.instructions).not_to eq(source_project_en.instructions) + end + + it 'sets the lesson project locale to nil' do + expect(lesson_project.locale).to be_nil + end + + it 'inherits origin from the source project' do + expect(lesson_project.origin).to eq(source_project.origin) + end + + it 'assigns the lesson id to the project' do + expect(lesson_project.lesson_id).to eq(response[:lesson].id) + end + + it 'assigns the teacher user id to the project' do + expect(lesson_project.user_id).to eq(teacher.id) + end + + it 'assigns the school id to the project' do + expect(lesson_project.school_id).to eq(school.id) + end + + it 'builds a school project for the school' do + expect(lesson_project.school_project.school_id).to eq(school.id) + end + + it 'uses the name from project_attributes when one is given' do + expect(lesson_project.name).to eq('My digital canvas') + end + + it 'does not change the source project' do + expect { response }.not_to(change { source_project.reload.attributes }) + end + + it 'does not copy scratch assets (global assets resolve through the lineage)' do + create(:scratch_asset, :with_file, project: nil, uploaded_user_id: nil) + expect { response }.not_to change(ScratchAsset, :count) + end + + context 'when the source project has no origin' do + let(:source_project) do + create(:scratch_project, identifier: 'not-backfilled-yet', locale: 'en', user_id: nil, origin: nil) + end + + it 'leaves the origin nil on the lesson project' do + expect(lesson_project.origin).to be_nil + end + end + + context 'when project_attributes has no name' do + let(:lesson_params) do + { + name: 'Test Lesson', + user_id: teacher.id, + school_id: school.id, + project_attributes: {} + } + end + + it 'falls back to the source project name' do + expect(lesson_project.name).to eq('Ma toile numérique') + end + end + + context 'when the lesson project is not a school project' do + let(:lesson_params) do + { + name: 'Test Lesson', + user_id: teacher.id, + project_attributes: { name: 'My digital canvas' } + } + end + + before do + allow(Sentry).to receive(:capture_exception) + end + + it 'returns a failed operation response' do + expect(response.failure?).to be(true) + end + + it 'returns the error message in the operation response' do + expect(response[:error]).to include('Error creating lesson') + end + + it 'does not create a lesson' do + expect { response }.not_to change(Lesson, :count) + end + + it 'does not create a project' do + expect { response }.not_to change(Project, :count) + end + + it 'sent the exception to Sentry' do + response + expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + end + end + end end diff --git a/spec/features/lesson/creating_a_lesson_spec.rb b/spec/features/lesson/creating_a_lesson_spec.rb index 93827bd78..4757f1d8a 100644 --- a/spec/features/lesson/creating_a_lesson_spec.rb +++ b/spec/features/lesson/creating_a_lesson_spec.rb @@ -246,4 +246,98 @@ expect(response).to have_http_status(:forbidden) end end + + # #create resolves `lesson[:source_project_identifier]` through + # ProjectLoader, authorizes it, and hands the Project to Lesson::Create + # so the lesson project is built as a remix instead of a stub. + describe 'creating a lesson from a source project' do + let(:source_content) { { 'targets' => [{ 'name' => 'Stage' }], 'monitors' => [], 'extensions' => [], 'meta' => {} } } + + let!(:source_project) do + create(:scratch_project, identifier: 'my-digital-canvas', locale: 'en', user_id: nil, school_id: nil, + name: 'My digital canvas', origin: Project::Origins::EXPERIENCE_CS, + instructions: 'English instructions') + .tap { |project| project.scratch_component.update!(content: source_content) } + end + + let(:params) do + { + lesson: { + name: 'Test Lesson', + school_id: school.id, + source_project_identifier: source_project.identifier, + project_attributes: { + name: 'My digital canvas', + locale: 'en' + } + } + } + end + + let(:lesson_project) { Lesson.find(JSON.parse(response.body, symbolize_names: true)[:id]).project } + + context 'when the source project is a shared Experience CS project' do + before do + school.update!(scratch_enabled: true) + post('/api/lessons', headers:, params:) + end + + it 'responds 201 Created' do + expect(response).to have_http_status(:created) + end + + it 'responds with the lesson JSON' do + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:name]).to eq('Test Lesson') + end + + it 'copies the scratch component content from the source project' do + expect(lesson_project.scratch_component.content).to eq(source_content) + end + + it 'gives the lesson project a new identifier' do + expect(lesson_project.identifier).not_to eq(source_project.identifier) + end + + it 'sets the lesson project locale to nil' do + expect(lesson_project.locale).to be_nil + end + + it 'inherits project_type from the source project' do + expect(lesson_project.project_type).to eq(source_project.project_type) + end + + it 'inherits origin from the source project' do + expect(lesson_project.origin).to eq(source_project.origin) + end + end + + context 'when the school does not have Scratch enabled' do + # The request carries no project_attributes[:project_type], so the scratch gate has to + # look at the source project to see that a Scratch project is about to be created. + let(:params) do + { + lesson: { + name: 'Test Lesson', + school_id: school.id, + source_project_identifier: source_project.identifier, + project_attributes: { + name: 'My digital canvas', + locale: 'en' + } + } + } + end + + before do + school.update!(scratch_enabled: false) + end + + it 'responds 403 Forbidden when only source_project_identifier points at a scratch project' do + post('/api/lessons', headers:, params:) + expect(response).to have_http_status(:forbidden) + end + end + end end