diff --git a/app/controllers/api/school_classes_controller.rb b/app/controllers/api/school_classes_controller.rb index f85fe7a7a..617d43e3c 100644 --- a/app/controllers/api/school_classes_controller.rb +++ b/app/controllers/api/school_classes_controller.rb @@ -7,7 +7,7 @@ class SchoolClassesController < ApiController before_action :load_and_authorize_school_class def index - school_classes = accessible_school_classes + school_classes = accessible_school_classes.active school_classes = school_classes.joins(:teachers).where(teachers: { teacher_id: current_user&.id }) if params[:my_classes] == 'true' @school_classes_with_teachers = school_classes.with_teachers diff --git a/app/models/ability.rb b/app/models/ability.rb index 02c844c1e..4e8afc7a9 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -62,7 +62,7 @@ def define_authenticated_non_student_abilities(user) def define_school_owner_abilities(school:) can(%i[read update destroy], School, id: school.id) can(%i[read], :school_member) - can(%i[read create import update destroy], SchoolClass, school: { id: school.id }) + can(%i[read create import update destroy], SchoolClass, school: { id: school.id }, deleted: false) can(%i[read show_context], Project, school_id: school.id, lesson: { visibility: %w[teachers students] }) can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id } }) can(%i[read create destroy], :school_owner) @@ -78,7 +78,7 @@ def define_school_teacher_abilities(user:, school:) can(%i[read], School, id: school.id) can(%i[read], :school_member) can(%i[create import], SchoolClass, school: { id: school.id }) - can(%i[read update destroy], SchoolClass, school: { id: school.id }, teachers: { teacher_id: user.id }) + can(%i[read update destroy], SchoolClass, school: { id: school.id }, teachers: { teacher_id: user.id }, deleted: false) can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id }, teachers: { teacher_id: user.id } }) can(%i[read], :school_owner) can(%i[read], :school_teacher) @@ -112,7 +112,7 @@ def define_school_student_abilities(user:, school:) ).select(:id) ).pluck(:id) can(%i[read], School, id: school.id) - can(%i[read], SchoolClass, school: { id: school.id }, students: { student_id: user.id }) + can(%i[read], SchoolClass, school: { id: school.id }, students: { student_id: user.id }, deleted: false) # Ensure no access to ClassMember resources, relationships otherwise allow access in some circumstances. can(%i[read], Lesson, school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } }) can(%i[read create update], Project, school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: visible_lesson_project_ids) diff --git a/app/models/school_class.rb b/app/models/school_class.rb index e35f9b4f6..1d235eb48 100644 --- a/app/models/school_class.rb +++ b/app/models/school_class.rb @@ -8,6 +8,8 @@ class SchoolClass < ApplicationRecord accepts_nested_attributes_for :teachers scope :with_teachers, ->(user_id) { joins(:teachers).where(teachers: { id: user_id }) } + scope :active, -> { where(deleted: false) } + scope :deleted, -> { where(deleted: true) } before_validation :assign_class_code, on: %i[create import] @@ -73,6 +75,17 @@ def submitted_count .count end + def deleted? + deleted + end + + def mark_as_deleted! + return if deleted? + + self.deleted = true + save!(validate: false) + end + private def school_class_has_at_least_one_teacher diff --git a/db/migrate/20260113131231_add_deleted_to_school_classes.rb b/db/migrate/20260113131231_add_deleted_to_school_classes.rb new file mode 100644 index 000000000..c82a14801 --- /dev/null +++ b/db/migrate/20260113131231_add_deleted_to_school_classes.rb @@ -0,0 +1,6 @@ +class AddDeletedToSchoolClasses < ActiveRecord::Migration[7.2] + def change + add_column :school_classes, :deleted, :boolean, default: false, null: false + add_index :school_classes, :deleted + end +end diff --git a/db/schema.rb b/db/schema.rb index 447f05824..08e18d087 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_12_08_134354) do +ActiveRecord::Schema[7.2].define(version: 2026_01_13_131231) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -246,7 +246,9 @@ t.string "code" t.integer "import_origin" t.string "import_id" + t.boolean "deleted", default: false, null: false t.index ["code", "school_id"], name: "index_school_classes_on_code_and_school_id", unique: true + t.index ["deleted"], name: "index_school_classes_on_deleted" t.index ["school_id"], name: "index_school_classes_on_school_id" end diff --git a/lib/concepts/school_class/operations/delete.rb b/lib/concepts/school_class/operations/delete.rb index f189f9caf..8c8992e63 100644 --- a/lib/concepts/school_class/operations/delete.rb +++ b/lib/concepts/school_class/operations/delete.rb @@ -5,7 +5,7 @@ class Delete class << self def call(school:, school_class_id:) response = OperationResponse.new - delete_school_class(school, school_class_id) + mark_school_class_as_deleted(school, school_class_id) response rescue StandardError => e Sentry.capture_exception(e) @@ -15,9 +15,9 @@ def call(school:, school_class_id:) private - def delete_school_class(school, school_class_id) + def mark_school_class_as_deleted(school, school_class_id) school_class = school.classes.find(school_class_id) - school_class.destroy! + school_class.mark_as_deleted! end end end diff --git a/spec/concepts/school_class/delete_spec.rb b/spec/concepts/school_class/delete_spec.rb index 347906f92..8ecf75f8f 100644 --- a/spec/concepts/school_class/delete_spec.rb +++ b/spec/concepts/school_class/delete_spec.rb @@ -18,16 +18,21 @@ expect(response.success?).to be(true) end - it 'deletes a school class' do - expect { described_class.call(school:, school_class_id:) }.to change(SchoolClass, :count).by(-1) + it 'marks the school class as deleted' do + described_class.call(school:, school_class_id:) + expect(school_class.reload.deleted?).to be true end - it 'deletes class students in the school class' do - expect { described_class.call(school:, school_class_id:) }.to change(ClassStudent, :count).by(-1) + it 'does not delete the school class record' do + expect { described_class.call(school:, school_class_id:) }.not_to change(SchoolClass, :count) end - it 'deletes class teachers in the school class' do - expect { described_class.call(school:, school_class_id:) }.to change(ClassTeacher, :count).by(-1) + it 'does not delete class students in the school class' do + expect { described_class.call(school:, school_class_id:) }.not_to change(ClassStudent, :count) + end + + it 'does not delete class teachers in the school class' do + expect { described_class.call(school:, school_class_id:) }.not_to change(ClassTeacher, :count) end context 'when deletion fails' do diff --git a/spec/features/school_class/deleting_a_school_class_spec.rb b/spec/features/school_class/deleting_a_school_class_spec.rb index 6d336a8d8..1344fc2c4 100644 --- a/spec/features/school_class/deleting_a_school_class_spec.rb +++ b/spec/features/school_class/deleting_a_school_class_spec.rb @@ -18,6 +18,11 @@ expect(response).to have_http_status(:no_content) end + it 'marks the class as deleted instead of destroying it' do + delete("/api/schools/#{school.id}/classes/#{school_class.id}", headers:) + expect(school_class.reload.deleted?).to be true + end + it 'responds 204 No Content when the user is the class teacher' do authenticated_in_hydra_as(teacher) @@ -53,4 +58,11 @@ delete("/api/schools/#{school.id}/classes/#{school_class.id}", headers:) expect(response).to have_http_status(:forbidden) end + + it 'responds 403 Forbidden when the class is already deleted' do + school_class.update!(deleted: true) + + delete("/api/schools/#{school.id}/classes/#{school_class.id}", headers:) + expect(response).to have_http_status(:forbidden) + end end diff --git a/spec/features/school_class/listing_school_classes_spec.rb b/spec/features/school_class/listing_school_classes_spec.rb index 7e47d7d70..fb8bb4d76 100644 --- a/spec/features/school_class/listing_school_classes_spec.rb +++ b/spec/features/school_class/listing_school_classes_spec.rb @@ -102,6 +102,30 @@ def create_remix_with_feedback(school_class:, student:, feedback_attrs: []) data = get_classes expect(data.size).to eq(1) end + + it 'does not include deleted classes' do + school_class.update!(deleted: true) + + data = get_classes + expect(data.pluck(:name)).not_to include('Test School Class') + end + + it 'does not include deleted classes for school-teachers' do + authenticated_in_hydra_as(teacher) + school_class.update!(deleted: true) + + data = get_classes + expect(data).to be_empty + end + + it 'does not include deleted classes for school-students' do + authenticated_in_hydra_as(student) + stub_user_info_api_for(teacher) + school_class.update!(deleted: true) + + data = get_classes + expect(data).to be_empty + end end describe 'submitted_count' do diff --git a/spec/features/school_class/showing_a_school_class_spec.rb b/spec/features/school_class/showing_a_school_class_spec.rb index 80f84f7cd..973cd0be3 100644 --- a/spec/features/school_class/showing_a_school_class_spec.rb +++ b/spec/features/school_class/showing_a_school_class_spec.rb @@ -104,6 +104,31 @@ get("/api/schools/#{school.id}/classes/#{school_class.id}", headers:) expect(response).to have_http_status(:forbidden) end + + it 'responds 403 Forbidden when the class is deleted even if student is a member' do + student = create(:student, school:) + authenticated_in_hydra_as(student) + create(:class_student, school_class:, student_id: student.id) + school_class.update!(deleted: true) + + get("/api/schools/#{school.id}/classes/#{school_class.id}", headers:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the class is deleted even if user is the teacher' do + authenticated_in_hydra_as(teacher) + school_class.update!(deleted: true) + + get("/api/schools/#{school.id}/classes/#{school_class.id}", headers:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the class is deleted even if user is a school owner' do + school_class.update!(deleted: true) + + get("/api/schools/#{school.id}/classes/#{school_class.id}", headers:) + expect(response).to have_http_status(:forbidden) + end end context 'when school and class codes are provided' do @@ -201,5 +226,30 @@ get("/api/schools/#{school.code}/classes/#{school_class.code}", headers:) expect(response).to have_http_status(:forbidden) end + + it 'responds 403 Forbidden when the class is deleted even if student is a member' do + student = create(:student, school:) + authenticated_in_hydra_as(student) + create(:class_student, school_class:, student_id: student.id) + school_class.update!(deleted: true) + + get("/api/schools/#{school.code}/classes/#{school_class.code}", headers:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the class is deleted even if user is the teacher' do + authenticated_in_hydra_as(teacher) + school_class.update!(deleted: true) + + get("/api/schools/#{school.code}/classes/#{school_class.code}", headers:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the class is deleted even if user is a school owner' do + school_class.update!(deleted: true) + + get("/api/schools/#{school.code}/classes/#{school_class.code}", headers:) + expect(response).to have_http_status(:forbidden) + end end end diff --git a/spec/features/school_class/updating_a_school_class_spec.rb b/spec/features/school_class/updating_a_school_class_spec.rb index 21a31fce6..972e8cfa7 100644 --- a/spec/features/school_class/updating_a_school_class_spec.rb +++ b/spec/features/school_class/updating_a_school_class_spec.rb @@ -84,4 +84,11 @@ put("/api/schools/#{school.id}/classes/#{school_class.id}", headers:, params:) expect(response).to have_http_status(:forbidden) end + + it 'responds 403 Forbidden when the class is deleted even if user is the teacher' do + school_class.update!(deleted: true) + + put("/api/schools/#{school.id}/classes/#{school_class.id}", headers:, params:) + expect(response).to have_http_status(:forbidden) + end end diff --git a/spec/models/school_class_spec.rb b/spec/models/school_class_spec.rb index d90034d28..9ed36f4c4 100644 --- a/spec/models/school_class_spec.rb +++ b/spec/models/school_class_spec.rb @@ -285,6 +285,50 @@ end end + describe '#deleted?' do + it 'is not deleted by default' do + school_class = create(:school_class, teacher_ids: [teacher.id], school:) + expect(school_class.deleted?).to be false + end + + it 'returns true when deleted' do + school_class = create(:school_class, teacher_ids: [teacher.id], school:) + school_class.update!(deleted: true) + expect(school_class.deleted?).to be true + end + end + + describe '#mark_as_deleted!' do + it 'sets deleted to true' do + school_class = create(:school_class, teacher_ids: [teacher.id], school:) + school_class.mark_as_deleted! + expect(school_class.deleted).to be true + end + + it 'does nothing if already deleted' do + school_class = create(:school_class, teacher_ids: [teacher.id], school:, deleted: true) + expect { school_class.mark_as_deleted! }.not_to(change { school_class.reload.updated_at }) + end + end + + describe '.active scope' do + it 'only returns active (non-deleted) classes' do + active_class = create(:school_class, teacher_ids: [teacher.id], school:, deleted: false) + create(:school_class, teacher_ids: [teacher.id], school:, deleted: true) + + expect(described_class.active).to contain_exactly(active_class) + end + end + + describe '.deleted scope' do + it 'only returns deleted classes' do + create(:school_class, teacher_ids: [teacher.id], school:, deleted: false) + deleted_class = create(:school_class, teacher_ids: [teacher.id], school:, deleted: true) + + expect(described_class.deleted).to contain_exactly(deleted_class) + end + end + describe 'auditing' do subject(:school_class) { create(:school_class, teacher_ids: [teacher.id], school:) }