-
Notifications
You must be signed in to change notification settings - Fork 637
fix: simplify call for adoption workflow #9964
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: simplify call for adoption workflow #9964
Conversation
jennifer-richards
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops - I think I have a question about authorization. See inline
ietf/doc/views_draft.py
Outdated
| doc.type_id != "draft", | ||
| group is None, | ||
| not is_doc_ietf_adoptable(doc), | ||
| doc.group.acronym == "none" and acronym is None, | ||
| doc.group.acronym == "none" and not can_adopt_draft(request.user, doc), | ||
| doc.group.acronym != "none" | ||
| and not is_authorized_in_doc_stream(request.user, doc), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc may not be in the "ietf" stream at this point - is there a check that the user is authorized to issue the adoption in that stream? It looks to me like it checks its current stream, but that might be a different stream. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the document is not in any stream, it will be attached to the group with acronym 'none' and can_adopt_draft does the checking you are looking for.
If the document is in some stream that is not the ietf stream, the group will not be of type wg and the query at L2080 will return None, so L2084 will reject access.
If the document is in the ietf stream, the awkwardly named is_authorized_in_doc_stream will perform the check you are looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restructured the condition to hopefully make this easier to follow.
The differences between can_adopt_draft and is_authorized_in_draft_stream could probably use further refactoring, but I don't think that's a job for this PR.
jennifer-richards
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for the explanation, refactor makes the logic a lot clearer
No description provided.