From 29313372e7159cee68dba8282b93fab9b75432fa Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Nov 2016 13:34:58 -0700 Subject: [PATCH] Improve some commenting/editing behaviors for recurring events Summary: Ref T11809. Currently, commenting on a recurring event hits the same "one or all?" dialog that other edits do. For comments and edits submitted via the comment widget, we can safely assume that you mean "just this one", since it doesn't really make sense to try to bulk-edit an event from that UI. Test Plan: Commented on a recurring event parent and an event in the series. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11809 Differential Revision: https://secure.phabricator.com/D16795 --- ...PhabricatorCalendarEventEditController.php | 13 +++++-- .../PhabricatorCalendarEventEditEngine.php | 36 +++++++++++++++++-- ...abricatorCalendarEventReplyTransaction.php | 4 +++ ...habricatorCalendarEventTransactionType.php | 8 ++++- .../editengine/PhabricatorEditEngine.php | 13 ++++++- 5 files changed, 68 insertions(+), 6 deletions(-) diff --git a/src/applications/calendar/controller/PhabricatorCalendarEventEditController.php b/src/applications/calendar/controller/PhabricatorCalendarEventEditController.php index 3211d8052b..928526f33f 100644 --- a/src/applications/calendar/controller/PhabricatorCalendarEventEditController.php +++ b/src/applications/calendar/controller/PhabricatorCalendarEventEditController.php @@ -34,7 +34,17 @@ final class PhabricatorCalendarEventEditController ->addCancelButton($cancel_uri); } } else if ($event->getIsRecurring()) { - $mode = $request->getStr('mode'); + + // If the user submits a comment or makes an edit via comment actions, + // always target only the current event. It doesn't make sense to add + // comments to every instance of an event, and the other actions don't + // make much sense to apply to all instances either. + if ($engine->isCommentAction()) { + $mode = PhabricatorCalendarEventEditEngine::MODE_THIS; + } else { + $mode = $request->getStr('mode'); + } + if (!$mode) { $form = id(new AphrontFormView()) ->setViewer($viewer) @@ -60,7 +70,6 @@ final class PhabricatorCalendarEventEditController ->addSubmitButton(pht('Continue')) ->addCancelButton($cancel_uri) ->setDisableWorkflowOnSubmit(true); - } $engine diff --git a/src/applications/calendar/editor/PhabricatorCalendarEventEditEngine.php b/src/applications/calendar/editor/PhabricatorCalendarEventEditEngine.php index f877bd9d13..c56cd7febb 100644 --- a/src/applications/calendar/editor/PhabricatorCalendarEventEditEngine.php +++ b/src/applications/calendar/editor/PhabricatorCalendarEventEditEngine.php @@ -295,7 +295,7 @@ final class PhabricatorCalendarEventEditEngine protected function willApplyTransactions($object, array $xactions) { $viewer = $this->getViewer(); - $this->rawTransactions = $xactions; + $this->rawTransactions = $this->cloneTransactions($xactions); $is_parent = $object->isParentEvent(); $is_child = $object->isChildEvent(); @@ -304,6 +304,30 @@ final class PhabricatorCalendarEventEditEngine $must_fork = ($is_child && $is_future) || ($is_parent && !$is_future); + 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) { + $must_fork = false; + } + } + if ($must_fork) { $fork_target = $object->loadForkTarget($viewer); if ($fork_target) { @@ -340,7 +364,7 @@ final class PhabricatorCalendarEventEditEngine } foreach ($targets as $target) { - $apply = clone $this->rawTransactions; + $apply = $this->cloneTransactions($this->rawTransactions); $this->applyTransactions($target, $apply); } } @@ -366,4 +390,12 @@ final class PhabricatorCalendarEventEditEngine } } + private function cloneTransactions(array $xactions) { + $result = array(); + foreach ($xactions as $xaction) { + $result[] = clone $xaction; + } + return $result; + } + } diff --git a/src/applications/calendar/xaction/PhabricatorCalendarEventReplyTransaction.php b/src/applications/calendar/xaction/PhabricatorCalendarEventReplyTransaction.php index e2ad9ea4be..2aad3d2bb2 100644 --- a/src/applications/calendar/xaction/PhabricatorCalendarEventReplyTransaction.php +++ b/src/applications/calendar/xaction/PhabricatorCalendarEventReplyTransaction.php @@ -8,6 +8,10 @@ abstract class PhabricatorCalendarEventReplyTransaction return $object->getUserInviteStatus($actor_phid); } + public function isInheritedEdit() { + return false; + } + public function applyExternalEffects($object, $value) { $acting_phid = $this->getActingAsPHID(); diff --git a/src/applications/calendar/xaction/PhabricatorCalendarEventTransactionType.php b/src/applications/calendar/xaction/PhabricatorCalendarEventTransactionType.php index 53c00969e9..bcb1e13c21 100644 --- a/src/applications/calendar/xaction/PhabricatorCalendarEventTransactionType.php +++ b/src/applications/calendar/xaction/PhabricatorCalendarEventTransactionType.php @@ -1,4 +1,10 @@ getController(); $request = $controller->getRequest(); - $action = $request->getURIData('editAction'); + $action = $this->getEditAction(); $capabilities = array(); $use_default = false; @@ -2098,6 +2098,17 @@ abstract class PhabricatorEditEngine PhabricatorPolicyCapability::CAN_EDIT); } + public function isCommentAction() { + return ($this->getEditAction() == 'comment'); + } + + public function getEditAction() { + $controller = $this->getController(); + $request = $controller->getRequest(); + return $request->getURIData('editAction'); + } + + /* -( Form Pages )--------------------------------------------------------- */