As a user, I want to be able to easily tell which of my review request DM's are still outstanding versus complete

[abinoda]

Requested by @douglasmamilor: https://twitter.com/DouglasMamilor/status/1043218531536527360

Also requested by @logicbomb421

We were wondering if it would be possible for Pull Reminders to notify us when another member of a team satisfies a pull request?

Currently our flow looks like:
PR review requested for TeamA -> everyone in TeamA gets a slack notification -> someone from TeamA approves on behalf of the team.

However, this leads to a lot of us opening PRs that another team member have signed off on. Wondering if it would be possible to trigger a notification back to the team when a member approves saying such? For instance, the above flow would then look like this:

[abinoda]

@douglasmamilor @cschultztech @mwarkentin @mmontreuil @JBKahn @nickpresta Hi all – I'm working on adding a /pullreminders slash command #72 and I'm hoping that it addresses this problem as well.

The slash command would allow you to get an up to date list of your assigned PRs whenever you're getting around to doing some reviews. This way you don't need to wade your way through your backlog of real-time notifications.

Let me know your thoughts. The Slash command should be released in the next few days.

[RothAndrew]

Closely related to this: I've been using the "PRs" page on the PullReminders website frequently. It would be great to be able to filter out all PRs that I've already approved.

[abinoda]

@RothAndrew I'm curious whether the new Slash command can be a solution to this? Whenever you want to do some code reviews you can run /pullreminders to get the up-to-date list. I'd love to hear what you think.

[RothAndrew]

@abinoda does that work now? typing /pullreminders in slack results in:

*/pullreminders* is not a valid command. In Slack, all messages that start with the "/" character are interpreted as commands.

If you are trying to send a message and not run a command, try preceding the "/" with an empty space.

[abinoda]

@RothAndrew Will follow-up with you over email. I sent out a mass email with instructions on enabling the command but the settings were messed up and it may have gone to your spam folder.

P.S. @douglasmamilor @cschultztech @mwarkentin @mmontreuil @JBKahn @nickpresta I'm curious if you guys have tried the new /pullreminders command and whether it resolves this issue or not!

[ashanbrown]

I'm lazy and don't really want to have to type /pullreminders. ;)

What I mentioned in my linked PR is:

I went to #pull-reminders in slack and saw that I had received a PR review request. I opened it and it had already been reviewed and merged. It would be cool if the DM could be updated with the review status and whether it has been merged. It could be done with just a checkmark icon next to it, and whatever github uses to note merged PRs. Someone else had commented that they'd like to see whether a PR has passed CI. This is also potentially something that could be annotated later on a DM.

I believe this is all possible within slack but it does require you to keep track of DMs that have been sent. I do think 24 hours would be plenty for a feature like this.

[abinoda]

@ashanbrown Laziness acknowledged ;)

You mentioned #pull-reminders (a team reminder) then referred to DMs. Could you clarify where you'd like to see this behavior (channel reminder, DM reminder, or DM real-time alert)?

[ashanbrown]

Sorry, I meant the dm.

[abinoda]

This feature is released 🎉 Once a review request is fulfilled, the original DM notification becomes crossed out:

cc: @douglasmamilor @cschultztech @mwarkentin @mmontreuil @JBKahn @nickpresta

[molynerd]

@abinoda just tried it out. first impressions are that it works really well, and will definitely reduce some PR review times on our side. we'll update you if anything changes. Thanks!

[abinoda]

@molynerd Good to hear! @ashanbrown Let me know how it's working for you once you see it in action.

[abinoda]

This is out of closed beta and now in effect for all accounts.

[RothAndrew]

@abinoda it works, but there's one unfortunate side effect that I'm seeing.

On my project, all PRs require 2 reviews to be merged. If I send a review request to a team, rather than an individual person, it gets crossed out for everyone after one person reviews it.

[ashanbrown]

