Skip to content

Add lesson to class with remixed project #547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
31 changes: 29 additions & 2 deletions app/controllers/api/lessons_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
module Api
class LessonsController < ApiController
before_action :authorize_user, except: %i[index show]
before_action :verify_school_class_belongs_to_school, only: :create
load_and_authorize_resource :lesson
before_action :verify_school_class_belongs_to_school, only: %i[create remix]
load_and_authorize_resource :lesson, except: :remix

def index
archive_scope = params[:include_archived] == 'true' ? Lesson : Lesson.unarchived
Expand Down Expand Up @@ -41,6 +41,26 @@ def create_copy
end
end

def remix
remix_origin = request.origin || request.referer
project = Project.find_by(identifier: lesson_params[:project_identifier])
authorize! :show, project

lesson = Lesson.new(lesson_params.except(:project_identifier))
lesson.project = Project.new(remixed_from_id: project.id) if project
authorize! :remix, lesson

result = Lesson::CreateRemix.call(lesson_params:, remix_origin:)

if result.success?
@lesson = result[:lesson]
@lesson_with_user = @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
Expand Down Expand Up @@ -78,6 +98,12 @@ def lesson_params
base_params.merge(user_id: current_user.id)
end

def remix_lesson_params
lesson_params.merge(params.fetch(:lesson, {}).permit(
{ project_attributes: [:identifier] }
))
end

