WIP for denying responses#2308
Closed
GCHQDeveloper36 wants to merge 2 commits intogchq:mainfrom
Closed
Conversation
Member
|
Closing for now, this can be looked at in the future |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently reviewerss can 'request changes' or 'approve' when responding to review, but there isn't any way to 'deny' and indicate that the request is unlikely to go any further regardless of changes provided by the submitter.
The most obvious addition is the 'Deny' button when on the response page. To avoid confusion, I've adjusted the colours of the 'request changes' and 'approve' buttons to more clearly highlight the difference between the actions. Definitely room for improvement here in the future. In particular, we're adding yet more frontend-filtering to figure out whether a review is 'archived' or not, which we still want to move to the backend.
There's also some work to categorise 'approve' and 'deny' as 'finalised' states which, whilst not 'locked', would usually indicate a role has provided their final response to the review. We could use this in the future to soft-lock the decision and warn the reviewer if they try to undo or otherwise change their review from a 'final' state into something else.
At the backend, there's some work to provide a 'summarised' response from the responses so far. The main path is from findReviews (used by the frontend) which will now also provide a 'decision' in the response. This decision is calculated by finding the 'latest' response for each role associated with a review, and using some basic logic to figure out what should be classed as the 'current' response for that role.
Still in draft/WIP - there's another commit to come which improves the backend filtering.