Skip to content
Draft
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
3 changes: 2 additions & 1 deletion app/controllers/api/projects/remixes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ def remix_params
:instructions,
{
image_list: [],
components: [%i[id name extension content index]]
components: [%i[id name extension content index]],
instructions: [[:markdown_content]]
}])
end
end
Expand Down
14 changes: 10 additions & 4 deletions app/controllers/api/projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def create
end

def update
result = Project::Update.call(project: @project, update_hash: project_params, current_user:)
result = Project::Update.call(project: @project, update_hash: project_params)

if result.success?
track_project_event('Project - Saved', @project)
Expand Down Expand Up @@ -94,22 +94,28 @@ def project_params
end

def base_params
params.fetch(:project, {}).permit(
params.fetch(:project, {}).permit(*permitted_project_attributes)
end

def permitted_project_attributes
attributes = [
:school_id,
:lesson_id,
:user_id,
:identifier,
:name,
:project_type,
:locale,
:instructions,
{
components: %i[id name extension content index default]
},
scratch_component: {},
parent: {},
image_list: []
)
]
return attributes if current_user&.student?

attributes + [:instructions, { instructions: [:markdown_content] }]
end

def school_owner?
Expand Down
12 changes: 12 additions & 0 deletions app/models/project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,18 @@ def scratch_component=(value)
super(value.is_a?(Hash) ? ScratchComponent.new(value) : value)
end

def instructions
self[:instruction_steps].nil? ? self[:instructions] : self[:instruction_steps]
end

def instructions=(value)
if value.is_a?(Array)
self[:instruction_steps] = value
else
self[:instructions] = value
end
end

def last_edited_at
# datetime that the project or one of its components was last updated
[updated_at, components.maximum(:updated_at)].compact.max
Expand Down
1 change: 0 additions & 1 deletion config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ en:
delete_default_component: "Cannot delete default file"
change_default_name: "Cannot amend default file name"
change_default_extension: "Cannot amend default file extension"
student_update_instructions: "Student cannot update project instructions"
remixing:
invalid_params: "Invalid parameters"
cannot_save: "Cannot create project remix"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class AddInstructionStepsToProjects < ActiveRecord::Migration[8.1]
def change
add_column :projects, :instruction_steps, :jsonb
end
end
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.

20 changes: 3 additions & 17 deletions lib/concepts/project/operations/update.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
class Project
class Update
class << self
def call(project:, update_hash:, current_user:)
def call(project:, update_hash:)
response = setup_response(project)

setup_deletions(response, update_hash)
update_project_attributes(response, update_hash, current_user)
update_project_attributes(response, update_hash)
update_component_attributes(response, update_hash)
persist_changes(response)
response
Expand Down Expand Up @@ -42,21 +42,7 @@ def validate_deletions(response)
response[:error] = I18n.t 'errors.project.editing.delete_default_component'
end

def student_project_instructions_updated?(response, update_hash, current_user)
is_school_project = response[:project].school.present?
user_is_student = current_user.student?
instructions_updated = response[:project].instructions != update_hash[:instructions]
is_school_project && user_is_student && instructions_updated
end

def validate_update(response, update_hash, current_user)
return unless student_project_instructions_updated?(response, update_hash, current_user)

response[:error] = I18n.t 'errors.project.editing.student_update_instructions'
end

def update_project_attributes(response, update_hash, current_user)
validate_update(response, update_hash, current_user)
def update_project_attributes(response, update_hash)
return if response.failure?

response[:project].assign_attributes(update_hash.slice(:name, :instructions))
Expand Down
3 changes: 1 addition & 2 deletions spec/concepts/project/update_default_component_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
require 'rails_helper'

RSpec.describe Project::Update, type: :unit do
subject(:update) { described_class.call(project:, update_hash:, current_user:) }
subject(:update) { described_class.call(project:, update_hash:) }

let(:current_user) { create(:user) }
let!(:project) { create(:project, :with_default_component) }
let(:default_component) { project.components.first }

Expand Down
3 changes: 1 addition & 2 deletions spec/concepts/project/update_delete_components_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
require 'rails_helper'

RSpec.describe Project::Update, type: :unit do
subject(:update) { described_class.call(project:, update_hash:, current_user:) }
subject(:update) { described_class.call(project:, update_hash:) }

let(:current_user) { create(:user) }
let!(:project) { create(:project, :with_default_component, :with_components) }
let(:component_to_delete) { project.components.last }
let(:default_component) { project.components.first }
Expand Down
3 changes: 1 addition & 2 deletions spec/concepts/project/update_invalid_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@
name: 'updated project name',
components: [default_component_hash, edited_component_hash, new_component_hash]
}
described_class.call(project:, update_hash:, current_user:)
described_class.call(project:, update_hash:)
end

