## Motivation
Code review efficiency is essential for increasing Dropbox engineering productivity. The current code review process can be updated to formalize and encourage (when appropriate) an existing best practice already in use today at Dropbox.
## Current State
Currently, when reviewing a diff, the reviewer has 2 options: Accept and Request changes. While these options indeed work in many cases, often enough, the general idea as reflected in the diff is correct, but there are some changes to be made. The reviewer trusts the author to implement the changes correctly without reviewing it another time, realizing such extra review will be neither necessary or productive.
## Current Workarounds
In that case, the current 2 options [Accept, Reject] do not fit, and the reviewer is often tempted to use a ‘conditional’ Accept. Meaning the diff is accepted provided that the author will make those changes. This has the drawback that the author could accidentally miss the reviewers comment and land the diff as is.
Another workaround involves working with remote offices. The reviewer prefer not to block the author, so they resign as a reviewer and suggest someone from the author’s office as reviewer. This has two big issues. First, the commit message will not have the original reviewer on it, so if there’s a prod issue, or someone is just going through git log for context, they wouldn’t see who the actual reviewer was. Second, the diff doesn’t appear in the author’s queue but in the new reviewer’s queue. And the next action should be taken by the author.
It’s debatable, but it seems that the above behavior described in those workarounds is a good practice, and might be the preferred solution to the problem presented above. This is due to reducing non-productive iterations from the review process.
## Proposed Solution
This situation above occurs often enough, and if we want to encourage and formalize this practice, then it would be useful to integrate this solution into the Phabricator tool. This can be implemented as a new state ‘conditional accept’ that is considered to be an accepted state by the arc diff tool. As an extra precaution from unintentional mistakes, arc land can prohibit landing a diff with conditional accept until a new change was introduced, or until all inline comments are marked as done, or both.
## Benefits
While this proposal has the potential to speed up the review and development process in general, it can have an even greater impact, when working with remote offices in opposite timezones. When each iteration on the diff can take a day, it can save an entire day in each diff (or even 2 days on a weekend).
Additionally, such approach amplifies a sense of empowerment, ownership, and autonomy of the author, all of which are desired outcomes.