Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions app/controllers/api/lessons_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class LessonsController < ApiController

before_action :authorize_user, except: %i[index show]
before_action :verify_school_class_belongs_to_school, only: :create
before_action :verify_can_create_scratch_projects, only: %i[create create_copy]
load_and_authorize_resource :lesson

def index
Expand All @@ -29,7 +30,7 @@ def show
end

def create
result = Lesson::Create.call(lesson_params:)
result = Lesson::Create.call(lesson_params: create_params)

if result.success?
@lesson_with_user = result[:lesson].with_user
Expand All @@ -40,7 +41,7 @@ def create
end

def create_copy
result = Lesson::CreateCopy.call(lesson: @lesson, lesson_params:)
result = Lesson::CreateCopy.call(lesson: @lesson, lesson_params: create_params)

if result.success?
@lesson_with_user = result[:lesson].with_user
Expand All @@ -53,7 +54,7 @@ def create_copy
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: update_params)

if result.success?
@lesson_with_user = result[:lesson].with_user
Expand All @@ -77,12 +78,18 @@ def filtered_lessons_scope
end

def verify_school_class_belongs_to_school
return if base_params[:school_class_id].blank?
return if school&.classes&.pluck(:id)&.include?(base_params[:school_class_id])
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'
end

def verify_can_create_scratch_projects
return unless scratch_project? && !school.scratch_enabled?

render json: { error: 'Forbidden' }, status: :forbidden
end
Comment thread
zetter-rpf marked this conversation as resolved.

def user_remixes(lessons)
lessons.map { |lesson| user_remix(lesson) }
end
Expand All @@ -97,11 +104,21 @@ def user_remix(lesson)
)
end

def lesson_params
base_params.merge(user_id: current_user.id)
def scratch_project?
create_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH
end

def update_params
params.fetch(:lesson, {}).permit(
:name,
:visibility,
{
project_attributes: [:name]
}
)
end

def base_params
def create_params
params.fetch(:lesson, {}).permit(
:school_id,
:school_class_id,
Expand All @@ -118,15 +135,15 @@ def base_params
{ scratch_component: {} }
]
}
)
).merge(user_id: current_user.id)
end

def school_owner?
school && current_user.school_owner?(school)
end

def school
@school ||= @lesson&.school || School.find_by(id: base_params[:school_id]) || SchoolClass.find_by(id: params[:school_class_id])&.school
@school ||= @lesson&.school || School.find_by(id: create_params[:school_id]) || SchoolClass.find_by(id: params[:school_class_id])&.school
end
end
end
12 changes: 9 additions & 3 deletions app/controllers/api/schools_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def show
end

def create
result = School::Create.call(school_params:, creator_id: current_user.id, token: current_user.token)
result = School::Create.call(school_params: create_params, creator_id: current_user.id, token: current_user.token)

if result.success?
@school = result[:school]
Expand All @@ -31,7 +31,7 @@ def create

def update
school = School.find(params[:id])
result = School::Update.call(school:, school_params:)
result = School::Update.call(school:, school_params: update_params)

if result.success?
@school = result[:school]
Expand Down Expand Up @@ -76,7 +76,7 @@ def import

private

def school_params
def create_params
params.expect(
school: %i[name
website
Expand All @@ -99,5 +99,11 @@ def school_params
user_origin]
)
end

def update_params
params.expect(
school: %i[scratch_enabled]
)
end
end
end
5 changes: 1 addition & 4 deletions app/controllers/api/scratch/assets_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@ module Scratch
class AssetsController < ScratchController
include ActiveStorage::SetCurrent

skip_before_action :authorize_user, only: [:show]
prepend_before_action :load_project_from_header, only: %i[show create]
authorize_resource :project_from_header

def show
filename_with_extension = "#{params[:id]}.#{params[:format]}"
authorize! :show, @project_from_header

