Smooth out various transaction/editing behaviors for Calendar

Summary:
Ref T11809.

  - Allow users to remove the "Until" date from recurring events.
  - When removing "Until", show a sensible string ("...set this event to repeat forever.")
  - When users go through the "Make Recurring" workflow, don't require them to explicitly select "Recurring: Recurring" from the dropdown. This intent is clear from clicking "Make Recurring".
  - When editing "All Future Events", don't literally apply date changes to them, since that doesn't make sense. We update the template, then reschedule any events which haven't been edited already. I think this is what users probably mean if they make this edit.
  - When creating an event with a non-default icon, don't show "alice changed the icon from Default to Party.".
  - Hide the "recurring mode" transaction, which had no string ("alice edited this Event.") and was redundant anyway.
  - Also, add a little piece of developer text to make hunting these things down easier.

Test Plan: Edited various events, parents, children, made events recur, set until, unset until, viewed transactions, rescheduled parents, rescheduled children.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11809

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

src/applications/calendar/editor/PhabricatorCalendarEventEditEngine.php

@@ -94,7 +94,7 @@ final class PhabricatorCalendarEventEditEngine
 
     // At least for now, just hide "Invitees" when editing all future events.
     // This may eventually deserve a more nuanced approach.
-    $hide_invitees = ($this->getSeriesEditMode() == self::MODE_FUTURE);
+    $is_future = ($this->getSeriesEditMode() == self::MODE_FUTURE);
 
     $fields = array(
       id(new PhabricatorTextEditField())
@@ -162,7 +162,7 @@ final class PhabricatorCalendarEventEditEngine
         ->setConduitTypeDescription(pht('New event host.'))
         ->setSingleValue($object->getHostPHID()),
       id(new PhabricatorDatasourceEditField())
-        ->setIsHidden($hide_invitees)
+        ->setIsHidden($is_future)
         ->setKey('inviteePHIDs')
         ->setAliases(array('invite', 'invitee', 'invitees', 'inviteePHID'))
         ->setLabel(pht('Invitees'))
@@ -193,8 +193,16 @@ final class PhabricatorCalendarEventEditEngine
         ->setConduitDescription(pht('Change the event icon.'))
         ->setConduitTypeDescription(pht('New event icon.'))
         ->setValue($object->getIcon()),
+
+      // NOTE: We're being a little sneaky here. This field is hidden and
+      // always has the value "true", so it makes the event recurring when you
+      // submit a form which contains the field. Then we put the the field on
+      // the "recurring" page in the "Make Recurring" dialog to simplify the
+      // workflow. This is still normal, explicit field from the perspective
+      // of the API.
+
       id(new PhabricatorBoolEditField())
-        ->setIsHidden($object->getIsRecurring())
+        ->setIsHidden(true)
         ->setKey('isRecurring')
         ->setLabel(pht('Recurring'))
         ->setOptions(pht('One-Time Event'), pht('Recurring Event'))
@@ -203,7 +211,7 @@ final class PhabricatorCalendarEventEditEngine
         ->setDescription(pht('One time or recurring event.'))
         ->setConduitDescription(pht('Make the event recurring.'))
         ->setConduitTypeDescription(pht('Mark the event as a recurring event.'))
-        ->setValue($object->getIsRecurring()),
+        ->setValue(true),
       id(new PhabricatorSelectEditField())
         ->setKey('frequency')
         ->setLabel(pht('Frequency'))
@@ -295,35 +303,35 @@ final class PhabricatorCalendarEventEditEngine
 
   protected function willApplyTransactions($object, array $xactions) {
     $viewer = $this->getViewer();
-    $this->rawTransactions = $this->cloneTransactions($xactions);
 
     $is_parent = $object->isParentEvent();
     $is_child = $object->isChildEvent();
     $is_future = ($this->getSeriesEditMode() === self::MODE_FUTURE);
 
+    // Figure out which transactions we can apply to the whole series of events.
+    // Some transactions (like comments) can never be bulk applied.
+    $inherited_xactions = array();
+    foreach ($xactions as $xaction) {
+      $modular_type = $xaction->getModularType();
+      if (!($modular_type instanceof PhabricatorCalendarEventTransactionType)) {
+        continue;
+      }
+
+      $inherited_edit = $modular_type->isInheritedEdit();
+      if ($inherited_edit) {
+        $inherited_xactions[] = $xaction;
+      }
+    }
+    $this->rawTransactions = $this->cloneTransactions($inherited_xactions);
+
     $must_fork = ($is_child && $is_future) ||
                  ($is_parent && !$is_future);
 
+    // We don't need to fork when editing a parent event if none of the edits
+    // can transfer to child events. For example, commenting on a parent is
+    // fine.
     if ($is_parent && !$is_future) {
-      // We don't necessarily need to fork if whatever we're editing is not
+      if (!$inherited_xactions) {
-      // inherited by children. For example, we can add a comment to the parent
-      // event without needing to fork. Test all the stuff we're doing and see
-      // if anything is actually inherited.
-
-      $inherited_edit = false;
-      foreach ($xactions as $xaction) {
-        $modular_type = $xaction->getTransactionImplementation();
-        if ($modular_type instanceof PhabricatorCalendarEventTransactionType) {
-          $inherited_edit = $modular_type->isInheritedEdit();
-          if ($inherited_edit) {
-            break;
-          }
-        }
-      }
-
-      // Nothing the user is trying to do requires us to fork, so we can just
-      // apply the changes normally.
-      if (!$inherited_edit) {
         $must_fork = false;
       }
     }

src/applications/calendar/storage/PhabricatorCalendarEvent.php

@@ -935,10 +935,14 @@ final class PhabricatorCalendarEvent extends PhabricatorCalendarDAO
       $datetime->newAbsoluteDateTime()->toDictionary());
   }
 
-  public function setUntilDateTime(PhutilCalendarDateTime $datetime) {
+  public function setUntilDateTime(PhutilCalendarDateTime $datetime = null) {
-    return $this->setParameter(
+    if ($datetime) {
-      'untilDateTime',
+      $value = $datetime->newAbsoluteDateTime()->toDictionary();
-      $datetime->newAbsoluteDateTime()->toDictionary());
+    } else {
+      $value = null;
+    }
+
+    return $this->setParameter('untilDateTime', $value);
   }
 
   public function setRecurrenceRule(PhutilCalendarRecurrenceRule $rrule) {

src/applications/calendar/xaction/PhabricatorCalendarEventDateTransaction.php

@@ -5,8 +5,17 @@ abstract class PhabricatorCalendarEventDateTransaction
 
   abstract protected function getInvalidDateMessage();
 
+  public function isInheritedEdit() {
+    return false;
+  }
+
   public function generateNewValue($object, $value) {
     $editor = $this->getEditor();
+
+    if ($value->isDisabled()) {
+      return null;
+    }
+
     return $value->newPhutilDateTime()
       ->setIsAllDay($editor->getNewIsAllDay())
       ->newAbsoluteDateTime()

src/applications/calendar/xaction/PhabricatorCalendarEventIconTransaction.php

@@ -13,6 +13,14 @@ final class PhabricatorCalendarEventIconTransaction
     $object->setIcon($value);
   }
 
+  public function shouldHide() {
+    if ($this->isCreateTransaction()) {
+      return true;
+    }
+
+    return false;
+  }
+
   public function getTitle() {
     $old = $this->getIconLabel($this->getOldValue());
     $new = $this->getIconLabel($this->getNewValue());

src/applications/calendar/xaction/PhabricatorCalendarEventRecurringTransaction.php

@@ -13,6 +13,17 @@ final class PhabricatorCalendarEventRecurringTransaction
     return (int)$value;
   }
 
+  public function isInheritedEdit() {
+    return false;
+  }
+
+  public function shouldHide() {
+    // This event isn't interesting on its own, and is accompanied by an
+    // "alice set this event to repeat weekly." event in normal circumstances
+    // anyway.
+    return true;
+  }
+
   public function applyInternalEffects($object, $value) {
     $object->setIsRecurring($value);
   }

src/applications/calendar/xaction/PhabricatorCalendarEventUntilDateTransaction.php

@@ -23,25 +23,41 @@ final class PhabricatorCalendarEventUntilDateTransaction
     $actor = $this->getActor();
     $editor = $this->getEditor();
 
-    $datetime = PhutilCalendarAbsoluteDateTime::newFromDictionary($value);
+    if ($value) {
-    $datetime->setIsAllDay($editor->getNewIsAllDay());
+      $datetime = PhutilCalendarAbsoluteDateTime::newFromDictionary($value);
-
+      $datetime->setIsAllDay($editor->getNewIsAllDay());
-    $object->setUntilDateTime($datetime);
+      $object->setUntilDateTime($datetime);
+    } else {
+      $object->setUntilDateTime(null);
+    }
   }
 
   public function getTitle() {
-    return pht(
+    if ($this->getNewValue()) {
-      '%s changed this event to repeat until %s.',
+      return pht(
-      $this->renderAuthor(),
+        '%s changed this event to repeat until %s.',
-      $this->renderNewDate());
+        $this->renderAuthor(),
+        $this->renderNewDate());
+    } else {
+      return pht(
+        '%s changed this event to repeat forever.',
+        $this->renderAuthor());
+    }
   }
 
   public function getTitleForFeed() {
-    return pht(
+    if ($this->getNewValue()) {
-      '%s changed %s to repeat until %s.',
+      return pht(
-      $this->renderAuthor(),
+        '%s changed %s to repeat until %s.',
-      $this->renderObject(),
+        $this->renderAuthor(),
-      $this->renderNewDate());
+        $this->renderObject(),
+        $this->renderNewDate());
+    } else {
+      return pht(
+        '%s changed %s to repeat forever.',
+        $this->renderAuthor(),
+        $this->renderObject());
+    }
   }
 
   protected function getInvalidDateMessage() {

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -1073,10 +1073,21 @@ abstract class PhabricatorApplicationTransaction
         break;
 
       default:
-        return pht(
+        // In developer mode, provide a better hint here about which string
-          '%s edited this %s.',
+        // we're missing.
-          $this->renderHandleLink($author_phid),
+        $developer_mode = 'phabricator.developer-mode';
-          $this->getApplicationObjectTypeName());
+        $is_developer = PhabricatorEnv::getEnvConfig($developer_mode);
+        if ($is_developer) {
+          return pht(
+            '%s edited this object (transaction type "%s").',
+            $this->renderHandleLink($author_phid),
+            $this->getTransactionType());
+        } else {
+          return pht(
+            '%s edited this %s.',
+            $this->renderHandleLink($author_phid),
+            $this->getApplicationObjectTypeName());
+        }
     }
   }
 
@@ -1615,6 +1626,10 @@ abstract class PhabricatorApplicationTransaction
       'editable by the transaction author.');
   }
 
+  public function getModularType() {
+    return null;
+  }
+
 
 /* -(  PhabricatorDestructibleInterface  )----------------------------------- */
 

src/applications/transactions/storage/PhabricatorModularTransaction.php

@@ -7,6 +7,10 @@ abstract class PhabricatorModularTransaction
 
   abstract public function getBaseTransactionClass();
 
+  public function getModularType() {
+    return $this->getTransactionImplementation();
+  }
+
   final protected function getTransactionImplementation() {
     if (!$this->implementation) {
       $this->implementation = $this->newTransactionImplementation();