Skip to content

Commit

Permalink
Modify peer review "unassign" behaviour to align with graders "unassi…
Browse files Browse the repository at this point in the history 
…gn" (#7274)
  • Loading branch information
@hemmatio
hemmatio authored Dec 1, 2024
1 parent 8085877 commit 5bd8e93
Show file tree
Hide file tree
Showing 4 changed files with 153 additions and 21 deletions.
1 change: 1 addition & 0 deletions Changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
12 changes: 10 additions & 2 deletions app/controllers/peer_reviews_controller.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 1 addition & 10 deletions app/models/peer_review.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand All @@ -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 = []

Expand All @@ -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

Expand Down
150 changes: 141 additions & 9 deletions spec/controllers/peer_reviews_controller_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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])
Expand All @@ -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))
Expand All @@ -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|
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 5bd8e93

Please sign in to comment.