def base_params
params.fetch(:lesson, {}).permit(
:school_id,
Expand All @@ -86,6 +112,7 @@ def base_params
:description,
:visibility,
:due_date,
:project_identifier,
{
project_attributes: [
:name,
Expand Down
10 changes: 9 additions & 1 deletion app/models/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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[remix], Lesson) do |lesson|
school_teacher_can_remix_lesson?(user:, school:, lesson:)
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:)
Expand Down Expand Up @@ -121,4 +124,9 @@ def school_teacher_can_manage_project?(user:, school:, project:)

is_my_project && (is_my_lesson || !project.lesson)
end

def school_teacher_can_remix_lesson?(user:, school:, lesson:)
original_project_is_public = Project.find(lesson.project.remixed_from_id).user_id.nil?
school_teacher_can_manage_lesson?(user:, school:, lesson:) && original_project_is_public
end
end
10 changes: 10 additions & 0 deletions app/models/lesson.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ 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 :remixed_lesson_projects_come_from_public_projects

scope :archived, -> { where.not(archived_at: nil) }
scope :unarchived, -> { where(archived_at: nil) }
Expand Down Expand Up @@ -74,4 +75,13 @@ 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 remixed_lesson_projects_come_from_public_projects
return if !project || !project.remixed_from_id

original_project = Project.find_by(id: project.remixed_from_id)
return if original_project&.user_id.nil?

errors.add(:project, "remixed project '#{original_project.id}' is not public")
end
end
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@

resources :lessons, only: %i[index create show update destroy] do
post :copy, on: :member, to: 'lessons#create_copy'
post :remix, on: :collection, to: 'lessons#remix'
end

resources :teacher_invitations, param: :token, only: :show do
Expand Down
43 changes: 43 additions & 0 deletions lib/concepts/lesson/operations/create_remix.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# frozen_string_literal: true

class Lesson
class CreateRemix
class << self
def call(lesson_params:, remix_origin:)
ActiveRecord::Base.transaction do
response = OperationResponse.new
response[:lesson] = build_remix(lesson_params, remix_origin)
response[:lesson].save!
response
rescue StandardError => e
Sentry.capture_exception(e)
errors = response[:lesson].errors.full_messages.join(',')
response[:error] = "Error creating remix of lesson: #{errors}"
response
end
end

private

def build_remix(lesson_params, remix_origin)
original_project = Project.find_by(identifier: lesson_params[:project_identifier])
lesson_copy = Lesson.new(name: original_project.name)
filtered_params = lesson_params.except(:project_identifier)
lesson_copy.assign_attributes(filtered_params)
lesson_copy.project = build_project_remix(original_project, lesson_params, remix_origin)

lesson_copy
end

def build_project_remix(original_project, lesson_params, remix_origin)
response = Project::CreateRemix.call(
params: { school_id: lesson_params[:school_id] },
user_id: lesson_params[:user_id],
original_project:,
remix_origin:
)
response[:project]
end
end
end
end
11 changes: 6 additions & 5 deletions lib/concepts/project/operations/create_remix.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ def call(params:, user_id:, original_project:, remix_origin:)
response
rescue StandardError => e
Sentry.capture_exception(e)
response[:error] = I18n.t('errors.project.remixing.cannot_save')
response[:error] = "#{I18n.t('errors.project.remixing.cannot_save')}: #{e.message}"
response
end

private

def validate_params(response, params, user_id, original_project, remix_origin)
valid = params[:identifier].present? && user_id.present? && original_project.present? && remix_origin.present?
valid = (params[:identifier].present? || original_project.identifier.present?) && user_id.present? && original_project.present? && remix_origin.present?
response[:error] = I18n.t('errors.project.remixing.invalid_params') unless valid
end

Expand All @@ -30,7 +30,6 @@ 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
Expand All @@ -43,7 +42,8 @@ def create_remix(original_project, params, user_id, remix_origin)
remix.audio.attach(audio_file.blob)
end

params[:components].each do |x|
(params[:components] || original_project.components).each do |x|
pp 'using original project components' if params[:components].nil?
remix.components.build(x.slice(:name, :extension, :content))
end

Expand All @@ -54,11 +54,12 @@ def format_project(original_project, params, user_id, remix_origin)
original_project.dup.tap do |proj|
proj.identifier = PhraseIdentifier.generate
proj.locale = nil
proj.name = params[:name]
proj.name = params[:name] || original_project.name
proj.user_id = user_id
proj.remixed_from_id = original_project.id
proj.remix_origin = remix_origin
proj.lesson_id = nil # Only the original can have a lesson id
proj.school_id = params[:school_id] || original_project.school_id
end
end
end
Expand Down
31 changes: 31 additions & 0 deletions spec/concepts/project/create_remix_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,37 @@
expect(create_remix[:error]).to eq(I18n.t('errors.project.remixing.cannot_save'))
end
end

context 'when the remix params contain only a school' do
let(:school) { create(:school) }
let(:teacher) { create(:teacher, school:) }
let!(:remix_params) { { school_id: school.id } }
subject!(:create_remix) { described_class.call(params: remix_params, user_id: teacher.id, original_project: original_project, remix_origin: remix_origin) }

it 'sets the name to the original project name' do
remixed_project = create_remix[:project]
expect(remixed_project.name).to eq(original_project.name)
end

it 'sets the school_id to the one provided by the remix params' do
remixed_project = create_remix[:project]
expect(remixed_project.school_id).to eq(school.id)
end

# fit 'creates components from the original project' do
# pp original_project.components.count
# pp remix_params
# # expect { create_remix }.to change(Component, :count).by(original_project.components.count)
# expect { create_remix }.to change(Project, :count).by(1)
# end

it 'copies the components from the original project' do
remixed_project = create_remix[:project]
expect(remixed_project.components.map { |c| component_props(c) }).to match_array(
original_project.components.map { |c| component_props(c) }
)
end
end
end

def component_props(component)
Expand Down
14 changes: 14 additions & 0 deletions spec/models/lesson_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,20 @@
lesson.visibility = 'invalid'
expect(lesson).to be_invalid
end

it 'requires original project to be public if project is a remix' do
original_project = create(:project, user_id: SecureRandom.uuid)
lesson.project.remixed_from_id = original_project.id

expect(lesson).to be_invalid
end

it 'is valid with a remixed project from a public project' do
original_project = create(:project, user_id: nil)
lesson.project.remixed_from_id = original_project.id

expect(lesson).to be_valid
end
end

describe '.archived' do
Expand Down