Skip to content

Commit c689980

Browse files
authored
Make SchoolProject destroy dependent Transitions (#667)
Fix an incorrect `depdendent:` rule between `SchoolProject` and `SchoolProjectTransition`. ## Status - Closes RaspberryPiFoundation/digital-editor-issues#1126 ## Points for consideration: - Security - Performance ## What's changed? ### Database and Model Relations and Nullify Rules Previously, when a SchoolProject was destroyed, there was a rule that dependent `school_project_transitions` would be nullified rather than destroyed. At the same time, the definition of a `SchoolProjectTransition` states that the `school_project_id`, which refers to the project being transitioned, CANNOT be null. Therefore, when PR [#637](#637) allowed deletion of Students and, consequently, Projects, the error in these relations and nullification rules was exposed. This leads to an error when deleting a Student who owns projects which have been transitioned. This PR changes the `dependent:` rule between SchoolProject and SchoolProjectTransition to destroy the transition when the Proejct is destroyed. It also adds a test to ensure this happens. ## Follow on Work While this PR fixes the immediate problem, there remains an issue that `SchoolStudentsController#destroy_batch` will return a `200` status code on both success and failure. I suggest we address this in a follow up issue. ## Steps to perform after deploying to production NA
1 parent 549f781 commit c689980

3 files changed

Lines changed: 18 additions & 2 deletions

File tree

app/models/school_project.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ class SchoolProject < ApplicationRecord
44
belongs_to :school
55
belongs_to :project
66
has_many :feedback, dependent: :destroy
7-
has_many :school_project_transitions, autosave: false, dependent: :nullify
7+
has_many :school_project_transitions, autosave: false, dependent: :destroy
88

99
include Statesman::Adapters::ActiveRecordQueries[
1010
transition_class: ::SchoolProjectTransition,

spec/models/school_project_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
it { is_expected.to belong_to(:school) }
2121
it { is_expected.to belong_to(:project) }
2222
it { is_expected.to have_many(:feedback).dependent(:destroy) }
23-
it { is_expected.to have_many(:school_project_transitions).dependent(:nullify) }
23+
it { is_expected.to have_many(:school_project_transitions).dependent(:destroy) }
2424

2525
describe '#status' do
2626
it 'defaults to unsubmitted' do

spec/services/student_removal_service_spec.rb

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,22 @@
7979
# Other student's project should remain
8080
expect(Project.exists?(other_school_project.id)).to be true
8181
end
82+
83+
it 'deletes school project transitions when deleting projects' do
84+
project = create(:project, user_id: student.id, school: school)
85+
school_project = project.school_project
86+
87+
# Transition the project to create SchoolProjectTransition records
88+
school_project.transition_status_to!(:submitted, student.id)
89+
school_project.transition_status_to!(:returned, teacher.id)
90+
91+
results = service.remove_students
92+
93+
expect(results.first[:error]).to be_nil
94+
expect(Project.exists?(project.id)).to be false
95+
expect(SchoolProject.exists?(school_project.id)).to be false
96+
expect(SchoolProjectTransition.where(school_project_id: school_project.id).count).to eq(0)
97+
end
8298
end
8399

84100
context 'when student does not have a role in the school' do

0 commit comments

Comments
 (0)