scratch_asset = ScratchAsset.find_visible_to_project(
project: @project_from_header,
Expand All @@ -23,8 +22,6 @@ def show
end

def create
authorize! :show, @project_from_header

filename_with_extension = "#{params[:id]}.#{params[:format]}"
scratch_asset = ScratchAsset.find_or_initialize_by(
project: @project_from_header,
Expand Down
19 changes: 11 additions & 8 deletions app/controllers/api/scratch/projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,19 @@ module Scratch
class ProjectsController < ScratchController
include RemixSelection

skip_before_action :authorize_user, only: [:show]
skip_before_action :check_scratch_feature, only: [:show]
before_action :load_project, only: %i[show update]
before_action :load_project, except: %i[create]
authorize_resource :project, except: %i[create]

before_action :ensure_create_is_a_remix, only: %i[create]
before_action :load_original_project, only: %i[create]
authorize_resource :original_project, only: %i[create]

def show
render json: @project.scratch_component.content_with_stage_first
end

def create
original_project = load_original_project(source_project_identifier)
return render json: { error: I18n.t('errors.admin.unauthorized') }, status: :unauthorized unless current_ability.can?(:show, original_project)

original_project = @original_project
remix_params = create_params
return render json: { error: I18n.t('errors.project.remixing.invalid_params') }, status: :bad_request if remix_params.dig(:scratch_component, :content).blank?

Expand Down Expand Up @@ -74,8 +73,8 @@ def create_params
}
end

def load_original_project(identifier)
Project.find_by!(identifier:, project_type: Project::Types::CODE_EDITOR_SCRATCH)
def load_original_project
@original_project = Project.find_by!(identifier: source_project_identifier, project_type: Project::Types::CODE_EDITOR_SCRATCH)
end

def scratch_content_params
Expand All @@ -100,6 +99,10 @@ def move_pending_scratch_upload_to_remix(pending_upload, remix_project)
rescue ActiveRecord::RecordNotUnique
pending_upload.destroy!
end

def load_project
@project = Project.find_by!(identifier: params[:id], project_type: Project::Types::CODE_EDITOR_SCRATCH)
end
end
end
end
13 changes: 3 additions & 10 deletions app/controllers/api/scratch/scratch_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,13 @@ module Api
module Scratch
class ScratchController < ApiController
before_action :authorize_user
before_action :check_scratch_feature
before_action :only_allow_schools_to_use_scratch

def check_scratch_feature
return if current_user.nil?

school = current_user&.schools&.first
return if Flipper.enabled?(:cat_mode, school)
def only_allow_schools_to_use_scratch
return true if current_user.schools.any?

raise ActiveRecord::RecordNotFound, 'Not Found'
end

def load_project
@project = Project.find_by!(identifier: params[:id], project_type: Project::Types::CODE_EDITOR_SCRATCH)
end
end
end
end
3 changes: 2 additions & 1 deletion app/views/api/schools/_school.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ json.call(
:country_code,
:verified_at,
:created_at,
:updated_at
:updated_at,
:scratch_enabled
)

include_roles = local_assigns.fetch(:roles, false)
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20260528141937_add_scratch_enabled_to_school.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddScratchEnabledToSchool < ActiveRecord::Migration[8.1]
def change
add_column :schools, :scratch_enabled, :boolean, default: false, null: false
end
end
Comment thread
zetter-rpf marked this conversation as resolved.
3 changes: 2 additions & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 41 additions & 0 deletions spec/features/lesson/creating_a_lesson_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
new_params = { lesson: params[:lesson].merge(user_id: 'ignored') }

post('/api/lessons', headers:, params: new_params)
expect(response).to have_http_status(:created)
data = JSON.parse(response.body, symbolize_names: true)

expect(data[:user_id]).to eq(teacher.id)
Expand Down Expand Up @@ -186,4 +187,44 @@
expect(response).to have_http_status(:unprocessable_content)
end
end

describe 'working with Scratch projects' do
let(:params) do
{
lesson: {
name: 'Test Lesson',
school_id: school.id,
project_attributes: {
name: 'Hello Scratch project',
project_type: Project::Types::CODE_EDITOR_SCRATCH,
scratch_component: {
content: {
example_data: 'true'
}
}
}
}
}
end

it 'creates a lesson with a scratch component when school has Scratch enabled' do
school.update!(scratch_enabled: true)
post('/api/lessons', headers:, params:)
expect(response).to have_http_status(:created)

data = JSON.parse(response.body, symbolize_names: true)

lesson_id = data[:id]

project = Lesson.find(lesson_id).project
expect(project.project_type).to eq(Project::Types::CODE_EDITOR_SCRATCH)
expect(project.scratch_component.content).to eq({ 'example_data' => 'true' })
end

it 'returns forbidden when school does not have Scratch enabled' do
school.update!(scratch_enabled: false)
post('/api/lessons', headers:, params:)
expect(response).to have_http_status(:forbidden)
end
end
end
6 changes: 4 additions & 2 deletions spec/features/lesson/updating_a_lesson_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,17 @@
expect(response).to have_http_status(:ok)
end

it 'responds 422 Unprocessable Entity when trying to re-assign the lesson to a different class' do
it 'does not allow re-assigning the lesson to a different class' do
school = create(:school, id: SecureRandom.uuid)
teacher = create(:teacher, school:)
school_class = create(:school_class, school:, teacher_ids: [teacher.id])

new_params = { lesson: params[:lesson].merge(school_class_id: school_class.id) }
put("/api/lessons/#{lesson.id}", headers:, params: new_params)
expect(response).to have_http_status(:ok)

expect(response).to have_http_status(:unprocessable_content)
lesson.reload
expect(lesson.school_class_id).not_to eq(school_class.id)
end
end
end
2 changes: 1 addition & 1 deletion spec/features/my_school/showing_my_school_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
school_json = school.to_json(only: %i[
id name website reference address_line_1 address_line_2 municipality administrative_area postal_code country_code code verified_at created_at updated_at district_name district_nces_id school_roll_number
])
expected_data = JSON.parse(school_json, symbolize_names: true).merge(roles: ['owner'], import_in_progress: school.import_in_progress?, auto_join_enabled: school.auto_join_enabled?)
expected_data = JSON.parse(school_json, symbolize_names: true).merge(roles: ['owner'], import_in_progress: school.import_in_progress?, auto_join_enabled: school.auto_join_enabled?, scratch_enabled: false)

get('/api/school', headers:)
data = JSON.parse(response.body, symbolize_names: true)
Expand Down
11 changes: 3 additions & 8 deletions spec/features/school/updating_a_school_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@
authenticated_in_hydra_as(owner)
end

let!(:school) { create(:school) }
let!(:school) { create(:school, scratch_enabled: false) }
let(:headers) { { Authorization: UserProfileMock::TOKEN } }
let(:owner) { create(:owner, school:) }

let(:params) do
{
school: {
name: 'New Name'
scratch_enabled: true
}
}
end
Expand All @@ -28,7 +28,7 @@
put("/api/schools/#{school.id}", headers:, params:)
data = JSON.parse(response.body, symbolize_names: true)

expect(data[:name]).to eq('New Name')
expect(data[:scratch_enabled]).to be(true)
end

it 'responds 404 Not Found when no school exists' do
Expand All @@ -41,11 +41,6 @@
expect(response).to have_http_status(:bad_request)
end

it 'responds 422 Unprocessable Entity when params are invalid' do
put("/api/schools/#{school.id}", headers:, params: { school: { name: ' ' } })
expect(response).to have_http_status(:unprocessable_content)
end

it 'responds 401 Unauthorized when no token is given' do
put "/api/schools/#{school.id}"
expect(response).to have_http_status(:unauthorized)
Expand Down
Loading
Loading