fix: Do not add user to the top of queue after reject (fixes #4505) #5427

kivinen · 2023-03-26T04:14:06Z

Added a checkbox in the reject review dialog to ask whether user wants to be added to the top of the queue or not. Default is off.

…ls#4505) Added a checkbox in the reject review dialog to ask whether user wants to be added to the top of the queue or not. Default is off.

Set the request_assignment_next also in LeastRecentlyUsedReviewerQueuePolicy so it can be used to override the assignment policy rules (i.e., if someone has once per month, but he rejects review with wants_to_be_next set to true, he will get new assignment immediately, not after one month).

rjsparks · 2023-03-28T03:59:04Z

The test fails are because of
https://github.com/kivinen/datatracker/blob/fix-4505/ietf/review/tests_policies.py#L512
and
https://github.com/kivinen/datatracker/blob/fix-4505/ietf/review/tests_policies.py#L728

But consider changing

def return_reviewer_to_rotation_top(self, reviewer_person, wants_to_be_next):

to

def return_reviewer_to_rotation_top(self, reviewer_person, wants_to_be_next=True):

With that change, the above two lines (#L512 and #L728) would not need to change.

ietf/review/policies.py

ietf/doc/views_review.py

kivinen · 2023-03-28T08:59:13Z

Robert Sparks writes:

If you check in what is now broken I can help diagnose better.

Done already.

I don't see how making the parameter optional removes the ability to test the feature. You're in control of what the tests provide when they call the method...

Perhaps, but I would like to also know why the tests are failing even when tests are updated. -- ***@***.***

rjsparks · 2023-03-28T09:32:51Z

Ok - found it. The confusion is in the similarly named things.

The failure you are seeing now is because you changed the signature of the test functions. Note the error

TypeError: test_return_reviewer_to_rotation_top() missing 2 required positional arguments: 'reviewer' and 'wants_to_be_next'

It's not the return_reviewer_to_rotation_top function that's being complained about - it's the definition of the test_return_reviewer_to_rotation_top function.

The test framework searches classes derived from TestCase, looking for methods that have names that start with test_ and calls them with no arguments other than self.

rjsparks · 2023-03-28T09:34:21Z

ietf/review/tests_policies.py

@@ -507,9 +507,9 @@ def test_default_reviewer_rotation_list_with_nextreviewerinteam(self):
        rotation = self.policy.default_reviewer_rotation_list()
        self.assertEqual(rotation, available_reviewers[2:] + available_reviewers[:1])

-    def test_return_reviewer_to_rotation_top(self):
+    def test_return_reviewer_to_rotation_top(self, person, wants_to_be_next):


The signature here should be just (self).

rjsparks · 2023-03-28T09:37:41Z

ietf/review/tests_policies.py

        reviewer = self.append_reviewer()
-        self.policy.return_reviewer_to_rotation_top(reviewer)
+        self.policy.return_reviewer_to_rotation_top(reviewer, wants_to_be_next)


Here, you need to choose a value for wants_to_be_next and pass it in. Call this twice (appending a new reviewer each time), once with False and once with True and check that the right thing happened with likes like what are at 513 below.

It would be good to add tests like that to the other policy test as well

Ok fixed now, and now the tests passes. I test both the wants to be next cases, in both policies. Actually this function is works identically for both policies.

self. Added test cases for test_return_reviewer_to_rotation_top both for RotateAlphabeticallyReviewerQueuePolicyTest and LeastRecentlyUsedReviewerQueuePolicyTest.

rjsparks · 2023-03-29T01:23:13Z

ietf/review/tests_policies.py

+        reviewer = self.append_reviewer()
+        self.policy.return_reviewer_to_rotation_top(reviewer, False)
+        self.assertFalse(self.reviewer_settings_for(reviewer).request_assignment_next)


Between 731 and 732, I think you need to make a new reviewer for the test (by repeating what's at 729.
That should happen both here and for the other policy.

Why? The first call to return_reviewer_to_rotation_top with parameter False, does not really do anything, it will not modify the request_assignment_next in any way, so it should still be false after that first call. Then on the second call we call it with wanting to be next set to True, and that will cause request_assignment_next to be set in both policies, and then we check that it is now set.

so the unit here is badly named now if all we're doing is setting and testing the "assign me next" bit. If the implementation is really only setting that bit and no other queue manipulation is made, then we should rename this. Otherwise, we'd be missing something moving someone who is already at head of queue (rotation top) to the head of the queue.

I'll add an issue to make that change later.

I agree that it is badly named. I think this is artifact that there is no clearly defined method of moving someone on the top, other than asking to be next. This function is called in two different situations, once when review request is rejected, and in that case I added the checkbox. It is also called when the document is withdrawn, and in that case this function will be called with wants_to_be_first set to true, as this happens when secretary messed up and assigned something that should not have been assigned, thus it is logical that the reviewer will get new assignment as soon as possible. On the other hand we could add similar checkbox to that location, but as that thing is only used by secretary and very rarely anyways, it might not be needed.

codecov · 2023-03-29T01:30:33Z

Codecov Report

Merging #5427 (6c887ea) into feat/postgres (736382a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@                Coverage Diff                @@
##           feat/postgres    #5427      +/-   ##
=================================================
- Coverage          88.80%   88.79%   -0.01%     
=================================================
  Files                285      285              
  Lines              39525    39538      +13     
=================================================
+ Hits               35100    35108       +8     
- Misses              4425     4430       +5

Impacted Files	Coverage Δ
ietf/doc/views_review.py	`94.96% <100.00%> (+<0.01%)`	⬆️
ietf/review/policies.py	`98.89% <100.00%> (+0.01%)`	⬆️

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rjsparks · 2023-03-29T23:30:45Z

ietf/review/tests_policies.py

+        reviewer = self.append_reviewer()
+        self.policy.return_reviewer_to_rotation_top(reviewer, False)
+        self.assertFalse(self.reviewer_settings_for(reviewer).request_assignment_next)


so the unit here is badly named now if all we're doing is setting and testing the "assign me next" bit. If the implementation is really only setting that bit and no other queue manipulation is made, then we should rename this. Otherwise, we'd be missing something moving someone who is already at head of queue (rotation top) to the head of the queue.

rjsparks · 2023-03-29T23:33:28Z

ietf/review/tests_policies.py

+        reviewer = self.append_reviewer()
+        self.policy.return_reviewer_to_rotation_top(reviewer, False)
+        self.assertFalse(self.reviewer_settings_for(reviewer).request_assignment_next)


I'll add an issue to make that change later.

kivinen added 2 commits March 26, 2023 13:11

fix: Do not add user to the top of queue after reject (fixes ietf-too…

c48582e

…ls#4505) Added a checkbox in the reject review dialog to ask whether user wants to be added to the top of the queue or not. Default is off.

rjsparks requested changes Mar 28, 2023

View reviewed changes

ietf/review/policies.py Show resolved Hide resolved

ietf/doc/views_review.py Show resolved Hide resolved

Added wants_to_be_next to test cases too.

2791040

rjsparks requested changes Mar 28, 2023

View reviewed changes

Fixed test function prototypes, they can't have any parameters, only

6c887ea

self. Added test cases for test_return_reviewer_to_rotation_top both for RotateAlphabeticallyReviewerQueuePolicyTest and LeastRecentlyUsedReviewerQueuePolicyTest.

rjsparks requested changes Mar 29, 2023

View reviewed changes

rjsparks approved these changes Mar 29, 2023

View reviewed changes

rjsparks merged commit d5bae45 into ietf-tools:feat/postgres Mar 29, 2023

github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Do not add user to the top of queue after reject (fixes #4505) #5427

fix: Do not add user to the top of queue after reject (fixes #4505) #5427

kivinen commented Mar 26, 2023

rjsparks commented Mar 28, 2023 •

edited

Loading

kivinen commented Mar 28, 2023 via email

rjsparks commented Mar 28, 2023

rjsparks Mar 28, 2023

rjsparks Mar 28, 2023

kivinen Mar 29, 2023

rjsparks Mar 29, 2023

kivinen Mar 29, 2023

rjsparks Mar 29, 2023

rjsparks Mar 29, 2023

kivinen Mar 30, 2023

codecov bot commented Mar 29, 2023

rjsparks Mar 29, 2023

rjsparks Mar 29, 2023

fix: Do not add user to the top of queue after reject (fixes #4505) #5427

fix: Do not add user to the top of queue after reject (fixes #4505) #5427

Conversation

kivinen commented Mar 26, 2023

rjsparks commented Mar 28, 2023 • edited Loading

kivinen commented Mar 28, 2023 via email

rjsparks commented Mar 28, 2023

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

codecov bot commented Mar 29, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjsparks commented Mar 28, 2023 •

edited

Loading