From e4c6ae5345188c26f30eb9533270575316ed51f5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Nov 2016 15:15:09 -0700 Subject: [PATCH] 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 --- .../PhabricatorCalendarEventEditEngine.php | 56 +++++++++++-------- .../storage/PhabricatorCalendarEvent.php | 12 ++-- ...habricatorCalendarEventDateTransaction.php | 9 +++ ...habricatorCalendarEventIconTransaction.php | 8 +++ ...catorCalendarEventRecurringTransaction.php | 11 ++++ ...catorCalendarEventUntilDateTransaction.php | 42 +++++++++----- .../PhabricatorApplicationTransaction.php | 23 ++++++-- .../storage/PhabricatorModularTransaction.php | 4 ++ 8 files changed, 120 insertions(+), 45 deletions(-) diff --git a/src/applications/calendar/editor/PhabricatorCalendarEventEditEngine.php b/src/applications/calendar/editor/PhabricatorCalendarEventEditEngine.php index c56cd7febb..5b31afbe1a 100644 --- a/src/applications/calendar/editor/PhabricatorCalendarEventEditEngine.php +++ b/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 - // 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) { + if (!$inherited_xactions) { $must_fork = false; } } diff --git a/src/applications/calendar/storage/PhabricatorCalendarEvent.php b/src/applications/calendar/storage/PhabricatorCalendarEvent.php index 2863ee64e8..440e35f980 100644 --- a/src/applications/calendar/storage/PhabricatorCalendarEvent.php +++ b/src/applications/calendar/storage/PhabricatorCalendarEvent.php @@ -935,10 +935,14 @@ final class PhabricatorCalendarEvent extends PhabricatorCalendarDAO $datetime->newAbsoluteDateTime()->toDictionary()); } - public function setUntilDateTime(PhutilCalendarDateTime $datetime) { - return $this->setParameter( - 'untilDateTime', - $datetime->newAbsoluteDateTime()->toDictionary()); + public function setUntilDateTime(PhutilCalendarDateTime $datetime = null) { + if ($datetime) { + $value = $datetime->newAbsoluteDateTime()->toDictionary(); + } else { + $value = null; + } + + return $this->setParameter('untilDateTime', $value); } public function setRecurrenceRule(PhutilCalendarRecurrenceRule $rrule) { diff --git a/src/applications/calendar/xaction/PhabricatorCalendarEventDateTransaction.php b/src/applications/calendar/xaction/PhabricatorCalendarEventDateTransaction.php index cc2cb59da9..253609a99e 100644 --- a/src/applications/calendar/xaction/PhabricatorCalendarEventDateTransaction.php +++ b/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() diff --git a/src/applications/calendar/xaction/PhabricatorCalendarEventIconTransaction.php b/src/applications/calendar/xaction/PhabricatorCalendarEventIconTransaction.php index d69cf20859..44daa13811 100644 --- a/src/applications/calendar/xaction/PhabricatorCalendarEventIconTransaction.php +++ b/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()); diff --git a/src/applications/calendar/xaction/PhabricatorCalendarEventRecurringTransaction.php b/src/applications/calendar/xaction/PhabricatorCalendarEventRecurringTransaction.php index 05f312ddc4..9e4904085f 100644 --- a/src/applications/calendar/xaction/PhabricatorCalendarEventRecurringTransaction.php +++ b/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); } diff --git a/src/applications/calendar/xaction/PhabricatorCalendarEventUntilDateTransaction.php b/src/applications/calendar/xaction/PhabricatorCalendarEventUntilDateTransaction.php index f3d7bf5595..3d0649eab8 100644 --- a/src/applications/calendar/xaction/PhabricatorCalendarEventUntilDateTransaction.php +++ b/src/applications/calendar/xaction/PhabricatorCalendarEventUntilDateTransaction.php @@ -23,25 +23,41 @@ final class PhabricatorCalendarEventUntilDateTransaction $actor = $this->getActor(); $editor = $this->getEditor(); - $datetime = PhutilCalendarAbsoluteDateTime::newFromDictionary($value); - $datetime->setIsAllDay($editor->getNewIsAllDay()); - - $object->setUntilDateTime($datetime); + if ($value) { + $datetime = PhutilCalendarAbsoluteDateTime::newFromDictionary($value); + $datetime->setIsAllDay($editor->getNewIsAllDay()); + $object->setUntilDateTime($datetime); + } else { + $object->setUntilDateTime(null); + } } public function getTitle() { - return pht( - '%s changed this event to repeat until %s.', - $this->renderAuthor(), - $this->renderNewDate()); + if ($this->getNewValue()) { + return pht( + '%s changed this event to repeat until %s.', + $this->renderAuthor(), + $this->renderNewDate()); + } else { + return pht( + '%s changed this event to repeat forever.', + $this->renderAuthor()); + } } public function getTitleForFeed() { - return pht( - '%s changed %s to repeat until %s.', - $this->renderAuthor(), - $this->renderObject(), - $this->renderNewDate()); + if ($this->getNewValue()) { + return pht( + '%s changed %s to repeat until %s.', + $this->renderAuthor(), + $this->renderObject(), + $this->renderNewDate()); + } else { + return pht( + '%s changed %s to repeat forever.', + $this->renderAuthor(), + $this->renderObject()); + } } protected function getInvalidDateMessage() { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index cd0557dafe..cc583d34c2 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -1073,10 +1073,21 @@ abstract class PhabricatorApplicationTransaction break; default: - return pht( - '%s edited this %s.', - $this->renderHandleLink($author_phid), - $this->getApplicationObjectTypeName()); + // In developer mode, provide a better hint here about which string + // we're missing. + $developer_mode = 'phabricator.developer-mode'; + $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 )----------------------------------- */ diff --git a/src/applications/transactions/storage/PhabricatorModularTransaction.php b/src/applications/transactions/storage/PhabricatorModularTransaction.php index c93a559108..e62d550bb9 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransaction.php +++ b/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();