Goals-oriented Merge Request review mode
I think we should encourage a different style of review that focuses on defining goals, submitting changes that meets those goals, and then branching off for follow-up work if - in that process - something could be cleaned up or improved.
Goals-Oriented Code Review
The Goals-Oriented Code Review (GOCR) would change the MR authorship process to ask an author to create a list of goals that their MR hopes to accomplish.
Take an example MR - "Swap legacy classes for standardized GitLab UI classes". In this case, no further goals are necessary. Presumably, every line of code diff in the MR will support the stated title. However, imagine a more complex MR like "Add the ability to navigate among commits in an MR."
In a more complex scenario, the author might break it down into goals like:
- Load commit data from the API
- Insert previous and next commits into the Vuex store
- Provide commit navigation data to the Diffs app
- Show previous and next commit buttons when the appropriate data is present
- Test previous/next commit navigation feature
For these more complex MRs, the critical question is: "Do these changes accomplish this goal?" Without a GOCR, it's not always clear what the author's sub-goals are, which leads to out-of-control bike-shedding.
In this scenario, the author should be prompted to classify their code according to their goals (of course, in the simpler scenario, all of the code is simply classified as in service of the MR title). The author selects files (MVP) or lines ("finished") and assigns them to one of their goals. Once they are happy, they can assign to a reviewer.
The Review
During review, the reviewer is asked to step through the goals in order. I think it makes sense to go through in the order the author created them so that - with enough planning - the author can create a cohesive story about every line of code. In any case, the reviewer is walked through each goal and for each file (MVP) or cluster of lines ("finished") they are asked "Did this change help accomplish the stated goal?"
The reviewer is given binary options: Yes or no. As an added feature enhancement, they may be able to indicate they believe the entire goal is either unnecessary or unclear. If they answer "Yes" to every change for every goal, the MR is implicitly approved and their job is done. For any changes they mark as "no", they can be asked at the end to clarify with a comment why it doesn't accomplish the stated goal, or, they can have the option to change their answer.
Once they are done (method TBD; a button? Just implicit by answering every goal?), the reviewer has an option to click a few buttons: "I want to open a follow-up issue regarding code style", "I need to prevent this MR from moving forward for a security reason", "I need to prevent this MR from moving forward for a performance reason". I'm not quite sure how to handle the last two options, but the former obviously opens a new issue, linked to the MR.
Mocks
https://wireframeapp.io/app/preview/816aa4ae
The Authoring UI
The Reviewing UI
Finalizing a Review
