Skip to content

trial either checking for owner role or if project has associations #471

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 14 commits into
base: main
Choose a base branch
from
Open
5 changes: 3 additions & 2 deletions app/controllers/api/lessons_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ class LessonsController < ApiController
def index
archive_scope = params[:include_archived] == 'true' ? Lesson : Lesson.unarchived
scope = params[:school_class_id] ? archive_scope.where(school_class_id: params[:school_class_id]) : archive_scope
@lessons_with_users = scope.accessible_by(current_ability).with_users
ordered_scope = scope.order(created_at: :asc)
@lessons_with_users = ordered_scope.accessible_by(current_ability).with_users
render :index, formats: [:json], status: :ok
end

Expand Down Expand Up @@ -41,7 +42,7 @@ def create_copy
end

def update
result = Lesson::Update.call(lesson: @lesson, lesson_params:)
result = Lesson::Update.call(lesson: @lesson, lesson_params:, current_user:)

if result.success?
@lesson_with_user = result[:lesson].with_user
Expand Down
13 changes: 10 additions & 3 deletions app/models/project.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# frozen_string_literal: true

class Project < ApplicationRecord
attr_accessor :current_user

belongs_to :school, optional: true
belongs_to :lesson, optional: true
belongs_to :parent, optional: true, class_name: :Project, foreign_key: :remixed_from_id, inverse_of: :remixes
Expand All @@ -17,7 +19,7 @@ class Project < ApplicationRecord
validate :identifier_cannot_be_taken_by_another_user
validates :locale, presence: true, unless: :user_id
validate :user_has_a_role_within_the_school
validate :user_is_a_member_or_the_owner_of_the_lesson
validate :user_is_a_member_or_the_owner_of_the_lesson_or_school

scope :internal_projects, -> { where(user_id: nil) }

Expand Down Expand Up @@ -79,8 +81,13 @@ def user_has_a_role_within_the_school
errors.add(:user, msg)
end

def user_is_a_member_or_the_owner_of_the_lesson
return if !lesson || user_id == lesson.user_id || lesson.school_class.members.exists?(student_id: user_id)
def user_is_a_member_or_the_owner_of_the_lesson_or_school
return if !lesson || user_id == lesson.user_id || lesson.school_class&.members&.exists?(student_id: user_id)

# either we explicitly check the user is an owner of the school
return if current_user&.school_owner?(lesson.school)
# or we bypass if the lesson is associated with a school but not a school class
return if lesson.school && !lesson.school_class && !lesson.school_class&.members&.any?

errors.add(:user, "'#{user_id}' is not the owner or a member of the lesson '#{lesson_id}'")
end
Expand Down
23 changes: 22 additions & 1 deletion lib/concepts/lesson/operations/update.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,39 @@
class Lesson
class Update
class << self
def call(lesson:, lesson_params:)
def call(lesson:, lesson_params:, current_user:)
response = OperationResponse.new
response[:lesson] = lesson
response[:lesson].assign_attributes(lesson_params)
response[:lesson].save!
if lesson_params[:name].present?
rename_lesson_project(current_user:, lesson: response[:lesson], name: lesson_params[:name])
rename_lesson_remixes(current_user:, lesson: response[:lesson], name: lesson_params[:name])
end
response
rescue StandardError => e
Sentry.capture_exception(e)
errors = response[:lesson].errors.full_messages.join(',')
response[:error] = "Error updating lesson: #{errors}"
response
end

def rename_lesson_project(current_user:, lesson:, name:)
return unless lesson.project

lesson.project.current_user = current_user
lesson.project.assign_attributes(name:)
lesson.project.save!
end

def rename_lesson_remixes(current_user:, lesson:, name:)
lesson_remixes = Project.where(remixed_from_id: lesson.project.id)
lesson_remixes.each do |remix|
remix.current_user = current_user
remix.assign_attributes(name:)
remix.save!
end
end
end
end
end
35 changes: 27 additions & 8 deletions spec/concepts/lesson/update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,43 @@
require 'rails_helper'

