diff --git a/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index 305b2a632..cbb0159d9 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/app/controllers/api/lessons_controller.rb @@ -41,10 +41,27 @@ def create_copy end end + def create_from_project + # authorize the project + if lesson_params[:project_identifier].present? + project = Project.find_by(identifier: lesson_params[:project_identifier]) + authorize! :update, project if project + end + + result = Lesson::CreateFromProject.call(lesson_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_entity + end + end + def update # TODO: Consider removing user_id from the lesson_params for update so users can update other users' lessons without changing ownership # OR consider dropping user_id on lessons and using teacher id/ids on the class instead - result = Lesson::Update.call(lesson: @lesson, lesson_params:) + result = Lesson::Update.call(lesson: @lesson, lesson_params: base_params) if result.success? @lesson_with_user = result[:lesson].with_user @@ -86,6 +103,7 @@ def base_params :description, :visibility, :due_date, + :project_identifier, { project_attributes: [ :name, diff --git a/app/models/ability.rb b/app/models/ability.rb index a7b65202e..b2f62a95c 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -83,13 +83,16 @@ def define_school_teacher_abilities(user:, school:) can(%i[create update destroy], Lesson) do |lesson| school_teacher_can_manage_lesson?(user:, school:, lesson:) end + can(%i[create_from_project], Lesson) do |lesson| + school_teacher_can_manage_lesson?(user:, school:, lesson:) && school_teacher_can_manage_project?(user:, school:, project: lesson.project) + end can(%i[read create_copy], Lesson, school_id: school.id, visibility: %w[teachers students]) can(%i[create], Project) do |project| school_teacher_can_manage_project?(user:, school:, project:) end can(%i[read update show_context], Project, school_id: school.id, lesson: { visibility: %w[teachers students] }) can(%i[read], Project, - remixed_from_id: Project.where(school_id: school.id, remixed_from_id: nil, lesson_id: Lesson.where(school_class_id: ClassTeacher.where(teacher_id: user.id).select(:school_class_id))).pluck(:id)) + remixed_from_id: Project.where(school_id: school.id, lesson_id: Lesson.where(school_class_id: ClassTeacher.where(teacher_id: user.id).select(:school_class_id))).pluck(:id)) end def define_school_student_abilities(user:, school:) diff --git a/app/models/lesson.rb b/app/models/lesson.rb index ae0f0dfde..178927ec2 100644 --- a/app/models/lesson.rb +++ b/app/models/lesson.rb @@ -17,6 +17,8 @@ class Lesson < ApplicationRecord validate :user_has_the_school_owner_or_school_teacher_role_for_the_school validate :user_is_the_school_teacher_for_the_school_class + validate :project_belongs_to_the_same_school + validate :project_belongs_to_the_same_user scope :archived, -> { where.not(archived_at: nil) } scope :unarchived, -> { where(archived_at: nil) } @@ -74,4 +76,16 @@ def user_is_the_school_teacher_for_the_school_class errors.add(:user, "'#{user_id}' is not the 'school-teacher' for school_class '#{school_class.id}'") end + + def project_belongs_to_the_same_school + return unless (project && (school || project&.school) && project&.school_id != school&.id) + + errors.add(:project, "must belong to the same school (#{school&.id}) as the lesson (#{id})") + end + + def project_belongs_to_the_same_user + return unless project && user_id && project.user_id != user_id + + errors.add(:project, "must belong to the same user (#{user_id}) as the lesson (#{id})") + end end diff --git a/app/models/project.rb b/app/models/project.rb index ebe956559..7db18960b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -31,6 +31,7 @@ module Types validate :project_with_instructions_must_belong_to_school validate :project_with_school_id_has_school_project validate :school_project_school_matches_project_school + validate :lesson_id_cannot_be_changed default_scope -> { where.not(project_type: Types::SCRATCH) } @@ -142,4 +143,10 @@ def school_project_school_matches_project_school errors.add(:school_project, 'School project school_id must match project school_id') end + + def lesson_id_cannot_be_changed + return unless lesson_id_changed? && lesson_id_was.present? + + errors.add(:lesson_id, 'cannot be changed once set') + end end diff --git a/app/views/api/lessons/index.json.jbuilder b/app/views/api/lessons/index.json.jbuilder index 6429df43c..74d8d0a43 100644 --- a/app/views/api/lessons/index.json.jbuilder +++ b/app/views/api/lessons/index.json.jbuilder @@ -23,7 +23,6 @@ json.array!(@lessons_with_users) do |lesson, user| :identifier, :project_type ) - json.project.finished(lesson.project.finished) if lesson.project.remixed_from_id.present? end json.user_name(user&.name) diff --git a/config/routes.rb b/config/routes.rb index f48edf7f1..939f3ba5c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -61,6 +61,7 @@ resources :lessons, only: %i[index create show update destroy] do post :copy, on: :member, to: 'lessons#create_copy' + post :create_from_project, on: :collection, to: 'lessons#create_from_project' end resources :teacher_invitations, param: :token, only: :show do diff --git a/lib/concepts/lesson/operations/create_from_project.rb b/lib/concepts/lesson/operations/create_from_project.rb new file mode 100644 index 000000000..3ffc303a2 --- /dev/null +++ b/lib/concepts/lesson/operations/create_from_project.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class Lesson + class CreateFromProject + class << self + def call(lesson_params:) + response = OperationResponse.new + response[:lesson] = build_lesson_from_project(lesson_params) + response[:lesson].save! + response + rescue StandardError => e + Sentry.capture_exception(e) + errors = response[:lesson].errors.full_messages.join(',') + response[:error] = "Error creating lesson from project: #{errors}" + response + end + + private + + def build_lesson_from_project(lesson_params) + project = Project.find_by(identifier: lesson_params[:project_identifier]) + lesson = Lesson.new( + name: project.name + ) + lesson.assign_attributes(lesson_params.except(:project_identifier)) + lesson.project = project + lesson + end + end + end +end diff --git a/spec/factories/lesson.rb b/spec/factories/lesson.rb index 68f0bce41..6a22ecdc8 100644 --- a/spec/factories/lesson.rb +++ b/spec/factories/lesson.rb @@ -6,7 +6,7 @@ sequence(:name) { |n| "Lesson #{n}" } description { 'Description' } visibility { 'teachers' } - project { create(:project, user_id:, name:) } + project { create(:project, user_id:, name:, school_id: school_id || school&.id || school_class&.school&.id) } trait :with_project_components do transient do diff --git a/spec/features/lesson/archiving_a_lesson_spec.rb b/spec/features/lesson/archiving_a_lesson_spec.rb index dd95cd434..f990aedf8 100644 --- a/spec/features/lesson/archiving_a_lesson_spec.rb +++ b/spec/features/lesson/archiving_a_lesson_spec.rb @@ -44,7 +44,7 @@ end it "responds 403 Forbidden when the user is not the lesson's owner" do - lesson.update!(user_id: SecureRandom.uuid) + authenticated_in_hydra_as(teacher) delete("/api/lessons/#{lesson.id}", headers:) expect(response).to have_http_status(:forbidden) diff --git a/spec/features/lesson/updating_a_lesson_spec.rb b/spec/features/lesson/updating_a_lesson_spec.rb index 948239e1c..1c7552ccb 100644 --- a/spec/features/lesson/updating_a_lesson_spec.rb +++ b/spec/features/lesson/updating_a_lesson_spec.rb @@ -12,6 +12,7 @@ } end let!(:lesson) { create(:lesson, name: 'Test Lesson', user_id: owner.id) } + let(:teacher_lesson) { create(:lesson, name: 'Test Teacher Lesson', user_id: teacher.id) } let(:teacher) { create(:teacher, school:) } let(:school) { create(:verified_school) } let(:owner) { create(:owner, school:, name: 'School Owner') } @@ -51,9 +52,7 @@ end it "responds 403 Forbidden when the user is not the lesson's owner" do - lesson.update!(user_id: SecureRandom.uuid) - - put("/api/lessons/#{lesson.id}", headers:, params:) + put("/api/lessons/#{teacher_lesson.id}", headers:, params:) expect(response).to have_http_status(:forbidden) end diff --git a/spec/models/lesson_spec.rb b/spec/models/lesson_spec.rb index 4eba9d729..0885b10cb 100644 --- a/spec/models/lesson_spec.rb +++ b/spec/models/lesson_spec.rb @@ -68,8 +68,19 @@ expect(lesson).to be_invalid end + it 'requires the user id to match the user_id on the project' do + lesson.project = build(:project, user_id: SecureRandom.uuid) + expect(lesson).to be_invalid + end + + it 'requires the project to belong to the same school as the lesson' do + lesson.project = build(:project, school: create(:school)) + expect(lesson).to be_invalid + end + context 'when the lesson has a school' do before do + lesson.project.update!(school:) lesson.update!(school:) end @@ -84,6 +95,7 @@ context 'when the lesson has a school_class' do before do + lesson.project.update!(school:) lesson.update!(school_class: create(:school_class, teacher_ids: [teacher.id], school:)) end