let(:current_user) { create(:user) }
let!(:project) { create(:project, :with_default_component, :with_components, component_count: 2) }
let(:editable_component) { project.components.last }
let(:default_component) { project.components.first }
Expand Down
31 changes: 3 additions & 28 deletions spec/concepts/project/update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@
components: component_hash,
instructions:
}
described_class.call(project:, update_hash:, current_user:)
described_class.call(project:, update_hash:)
end

let(:current_user) { create(:user) }
let!(:project) { create(:project, :with_default_component, :with_components) }
let(:editable_component) { project.components.last }
let(:default_component) { project.components.first }
Expand Down Expand Up @@ -97,10 +96,9 @@
end
end

context 'when the instructions have changed and the current user is a teacher' do
context 'when the instructions have changed' do
let(:school) { create(:school) }
let!(:current_user) { create(:teacher, school:) }
let!(:project) { create(:project, :with_instructions, school:, user_id: current_user.id) }
let!(:project) { create(:project, :with_instructions, school:, user_id: create(:teacher, school:).id) }
let(:instructions) { 'new instructions' }

it 'returns success? true' do
Expand All @@ -111,29 +109,6 @@
expect { update }.to change { project.reload.instructions }.to('new instructions')
end
end

context 'when the instructions have changed and the current user is a student' do
let(:school) { create(:school) }
let!(:current_user) { create(:student, school:) }
let!(:project) { create(:project, :with_instructions, school:, user_id: current_user.id) }
let(:instructions) { 'new instructions' }

it 'returns success? false' do
expect(update.success?).to be(false)
end

it 'does not update project name' do
expect { update }.not_to change { project.reload.name }
end

it 'does not update project instructions' do
expect { update }.not_to change { project.reload.instructions }
end

it 'returns an error message' do
expect(update[:error]).to eq('Student cannot update project instructions')
end
end
end

def component_properties_hash(component)
Expand Down
18 changes: 18 additions & 0 deletions spec/models/project_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,24 @@
end
end

describe '#instructions' do
let(:project) { create(:project, :with_instructions, school:, user_id: create(:teacher, school:).id) }

it 'falls back to the legacy text column when instruction_steps has never been set' do
expect(project.instructions).to eq(project[:instructions])
end

it 'returns instruction_steps once set, without touching the legacy column' do
project.update!(instructions: [{ markdown_content: 'step 1' }])
expect(project.instructions).to eq([{ 'markdown_content' => 'step 1' }])
end

it 'does not fall back to stale legacy text once instruction_steps is set to an empty array' do
project.update!(instructions: [])
expect(project.instructions).to eq([])
end
end

describe 'auditing' do
let(:school) { create(:school) }
let(:teacher) { create(:teacher, school:) }
Expand Down
10 changes: 10 additions & 0 deletions spec/requests/projects/show_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@
get("/api/projects/#{project.identifier}", headers:)
expect(response.parsed_body).not_to have_key('finished')
end

it 'returns instructions in the instruction steps format when saved that way' do
project.update!(instructions: [{ markdown_content: 'step 1' }, { markdown_content: 'step 2' }])

get("/api/projects/#{project.identifier}", headers:)

expect(response.parsed_body['instructions']).to eq(
[{ 'markdown_content' => 'step 1' }, { 'markdown_content' => 'step 2' }]
)
end
end

context 'when loading a student\'s project' do
Expand Down
14 changes: 12 additions & 2 deletions spec/requests/projects/update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,14 @@
put("/api/projects/#{project.identifier}", params:, headers:)
expect(response.body).to include('updated instructions')
end

it 'saves and returns instructions in the instruction steps format' do
params[:project][:instructions] = [{ markdown_content: 'step 1' }, { markdown_content: 'step 2' }]
put("/api/projects/#{project.identifier}", params:, headers:)

expect(project.reload.instructions).to eq([{ 'markdown_content' => 'step 1' }, { 'markdown_content' => 'step 2' }])
expect(response.parsed_body['instructions']).to eq([{ 'markdown_content' => 'step 1' }, { 'markdown_content' => 'step 2' }])
end
end

context 'when authed user is a teacher updating a class project' do
Expand Down Expand Up @@ -197,10 +205,12 @@
expect(response).to have_http_status(:ok)
end

it 'returns unprocessable entity if instructions updated' do
it 'ignores an attempt to update instructions' do
params[:project][:instructions] = 'updated instructions'
put("/api/projects/#{project.identifier}", params:, headers:)
expect(response).to have_http_status(:unprocessable_content)

expect(response).to have_http_status(:ok)
expect(project.reload.instructions).to be_nil
end

it 'records a project saved event' do
Expand Down
Loading