-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Remove automatically suggested document for document that is already has review request (fixes #3211) #5425
Conversation
in question, ignore the automatic suggestion for that document. Fixes ietf-tools#3211.
Codecov Report
@@ Coverage Diff @@
## main #5425 +/- ##
=======================================
Coverage 88.67% 88.67%
=======================================
Files 288 288
Lines 40001 40003 +2
=======================================
+ Hits 35471 35473 +2
Misses 4530 4530
|
Robert Sparks writes:
@rjsparks commented on this pull request.
In ietf/review/utils.py:
> @@ -588,10 +588,12 @@ def blocks(existing, request):
and existing.reviewassignment_set.filter(state_id__in=("assigned", "accepted")).exists()
and (not existing.requested_rev or existing.requested_rev == request.doc.rev))
request_closed = existing.state_id not in ('requested','assigned')
+ # Is there a review request for this document already in system
+ requested = existing.state_id in ('requested')
Would it get the same result in the cases you are testing if you added and
existing.type_id == request.type_id here, or is it specifically existing, say,
early assignments that cause problems when LC is requested?
Both can happen I think. I.e., I might have early review request that
got rejeced, but then the document went to LC, and got autosuggested,
thus there is old rejected early review that is open as a request, and
there is new LC request. I do not really care about the review type, I
only care that the review gets done at some point...
Same thing with LC review that got rejected and then it might already
be in the telechat.
--
***@***.***
|
I want to add some tests to this before merging, but it will be week after next. |
Robert Sparks writes:
I want to add some tests to this before merging, but it will be week
after next.
Thats ok, as the secretaries are already aware of this bug, and will
most likely still catch those cases before doing duplicate
assignments, so few weeks delay is not an issue.
--
***@***.***
|
I am very sorry that I haven't gotten the remaining tests in and have this into main already. It will be my initial sprint task if it's not done before then. |
@kivinen - Please review the additional condition I added (and tested). |
Added check that if there is already review request for the document in question, ignore the automatic suggestion for that document. Fixes #3211.