Improvements to grouping deletion handling

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)

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.

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)

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

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: '

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