diff --git a/Changelog.md b/Changelog.md index e0eefac0c0..f02e777d6d 100644 --- a/Changelog.md +++ b/Changelog.md @@ -8,6 +8,7 @@ ### 🚨 Breaking changes ### ✨ New features and improvements +- Enforced restriction on grouping deletion when submissions exist (#8134) - Improved table selection column styling, fixed table overflow in containers, and hid unused scrollbars (#8133) - Displayed who last updated a mark to TAs and instructors in the grading view (#8131) - Automatically populate course start and end dates from the term when a course is created via Canvas LTI (#8057) @@ -32,6 +33,7 @@ - Added GET /test_runs API route (#8055) ### 🐛 Bug fixes +- Fixed bulk grouping deletion assignment scoping (#8134) - Ensured random grader assignment excludes ineligible roles and recalculates weights using eligible graders (#8073) - Prevented grader assignment and unassignment operations from modifying groupings belonging to other assignments (#8072) - Fixed the "Fix" link being clipped off the screen for long QR-scan error messages on the exam scan log table (#8070) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 2b68b443c2..3f087361c5 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -22,25 +22,16 @@ def new end def remove_group - # When a success div exists we can return successfully removed groups - groupings = Grouping.where(id: params[:grouping_id]) + assignment = current_course.assignments.find(params[:assignment_id]) + groupings = assignment.groupings.where(id: params[:grouping_id]) errors = [] - @removed_groupings = [] Repository.get_class.update_permissions_after(only_on_request: true) do groupings.each do |grouping| - grouping.student_memberships.each do |member| - grouping.remove_member(member.id) + unless grouping.destroy + errors.push(grouping.group.group_name) end end end - groupings.each do |grouping| - if grouping.has_submission? - errors.push(grouping.group.group_name) - else - grouping.delete_grouping - @removed_groupings.push(grouping) - end - end if errors.any? err_groups = errors.join(', ') flash_message(:error, I18n.t('groups.delete_group_has_submission') + err_groups) @@ -444,8 +435,8 @@ def create end def destroy - @assignment = Assignment.find(params[:assignment_id]) - @grouping = current_role.accepted_grouping_for(@assignment.id) + @assignment = current_course.assignments.find(params[:assignment_id]) + @grouping = @assignment.groupings.find(params[:id]) m_logger = MarkusLogger.instance if @grouping.nil? m_logger.log('Failed to delete group, since no accepted group for this user existed.' \ @@ -554,8 +545,6 @@ def global_actions students = Student.where(id: student_ids) case params[:global_actions] - when 'delete' - delete_groupings(groupings) when 'invalid' invalidate_groupings(groupings) when 'valid' @@ -653,24 +642,6 @@ def validate_groupings(groupings) groupings.each(&:validate_grouping) end - # Deletes the given list of groupings if possible. Removes each member first. - def delete_groupings(groupings) - # If any groupings have a submission raise an error. - if groupings.any?(&:has_submission?) - raise I18n.t('groups.could_not_delete') # should add names of grouping we could not delete - else - # Remove each student from every group. - Repository.get_class.update_permissions_after(only_on_request: true) do - groupings.each do |grouping| - grouping.student_memberships.each do |mem| - grouping.remove_member(mem.id) - end - grouping.delete_grouping - end - end - end - end - # Adds students to grouping. `groupings` should be an array with # only one element, which is the grouping that is supposed to be # added to. diff --git a/app/models/grouping.rb b/app/models/grouping.rb index f323c42a2a..ee84eb4f8c 100644 --- a/app/models/grouping.rb +++ b/app/models/grouping.rb @@ -70,7 +70,7 @@ class Grouping < ApplicationRecord class_name: 'Student', through: :accepted_student_memberships, source: :role - has_many :submissions + has_many :submissions, dependent: :restrict_with_error has_one :current_submission_used, -> { where submission_version_used: true }, class_name: 'Submission', @@ -487,13 +487,6 @@ def remove_member(mbr_id) end end - def delete_grouping - Repository.get_class.update_permissions_after(only_on_request: true) do - student_memberships.includes(:role).find_each(&:destroy) - end - self.destroy - end - # Removes the member rejected by its membership id # Used as safeguard when student deletes the record def remove_rejected(mbr_id) diff --git a/config/locales/models/groupings/en.yml b/config/locales/models/groupings/en.yml index 1baaa761a1..2ffafa4088 100644 --- a/config/locales/models/groupings/en.yml +++ b/config/locales/models/groupings/en.yml @@ -15,4 +15,8 @@ en: errors: models: grouping: + attributes: + base: + restrict_dependent_destroy: + has_many: This grouping could not be deleted because it has associated %{record}. different_assignment_grouping: tags must belong to the same assignment as this grouping diff --git a/config/locales/views/groups/en.yml b/config/locales/views/groups/en.yml index 92c4fd7c6d..3527b4a79c 100644 --- a/config/locales/views/groups/en.yml +++ b/config/locales/views/groups/en.yml @@ -13,7 +13,6 @@ en: no_active_students: 'Group ''%{group}'' not cloned: zero active students left.' no_member: 'Member ''%{member}'' not added to group ''%{group}'': %{error}.' other: 'Group ''%{group}'' not cloned: %{error}.' - could_not_delete: Could not delete the following groups - note that groups cannot be deleted for this assignment if a submission has already been created for them for grading. delete: Delete group(s) delete_confirm: This will remove all members in the group and delete all information associated with this group. Continue? delete_group_has_submission: 'The following groupings could not be deleted since they have submissions: ' diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index c6969b4f84..6e43687a1d 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -58,80 +58,104 @@ end context 'when grouping has no submissions' do - before do - allow(grouping).to receive(:delete_grouping) - allow(grouping).to receive(:has_submission?).and_return(false) - end - - it 'should not flash an error message' do + it 'does not flash an error message' do delete_as instructor, :remove_group, params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } expect(flash[:error]).to be_nil end - it 'populates @removed_groupings with deleted groupings' do + it 'attempts to update permissions file' do + expect(Repository.get_class).to receive(:update_permissions_after) delete_as instructor, :remove_group, params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } - expect(assigns(:removed_groupings)).to match_array([grouping]) end - it 'calls grouping.has_submission?' do - expect(grouping).to receive(:has_submission?).and_return(false) + it 'returns the :ok status code' do delete_as instructor, :remove_group, params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } + expect(response).to have_http_status(:ok) end - it 'calls grouping.delete_groupings' do - expect(grouping).to receive(:delete_grouping) + it 'removes the grouping' do delete_as instructor, :remove_group, params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } + expect(assignment.groupings.find_by(id: grouping.id)).to be_nil end + end - it 'should attempt to update permissions file' do - expect(Repository.get_class).to receive(:update_permissions_after) + context 'when grouping has submissions' do + before do + create(:version_used_submission, grouping: grouping) delete_as instructor, :remove_group, params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } end - it 'should return the :ok status code' do + it 'reports an error message' do + expect(flash[:error]).to be_present + end + + it 'returns the :ok status code' do delete_as instructor, :remove_group, params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } expect(response).to have_http_status(:ok) end - end - - context 'when grouping has submissions' do - before do - allow(grouping).to receive(:has_submission?).and_return(true) + it 'attempts to update permissions file' do + expect(Repository.get_class).to receive(:update_permissions_after) delete_as instructor, :remove_group, params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } end - it 'should have an error message in the flash queue' do - expect(flash[:error]).to be_present + it 'does not remove the grouping' do + delete_as instructor, :remove_group, + params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } + expect(assignment.groupings.find_by(id: grouping.id)).to be_present end + end + + context 'when the grouping belongs to a different assignment' do + let!(:other_grouping) { create(:grouping) } - it 'assigns empty array to @removed_groupings' do - expect(assigns(:removed_groupings)).to be_empty + it 'does not remove the grouping' do + delete_as instructor, :remove_group, + params: { course_id: course.id, grouping_id: [other_grouping.id], assignment_id: assignment } + expect(other_grouping.assignment.groupings.find_by(id: other_grouping.id)).to be_present end - it 'calls grouping.has_submission?' do - expect(grouping).to receive(:has_submission?).and_return(true) + it 'does not flash an error message' do delete_as instructor, :remove_group, - params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } + params: { course_id: course.id, grouping_id: [other_grouping.id], assignment_id: assignment } + expect(flash[:error]).to be_nil end - it 'should return the :ok status code' do + it 'returns the :ok status code' do delete_as instructor, :remove_group, - params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } + params: { course_id: course.id, grouping_id: [other_grouping.id], assignment_id: assignment } expect(response).to have_http_status(:ok) end + end - it 'should attempt to update permissions file' do - expect(Repository.get_class).to receive(:update_permissions_after) + context 'when multiple groupings are selected and only some have submissions' do + let!(:other_grouping) { create(:grouping, assignment: assignment) } + + before do + create(:version_used_submission, grouping: grouping) delete_as instructor, :remove_group, - params: { course_id: course.id, grouping_id: [grouping.id], assignment_id: assignment } + params: { course_id: course.id, grouping_id: [grouping.id, other_grouping.id], + assignment_id: assignment } + end + + it 'removes the grouping without a submission' do + expect(assignment.groupings.find_by(id: other_grouping.id)).to be_nil + end + + it 'does not remove the grouping with a submission' do + expect(assignment.groupings.find_by(id: grouping.id)).to be_present + end + + it 'reports an error message naming only the grouping that could not be removed' do + expect(flash[:error].join).to include(grouping.group.group_name) + expect(flash[:error].join).not_to include(other_grouping.group.group_name) end end end @@ -878,29 +902,6 @@ end end - describe '#delete_groupings' do - let!(:grouping) { create(:grouping_with_inviter) } - let!(:grouping_with_submission) { create(:grouping_with_inviter_and_submission) } - - it 'should delete groupings without submissions' do - post_as instructor, :global_actions, params: { course_id: course.id, - assignment_id: grouping.assignment.id, - groupings: [grouping.id], - global_actions: 'delete' } - - expect(Grouping.all.size).to eq 1 - end - - it 'should not delete groupings with submissions' do - post_as instructor, :global_actions, params: { course_id: course.id, - assignment_id: grouping_with_submission.assignment.id, - groupings: [grouping_with_submission.id], - global_actions: 'delete' } - - expect(Grouping.all.size).to eq 2 - end - end - describe '#add_members' do let(:grouping) { create(:grouping_with_inviter) } let(:grouping2) { create(:grouping_with_inviter, assignment: grouping.assignment) } @@ -1361,6 +1362,59 @@ it 'should respond with success' do expect(subject).to respond_with(:redirect) end + + context 'when the student is the inviter of a deletable grouping with no submission' do + let!(:own_grouping) { create(:grouping_with_inviter, assignment: @assignment, inviter: @current_student) } + + before do + delete_as @current_student, :destroy, + params: { course_id: course.id, assignment_id: @assignment.id, id: own_grouping.id } + end + + it 'removes the grouping' do + expect(Grouping.find_by(id: own_grouping.id)).to be_nil + end + + it 'redirects to the assignment page' do + expect(response).to redirect_to(course_assignment_path(course, @assignment)) + end + + it 'flashes a success message' do + expect(flash[:success]).to be_present + end + end + + context 'when the grouping has a submission' do + let!(:own_grouping) do + create(:grouping_with_inviter_and_submission, assignment: @assignment, inviter: @current_student) + end + + before do + delete_as @current_student, :destroy, + params: { course_id: course.id, assignment_id: @assignment.id, id: own_grouping.id } + end + + it 'does not remove the grouping' do + expect(Grouping.find_by(id: own_grouping.id)).to be_present + end + + it 'redirects to the assignment page' do + expect(response).to redirect_to(course_assignment_path(course, @assignment)) + end + end + + context 'when the grouping belongs to a different assignment than the one specified in the request' do + let(:other_assignment) { create(:assignment, course: course) } + let!(:own_grouping) { create(:grouping_with_inviter, assignment: other_assignment, inviter: @current_student) } + + it 'does not remove the grouping and raises an error' do + expect do + delete_as @current_student, :destroy, + params: { course_id: course.id, assignment_id: @assignment.id, id: own_grouping.id } + end.to raise_error(ActiveRecord::RecordNotFound) + expect(Grouping.find_by(id: own_grouping.id)).to be_present + end + end end describe 'POST #invite_member' do diff --git a/spec/models/grouping_spec.rb b/spec/models/grouping_spec.rb index 458f1c5b59..70d82b1e57 100644 --- a/spec/models/grouping_spec.rb +++ b/spec/models/grouping_spec.rb @@ -5,7 +5,7 @@ it { is_expected.to belong_to(:group) } it { is_expected.to belong_to(:assignment) } it { is_expected.to have_many(:memberships) } - it { is_expected.to have_many(:submissions) } + it { is_expected.to have_many(:submissions).dependent(:restrict_with_error) } it { is_expected.to have_many(:notes) } it { is_expected.to have_one(:extension).dependent(:destroy) } it { is_expected.to have_one(:course) } @@ -228,14 +228,6 @@ end end - describe '.delete_grouping' do - it 'makes an attempt to update repository permissions when deleting a group' do - g = groupings - expect(Repository.get_class).to receive(:update_permissions_after) - g[0].delete_grouping - end - end - describe '.assign_tas' do it 'updates repository permissions exactly once after assigning all TAs' do expect(UpdateRepoPermissionsJob).to receive(:perform_later).once @@ -811,6 +803,30 @@ def expect_updated_criteria_coverage_count_eq(expected_count) end end + describe '#destroy' do + context 'when the grouping has a submission' do + before { create(:version_used_submission, grouping: @grouping) } + + it 'does not destroy the grouping' do + expect { @grouping.destroy }.not_to(change { Grouping.exists?(@grouping.id) }) + end + + it 'adds an error to the grouping' do + @grouping.destroy + expect(@grouping.errors.full_messages).to include( + I18n.t('activerecord.errors.models.grouping.attributes.base.restrict_dependent_destroy.has_many', + record: 'submissions') + ) + end + end + + context 'when the grouping has no submission' do + it 'destroys the grouping' do + expect { @grouping.destroy }.to change { Grouping.exists?(@grouping.id) }.from(true).to(false) + end + end + end + context 'when the group has no students' do before do @student01 = create(:student)