Why are we pushing this check to Herald? Shouldn't the fact that it's accepted mean it's accepted?

Someone else who's not in the group could've accepted, and the group could be a non-blocking reviewer.

Specifically this should work as expected. If a blocking review is a group like Blessed Reviewers, the revision is not accepted until someone in that group accepts. Outside reviews do not satisfy the full acceptance of a revision. Are you seeing differently?

Oh, the group could be a non-blocking reviewer. I think moving a post-check to Herald doesn't solve the underlying problem of reviews being marked as accepted that shouldn't be. Can you go into more detail on what's going on there so we can identify the core problem?

Most likely T731 is what you want.

At our company, we have several interns employed. We want them to learn from each other, as well as from us, so we've tried to set up a workflow that makes the interns review each other. When an intern accepts the reviews, we want the supervisor of the author to have another look and give the final go.

Currently, we have this set up as a non-formal workflow. This means that one of the interns has to add the supervisor as a reviewer, after the first review step has been completed. But when this supervisor is added, the author could land the revision without approval of the supervisor and pass our current check of "Accepted Differential Revision exists" AND "Differential reviewers include any of <group supervisors>".

Before, we made Herald add the group of supervisors as blocking reviewers to each diff made by an intern. The downside of this was that it caused a flood of notifications to go to the supervisors, while they weren't even involved in that step of the review process.

We might need to change our non-formal workflow so the reviewing intern adds the supervisor as a blocking reviewer. This way, the author can't land the revision without approval. Still, we would prefer at least some formal fallback that prevents our interns from trying to land stuff without going through the entire workflow.

But the root problem is code reviews are getting accepted that shouldn't be, correct?

The downside of this was that it caused a flood of notifications to go to the supervisors

Could you do something like this?

• Create two projects, "Supervisors (Blocking)" and "Supervisors (Ready for Approval)".
• Supervisors are in both projects.
• Herald adds "Supervisors (Blocking)" as a blocking reviewer.
• Interns add "Supervisors (Ready for Approval)" as a reviewer once they're ready to complete the flow.
• If supervisors only want mail once things are ready, they can disable earlier mail in Supervisors (Blocking) → Members → Disable Mail.

Then your flow should be pretty much unchanged. I think the only cost is that you have to add members to two groups when supervisors change, which seems manageable, and explain the difference between the groups to interns, but you already have a fairly custom flow for that anyway.

(The proposed field also isn't particularly difficult to build, but I think it doesn't seem dramatically better than this flow offhand?)

In T11250#183164, @epriestley wrote:

The downside of this was that it caused a flood of notifications to go to the supervisors

Could you do something like this?

• Create two projects, "Supervisors (Blocking)" and "Supervisors (Ready for Approval)".
• Supervisors are in both projects.
• Herald adds "Supervisors (Blocking)" as a blocking reviewer.
• Interns add "Supervisors (Ready for Approval)" as a reviewer once they're ready to complete the flow.
• If supervisors only want mail once things are ready, they can disable earlier mail in Supervisors (Blocking) → Members → Disable Mail.

It would likely be possible to avoid even the cost to change supervisors in multiple locations by making one a sub-project of the other. The membership would remain the same across both that way.

This is awesome! I would've never found the ability to ignore emails from a project on my own. That's just the feature I needed to set this up correctly. Thanks guys for pointing me in the right direction.

Looks like I no longer have a use case for this request, so I guess we can close it.