Fix transaction handling in PhabricatorUserEditor->createNewUser()

Summary:
See https://github.com/facebook/phabricator/issues/117

  - The $user save can hit a duplicate key exception like the email, but we don't handle it correctly.
  - When the $user saves but the $email does not, the $user is left with a (rolled-back, invalid) ID. This makes the UI glitch out a bit. Wipe the ID if we abort the transaction.
  - We show the "Required" star marker even if the email is filled in.

The ID issue is sort of a general problem, but I think it's fairly rare: you must be doing inserts on related objects and the caller must catch the transaction failure and attempt to handle it in some way.

I can think of three approaches:

  - Manually "roll back" the objects inside the transaction, as here. Seems OK if this really is a rare problem.
  - Automatically roll back the 'id' and 'phid' columns (if they exist). Seems reasonable but maybe more complicated than necessary. Won't get every case right. For instance, if we inserted a third object here and that failed, $email would still have the userPHID set.
  - Automatically roll back the entire object. We can do this by cloning all the writable fields. Seems like it might be way too magical, but maybe the right solution? Might have weird bugs with nonwritable fields and other random stuff.

We can trigger the rollback by storing objects we updated on the transaction, and either throwing them away or rolling them back on saveTransaction() / killTransaction().

These fancier approaches all seem to have some tradeoffs though, and I don't think we need to pick one yet, since this has only caused problems in one case.

Test Plan: Tried to create a new user (via People -> Create New User) with a duplicate username. Got a proper UI message with no exception and no UI glitchiness.

Reviewers: btrahan, vrana, hgrimberg, hgrimberg01

Reviewed By: hgrimberg01

CC: aran

Differential Revision: https://secure.phabricator.com/D2650

src/applications/people/PhabricatorUserEditor.php

@@ -68,13 +68,16 @@ final class PhabricatorUserEditor {
     $this->willAddEmail($email);
 
     $user->openTransaction();
-      $user->save();
-
-      $email->setUserPHID($user->getPHID());
-
       try {
+        $user->save();
+        $email->setUserPHID($user->getPHID());
         $email->save();
       } catch (AphrontQueryDuplicateKeyException $ex) {
+        // We might have written the user but failed to write the email; if
+        // so, erase the IDs we attached.
+        $user->setID(null);
+        $user->setPHID(null);
+
         $user->killTransaction();
         throw $ex;
       }

src/applications/people/controller/PhabricatorPeopleEditController.php

@@ -139,6 +139,8 @@ final class PhabricatorPeopleEditController
         } else if (!PhabricatorUserEmail::isAllowedAddress($new_email)) {
           $e_email = 'Invalid';
           $errors[] = PhabricatorUserEmail::describeAllowedAddresses();
+        } else {
+          $e_email = null;
         }
 
         if ($request->getStr('role') == 'agent') {