RSpec.describe Lesson::Update, type: :unit do
let(:lesson) { create(:lesson, name: 'Test Lesson') }
let(:school) { create(:school) }
let(:teacher) { create(:teacher, school:) }
let(:student) { create(:student, school:) }
let(:lesson) { create(:lesson, name: 'Test Lesson', user_id: teacher.id) }
let!(:student_project) { create(:project, remixed_from_id: lesson.project.id, user_id: student.id) }
let(:current_user) { authenticated_user }

let(:lesson_params) do
{ name: 'New Name' }
end

before do
authenticated_in_hydra_as(teacher)
end

it 'returns a successful operation response' do
response = described_class.call(lesson:, lesson_params:)
response = described_class.call(lesson:, lesson_params:, current_user:)
expect(response.success?).to be(true)
end

it 'updates the lesson' do
response = described_class.call(lesson:, lesson_params:)
response = described_class.call(lesson:, lesson_params:, current_user:)
expect(response[:lesson].name).to eq('New Name')
end

it 'updates the project name' do
described_class.call(lesson:, lesson_params:, current_user:)
expect(lesson.project.name).to eq('New Name')
end

it 'updates the student project name' do
described_class.call(lesson:, lesson_params:, current_user:)
expect(student_project.reload.name).to eq('New Name')
end

it 'returns the lesson in the operation response' do
response = described_class.call(lesson:, lesson_params:)
response = described_class.call(lesson:, lesson_params:, current_user:)
expect(response[:lesson]).to be_a(Lesson)
end

Expand All @@ -32,22 +51,22 @@
end

it 'does not update the lesson' do
response = described_class.call(lesson:, lesson_params:)
response = described_class.call(lesson:, lesson_params:, current_user:)
expect(response[:lesson].reload.name).to eq('Test Lesson')
end

it 'returns a failed operation response' do
response = described_class.call(lesson:, lesson_params:)
response = described_class.call(lesson:, lesson_params:, current_user:)
expect(response.failure?).to be(true)
end

it 'returns the error message in the operation response' do
response = described_class.call(lesson:, lesson_params:)
response = described_class.call(lesson:, lesson_params:, current_user:)
expect(response[:error]).to match(/Error updating lesson/)
end

it 'sent the exception to Sentry' do
described_class.call(lesson:, lesson_params:)
described_class.call(lesson:, lesson_params:, current_user:)
expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError))
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/factories/lesson.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@
sequence(:name) { |n| "Lesson #{n}" }
description { 'Description' }
visibility { 'teachers' }
project { create(:project) }
project { create(:project, user_id:) }
end
end
24 changes: 13 additions & 11 deletions spec/features/lesson/updating_a_lesson_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,23 @@
require 'rails_helper'

RSpec.describe 'Updating a lesson', type: :request do
before do
authenticated_in_hydra_as(owner)
stub_user_info_api_for(teacher)
end

let(:headers) { { Authorization: UserProfileMock::TOKEN } }
let!(:lesson) { create(:lesson, name: 'Test Lesson', user_id: owner.id) }
let(:owner) { create(:owner, school:, name: 'School Owner') }
let(:teacher) { create(:teacher, school:) }
let(:school) { create(:school) }

let(:params) do
{
lesson: {
name: 'New Name'
}
}
end
let!(:lesson) { create(:lesson, name: 'Test Lesson', user_id: owner.id) }
let(:teacher) { create(:teacher, school:) }
let(:school) { create(:verified_school) }
let(:owner) { create(:owner, school:, name: 'School Owner') }

before do
authenticated_in_hydra_as(owner)
stub_user_info_api_for(teacher)
end

it 'responds 200 OK' do
put("/api/lessons/#{lesson.id}", headers:, params:)
Expand Down Expand Up @@ -59,9 +58,12 @@
end

context 'when the lesson is associated with a school (library)' do
let(:school) { create(:school) }
let!(:lesson) { create(:lesson, school:, name: 'Test Lesson', visibility: 'teachers', user_id: teacher.id) }

before do
lesson
end

it 'responds 200 OK when the user is a school-owner' do
put("/api/lessons/#{lesson.id}", headers:, params:)
expect(response).to have_http_status(:ok)
Expand Down