From 5bd8e931816c4133a804c0bfea38ff2c9b145c9b Mon Sep 17 00:00:00 2001 From: Omid Hemmati <129822623+hemmatio@users.noreply.github.com> Date: Sun, 1 Dec 2024 11:48:17 -0500 Subject: [PATCH] Modify peer review "unassign" behaviour to align with graders "unassign" (#7274) --- Changelog.md | 1 + app/controllers/peer_reviews_controller.rb | 12 +- app/models/peer_review.rb | 11 +- .../peer_reviews_controller_spec.rb | 150 ++++++++++++++++-- 4 files changed, 153 insertions(+), 21 deletions(-) diff --git a/Changelog.md b/Changelog.md index f9ace4d4d1..6935167d06 100644 --- a/Changelog.md +++ b/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 diff --git a/app/controllers/peer_reviews_controller.rb b/app/controllers/peer_reviews_controller.rb index 2389145dea..9039266a0c 100644 --- a/app/controllers/peer_reviews_controller.rb +++ b/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 diff --git a/app/models/peer_review.rb b/app/models/peer_review.rb index 852a751566..825719a151 100644 --- a/app/models/peer_review.rb +++ b/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 diff --git a/spec/controllers/peer_reviews_controller_spec.rb b/spec/controllers/peer_reviews_controller_spec.rb index 0fe39324d7..e25bb80d46 100644 --- a/spec/controllers/peer_reviews_controller_spec.rb +++ b/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