diff --git a/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index 6ba0c8525..63408355e 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/app/controllers/api/lessons_controller.rb @@ -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 @@ -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 diff --git a/app/models/project.rb b/app/models/project.rb index 69599fb6f..9d58ee413 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -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 @@ -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) } @@ -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 diff --git a/lib/concepts/lesson/operations/update.rb b/lib/concepts/lesson/operations/update.rb index 8f4de6c02..582780aa8 100644 --- a/lib/concepts/lesson/operations/update.rb +++ b/lib/concepts/lesson/operations/update.rb @@ -3,11 +3,15 @@ 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) @@ -15,6 +19,23 @@ def call(lesson:, lesson_params:) 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 diff --git a/spec/concepts/lesson/update_spec.rb b/spec/concepts/lesson/update_spec.rb index b46e56cd0..c3971502b 100644 --- a/spec/concepts/lesson/update_spec.rb +++ b/spec/concepts/lesson/update_spec.rb @@ -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 @@ -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 diff --git a/spec/factories/lesson.rb b/spec/factories/lesson.rb index cc7da2fc0..d78ac5ed9 100644 --- a/spec/factories/lesson.rb +++ b/spec/factories/lesson.rb @@ -6,6 +6,6 @@ sequence(:name) { |n| "Lesson #{n}" } description { 'Description' } visibility { 'teachers' } - project { create(:project) } + project { create(:project, user_id:) } end end diff --git a/spec/features/lesson/updating_a_lesson_spec.rb b/spec/features/lesson/updating_a_lesson_spec.rb index b43f04e53..47adab9a2 100644 --- a/spec/features/lesson/updating_a_lesson_spec.rb +++ b/spec/features/lesson/updating_a_lesson_spec.rb @@ -3,17 +3,7 @@ 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: { @@ -21,6 +11,15 @@ } } 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:) @@ -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)