fix: Remove automatically suggested document for document that is already has review request (fixes #3211)#5425
Merged
rjsparks merged 5 commits intoietf-tools:mainfrom Jul 23, 2023
Merged
Conversation
in question, ignore the automatic suggestion for that document. Fixes ietf-tools#3211.
rjsparks
reviewed
Mar 26, 2023
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
|
Contributor
Author
|
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.
--
***@***.***
|
Member
|
I want to add some tests to this before merging, but it will be week after next. |
Contributor
Author
|
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.
--
***@***.***
|
Member
|
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. |
Member
|
@kivinen - Please review the additional condition I added (and tested). |
jennifer-richards
approved these changes
Jul 11, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added check that if there is already review request for the document in question, ignore the automatic suggestion for that document. Fixes #3211.