Prevent deletion of graded peer reviews #7263

hemmatio · 2024-10-10T03:06:06Z

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.)
This PR aims to fix #7119. Currently, instructors are able to unassign peer reviewers no matter the situation. This PR prevents peer reviewers from being unassigned in the case that grade data exists, annotations exist, or the peer review is set to complete.

Screenshots of your changes (if applicable)

Case 1: Successfully unassigned all

Case 2: Could not unassign any

Case 3: Unassigned some, could not unassign rest (less than 5)

Case 4: Unassigned some, could not unassign rest (more than 5)

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)	X
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)	X
♻️ 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.)
09/10/24: This is still a WIP, I just wanted to save my work on GitHub.

15/10/24: Added more error/success flashes, moved flash messages to locale file, modified logic of mark checking to check for non-nil mark criteron

22/10/24: Updated error messages to include reviewer and reviewee pairs to prevent any confusion in the case that the same reviewer cannot be unassigned in multiple different cases. Updated error flashes to be truncated when more than 5 reviewers could not be unassigned. Updated the exclamation mark in error flashes to be centred when the message is more than one line long. Added testcase which mocks that has_marks_or_annotations? returns true, then ensures that no peer reviews are deleted in that case.
Also: Is it necessary to update the project documentation for this? I see that the peer review wiki does not have anything yet, which I believe would be the related documentation.

23/10/24: Added more comprehensive testcases to cover all written lines of code. Mocks behaviour for each flashed message, and fully covers logic of has_marks_or_annotations? checking each criterion for a non-nil mark.

24/10/24: When selecting an entire row, the necessary checks were being bypassed. To fix this, I processed each row selection to be treated as if individual reviewers were selected.
For reference, an image of "row selection":

28/10/24: Left to do: Make separate PR for row deletion, add more testcases, update testcases to check database for deletion.

30-31/10/24: Updated testcases in peer_reviews_controller_spec.rb
Created new (more comprehensive) testcases for peer_review_spec.rb, involving the testing of has_marks_or_annotations? and check_marks_or_annotations, as well as the before_destroy callback. I opted to not create separate testcases for unassign, as this is already handled by the controller test cases.
Created new PR for the row deletion error.

4/11/24: Made changes to clean up code. Still need to switch from mocking to direct object usage for peer_review_controller tests.

6/11/24: FINALLY! Made tests in peer_review_controller_spec.rb stub rather than mock. TODO: Work on the other PR.

… issue-7119

…r and reviewee pairs. Updated css stylesheet to centre the exclamation mark in error flashes when the error message is multiple lines. Added testcase to ensure that has_marks_or_annotations? works as expected, and prevents peer reviewers from being unassigned

… issue-7119

hemmatio · 2024-10-23T21:48:38Z

Sorry, I requested a review prematurely. I'll re-request once I am fully prepared for a review.

coveralls · 2024-10-23T22:02:41Z

Pull Request Test Coverage Report for Build 11783156360

Details

