search

databricks / spark-pr-dashboard

Dashboard to aid in Spark pull request reviews
spark-prs.appspot.com
Apache License 2.0
54 stars 46 forks source link

[WIP] Support 'review status' lifecycle / tagging of PRs based on who is blocking on review / updates #39

Closed JoshRosen closed 9 years ago

JoshRosen commented 9 years ago

The purpose of this PR is to start a discussion around whether we should implement features for tracking some sort of 'review status' lifecycle for PRs. My proposal is to extend the UI with features for conveying whether the review process is blocked on further review from reviewers or on an update from the PR's author. This could be very helpful for rapidly identifying which PRs need more review, since sometimes the most-recent part of the queue will be cluttered up with PRs where I've left a bunch of comments for an author and they take several days to address them.

I was thinking of adding a 'Review Status' column that's viewable by everyone but only editable by users with the proper permissions:

image

By default, all PRs start out in the 'Review needed' state. Once a reviewer feels that the PR is now blocked on the author's update, they may change the status to 'Update needed':

image

When a PR is in the 'Updated needed' state, its row will be slightly transparent (but restored to full opacity on hover):

image

On the backend, we can add some logic for automatically transitioning from 'Update needed' to 'Review needed': if a new commit is added to the PR, its title or description is updated, or the author leaves a comment, then we will automatically mark it as 'Review needed.'

The backend support for this would be helpful in identifying hanging / abandoned PRs.

The screenshots here are just a rough UI mockup that I threw together in a couple of minutes. I'd love feedback on the overall concept / workflow. Is this a useful feature? Should the subdued colors be used on the main page, or only on per-reviewer dashboards?

I also have a couple of React.js-related questions about how to handle data-binding / refresh logic, but I'll discuss those offline.

shivaram commented 9 years ago

@JoshRosen This looks great. You should probably post this to the dev mailing list to get more feedback. I think this will be pretty useful to filter out old PRs as you said and also for reviewers to figure out which PRs are ready for another look.

marmbrus commented 9 years ago

I would really like this.

Subdued colors by default with the option to filter entirely would be nice. Another thing to consider is never putting the PR into "review needed" if the title contains the text "WIP".

JoshRosen commented 9 years ago

Closing this for now to clear out my PR dashboard; will re-open later if I ever end up working on this.