Page MenuHomePhabricator

Don't consider accepting on behalf of valid-but-accepted reviewers to be a validation error
ClosedPublic

Authored by epriestley on May 25 2017, 9:19 PM.
Tags
None
Referenced Files
F15576533: D18019.id43341.diff
Tue, May 6, 9:28 AM
F15575822: D18019.id43341.diff
Tue, May 6, 7:06 AM
F15532335: D18019.id43341.diff
Wed, Apr 23, 3:35 PM
F15530056: D18019.diff
Wed, Apr 23, 1:05 AM
F15528058: D18019.id43343.diff
Tue, Apr 22, 8:36 AM
F15519125: D18019.id.diff
Sat, Apr 19, 8:05 PM
F15516686: D18019.diff
Fri, Apr 18, 8:40 PM
F15481065: D18019.id43343.diff
Tue, Apr 8, 5:55 PM
Subscribers

Details

Summary

Fixes T12757. Here's a simple repro for this:

  • Add a package you own as a reviewer to a revision you're reviewing.
  • Open two windows, select "Accept", don't submit the form.
  • Submit the form in window A.
  • Submit the fomr in window B.

Previously, window B would show an error, because we considered accepting on behalf of the package invalid, as the package had already accepted.

Instead, let repeat-accepts through without complaint.

Some product stuff:

  • We could roadblock users with a more narrow validation error message here instead, like "Package X has already been accepted.", but I think this would be more annoying than helpful.
  • If your accept has no effect (i.e., everything you're accepting for has already accepted) we currently just let it through. I think this is fine -- and a bit tricky to tailor -- but the ideal/consistent beavior is to do a "no effect" warning like "All the reviewers you're accepting for have already accepted.". This is sufficiently finnicky/rare (and probably not terribly useful/desiable in this specific case)that I'm just punting.
Test Plan

Did the flow above, got an "Accept" instead of a validation error.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lvital added a subscriber: lvital.
lvital added inline comments.
src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
20–25

👍

This revision is now accepted and ready to land.May 25 2017, 9:22 PM
This revision was automatically updated to reflect the committed changes.