Improve usability of Phrequent "Stop Time" dialog

Summary:
Fixes T5848.

  - Disallow tracking negative time.
  - Preserve note if there's an error with the time selection.
  - Show start time and duration.
  - Slightly better error messages.

Test Plan: Started and stopped time. Tried to select future/negative ranges.

Reviewers: hach-que, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5848

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

src/applications/phrequent/controller/PhrequentTrackController.php

@@ -13,20 +13,16 @@ final class PhrequentTrackController
 
   public function processRequest() {
     $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();
-    $editor = new PhrequentTrackingEditor();
 
     $phid = $this->phid;
     $handle = id(new PhabricatorHandleQuery())
-      ->setViewer($user)
+      ->setViewer($viewer)
       ->withPHIDs(array($phid))
       ->executeOne();
+    $done_uri = $handle->getURI();
 
-    if (!$this->isStartingTracking() &&
+    $current_timer = null;
-        !$this->isStoppingTracking()) {
-      throw new Exception('Unrecognized verb: '.$this->verb);
-    }
-
     switch ($this->verb) {
       case 'start':
         $button_text = pht('Start Tracking');
@@ -41,62 +37,119 @@ final class PhrequentTrackController
         $inner_text = pht('What time did you stop working?');
         $action_text = pht('Stop Timer');
         $label_text = pht('Stop Time');
+
+
+        $current_timer = id(new PhrequentUserTimeQuery())
+          ->setViewer($viewer)
+          ->withUserPHIDs(array($viewer->getPHID()))
+          ->withObjectPHIDs(array($phid))
+          ->withEnded(PhrequentUserTimeQuery::ENDED_NO)
+          ->executeOne();
+        if (!$current_timer) {
+          return $this->newDialog()
+            ->setTitle(pht('Not Tracking Time'))
+            ->appendParagraph(
+              pht(
+                'You are not currently tracking time on this object.'))
+            ->addCancelButton($done_uri);
+        }
         break;
+      default:
+        return new Aphront404Response();
     }
 
+    $errors = array();
+    $v_note = null;
+    $e_date = null;
+
     $epoch_control = id(new AphrontFormDateControl())
-      ->setUser($user)
+      ->setUser($viewer)
       ->setName('epoch')
       ->setLabel($action_text)
       ->setValue(time());
 
-    $err = array();
-
     if ($request->isDialogFormPost()) {
+      $v_note = $request->getStr('note');
       $timestamp = $epoch_control->readValueFromRequest($request);
-      $note = $request->getStr('note');
 
-      if (!$epoch_control->isValid() || $timestamp > time()) {
+      if (!$epoch_control->isValid()) {
-        $err[] = pht('Invalid date, please enter a valid non-future date');
+        $errors[] = pht('Please choose an valid date.');
+        $e_date = pht('Invalid');
+      } else {
+        $max_time = PhabricatorTime::getNow();
+        if ($timestamp > $max_time) {
+          if ($this->isStoppingTracking()) {
+            $errors[] = pht(
+              'You can not stop tracking time at a future time. Enter the '.
+              'current time, or a time in the past.');
+          } else {
+            $errors[] = pht(
+              'You can not start tracking time at a future time. Enter the '.
+              'current time, or a time in the past.');
+          }
+          $e_date = pht('Invalid');
+        }
+
+        if ($this->isStoppingTracking()) {
+          $min_time = $current_timer->getDateStarted();
+          if ($min_time > $timestamp) {
+            $errors[] = pht(
+              'Stop time must be after start time.');
+            $e_date = pht('Invalid');
+          }
+        }
       }
 
-      if (!$err) {
+      if (!$errors) {
+        $editor = new PhrequentTrackingEditor();
         if ($this->isStartingTracking()) {
-          $editor->startTracking($user, $this->phid, $timestamp);
+          $editor->startTracking($viewer, $this->phid, $timestamp);
         } else if ($this->isStoppingTracking()) {
-          $editor->stopTracking($user, $this->phid, $timestamp, $note);
+          $editor->stopTracking($viewer, $this->phid, $timestamp, $v_note);
         }
-        return id(new AphrontRedirectResponse());
+
+        return id(new AphrontRedirectResponse())->setURI($done_uri);
       }
 
     }
 
+    $epoch_control->setError($e_date);
+
     $dialog = $this->newDialog()
       ->setTitle($title_text)
-      ->setWidth(AphrontDialogView::WIDTH_FORM);
+      ->setWidth(AphrontDialogView::WIDTH_FORM)
-
+      ->setErrors($errors)
-    if ($err) {
+      ->appendParagraph($inner_text);
-      $dialog->setErrors($err);
-    }
 
     $form = new PHUIFormLayoutView();
-    $form
+
-      ->appendChild(hsprintf(
+    if ($this->isStoppingTracking()) {
-        '<p>%s</p><br />', $inner_text));
+      $start_time = $current_timer->getDateStarted();
+      $start_string = pht(
+        '%s (%s ago)',
+        phabricator_datetime($start_time, $viewer),
+        phutil_format_relative_time(PhabricatorTime::getNow() - $start_time));
+
+      $form->appendChild(
+        id(new AphrontFormStaticControl())
+          ->setLabel(pht('Started At'))
+          ->setValue($start_string));
+    }
 
     $form->appendChild($epoch_control);
 
     if ($this->isStoppingTracking()) {
-      $form
+      $form->appendChild(
-        ->appendChild(
+        id(new AphrontFormTextControl())
-          id(new AphrontFormTextControl())
+          ->setLabel(pht('Note'))
-            ->setLabel(pht('Note'))
+          ->setName('note')
-            ->setName('note'));
+          ->setValue($v_note));
     }
 
     $dialog->appendChild($form);
 
-    $dialog->addCancelButton($handle->getURI());
+    $dialog->addCancelButton($done_uri);
+
     $dialog->addSubmitButton($action_text);
 
     return $dialog;

src/applications/phrequent/event/PhrequentUIEventListener.php

@@ -45,14 +45,12 @@ final class PhrequentUIEventListener
         ->setName(pht('Start Tracking Time'))
         ->setIcon('fa-clock-o')
         ->setWorkflow(true)
-        ->setRenderAsForm(true)
         ->setHref('/phrequent/track/start/'.$object->getPHID().'/');
     } else {
       $track_action = id(new PhabricatorActionView())
         ->setName(pht('Stop Tracking Time'))
         ->setIcon('fa-clock-o red')
         ->setWorkflow(true)
-        ->setRenderAsForm(true)
         ->setHref('/phrequent/track/stop/'.$object->getPHID().'/');
     }