-
Notifications
You must be signed in to change notification settings - Fork 5
Bulk lesson creation #853
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
Merged
Merged
Bulk lesson creation #853
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
0835fcd
Add bulk lesson creation via lesson_projects
PetarSimonovic cd61856
DRY up shared create params in LessonsController
PetarSimonovic 80eb0fc
Create lessons BatchController
PetarSimonovic aba356b
Authorize each lesson in batch create with CanCan's authorize!
PetarSimonovic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This controller is getting longer now and I think having different code paths for the create action and many of the helpers does make it harder to follow and easier to introduce bugs (although I like having the attributes shared between them as it means that they are updated together).
Do you think it's worth improving this as part of this change? I wondered if it would be better or worse to have a different bulk controller, or if other behaviour could be moved outside of the controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've separated bulk creation into its own controller.
I've also created a LessonCreation concern for params and logic that are shared between single and batch lesson creation.
Let me know if LessonCreation makes it harder to follow the behaviour and I can move those methods back into the controllers.