Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Row selection for peer reviewer unassigning bypasses current unassign method #7274

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
20aa634
indicated error with an inline comment
hemmatio Oct 31, 2024
8f9f4fc
merged in master
hemmatio Nov 14, 2024
9b8a554
added logic in peer_reviews_controller.rb 'unassign' call to follow s…
hemmatio Nov 14, 2024
0395ea2
testcase for successful row unassign
hemmatio Nov 18, 2024
5897d73
added testcases involving row selection error flashes, as well as a m…
hemmatio Nov 18, 2024
35e93d8
removed incorrect signifiers of instance methods from testcase
hemmatio Nov 18, 2024
b145503
added testcase for peer review reviewer id getter method
hemmatio Nov 18, 2024
9f3996e
removed deprecated code causing forceful row deletion
hemmatio Nov 18, 2024
8bfba79
removed unnecessary selected_reviewee_group_ids parameter from unassi…
hemmatio Nov 18, 2024
181ec39
updated a test case to run in the correct context block
hemmatio Nov 19, 2024
da70fcd
removed unused method, self.delete_all_peer_reviews_for()
hemmatio Nov 19, 2024
6ae354a
marked peer_review instance tests with the necessary hash (#), and ad…
hemmatio Nov 22, 2024
0d6ead5
marked peer_review instance tests with the necessary hash (#), and ad…
hemmatio Nov 22, 2024
1226211
Merge branch 'master' of https://github.com/MarkUsProject/Markus into…
hemmatio Nov 22, 2024
942c6c6
adjusted message flash checking for testcases
hemmatio Nov 22, 2024
625b64d
adjusted message flash checking for testcases
hemmatio Nov 22, 2024
55f5784
updated expected error to be more specific
hemmatio Nov 22, 2024
69cf8ef
updated mixed selection case
hemmatio Nov 22, 2024
b770965
updated row selection logic to use sql queries
hemmatio Nov 28, 2024
fdf1891
Merge branch 'master' of https://github.com/MarkUsProject/Markus into…
hemmatio Nov 28, 2024
712f2c4
Merge branch 'master' of https://github.com/MarkUsProject/Markus into…
hemmatio Dec 1, 2024
873c34e
updated changelog
hemmatio Dec 1, 2024
85c2028
further refined peer review query
hemmatio Dec 1, 2024
dbba456
updated changelog.md
hemmatio Dec 1, 2024
53c38c3
removed unneeded methods get_reviewee_id and get_reviewer_id, as well…
hemmatio Dec 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions app/controllers/peer_reviews_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,23 @@ def assign_groups
return
end
when 'unassign'
unless selected_reviewee_group_ids.empty? # if a row was selected
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check isn't necessary (the .each below will not execute if selected_reviewee_group_ids is empty)

selected_reviewee_group_ids.each do |reviewee_id| # row selected reviewee
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This algorithm is correct but pretty inefficient, as it involves a triply-nested loop.

You can improve this in a few ways:

  1. First, by joining on relevant PeerReview associations, which in this case are :reviewer and reviewee (both of these refer to Groupings, but Rails will handle the SQL generation to avoid name conflicts---inspect the logged SQL query for more information)
  2. You can use where on those associations, e.g. .where(reviewer: { id: ... }) and again Rails will produce the correct query
  3. You can pass in an array of ids to a where condition, and the resulting SQL query become an IN operation

As usual for complex queries, I recommend experimenting in the Rails console first before writing code to modify this method.

selected_reviewer_group_ids.each do |reviewer_id| # selected reviewers on left side
peer_reviews = PeerReview.joins(result: :submission).where(submissions: { grouping_id: reviewee_id })
peer_reviews.each do |peer_review| # all peer reviews associated with the reviewee
if peer_review.get_reviewer_id.to_s == reviewer_id.to_s
# if the selected reviewer is the reviewer of the peer review
# in other words, if the reviewer is assigned to the reviewee's row
reviewers_to_remove_from_reviewees_map[reviewee_id] ||= {}
reviewers_to_remove_from_reviewees_map[reviewee_id][reviewer_id] = true # mark for unassignment
end
end
end
end
selected_reviewee_group_ids = [] # prevent whole row from being deleted
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure selected_reviewee_group_ids can be removed from the PeerReview.unassign() call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been removed!

# now the rest of unassign follows as expected
end
deleted_count, undeleted_reviews = PeerReview.unassign(selected_reviewee_group_ids,
reviewers_to_remove_from_reviewees_map)
if !undeleted_reviews.empty? && deleted_count == 0
Expand Down
4 changes: 4 additions & 0 deletions app/models/peer_review.rb
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ def self.get_mappings_for(assignment)
mappings.group_by { |x| x[0] }.transform_values { |pairs| pairs.pluck(1) }
end

def get_reviewer_id
self.reviewer.id
end

def self.from_csv(assignment, data)
reviewer_map = assignment.groupings.includes(:group).index_by { |g| g.group.group_name }
reviewee_map = assignment.parent_assignment.groupings.includes(:group).index_by { |g| g.group.group_name }
Expand Down