#4081 reviewers from past review rounds are now valid review assignment choices.

[ajrbyers]

This PR tweaks the logic by no longer excluding the past_reviewers when calling get_reviewer_candidates. They are instead filtered out at the end when setting reviewers in the context argument. This means that past_reviewers remain valid reviewers but don't display twice in the review list.

Closes #4081

[joemull]

The logic fix is working but the display is still a bit confusing, because the table is missing a few cells on the empty row:

I got it to load properly by adding two tds:

diff --git a/src/templates/admin/review/add_review_assignment.html b/src/templates/admin/review/add_review_assignment.html
index 01652349..82d38cd4 100644
--- a/src/templates/admin/review/add_review_assignment.html
+++ b/src/templates/admin/review/add_review_assignment.html
@@ -75,6 +75,8 @@
                                 <td></td>
                                 <td></td>
                                 <td></td>
+                                <td></td>
+                                <td></td>
                             </tr>
                         {% endfor %}
                         </tbody>

But this still is still not quite accurate because the "empty" notice is displaying when the options are not empty. Can the set of reviewers be fixed a bit more comprehensively in the view using querysets and copied querysets if needed, so the template doesn't have to do so much logic? You could use queryset annotation to create the variables needed in the template that indicate whether the reviewer is suggested or past.

[ajrbyers]

I've moved the setting of is_past_reviewer and is_suggested_reviewer over to optional annotations on the func that gets reviewer candidates.

I resolved the colspan issue by setting the colspan to 10, this ensures that if the table is 6, 7 or 8 cols wide the empty message will span all of the columns (this is supported cross browser so long as we don't set the table layout to fixed).

Both settings off:

Past Reviewers on:

Both settings on:

[mauromsl]

Some questions/comments inline, but certainly much (c)leaner than it was!

[mauromsl]

Thanks for this! Added one last comment I think would be good to make the logic even simpler and also reduce config bloat

[mauromsl]

Just a comment with an intended typo, but otherwise good!

[joemull]

This is a thing of beauty! I tested it out and everything is working perfectly. The extra help text is nice too. The only thing I'd tweak are two comma splices:

...this process is silent, so they will not...

...will appear at the top of this list. They will show with...