Row selection for peer reviewer unassigning bypasses current unassign method

[hemmatio]

Proposed Changes

(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)
When selecting an entire row, the necessary checks from #7263 are being bypassed. For reference, an image of "row selection":
Proposed fix: make the row-selected behaviour consistent with the behaviour from the Graders tab.

An image of the Graders tab is as follows:

...

Screenshots of your changes (if applicable)

Examples of acceptable selections for unassignment: Flashed messages and overall behaviour are functionally identical to #7263

Associated documentation repository pull request (if applicable)

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)	X
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

• I have updated the project Changelog (this is required for all changes).
• I have verified that the pre-commit.ci checks have passed.
• I have verified that the CI tests have passed.
• I have reviewed the test coverage changes reported by Coveralls.
• I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)
14/11/24: I've gone through and replicated the Graders tab logic for the row selection case. I still need to add tests.

19/11/24: Tests have been added for various row selection cases. I have removed the method delete_all_reviews_for() in the PeerReview class, as well as all calls for it within the peer review unassign logic, as it is not useful.

[hemmatio]

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

[hemmatio]

It has been removed!

[coveralls]

Pull Request Test Coverage Report for Build 12104886027

Details

• 65 of 65 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 91.758%

---

Totals
Change from base Build 12097999442:	0.01%
Covered Lines:	41173
Relevant Lines:	44196

---

💛 - Coveralls

[david-yz-liu]

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

[david-yz-liu]

this block shouldn't be what you want (it doesn't make sense to mix reviewer group ids with reviewee group ids)

[david-yz-liu]

This change isn't correct; the # indicates that this is an instance method, as a Ruby convention

[hemmatio]

self.get_num_marked and self.get_num_collected are marked with self., which from my knowledge, makes them class methods rather than instance methods. However, their corresponding test cases are also prefixed with #. Which naming convention should these test cases follow?

[david-yz-liu]

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.

[hemmatio]

Marking as "ready for review" to see how ci/cd tests run, still not 100% on these changes. Will re-request a review once I am ready for your review :)

[david-yz-liu]

@hemmatio nice work. I left a few comments for further improvement, but I think you're quite close. Also make sure to update the Changelog (this is indicated in the PR template checklist, which you should remember to fill out)

[david-yz-liu]

I'm not sure why adding Array is needed here, as this PR isn't changing the front-end at all? If you're having trouble with the tests you've added, you can call Array in the tests themselves.

[hemmatio]

That's exactly what it was, I was having issues with tests. There seemed to be some cases where the group ids weren't being treated as Arrays, but strangely I did not experience test failures after removing this change.

[david-yz-liu]

Also, these don't need to be separate where clauses, as .where takes an arbitrary number of arguments and treats them as an AND, which is what you want in this case

[david-yz-liu]

@hemmatio great work!

By the way, I missed your earlier question about naming conventions for tests: the # symbol is used to prefix instance method names; the . is used to prefix class method names. Somewhat confusingly, class methods are defined using "self":

class A:
  def my_instance_method  # no "self"
    ...
  end

  def self.my_class_method  # "self" indicates this is a class method
    ...
  end