Row selection for peer reviewer unassigning bypasses current unassign method

Changelog.md

@@ -13,6 +13,7 @@
 - Ensure we handle JSON parsing exceptions when converting Jupyter Notebooks (#7308)
 - Fixed bug in grading context menu for editing/deleting annotations (#7309)
 - Fixed bug in grading annotations table when deleting annotations (#7309)
+- Ensures row selection for peer reviewer unassigning has the same validation checks as individual selections (#7274)
 
 ### 🔧 Internal changes
 

app/controllers/peer_reviews_controller.rb

@@ -157,8 +157,16 @@ def assign_groups
         return
       end
     when 'unassign'
-      deleted_count, undeleted_reviews = PeerReview.unassign(selected_reviewee_group_ids,
-                                                             reviewers_to_remove_from_reviewees_map)
+      peer_reviews_filtered = PeerReview.joins(:reviewer, :reviewee)
+                                        .where(reviewer: { id: selected_reviewer_group_ids },
+                                               reviewee: { id: selected_reviewee_group_ids })
+                                        .pluck(['reviewer.id', 'reviewee.id'])
+      peer_reviews_filtered.each do |reviewer_id, reviewiee_id|
+        reviewers_to_remove_from_reviewees_map[reviewiee_id] ||= {} # Initialize if does not exist
+        reviewers_to_remove_from_reviewees_map[reviewiee_id][reviewer_id] = true
+      end
+
+      deleted_count, undeleted_reviews = PeerReview.unassign(reviewers_to_remove_from_reviewees_map)
       if !undeleted_reviews.empty? && deleted_count == 0
         flash_now(:error, t('peer_reviews.errors.cannot_unassign_any_reviewers'))
         return

app/models/peer_review.rb

@@ -53,11 +53,6 @@ def self.delete_peer_review_between(reviewer, reviewee)
     end
   end
 
-  # Deletes all peer reviewers for the reviewee groupings
-  def self.delete_all_peer_reviews_for(reviewee_id)
-    self.joins(result: :submission).where(submissions: { grouping_id: reviewee_id }).destroy_all
-  end
-
   def self.assign(reviewer_groups, reviewee_groups)
     reviewer_groups.each do |reviewer_group|
       reviewee_groups.each do |reviewee_group|
@@ -69,7 +64,7 @@ def self.assign(reviewer_groups, reviewee_groups)
     end
   end
 
-  def self.unassign(selected_reviewee_group_ids, reviewers_to_remove_from_reviewees_map)
+  def self.unassign(reviewers_to_remove_from_reviewees_map)
     deleted_count = 0
     undeleted_reviews = []
 
@@ -93,10 +88,6 @@ def self.unassign(selected_reviewee_group_ids, reviewers_to_remove_from_reviewee
         end
       end
     end
-
-    if undeleted_reviews.empty?
-      self.delete_all_peer_reviews_for(selected_reviewee_group_ids)
-    end
     [deleted_count, undeleted_reviews]
   end
 

spec/controllers/peer_reviews_controller_spec.rb

@@ -228,9 +228,12 @@
 
       context 'all reviewers for selected reviewees' do
         before do
+          reviewers_to_remove_from_reviewees_map = {}
+          reviewers_to_remove_from_reviewees_map[@selected_reviewee_group_ids[0]] =
+            @selected_reviewer_group_ids.index_with { true }
           post_as role, :assign_groups,
                   params: { actionString: 'unassign',
-                            selectedRevieweeGroupIds: @selected_reviewee_group_ids[0],
+                            selectedReviewerInRevieweeGroups: reviewers_to_remove_from_reviewees_map,
                             assignment_id: @pr_id,
                             course_id: course.id }
         end
@@ -268,6 +271,37 @@
         it 'removes selected reviewer as reviewer for selected reviewee' do
           expect(PeerReview.review_exists_between?(@reviewer, @reviewee)).to be false
         end
+
+        context 'when row(s) of reviewee(s) are selected' do
+          before do
+            @reviewers_to_remove_from_reviewees_map = {} # no individual checkboxes are selected
+          end
+
+          context 'when applicable reviewers are selected' do
+            before do
+              reviewer_ids = PeerReview.joins(:reviewee).where(groupings: { id: @selected_reviewee_group_ids })
+                                       .pluck(:reviewer_id)
+              @selected_reviewer_group_ids = reviewer_ids
+              post_as role, :assign_groups,
+                      params: { actionString: 'unassign',
+                                selectedRevieweeGroupIds: @selected_reviewee_group_ids,
+                                selectedReviewerGroupIds: @selected_reviewer_group_ids,
+                                selectedReviewerInRevieweeGroups: @reviewers_to_remove_from_reviewees_map,
+                                assignment_id: @pr_id,
+                                course_id: course.id }
+            end
+
+            it 'deletes the correct number of peer reviews' do
+              expect(@assignment_with_pr.peer_reviews.count).to eq 0
+            end
+
+            it 'flashes the correct message' do
+              expect(flash[:success].map { |f| extract_text f }).to eq [I18n.t(
+                'peer_reviews.unassigned_reviewers_successfully', deleted_count: 8.to_s
+              )]
+            end
+          end
+        end
       end
 
       context 'selected reviews have marks or annotations' do
@@ -307,6 +341,37 @@
           end
         end
 
+        context 'when row(s) of reviewee(s) who cannot be unassigned are selected' do
+          before do
+            @reviewers_to_remove_from_reviewees_map = {} # no individual checkboxes are selected
+          end
+
+          context 'when all applicable reviewers are selected' do
+            before do
+              @selected_reviewer_group_ids = PeerReview.joins(:reviewee)
+                                                       .where(groupings: { id: @selected_reviewee_group_ids })
+                                                       .pluck(:reviewer_id)
+              post_as role, :assign_groups,
+                      params: { actionString: 'unassign',
+                                selectedRevieweeGroupIds: @selected_reviewee_group_ids,
+                                selectedReviewerGroupIds: @selected_reviewer_group_ids,
+                                selectedReviewerInRevieweeGroups: @reviewers_to_remove_from_reviewees_map,
+                                assignment_id: @pr_id,
+                                course_id: course.id }
+            end
+
+            it 'flashes the correct message' do
+              expect(flash[:error].map { |f| extract_text f }).to eq [I18n.t(
+                'peer_reviews.errors.cannot_unassign_any_reviewers'
+              )]
+            end
+
+            it 'does not delete any peer reviews' do
+              expect(@assignment_with_pr.peer_reviews.count).to eq @num_peer_reviews
+            end
+          end
+        end
+
         context 'when some reviewers are unassigned, but more than 5 are not' do
           before do
             @assignment_with_pr.peer_reviews.first.result.update(marking_state: Result::MARKING_STATES[:incomplete])
@@ -320,11 +385,10 @@
 
           it 'flashes the correct message' do
             undeleted_reviews = @assignment_with_pr.peer_reviews.map do |review|
-              I18n.t('activerecord.models.peer_review.reviewer_assigned_to_reviewee',
+              I18n.t('activerecord.models.peer_review.cannot_unassign_all_reviewers',
                      reviewer_group_name: review.reviewer.group.group_name,
                      reviewee_group_name: review.result.grouping.group.group_name)
             end
-
             flashed_error = flash[:error].map { |f| extract_text f }[0]
             expect(flashed_error).to include('Successfully unassigned 1 peer reviewer(s)')
             expect(flashed_error).to include(I18n.t('additional_not_shown', count: undeleted_reviews.length - 6))
@@ -335,6 +399,44 @@
           end
         end
 
+        context 'when some rows of reviewees are selected and some reviewers are unassigned' do
+          before do
+            @assignment_with_pr.peer_reviews.first.result.update(marking_state: Result::MARKING_STATES[:incomplete])
+            @reviewers_to_remove_from_reviewees_map = {}
+          end
+
+          context 'when all applicable reviewers are selected' do
+            before do
+              @selected_reviewer_group_ids = PeerReview.joins(:reviewee)
+                                                       .where(groupings: { id: @selected_reviewee_group_ids })
+                                                       .pluck(:reviewer_id)
+              post_as role, :assign_groups,
+                      params: { actionString: 'unassign',
+                                selectedRevieweeGroupIds: @selected_reviewee_group_ids,
+                                selectedReviewerGroupIds: @selected_reviewer_group_ids,
+                                selectedReviewerInRevieweeGroups: @reviewers_to_remove_from_reviewees_map,
+                                assignment_id: @pr_id,
+                                course_id: course.id }
+            end
+
+            it 'flashes the correct message' do
+              undeleted_reviews = @assignment_with_pr.peer_reviews.map do |review|
+                I18n.t('activerecord.models.peer_review.cannot_unassign_all_reviewers',
+                       reviewer_group_name: review.reviewer.group.group_name,
+                       reviewee_group_name: review.result.grouping.group.group_name)
+              end
+
+              flashed_error = flash[:error].map { |f| extract_text f }[0]
+              expect(flashed_error).to include('Successfully unassigned 1 peer reviewer(s)')
+              expect(flashed_error).to include(I18n.t('additional_not_shown', count: undeleted_reviews.length - 6))
+            end
+
+            it 'deletes the correct number of peer reviews' do
+              expect(@assignment_with_pr.peer_reviews.count).to eq @num_peer_reviews - 1
+            end
+          end
+        end
+
         context 'when some reviewers are unassigned, but less than 5 are not' do
           before do
             (1..6).each do |i|
@@ -349,12 +451,6 @@
           end
 
           it 'flashes the correct message' do
-            @assignment_with_pr.peer_reviews.map do |review|
-              I18n.t('activerecord.models.peer_review.reviewer_assigned_to_reviewee',
-                     reviewer_group_name: review.reviewer.group.group_name,
-                     reviewee_group_name: review.result.grouping.group.group_name)
-            end
-
             flashed_error = flash[:error].map { |f| extract_text f }[0]
             expect(flashed_error).to include('Successfully unassigned 6 peer reviewer(s)')
           end
@@ -363,6 +459,42 @@
             expect(@assignment_with_pr.peer_reviews.count).to eq @num_peer_reviews - 6
           end
         end
+
+        context 'when some rows of reviewees and some individual reviewers are selected' do
+          before do
+            [0, 2].each do |i| # mark 1st and 3rd peer reviews as unassign-able
+              @assignment_with_pr.peer_reviews[i].result.update(marking_state: Result::MARKING_STATES[:incomplete])
+            end
+            @selected_reviewee_group_ids.last(2).each do |reviewee_id| # individually select 2nd and 3rd reviewers
+              @reviewers_to_remove_from_reviewees_map[reviewee_id] = @selected_reviewer_group_ids.index_with { true }
+            end
+            @selected_reviewer_group_ids = PeerReview.joins(:reviewee)
+                                                     .where(groupings: { id: @selected_reviewee_group_ids })
+                                                     .pluck(:reviewer_id)[0] # select the 1st reviewee row
+            row_reviewee = @selected_reviewee_group_ids[0]
+            row_reviewer = @selected_reviewer_group_ids
+            @reviewers_to_remove_from_reviewees_map[row_reviewee][row_reviewer] = false
+            # ensure row is not individually checked
+
+            post_as role, :assign_groups,
+                    params: { actionString: 'unassign',
+                              selectedRevieweeGroupIds: @selected_reviewee_group_ids,
+                              selectedReviewerGroupIds: @selected_reviewer_group_ids,
+                              selectedReviewerInRevieweeGroups: @reviewers_to_remove_from_reviewees_map,
+                              assignment_id: @pr_id,
+                              course_id: course.id }
+          end
+
+          it 'flashes the correct message' do
+            flashed_error = flash[:error].map { |f| extract_text f }[0]
+            expect(flashed_error).to include('Successfully unassigned 2 peer reviewer(s), but could not unassign the ' \
+                                             'following due to existing marks or annotations: ')
+          end
+
+          it 'deletes the correct number of peer reviews' do
+            expect(@assignment_with_pr.peer_reviews.count).to eq @num_peer_reviews - 2
+          end
+        end
       end
     end
   end