Streamline Legalpad signature workflow

Summary:
Generally reduces friction, standardizes, and simplifies this workflow. Particularly, this removes "address" and "phone", which I think we can wait for user demand for.

For logged-in users, we just always use their primary email.

Test Plan: See screenshots.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

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

resources/celerity/map.php

@@ -7,7 +7,7 @@
 return array(
   'names' =>
   array(
-    'core.pkg.css' => 'e428441c',
+    'core.pkg.css' => '3f0f5da2',
     'core.pkg.js' => '8c184823',
     'darkconsole.pkg.js' => 'df001cab',
     'differential.pkg.css' => '4a93db37',
@@ -20,7 +20,7 @@ return array(
     'rsrc/css/aphront/context-bar.css' => '1c3b0529',
     'rsrc/css/aphront/dark-console.css' => '6378ef3d',
     'rsrc/css/aphront/dialog-view.css' => '4dbbe3bb',
-    'rsrc/css/aphront/error-view.css' => '9f1d5518',
+    'rsrc/css/aphront/error-view.css' => '3462dbee',
     'rsrc/css/aphront/lightbox-attachment.css' => '7acac05d',
     'rsrc/css/aphront/list-filter-view.css' => '2ae43867',
     'rsrc/css/aphront/multi-column.css' => '1b95ab2e',
@@ -499,7 +499,7 @@ return array(
     'aphront-contextbar-view-css' => '1c3b0529',
     'aphront-dark-console-css' => '6378ef3d',
     'aphront-dialog-view-css' => '4dbbe3bb',
-    'aphront-error-view-css' => '9f1d5518',
+    'aphront-error-view-css' => '3462dbee',
     'aphront-list-filter-view-css' => '2ae43867',
     'aphront-multi-column-view-css' => '1b95ab2e',
     'aphront-pager-view-css' => '2e3539af',

src/__phutil_library_map__.php

@@ -860,6 +860,7 @@ phutil_register_library_map(array(
     'LegalpadDocument' => 'applications/legalpad/storage/LegalpadDocument.php',
     'LegalpadDocumentBody' => 'applications/legalpad/storage/LegalpadDocumentBody.php',
     'LegalpadDocumentCommentController' => 'applications/legalpad/controller/LegalpadDocumentCommentController.php',
+    'LegalpadDocumentDoneController' => 'applications/legalpad/controller/LegalpadDocumentDoneController.php',
     'LegalpadDocumentEditController' => 'applications/legalpad/controller/LegalpadDocumentEditController.php',
     'LegalpadDocumentEditor' => 'applications/legalpad/editor/LegalpadDocumentEditor.php',
     'LegalpadDocumentListController' => 'applications/legalpad/controller/LegalpadDocumentListController.php',
@@ -3613,6 +3614,7 @@ phutil_register_library_map(array(
       1 => 'PhabricatorMarkupInterface',
     ),
     'LegalpadDocumentCommentController' => 'LegalpadController',
+    'LegalpadDocumentDoneController' => 'LegalpadController',
     'LegalpadDocumentEditController' => 'LegalpadController',
     'LegalpadDocumentEditor' => 'PhabricatorApplicationTransactionEditor',
     'LegalpadDocumentListController' => 'LegalpadController',

src/applications/legalpad/application/PhabricatorApplicationLegalpad.php

@@ -46,6 +46,7 @@ final class PhabricatorApplicationLegalpad extends PhabricatorApplication {
         'edit/(?P<id>\d+)/' => 'LegalpadDocumentEditController',
         'comment/(?P<id>\d+)/' => 'LegalpadDocumentCommentController',
         'view/(?P<id>\d+)/' => 'LegalpadDocumentManageController',
+        'done/' => 'LegalpadDocumentDoneController',
         'verify/(?P<code>[^/]+)/' =>
         'LegalpadDocumentSignatureVerificationController',
         'signatures/(?P<id>\d+)/' => 'LegalpadDocumentSignatureListController',

src/applications/legalpad/controller/LegalpadDocumentDoneController.php

@@ -0,0 +1,22 @@
+<?php
+
+final class LegalpadDocumentDoneController extends LegalpadController {
+
+  public function shouldRequireLogin() {
+    return false;
+  }
+
+  public function processRequest() {
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+
+    return $this->newDialog()
+      ->setTitle(pht('Verify Signature'))
+      ->appendParagraph(
+        pht(
+          'Thank you for signing this document. Please check your email '.
+          'to verify your signature and complete the process.'))
+      ->addCancelButton('/', pht('Okay'));
+  }
+
+}

src/applications/legalpad/controller/LegalpadDocumentListController.php

@@ -4,6 +4,10 @@ final class LegalpadDocumentListController extends LegalpadController {
 
   private $queryKey;
 
+  public function shouldAllowPublic() {
+    return true;
+  }
+
   public function willProcessRequest(array $data) {
     $this->queryKey = idx($data, 'queryKey');
   }

src/applications/legalpad/controller/LegalpadDocumentSignController.php

@@ -21,145 +21,162 @@ final class LegalpadDocumentSignController extends LegalpadController {
       ->withIDs(array($this->id))
       ->needDocumentBodies(true)
       ->executeOne();
-
     if (!$document) {
       return new Aphront404Response();
     }
 
     $signer_phid = null;
-    $signature = null;
     $signature_data = array();
     if ($viewer->isLoggedIn()) {
       $signer_phid = $viewer->getPHID();
       $signature_data = array(
-        'email' => $viewer->loadPrimaryEmailAddress());
+        'name' => $viewer->getRealName(),
+        'email' => $viewer->loadPrimaryEmailAddress(),
+      );
     } else if ($request->isFormPost()) {
       $email = new PhutilEmailAddress($request->getStr('email'));
-      $email_obj = id(new PhabricatorUserEmail())
+      if (strlen($email->getDomainName())) {
-        ->loadOneWhere('address = %s', $email->getAddress());
+        $email_obj = id(new PhabricatorUserEmail())
-      if ($email_obj) {
+          ->loadOneWhere('address = %s', $email->getAddress());
-        return $this->signInResponse();
+        if ($email_obj) {
+          return $this->signInResponse();
+        }
+        $external_account = id(new PhabricatorExternalAccountQuery())
+          ->setViewer($viewer)
+          ->withAccountTypes(array('email'))
+          ->withAccountDomains(array($email->getDomainName()))
+          ->withAccountIDs(array($email->getAddress()))
+          ->loadOneOrCreate();
+        if ($external_account->getUserPHID()) {
+          return $this->signInResponse();
+        }
+        $signer_phid = $external_account->getPHID();
       }
-      $external_account = id(new PhabricatorExternalAccountQuery())
-        ->setViewer($viewer)
-        ->withAccountTypes(array('email'))
-        ->withAccountDomains(array($email->getDomainName()))
-        ->withAccountIDs(array($email->getAddress()))
-        ->loadOneOrCreate();
-      if ($external_account->getUserPHID()) {
-        return $this->signInResponse();
-      }
-      $signer_phid = $external_account->getPHID();
     }
 
+    $signature = null;
     if ($signer_phid) {
+      // TODO: This is odd and should probably be adjusted after grey/external
+      // accounts work better, but use the omnipotent viewer to check for a
+      // signature so we can pick up anonymous/grey signatures.
+
       $signature = id(new LegalpadDocumentSignatureQuery())
-        ->setViewer($viewer)
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
         ->withDocumentPHIDs(array($document->getPHID()))
         ->withSignerPHIDs(array($signer_phid))
         ->withDocumentVersions(array($document->getVersions()))
         ->executeOne();
+
+      if ($signature && !$viewer->isLoggedIn()) {
+        return $this->newDialog()
+          ->setTitle(pht('Already Signed'))
+          ->appendParagraph(pht('You have already signed this document!'))
+          ->addCancelButton('/'.$document->getMonogram(), pht('Okay'));
+      }
     }
 
+    $signed_status = null;
     if (!$signature) {
       $has_signed = false;
-      $error_view = null;
       $signature = id(new LegalpadDocumentSignature())
         ->setSignerPHID($signer_phid)
         ->setDocumentPHID($document->getPHID())
         ->setDocumentVersion($document->getVersions())
         ->setSignatureData($signature_data);
+
+      // If the user is logged in, show a notice that they haven't signed.
+      // If they aren't logged in, we can't be as sure, so don't show anything.
+      if ($viewer->isLoggedIn()) {
+        $signed_status = id(new AphrontErrorView())
+          ->setSeverity(AphrontErrorView::SEVERITY_WARNING)
+          ->setErrors(
+            array(
+              pht('You have not signed this document yet.'),
+            ));
+      }
     } else {
       $has_signed = true;
-      if ($signature->isVerified()) {
-        $title = pht('Already Signed');
-        $body = $this->getVerifiedSignatureBlurb();
-      } else {
-        $title = pht('Already Signed but...');
-        $body = $this->getUnverifiedSignatureBlurb();
-      }
-      $error_view = id(new AphrontErrorView())
-        ->setSeverity(AphrontErrorView::SEVERITY_NOTICE)
-        ->setTitle($title)
-        ->appendChild($body);
       $signature_data = $signature->getSignatureData();
+
+      // In this case, we know they've signed.
+      $signed_at = $signature->getDateCreated();
+      $signed_status = id(new AphrontErrorView())
+        ->setSeverity(AphrontErrorView::SEVERITY_NOTICE)
+        ->setErrors(
+          array(
+            pht(
+              'You signed this document on %s.',
+              phabricator_datetime($signed_at, $viewer)),
+          ));
     }
 
     $e_name = true;
     $e_email = true;
-    $e_address_1 = true;
+    $e_agree = null;
+
     $errors = array();
     if ($request->isFormPost() && !$has_signed) {
       $name = $request->getStr('name');
-      $email = $request->getStr('email');
-      $address_1 = $request->getStr('address_1');
-      $address_2 = $request->getStr('address_2');
-      $phone = $request->getStr('phone');
       $agree = $request->getExists('agree');
 
-      if (!$name) {
+      if (!strlen($name)) {
         $e_name = pht('Required');
         $errors[] = pht('Name field is required.');
+      } else {
+        $e_name = null;
       }
       $signature_data['name'] = $name;
 
-      $addr_obj = null;
+      if ($viewer->isLoggedIn()) {
-      if (!$email) {
+        $email = $viewer->loadPrimaryEmailAddress();
-        $e_email = pht('Required');
-        $errors[] = pht('Email field is required.');
       } else {
-        $addr_obj = new PhutilEmailAddress($email);
+        $email = $request->getStr('email');
-        $domain = $addr_obj->getDomainName();
+
-        if (!$domain) {
+        $addr_obj = null;
-          $e_email = pht('Invalid');
+        if (!strlen($email)) {
-          $errors[] = pht('A valid email is required.');
+          $e_email = pht('Required');
+          $errors[] = pht('Email field is required.');
+        } else {
+          $addr_obj = new PhutilEmailAddress($email);
+          $domain = $addr_obj->getDomainName();
+          if (!$domain) {
+            $e_email = pht('Invalid');
+            $errors[] = pht('A valid email is required.');
+          } else {
+            $e_email = null;
+          }
         }
       }
       $signature_data['email'] = $email;
 
-      if (!$address_1) {
-        $e_address_1 = pht('Required');
-        $errors[] = pht('Address line 1 field is required.');
-      }
-      $signature_data['address_1'] = $address_1;
-      $signature_data['address_2'] = $address_2;
-      $signature_data['phone'] = $phone;
       $signature->setSignatureData($signature_data);
 
       if (!$agree) {
         $errors[] = pht(
           'You must check "I agree to the terms laid forth above."');
+        $e_agree = pht('Required');
       }
 
-      $verified = LegalpadDocumentSignature::UNVERIFIED;
+      if ($viewer->isLoggedIn()) {
-      if ($viewer->isLoggedIn() && $addr_obj) {
+        $verified = LegalpadDocumentSignature::VERIFIED;
-        $email_obj = id(new PhabricatorUserEmail())
+      } else {
-          ->loadOneWhere('address = %s', $addr_obj->getAddress());
+        $verified = LegalpadDocumentSignature::UNVERIFIED;
-        if ($email_obj && $email_obj->getUserPHID() == $viewer->getPHID()) {
-          $verified = LegalpadDocumentSignature::VERIFIED;
-        }
       }
       $signature->setVerified($verified);
 
       if (!$errors) {
         $signature->save();
-        $has_signed = true;
+
-        if ($signature->isVerified()) {
+        // If the viewer is logged in, send them to the document page, which
-          $body = $this->getVerifiedSignatureBlurb();
+        // will show that they have signed the document. Otherwise, send them
+        // to a completion page.
+        if ($viewer->isLoggedIn()) {
+          $next_uri = '/'.$document->getMonogram();
         } else {
-          $body = $this->getUnverifiedSignatureBlurb();
+          $next_uri = $this->getApplicationURI('done/');
-          $this->sendVerifySignatureEmail(
-            $document,
-            $signature);
         }
-        $error_view = id(new AphrontErrorView())
+
-          ->setSeverity(AphrontErrorView::SEVERITY_NOTICE)
+        return id(new AphrontRedirectResponse())->setURI($next_uri);
-          ->setTitle(pht('Signature Successful'))
-          ->appendChild($body);
-      } else {
-        $error_view = id(new AphrontErrorView())
-          ->setTitle(pht('Error in submission.'))
-          ->setErrors($errors);
       }
     }
 
@@ -171,6 +188,10 @@ final class LegalpadDocumentSignController extends LegalpadController {
       LegalpadDocumentBody::MARKUP_FIELD_TEXT);
     $engine->process();
 
+    $document_markup = $engine->getOutput(
+      $document_body,
+      LegalpadDocumentBody::MARKUP_FIELD_TEXT);
+
     $title = $document_body->getTitle();
 
     $manage_uri = $this->getApplicationURI('view/'.$document->getID().'/');
@@ -193,25 +214,36 @@ final class LegalpadDocumentSignController extends LegalpadController {
           ->setDisabled(!$can_edit)
           ->setWorkflow(!$can_edit));
 
-    $signature_form = $this->buildSignatureForm(
+    $content = id(new PHUIDocumentView())
-      $document_body,
+      ->addClass('legalpad')
-      $signature,
+      ->setHeader($header)
-      $has_signed,
+      ->appendChild(
-      $e_name,
+        array(
-      $e_email,
+          $signed_status,
-      $e_address_1);
+          $document_markup,
+        ));
 
-    $content = $this->buildDocument(
+    if (!$has_signed) {
-      $header,
+      $error_view = null;
-      $engine,
+      if ($errors) {
-      $document_body);
+        $error_view = id(new AphrontErrorView())
+          ->setErrors($errors);
+      }
 
-    $content->appendChild(
+      $signature_form = $this->buildSignatureForm(
-      array(
+        $document_body,
-        id(new PHUIHeaderView())->setHeader(pht('Agree and Sign Document')),
+        $signature,
-        $error_view,
+        $e_name,
-        $signature_form,
+        $e_email,
-      ));
+        $e_agree);
+
+      $content->appendChild(
+        array(
+          id(new PHUIHeaderView())->setHeader(pht('Agree and Sign Document')),
+          $error_view,
+          $signature_form,
+        ));
+    }
 
     $crumbs = $this->buildApplicationCrumbs();
     $crumbs->addTextCrumb($document->getMonogram());
@@ -227,35 +259,16 @@ final class LegalpadDocumentSignController extends LegalpadController {
       ));
   }
 
-  private function buildDocument(
-    PHUIHeaderView $header,
-    PhabricatorMarkupEngine $engine,
-    LegalpadDocumentBody $body) {
-
-    return id(new PHUIDocumentView())
-      ->addClass('legalpad')
-      ->setHeader($header)
-      ->appendChild($engine->getOutput(
-        $body,
-        LegalpadDocumentBody::MARKUP_FIELD_TEXT));
-  }
-
   private function buildSignatureForm(
     LegalpadDocumentBody $body,
     LegalpadDocumentSignature $signature,
-    $has_signed = false,
+    $e_name,
-    $e_name = true,
+    $e_email,
-    $e_email = true,
+    $e_agree) {
-    $e_address_1 = true) {
 
     $viewer = $this->getRequest()->getUser();
-    if ($has_signed) {
-      $instructions = pht('Thank you for signing and agreeing.');
-    } else {
-      $instructions = pht('Please enter the following information.');
-    }
-
     $data = $signature->getSignatureData();
+
     $form = id(new AphrontFormView())
       ->setUser($viewer)
       ->appendChild(
@@ -263,61 +276,34 @@ final class LegalpadDocumentSignController extends LegalpadController {
         ->setLabel(pht('Name'))
         ->setValue(idx($data, 'name', ''))
         ->setName('name')
-        ->setError($e_name)
+        ->setError($e_name));
-        ->setDisabled($has_signed))
+
-      ->appendChild(
+    if (!$viewer->isLoggedIn()) {
+      $form->appendChild(
         id(new AphrontFormTextControl())
-        ->setLabel(pht('Email'))
+          ->setLabel(pht('Email'))
-        ->setValue(idx($data, 'email', ''))
+          ->setValue(idx($data, 'email', ''))
-        ->setName('email')
+          ->setName('email')
-        ->setError($e_email)
+          ->setError($e_email));
-        ->setDisabled($has_signed))
+    }
-      ->appendChild(
+
-        id(new AphrontFormTextControl())
+    $form
-        ->setLabel(pht('Address line 1'))
-        ->setValue(idx($data, 'address_1', ''))
-        ->setName('address_1')
-        ->setError($e_address_1)
-        ->setDisabled($has_signed))
-      ->appendChild(
-        id(new AphrontFormTextControl())
-        ->setLabel(pht('Address line 2'))
-        ->setValue(idx($data, 'address_2', ''))
-        ->setName('address_2')
-        ->setDisabled($has_signed))
-      ->appendChild(
-        id(new AphrontFormTextControl())
-        ->setLabel(pht('Phone'))
-        ->setValue(idx($data, 'phone', ''))
-        ->setName('phone')
-        ->setDisabled($has_signed))
       ->appendChild(
         id(new AphrontFormCheckboxControl())
-        ->addCheckbox(
+          ->setError($e_agree)
-          'agree',
+          ->addCheckbox(
-          'agree',
+            'agree',
-          pht('I agree to the terms laid forth above.'),
+            'agree',
-          $has_signed)
+            pht('I agree to the terms laid forth above.'),
-        ->setDisabled($has_signed))
+            false))
       ->appendChild(
         id(new AphrontFormSubmitControl())
-        ->setValue(pht('Sign and Agree'))
+          ->setValue(pht('Sign Document'))
-        ->setDisabled($has_signed)
+          ->addCancelButton($this->getApplicationURI()));
-        ->addCancelButton($this->getApplicationURI()));
 
     return $form;
   }
 
-  private function getVerifiedSignatureBlurb() {
-    return pht('Thank you for signing and agreeing.');
-  }
-
-  private function getUnverifiedSignatureBlurb() {
-    return pht('Thank you for signing and agreeing. However, you must '.
-               'verify your email address. Please check your email '.
-               'and follow the instructions.');
-  }
-
   private function sendVerifySignatureEmail(
     LegalpadDocument $doc,
     LegalpadDocumentSignature $signature) {

webroot/rsrc/css/aphront/error-view.css

@@ -77,6 +77,12 @@ h1.aphront-error-view-head {
   background-color: #fff;
 }
 
+.legalpad .aphront-error-view {
+  margin: 0;
+  border-width: 0 0 1px 0;
+  border-bottom: 1px solid {$lightblueborder};
+}
+
 .aphront-dialog-body .aphront-error-view {
   margin: -16px -16px 16px -16px;
   border-width: 0 0 1px 0;