120 of 120 (100.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 91.691%

Totals
Change from base Build 11770985819:	0.02%
Covered Lines:	40823
Relevant Lines:	43852

💛 - Coveralls

…iews_controller.rb

…ections (Previously row selections bypassed the necessary checks)

david-yz-liu · 2024-10-24T14:08:45Z

app/models/peer_review.rb

+        unless reviewee_group.nil? || reviewer_group.nil?
+          peer_review = reviewer_group.review_for(reviewee_group)
+          unless peer_review.nil?
+            if peer_review.has_marks_or_annotations?


This check (peer_review.has_marks_or_annotations?) is good, but you should reorganize as follows: instead of adding it here, add it as a before_destroy callback. The delete_peer_review_between method calls destroy, and will trigger the callback. This is more robust because it will affect everywhere that a peer review is attempted to be destroyed, not just this particular method.

(It's possible to also skip callbacks, in case we ever want to enable that in the future.)

david-yz-liu · 2024-10-24T14:09:30Z

app/models/peer_review.rb

+          unless peer_review.nil?
+            if peer_review.has_marks_or_annotations?
+              delete_all_reviews = false
+              undeleted_reviews.append(reviewer_group.get_all_students_in_group +


Use the group name rather than the students (the latter will make the message look weird when students are working in groups)

Make this entire string internationalized

david-yz-liu · 2024-10-24T14:10:04Z

app/models/peer_review.rb

+  def has_marks_or_annotations?
+    result = self.result
+    marks_not_nil = false
+    result.marks.each do |mark|


This is inefficient, and can be simplified into a single query with exists?

david-yz-liu · 2024-10-24T14:11:41Z

app/models/peer_review.rb

+    deleted_count = 0
+    undeleted_reviews = []
+
+    # When an entire row is selected for unassignment


This is interesting, it's seems you're actually fixing a separate bug here; please pull this out and make into into a new pull request with a description of just this bug.

david-yz-liu · 2024-10-24T14:18:48Z

app/controllers/peer_reviews_controller.rb

-      PeerReview.unassign(selected_reviewee_group_ids, reviewers_to_remove_from_reviewees_map)
+      reviews_deleted, deleted_count, undeleted_reviews = PeerReview.unassign(selected_reviewee_group_ids,
+                                                                              reviewers_to_remove_from_reviewees_map)
+      if !reviews_deleted && deleted_count == 0


I don't think this case needs a separate message, you can merge it with the next one (length <= 5). See also my suggestion for rewording the message down below.

david-yz-liu · 2024-10-24T14:23:36Z

app/assets/stylesheets/common/core.scss

@@ -498,6 +498,8 @@ option.uncollected {
 .flash-content {
  display: inline-block;
  padding-left: 5pt;
+  vertical-align: middle;
+  max-width: calc(100% - 3em);


Keep all rules alphabetically ordered (I'm surprised the pre-commit hooks didn't pick up on this, but that's something I'll look into separately!)

david-yz-liu · 2024-10-24T14:25:45Z

config/locales/views/peer_reviews/en.yml

@@ -9,6 +9,9 @@ en:
    assigned_reviewers_header: Reviewers
    errors:
      cannot_allow_reviewer_to_be_reviewee: A student cannot be a reviewer and reviewee
+      cannot_unassign_all_reviewers: 'Cannot unassign all selected peer reviewers due to existing marks or annotations. %{deleted_count}  peer reviewer(s) were unassigned, but the following could not be: %{undeleted_reviews}.'


Reword to "Successfully unassigned %{deleted_count} peer reviewer(s), but could not unassign the following due to existing marks or annotations: ..."

david-yz-liu · 2024-10-24T14:28:07Z

spec/controllers/peer_reviews_controller_spec.rb

+          @reviewer = Grouping.find_by(id: @selected_reviewer_group_ids[0])
+          @reviewee = Grouping.find_by(id: @selected_reviewee_group_ids[1])
+          allow_any_instance_of(PeerReview).to receive(:has_marks_or_annotations?).and_return(true)
+          selected_group = {}


It's not really necessary to create separate hash variables like this, you can define it inline as { @reviewee.id => { @reviewer.id => true } } or something similar

david-yz-liu · 2024-10-24T14:29:01Z

spec/controllers/peer_reviews_controller_spec.rb

+          end
+        end
+
+        it 'does not delete the peer review' do


It's good to be testing whether or not the database was actually changed here, and you should be doing it in the other tests as well

david-yz-liu · 2024-10-24T14:29:53Z

spec/models/peer_review_spec.rb

@@ -88,4 +88,23 @@
      end
    end
  end
+
+  describe '#has_marks_or_annotations?' do


This set of test cases is incomplete, you should be adding more cases (don't just rely on code coverage to tell you whether the tests are complete or not)

…, added before_destroy callback to peer_review deletion, simplified some inefficient code

… issue-7119

…check_marks_or_annotations

david-yz-liu · 2024-11-04T01:57:49Z

app/controllers/peer_reviews_controller.rb

+        message = t('peer_reviews.errors.cannot_unassign_all_reviewers',
+                    deleted_count: deleted_count.to_s, undeleted_reviews: undeleted_reviews.first(5).join(', '))
+        if undeleted_reviews.length > 5
+          message += " #{t('additional_not_shown')}"


add a count argument here as well (so the message becomes Additional {n} not shown)

david-yz-liu · 2024-11-04T01:58:41Z

app/models/peer_review.rb

+  def has_marks_or_annotations?
+    result = self.result
+    marks_not_nil = result
+                    .marks.where.not(mark: 0).exists?


this should be looking for nil marks, not 0 marks

david-yz-liu · 2024-11-04T01:59:09Z

app/models/peer_review.rb

@@ -69,16 +70,49 @@ def self.assign(reviewer_groups, reviewee_groups)
  end

  def self.unassign(selected_reviewee_group_ids, reviewers_to_remove_from_reviewees_map)
+    delete_all_reviews = true


A better name for this variable is all_reviews_deleted (correctly implies it's a boolean).

However, this variable actually isn't needed at all, as its value is equivalent to undeleted_reviews.empty? So you can simplify the code further by removing this variable entirely.

david-yz-liu · 2024-11-04T02:03:48Z

spec/models/peer_review_spec.rb

+        create(:mark, criterion: criterion, result: peer_review.result, mark: 2.5)
+      end
+
+      it 'returns true when there are non-nil marks' do


in these descriptions, do not repeat phrasing from the context ("when there are..."). The full test label concatenates the context and it descriptions.

This is a huge oversight on my part, wow. Thanks for pointing this out.

david-yz-liu · 2024-11-04T02:05:17Z

spec/controllers/peer_reviews_controller_spec.rb

+        before do
+          @reviewer = Grouping.find_by(id: @selected_reviewer_group_ids[0])
+          @reviewee = Grouping.find_by(id: @selected_reviewee_group_ids[1])
+          allow_any_instance_of(PeerReview).to receive(:has_marks_or_annotations?).and_return(true)


Overall, don't rely on mocks in this test suite. You can update the test objects directly to create marks/annotations for the relevant result(s).

… issue-7119

…s_or_annotations logic, simplified unassign return, fixed repeated testcase descriptions

…han mock

… issue-7119

hemmatio · 2024-11-07T04:26:13Z

Not sure why these precommit tests are failing, they're failing in the exact same way on other member's PRs as well. None of my changes should have caused these failures.

… issue-7119

david-yz-liu

@hemmatio great work!

hemmatio added 7 commits October 9, 2024 23:00

Rubocop fixes 3

6255df2

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

2e2abfc

… issue-7119

rubocop fixes

a91abc0

normalized i18n keys in locale file

1fb0715

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

f875fef

… issue-7119

fixed grammar error in success flash

da725bb

hemmatio marked this pull request as ready for review October 23, 2024 21:36

hemmatio requested a review from david-yz-liu October 23, 2024 21:37

Updated changelog.md

d07e887

hemmatio marked this pull request as draft October 23, 2024 21:48

hemmatio marked this pull request as ready for review October 23, 2024 21:48

hemmatio removed the request for review from david-yz-liu October 23, 2024 21:49

hemmatio marked this pull request as draft October 23, 2024 22:13

Created testcases to improve coverage for peer_review.rb and peer_rev…

0c18900

…iews_controller.rb

hemmatio marked this pull request as ready for review October 24, 2024 03:19

Process entire row selection to be treated as individual checkbox sel…

3d0b8dc

…ections (Previously row selections bypassed the necessary checks)

david-yz-liu reviewed Oct 24, 2024

View reviewed changes

hemmatio marked this pull request as draft October 28, 2024 22:36

hemmatio added 5 commits October 28, 2024 18:44

Made changes to existing code to remove unneccessary error flash case…

14a9eab

…, added before_destroy callback to peer_review deletion, simplified some inefficient code

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

a22e793

… issue-7119

mocked peer review deletion and confirmed with database in testcases

f263045

added more comprehensive testcases for has_marks_or_annotations? and …

016e382

…check_marks_or_annotations

normalized i18n data for peer_reviews/en.yml

88c79f7

hemmatio marked this pull request as ready for review October 31, 2024 07:10

hemmatio requested a review from david-yz-liu October 31, 2024 07:38

david-yz-liu reviewed Nov 4, 2024

View reviewed changes

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

ca08735

… issue-7119

hemmatio marked this pull request as draft November 5, 2024 03:59

hemmatio added 4 commits November 4, 2024 23:04

Updated additional not shown message to contain count, fixed has_mark…

d96111b

…s_or_annotations logic, simplified unassign return, fixed repeated testcase descriptions

Updated testcases in peer_reviews_controller_spec.rb to stub rather t…

8049e27

…han mock

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

7f22161

… issue-7119

Re-triggering precommit tests

ac4538f

hemmatio added 3 commits November 7, 2024 01:41

removed unnecessary bang method

253e11d

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

57b1fa1

… issue-7119

pulled precommit test fixes

cc98c20

hemmatio marked this pull request as ready for review November 11, 2024 14:20

hemmatio marked this pull request as draft November 11, 2024 14:45

normalized i18n keys

68d1e18

hemmatio marked this pull request as ready for review November 11, 2024 17:17

david-yz-liu approved these changes Nov 12, 2024

View reviewed changes

david-yz-liu merged commit 209f67f into MarkUsProject:master Nov 12, 2024
8 checks passed

hemmatio deleted the issue-7119 branch November 13, 2024 03:37

hemmatio mentioned this pull request Nov 14, 2024

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

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent deletion of graded peer reviews #7263

Prevent deletion of graded peer reviews #7263

hemmatio commented Oct 10, 2024 •

edited

Loading

hemmatio commented Oct 23, 2024

coveralls commented Oct 23, 2024 •

edited

Loading

david-yz-liu Oct 24, 2024

david-yz-liu Oct 24, 2024

david-yz-liu Oct 24, 2024

david-yz-liu Oct 24, 2024

david-yz-liu Oct 24, 2024

david-yz-liu Oct 24, 2024

david-yz-liu Oct 24, 2024

david-yz-liu Oct 24, 2024

david-yz-liu Oct 24, 2024

david-yz-liu Oct 24, 2024

david-yz-liu Nov 4, 2024

david-yz-liu Nov 4, 2024

david-yz-liu Nov 4, 2024

david-yz-liu Nov 4, 2024

hemmatio Nov 5, 2024 •

edited

Loading

david-yz-liu Nov 4, 2024

hemmatio commented Nov 7, 2024

david-yz-liu left a comment

Prevent deletion of graded peer reviews #7263

Prevent deletion of graded peer reviews #7263

Conversation

hemmatio commented Oct 10, 2024 • edited Loading

Proposed Changes

Type of Change

Checklist

Questions and Comments

hemmatio commented Oct 23, 2024

coveralls commented Oct 23, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11783156360

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemmatio Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemmatio commented Nov 7, 2024

david-yz-liu left a comment

Choose a reason for hiding this comment

hemmatio commented Oct 10, 2024 •

edited

Loading

coveralls commented Oct 23, 2024 •

edited

Loading

hemmatio Nov 5, 2024 •

edited

Loading