Works awesome for me, but Andrew’s concern might be resolved just by adding a check mark next to it for each review. I assume requiring multiple reviews is not a github feature itself. I am curious to see how it works when we have two reviewer groups assigned. Is it considered resolved when my review is no longer needed or when the PR is approved by all groups?

…

On Tue, Feb 26, 2019 at 8:03 AM Andrew Roth ***@***.***> wrote: @abinoda <https://github.com/abinoda> it works, but there's one unfortunate side effect that I'm seeing. On my project, all PRs require 2 reviews to be merged. If I send a review request to a team, rather than an individual person, it gets crossed out for everyone after one person reviews it. [image: screen shot 2019-02-26 at 10 50 07 am] <https://user-images.githubusercontent.com/16000938/53427050-2dfe9180-39b6-11e9-9fbf-e868086660f4.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#73 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABxZ_mDlAla3Pfu5UgeWFGCHmIJXEOVMks5vRVrngaJpZM4W00iy> .

[RothAndrew]

@ashanbrown It is actually a github feature. Here's a screenshot:

[abinoda]

@RothAndrew Interesting side effect. Thank you for sharing.

I'm open to your suggestions for solutions. We determine that a review request is fulfilled using GitHub's API, which, similar to their UI, treats a team review request as completed regardless of the "Required approving reviews" setting. So without some crazy logic on our end we won't be able to change this behavior.

One of my initial reactions was that your team should check out Pull Assigner, which would solve this problem by converting team review requests into two explicit individual review requests. Have you looked into this?

Another option would be for your team to have the first reviewer re-request another review from the team after they submit a review, this way you'd be keeping GitHub's pull request state accurate with where things stand based on your workflow. This would also benefit the reminders since they suffer from the same issue I described earlier where a single review will cancel out the team review request.

Let me know your thoughts.

[mwarkentin]

@abinoda which API are you using? I believe they might have multiple values - IIRC there's a difference between "approved" (which I think happens after a single approval) and "mergeable" which happens when all protected branch settings are matched (including multiple approvals). Might work if you switch to that? Although that seems like it'd also pull in things like CI success into the equation..

[mwarkentin]

Here's a similar issue on another tool: runatlantis/atlantis#43

[abinoda]

@mwarkentin This endpoint – https://developer.github.com/v3/pulls/review_requests/#list-review-requests. But this isn't an API issue, it's how GitHub works even in its UI. Once a single person from a team has responded to a team review request, that review request is considered fulfilled.

FWIW, whether a pull request is "Approved" or whether or not a review request exists are two separate things. For this feature we're just looking at whether a specific review request (for which we send a DM notification for) has been fulfilled or not. It has nothing to do with whether the PR has been approved.

@mwarkentin Out of curiosity are you running into any issues yourself with this feature?

[mwarkentin]

Nope, seems to be working as expected, but I don't think we have any repos where we require >1 approval before merge - I can see how that might be a bit misleading in that case.

[abinoda]

@mwarkentin Yeah – the problem Andrew is running into only happens when using TEAM review requests coupled with a policy of requiring more than one review from that team.

I think our new tool Pull Assigner could be the ideal solution for Andrew along with other benefits that the tool offers.

Otherwise, if we could change GitHub, the solution would be for team review requests to not get marked as fulfilled until a pull request is approved or reviewed twice. Or to automatically re-request the team after the first review.

We could achieve something like this using a GitHub Action if it came down to it. We've created Actions for customers in other unique situations:

• https://github.com/pullreminders/label-when-approved-action
• https://github.com/pullreminders/assignee-to-reviewer-action

[RothAndrew]

@abinoda I can't think of a workaround. I agree the way Github does reviews when it comes to teams is clunky.

Internally, for my team, I think we're just going to say "Stop requesting reviews from teams". I've seen the Pull Assigner functionality, but that doesn't quite fit what we want.

[abinoda]

@RothAndrew Sounds good – will email you separately about Pull Assigner b/c I'd like to understand that better.