Skip to content

feat: Allow review rejections to be undone#6312

Merged
rjsparks merged 7 commits into
ietf-tools:mainfrom
pselkirk:feat-5978
Sep 27, 2023
Merged

feat: Allow review rejections to be undone#6312
rjsparks merged 7 commits into
ietf-tools:mainfrom
pselkirk:feat-5978

Conversation

@pselkirk

Copy link
Copy Markdown
Contributor

Addresses most of #5978 and part of #6169

@rjsparks

Copy link
Copy Markdown
Member

There's at least one rough edge (and this all needs tests).

I ran through having cellar-flac assigned to me, and then rejecting/accepting/rejecting a few times. In my dev instance I've rejected the assignment there are no other assignements, and the open assignments management page doesn't allow the secretary to assign it to me again. The request, as I understand it, is to have me show up on the list here, but marked "has previously rejected reviewing this doc":

image

(right side truncated to not overshare current states)

@rjsparks

Copy link
Copy Markdown
Member

@russhousley @ajeanmahoney @kivinen

This is what the assignment page will look like for a reviewer that has rejected a review before. This is Russ' view after both of us have been assigned and rejected the document.

Is the UI here intuitive enough? And if you have this as secretaries (if you were logged in, Jean, you would see accept buttons for both of us), would you be ok not having the person show up in the list above?

image

… of suggested reviewers.

This largely un-does d105f8b, at the request of at least one team secretary.
@pselkirk pselkirk changed the title feat: Allow reviewer to accept a review they're previously rejected feat: Allow reviewer to accept a review they've previously rejected Sep 20, 2023
@codecov

codecov Bot commented Sep 20, 2023

Copy link
Copy Markdown

Codecov Report

Merging #6312 (c86b85d) into main (32bb4ef) will decrease coverage by 0.02%.
Report is 47 commits behind head on main.
The diff coverage is 61.53%.

@@            Coverage Diff             @@
##             main    #6312      +/-   ##
==========================================
- Coverage   88.69%   88.67%   -0.02%     
==========================================
  Files         290      290              
  Lines       40390    40428      +38     
==========================================
+ Hits        35823    35851      +28     
- Misses       4567     4577      +10     
Files Coverage Δ
ietf/doc/views_review.py 95.16% <100.00%> (+0.20%) ⬆️
ietf/review/utils.py 92.13% <75.00%> (+0.03%) ⬆️
ietf/review/policies.py 97.47% <50.00%> (-1.43%) ⬇️

... and 16 files with indirect coverage changes

one_assignment still has to exclude reviewers who rejected the assignment,
or they could end up being the suggested reviewer.
@pselkirk pselkirk changed the title feat: Allow reviewer to accept a review they've previously rejected feat: Allow review rejections to be undone Sep 20, 2023
@ajeanmahoney

Copy link
Copy Markdown
Collaborator

I would typically work from the list on the Manage unassigned open review requests page rather than an individual review request page. As secretary, I would expect to see all available reviewers.

On the individual review request page, the secretary is reassigning to Russ, yes?

Maybe add a date to "Assignment rejected" under the reviewer name, e.g., "Assignment previously rejected 18 Sept 2023". Maybe there should be a Reject again button?

Comment thread ietf/review/policies.py Outdated
assignment.assigned_on = timezone.now()
assignment.save()
return assignment
return review_req.reviewassignment_set.create(state_id='assigned', reviewer=reviewer, assigned_on=timezone.now())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider putting this in an else: clause

@pselkirk
pselkirk marked this pull request as ready for review September 26, 2023 22:21
@rjsparks
rjsparks merged commit c718fed into ietf-tools:main Sep 27, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants