From 4d89afcc6149974a08fe57f1dc009426fad39b35 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 14:29:23 -0700 Subject: [PATCH 01/27] Remove requireCapabilities() from ApplicationTransactionEditor and require CAN_EDIT by default Summary: Depends on D19585. Ref T13164. Almost all transactions require CAN_EDIT on the object, but they generally do not enforce this directly today. Instead, this is effectively enforced by Controllers, API methods, and EditEngine doing a `CAN_EDIT` check when loading the object to be edited. A small number of transactions do not require CAN_EDIT, and instead require only a weaker/lesser permission. These are: - Joining a project which you have CAN_JOIN on. - Leaving a project which isn't locked. - Joining a Conpherence thread you can see (today, no separate CAN_JOIN permission for Conpherence). - Leaving a Conpherence thread. - Unsubscribing. - Using the special `!history` command from email. Additionally, these require CAN_INTERACT, which is weaker than CAN_EDIT: - Adding comments. - Subscribing. - Awarding tokens. Soon, I want to add "disabling users" to this list, so that you can disable users if you have "Can Disable User" permission, even if you can not otherwise edit users. It's possible this list isn't exhaustive, so this change might break something by adding a policy check to a place where we previously didn't have one. If so, we can go weaken that policy check to the appropriate level. Enforcement of these special cases is currently weird: - We mostly don't actually enforce CAN_EDIT in the Editor; instead, it's enforced before you get to the editor (in EditEngine/Controllers). - To apply a weaker requirement (like leaving comments or leaving a project), we let you get through the Controller without CAN_EDIT, then apply the weaker policy check in the Editor. - Some transactions apply a confusing/redundant explicit CAN_EDIT policy check. These mostly got cleaned up in previous changes. Instead, the new world order is: - Every transaction has capability/policy requirements. - The default is CAN_EDIT, but transactions can weaken this explicitly they want. - So now we'll get requirements right in the Editor, even if Controllers or API endpoints make a mistake. - And you don't have to copy/paste a bunch of code to say "yes, every transaction should require CAN_EDIT". Test Plan: - Tried to add members to a Conpherence thread I could not edit (permissions error). - Left a Conpherence thread I could not edit (worked properly). - Joined a thread I could see but could not edit (worked properly). - Tried to join a thread I could not see (permissions error). - Implemented `requireCapabilites()` on ManiphestTransactionEditor and tried to edit a task (upgrade guidance error). - Mentioned an object I can not edit on another object (works). - Mentioned another object on an object I can not edit (works). - Added a `{F...}` reference to an object I can not edit (works). - Awarded tokens to an object I can not edit (works). - Subscribed/unsubscribed from an object I can not edit (works). - Muted/unmuted an object I can not edit (works). - Tried to do other types of edits to an object I can not edit (correctly results in a permissions error). - Joined and left a project I can not edit (works). - Tried to edit and add members to a project I can not edit (permissions error). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19586 --- .../conpherence/editor/ConpherenceEditor.php | 43 ---- ...npherenceThreadParticipantsTransaction.php | 30 +++ .../PhabricatorProjectTransactionEditor.php | 52 ----- ...PhabricatorSubscriptionsMuteController.php | 2 - ...habricatorApplicationTransactionEditor.php | 184 +++++++++++++++--- .../PhabricatorModularTransactionType.php | 26 +++ 6 files changed, 214 insertions(+), 123 deletions(-) diff --git a/src/applications/conpherence/editor/ConpherenceEditor.php b/src/applications/conpherence/editor/ConpherenceEditor.php index 3d451e78d3..3b4ad09e11 100644 --- a/src/applications/conpherence/editor/ConpherenceEditor.php +++ b/src/applications/conpherence/editor/ConpherenceEditor.php @@ -146,49 +146,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { return $xactions; } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - parent::requireCapabilities($object, $xaction); - - switch ($xaction->getTransactionType()) { - case ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE: - $old_map = array_fuse($xaction->getOldValue()); - $new_map = array_fuse($xaction->getNewValue()); - - $add = array_keys(array_diff_key($new_map, $old_map)); - $rem = array_keys(array_diff_key($old_map, $new_map)); - - $actor_phid = $this->getActingAsPHID(); - - $is_join = (($add === array($actor_phid)) && !$rem); - $is_leave = (($rem === array($actor_phid)) && !$add); - - if ($is_join) { - // Anyone can join a thread they can see. - } else if ($is_leave) { - // Anyone can leave a thread. - } else { - // You need CAN_EDIT to add or remove participants. For additional - // discussion, see D17696 and T4411. - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - } - break; - - case ConpherenceThreadTitleTransaction::TRANSACTIONTYPE: - case ConpherenceThreadTopicTransaction::TRANSACTIONTYPE: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - break; - } - } - protected function shouldSendMail( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php b/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php index 681c6d639c..c0e4c7c501 100644 --- a/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php +++ b/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php @@ -114,4 +114,34 @@ final class ConpherenceThreadParticipantsTransaction return $errors; } + public function getRequiredCapabilities( + $object, + PhabricatorApplicationTransaction $xaction) { + + $old_map = array_fuse($xaction->getOldValue()); + $new_map = array_fuse($xaction->getNewValue()); + + $add = array_keys(array_diff_key($new_map, $old_map)); + $rem = array_keys(array_diff_key($old_map, $new_map)); + + $actor_phid = $this->getActingAsPHID(); + + $is_join = (($add === array($actor_phid)) && !$rem); + $is_leave = (($rem === array($actor_phid)) && !$add); + + if ($is_join) { + // Anyone can join a thread they can see. + return null; + } + + if ($is_leave) { + // Anyone can leave a thread. + return null; + } + + // You need CAN_EDIT to add or remove participants. For additional + // discussion, see D17696 and T4411. + return PhabricatorPolicyCapability::CAN_EDIT; + } + } diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index ef0cc65c8e..581ec23153 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -115,58 +115,6 @@ final class PhabricatorProjectTransactionEditor return $errors; } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case PhabricatorTransactions::TYPE_EDGE: - switch ($xaction->getMetadataValue('edge:type')) { - case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST: - $old = $xaction->getOldValue(); - $new = $xaction->getNewValue(); - - $add = array_keys(array_diff_key($new, $old)); - $rem = array_keys(array_diff_key($old, $new)); - - $actor_phid = $this->requireActor()->getPHID(); - - $is_join = (($add === array($actor_phid)) && !$rem); - $is_leave = (($rem === array($actor_phid)) && !$add); - - if ($is_join) { - // You need CAN_JOIN to join a project. - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_JOIN); - } else if ($is_leave) { - // You usually don't need any capabilities to leave a project. - if ($object->getIsMembershipLocked()) { - // you must be able to edit though to leave locked projects - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - } - } else { - if (!$this->getIsNewObject()) { - // You need CAN_EDIT to change members other than yourself. - // (PHI193) Just skip this check if we're creating a project. - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - } - } - return; - } - break; - } - - return parent::requireCapabilities($object, $xaction); - } - protected function willPublish(PhabricatorLiskDAO $object, array $xactions) { // NOTE: We're using the omnipotent user here because the original actor // may no longer have permission to view the object. diff --git a/src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php b/src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php index 1369643ffc..e29d5e3802 100644 --- a/src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php +++ b/src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php @@ -44,8 +44,6 @@ final class PhabricatorSubscriptionsMuteController ); } - $muted_type = PhabricatorMutedByEdgeType::EDGECONST; - $xaction = id($object->getApplicationTransactionTemplate()) ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) ->setMetadataValue('edge:type', $muted_type) diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 7e98ebb364..1873fcb6e4 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -976,6 +976,27 @@ abstract class PhabricatorApplicationTransactionEditor $this->adjustTransactionValues($object, $xaction); } + // Now that we've merged and combined transactions, check for required + // capabilities. Note that we're doing this before filtering + // transactions: if you try to apply an edit which you do not have + // permission to apply, we want to give you a permissions error even + // if the edit would have no effect. + $this->applyCapabilityChecks($object, $xactions); + + // See T13186. Fatal hard if this object has an older + // "requireCapabilities()" method. The code may rely on this method being + // called to apply policy checks, so err on the side of safety and fatal. + // TODO: Remove this check after some time has passed. + if (method_exists($this, 'requireCapabilities')) { + throw new Exception( + pht( + 'Editor (of class "%s") implements obsolete policy method '. + 'requireCapabilities(). The implementation for this Editor '. + 'MUST be updated. See <%s> for discussion.', + get_class($this), + 'https://secure.phabricator.com/T13186')); + } + $xactions = $this->filterTransactions($object, $xactions); // TODO: Once everything is on EditEngine, just use getIsNewObject() to @@ -994,12 +1015,6 @@ abstract class PhabricatorApplicationTransactionEditor } } - // Now that we've merged, filtered, and combined transactions, check for - // required capabilities. - foreach ($xactions as $xaction) { - $this->requireCapabilities($object, $xaction); - } - $xactions = $this->sortTransactions($xactions); $file_phids = $this->extractFilePHIDs($object, $xactions); @@ -1459,34 +1474,151 @@ abstract class PhabricatorApplicationTransactionEditor } } - protected function requireCapabilities( + private function applyCapabilityChecks( PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { + array $xactions) { + assert_instances_of($xactions, 'PhabricatorApplicationTransaction'); + + $can_edit = PhabricatorPolicyCapability::CAN_EDIT; if ($this->getIsNewObject()) { - return; + // If we're creating a new object, we don't need any special capabilities + // on the object. The actor has already made it through creation checks, + // and objects which haven't been created yet often can not be + // meaningfully tested for capabilities anyway. + $required_capabilities = array(); + } else { + if (!$xactions && !$this->xactions) { + // If we aren't doing anything, require CAN_EDIT to improve consistency. + $required_capabilities = array($can_edit); + } else { + $required_capabilities = array(); + + foreach ($xactions as $xaction) { + $type = $xaction->getTransactionType(); + + $xtype = $this->getModularTransactionType($type); + if (!$xtype) { + $capabilities = $this->getLegacyRequiredCapabilities($xaction); + } else { + $capabilities = $xtype->getRequiredCapabilities($object, $xaction); + } + + // For convenience, we allow flexibility in the return types because + // it's very unusual that a transaction actually requires multiple + // capability checks. + if ($capabilities === null) { + $capabilities = array(); + } else { + $capabilities = (array)$capabilities; + } + + foreach ($capabilities as $capability) { + $required_capabilities[$capability] = $capability; + } + } + } } - $actor = $this->requireActor(); - switch ($xaction->getTransactionType()) { - case PhabricatorTransactions::TYPE_COMMENT: - PhabricatorPolicyFilter::requireCapability( - $actor, - $object, - PhabricatorPolicyCapability::CAN_VIEW); - break; - case PhabricatorTransactions::TYPE_VIEW_POLICY: - case PhabricatorTransactions::TYPE_EDIT_POLICY: - case PhabricatorTransactions::TYPE_JOIN_POLICY: - case PhabricatorTransactions::TYPE_SPACE: - PhabricatorPolicyFilter::requireCapability( - $actor, - $object, - PhabricatorPolicyCapability::CAN_EDIT); - break; + $required_capabilities = array_fuse($required_capabilities); + $actor = $this->getActor(); + + if ($required_capabilities) { + id(new PhabricatorPolicyFilter()) + ->setViewer($actor) + ->requireCapabilities($required_capabilities) + ->raisePolicyExceptions(true) + ->apply(array($object)); } } + private function getLegacyRequiredCapabilities( + PhabricatorApplicationTransaction $xaction) { + + $type = $xaction->getTransactionType(); + switch ($type) { + case PhabricatorTransactions::TYPE_COMMENT: + // TODO: Comments technically require CAN_INTERACT, but this is + // currently somewhat special and handled through EditEngine. For now, + // don't enforce it here. + return null; + case PhabricatorTransactions::TYPE_SUBSCRIBERS: + // TODO: Removing subscribers other than yourself should probably + // require CAN_EDIT permission. You can do this via the API but + // generally can not via the web interface. + return null; + case PhabricatorTransactions::TYPE_TOKEN: + // TODO: This technically requires CAN_INTERACT, like comments. + return null; + case PhabricatorTransactions::TYPE_HISTORY: + // This is a special magic transaction which sends you history via + // email and is only partially supported in the upstream. You don't + // need any capabilities to apply it. + return null; + case PhabricatorTransactions::TYPE_EDGE: + return $this->getLegacyRequiredEdgeCapabilities($xaction); + default: + // For other older (non-modular) transactions, always require exactly + // CAN_EDIT. Transactions which do not need CAN_EDIT or need additional + // capabilities must move to ModularTransactions. + return PhabricatorPolicyCapability::CAN_EDIT; + } + } + + private function getLegacyRequiredEdgeCapabilities( + PhabricatorApplicationTransaction $xaction) { + + // You don't need to have edit permission on an object to mention it or + // otherwise add a relationship pointing toward it. + if ($this->getIsInverseEdgeEditor()) { + return null; + } + + $edge_type = $xaction->getMetadataValue('edge:type'); + switch ($edge_type) { + case PhabricatorMutedByEdgeType::EDGECONST: + // At time of writing, you can only write this edge for yourself, so + // you don't need permissions. If you can eventually mute an object + // for other users, this would need to be revisited. + return null; + case PhabricatorObjectMentionsObjectEdgeType::EDGECONST: + return null; + case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST: + $old = $xaction->getOldValue(); + $new = $xaction->getNewValue(); + + $add = array_keys(array_diff_key($new, $old)); + $rem = array_keys(array_diff_key($old, $new)); + + $actor_phid = $this->requireActor()->getPHID(); + + $is_join = (($add === array($actor_phid)) && !$rem); + $is_leave = (($rem === array($actor_phid)) && !$add); + + if ($is_join) { + // You need CAN_JOIN to join a project. + return PhabricatorPolicyCapability::CAN_JOIN; + } + + if ($is_leave) { + $object = $this->object; + // You usually don't need any capabilities to leave a project... + if ($object->getIsMembershipLocked()) { + // ...you must be able to edit to leave locked projects, though. + return PhabricatorPolicyCapability::CAN_EDIT; + } else { + return null; + } + } + + // You need CAN_EDIT to change members other than yourself. + return PhabricatorPolicyCapability::CAN_EDIT; + default: + return PhabricatorPolicyCapability::CAN_EDIT; + } + } + + private function buildSubscribeTransaction( PhabricatorLiskDAO $object, array $xactions, diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index b0714aeccf..3d81e02c38 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -366,4 +366,30 @@ abstract class PhabricatorModularTransactionType $capability); } + /** + * Get a list of capabilities the actor must have on the object to apply + * a transaction to it. + * + * Usually, you should use this to reduce capability requirements when a + * transaction (like leaving a Conpherence thread) can be applied without + * having edit permission on the object. You can override this method to + * remove the CAN_EDIT requirement, or to replace it with a different + * requirement. + * + * If you are increasing capability requirements and need to add an + * additional capability or policy requirement above and beyond CAN_EDIT, it + * is usually better implemented as a validation check. + * + * @param object Object being edited. + * @param PhabricatorApplicationTransaction Transaction being applied. + * @return null|const|list A capability constant (or list of + * capability constants) which the actor must have on the object. You can + * return `null` as a shorthand for "no capabilities are required". + */ + public function getRequiredCapabilities( + $object, + PhabricatorApplicationTransaction $xaction) { + return PhabricatorPolicyCapability::CAN_EDIT; + } + } From 2f7b10c023b96722dee3489419cfb5acd74f363a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 27 Aug 2018 06:32:21 -0700 Subject: [PATCH 02/27] Replace "Disable User" web UI flow with transactions Summary: Ref T13189. See PHI642. Upgrades the "Disable" action in the web UI to be transaction-based. This technically breaks things a little (you can't disable non-bot users, since they now require CAN_EDIT and you won't have it) but an upcoming change will fix the permissions issue. Test Plan: Disabled and enabled a (bot) user from the web UI. Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19604 --- .../PhabricatorPeopleDisableController.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/applications/people/controller/PhabricatorPeopleDisableController.php b/src/applications/people/controller/PhabricatorPeopleDisableController.php index a556f8e519..f51f42047b 100644 --- a/src/applications/people/controller/PhabricatorPeopleDisableController.php +++ b/src/applications/people/controller/PhabricatorPeopleDisableController.php @@ -39,9 +39,18 @@ final class PhabricatorPeopleDisableController } if ($request->isFormPost()) { - id(new PhabricatorUserEditor()) + $xactions = array(); + + $xactions[] = id(new PhabricatorUserTransaction()) + ->setTransactionType(PhabricatorUserDisableTransaction::TRANSACTIONTYPE) + ->setNewValue($should_disable); + + id(new PhabricatorUserTransactionEditor()) ->setActor($viewer) - ->disableUser($user, $should_disable); + ->setContentSourceFromRequest($request) + ->setContinueOnMissingFields(true) + ->setContinueOnNoEffect(true) + ->applyTransactions($user, $xactions); return id(new AphrontRedirectResponse())->setURI($done_uri); } From 8cf56913d8ef1a5c8cdbfbe94057177b00d83f34 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 15:32:30 -0700 Subject: [PATCH 03/27] Deprecate "user.enable" and "user.disable" API methods, redefine them in terms of "user.edit" Summary: Depends on D19604. Ref T13189. See PHI642. Deprecates these in favor of "user.edit", redefines them in terms of it, and removes the old `disableUser()` method. I kept the "is admin" permissions check for consistency, since these methods have always said "(admin only)". This check may not be the most tailored check soon, but we can just keep executing it in addition to the real check. For now, this change stops this method from actually disabling non-bot users (since it implicitly adds a CAN_EDIT requirement, and even administrators don't have that). An upcoming change will fix that. Test Plan: Enabled and disabled a (bot) user via these methods. Checked API UI, saw them marked as "disabled". Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19605 --- .../conduit/UserDisableConduitAPIMethod.php | 28 +++++++++++-- .../conduit/UserEnableConduitAPIMethod.php | 28 +++++++++++-- .../people/editor/PhabricatorUserEditor.php | 39 ------------------- 3 files changed, 48 insertions(+), 47 deletions(-) diff --git a/src/applications/people/conduit/UserDisableConduitAPIMethod.php b/src/applications/people/conduit/UserDisableConduitAPIMethod.php index 3512d3f742..a6f2e2c4f1 100644 --- a/src/applications/people/conduit/UserDisableConduitAPIMethod.php +++ b/src/applications/people/conduit/UserDisableConduitAPIMethod.php @@ -10,6 +10,14 @@ final class UserDisableConduitAPIMethod extends UserConduitAPIMethod { return pht('Permanently disable specified users (admin only).'); } + public function getMethodStatus() { + return self::METHOD_STATUS_DEPRECATED; + } + + public function getMethodStatusDescription() { + return pht('Obsoleted by method "user.edit".'); + } + protected function defineParamTypes() { return array( 'phids' => 'required list', @@ -43,11 +51,23 @@ final class UserDisableConduitAPIMethod extends UserConduitAPIMethod { throw new ConduitException('ERR-BAD-PHID'); } - foreach ($users as $user) { - id(new PhabricatorUserEditor()) - ->setActor($actor) - ->disableUser($user, true); + foreach ($phids as $phid) { + $params = array( + 'transactions' => array( + array( + 'type' => 'disabled', + 'value' => true, + ), + ), + 'objectIdentifier' => $phid, + ); + + id(new ConduitCall('user.edit', $params)) + ->setUser($actor) + ->execute(); } + + return null; } } diff --git a/src/applications/people/conduit/UserEnableConduitAPIMethod.php b/src/applications/people/conduit/UserEnableConduitAPIMethod.php index f7cb117df6..3e4e09b778 100644 --- a/src/applications/people/conduit/UserEnableConduitAPIMethod.php +++ b/src/applications/people/conduit/UserEnableConduitAPIMethod.php @@ -10,6 +10,14 @@ final class UserEnableConduitAPIMethod extends UserConduitAPIMethod { return pht('Re-enable specified users (admin only).'); } + public function getMethodStatus() { + return self::METHOD_STATUS_DEPRECATED; + } + + public function getMethodStatusDescription() { + return pht('Obsoleted by method "user.edit".'); + } + protected function defineParamTypes() { return array( 'phids' => 'required list', @@ -43,11 +51,23 @@ final class UserEnableConduitAPIMethod extends UserConduitAPIMethod { throw new ConduitException('ERR-BAD-PHID'); } - foreach ($users as $user) { - id(new PhabricatorUserEditor()) - ->setActor($actor) - ->disableUser($user, false); + foreach ($phids as $phid) { + $params = array( + 'transactions' => array( + array( + 'type' => 'disabled', + 'value' => false, + ), + ), + 'objectIdentifier' => $phid, + ); + + id(new ConduitCall('user.edit', $params)) + ->setUser($actor) + ->execute(); } + + return null; } } diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php index 786ead79c3..58c2ba1ff1 100644 --- a/src/applications/people/editor/PhabricatorUserEditor.php +++ b/src/applications/people/editor/PhabricatorUserEditor.php @@ -293,45 +293,6 @@ final class PhabricatorUserEditor extends PhabricatorEditor { return $this; } - /** - * @task role - */ - public function disableUser(PhabricatorUser $user, $disable) { - $actor = $this->requireActor(); - - if (!$user->getID()) { - throw new Exception(pht('User has not been created yet!')); - } - - $user->openTransaction(); - $user->beginWriteLocking(); - - $user->reload(); - if ($user->getIsDisabled() == $disable) { - $user->endWriteLocking(); - $user->killTransaction(); - return $this; - } - - $log = PhabricatorUserLog::initializeNewLog( - $actor, - $user->getPHID(), - PhabricatorUserLog::ACTION_DISABLE); - $log->setOldValue($user->getIsDisabled()); - $log->setNewValue($disable); - - $user->setIsDisabled((int)$disable); - $user->save(); - - $log->save(); - - $user->endWriteLocking(); - $user->saveTransaction(); - - return $this; - } - - /** * @task role */ From 058952e72ecf032735ea019e33014c9981229023 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 08:50:32 -0700 Subject: [PATCH 04/27] Add a "Can Disable Users" capability to the "People" application Summary: Depends on D19605. Ref T13189. See PHI642. This adds a separate "Can Disable Users" capability, and makes the underlying transaction use it. This doesn't actually let you weaken the permission, since all pathways need more permissions: - `user.edit` needs CAN_EDIT. - `user.disable/enable` need admin. - Web UI workflow needs admin. Upcoming changes will update these pathways. Without additional changes, this does let you //strengthen// the permission. This also fixes the inability to disable non-bot users via the web UI. Test Plan: - Set permission to "No One", tried to disable users. Got a tailored policy error. - Set permission to "All Users", disabled/enabled a non-bot user. Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19606 --- src/__phutil_library_map__.php | 2 ++ .../application/PhabricatorPeopleApplication.php | 3 +++ .../capability/PeopleDisableUsersCapability.php | 16 ++++++++++++++++ .../PhabricatorUserDisableTransaction.php | 14 ++++++++++++++ 4 files changed, 35 insertions(+) create mode 100644 src/applications/people/capability/PeopleDisableUsersCapability.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index af6fa96ba3..05ef3c96fe 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2042,6 +2042,7 @@ phutil_register_library_map(array( 'PasteSearchConduitAPIMethod' => 'applications/paste/conduit/PasteSearchConduitAPIMethod.php', 'PeopleBrowseUserDirectoryCapability' => 'applications/people/capability/PeopleBrowseUserDirectoryCapability.php', 'PeopleCreateUsersCapability' => 'applications/people/capability/PeopleCreateUsersCapability.php', + 'PeopleDisableUsersCapability' => 'applications/people/capability/PeopleDisableUsersCapability.php', 'PeopleHovercardEngineExtension' => 'applications/people/engineextension/PeopleHovercardEngineExtension.php', 'PeopleMainMenuBarExtension' => 'applications/people/engineextension/PeopleMainMenuBarExtension.php', 'PeopleUserLogGarbageCollector' => 'applications/people/garbagecollector/PeopleUserLogGarbageCollector.php', @@ -7592,6 +7593,7 @@ phutil_register_library_map(array( 'PasteSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'PeopleBrowseUserDirectoryCapability' => 'PhabricatorPolicyCapability', 'PeopleCreateUsersCapability' => 'PhabricatorPolicyCapability', + 'PeopleDisableUsersCapability' => 'PhabricatorPolicyCapability', 'PeopleHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', 'PeopleMainMenuBarExtension' => 'PhabricatorMainMenuBarExtension', 'PeopleUserLogGarbageCollector' => 'PhabricatorGarbageCollector', diff --git a/src/applications/people/application/PhabricatorPeopleApplication.php b/src/applications/people/application/PhabricatorPeopleApplication.php index 6322b29b24..9238d8da3b 100644 --- a/src/applications/people/application/PhabricatorPeopleApplication.php +++ b/src/applications/people/application/PhabricatorPeopleApplication.php @@ -97,6 +97,9 @@ final class PhabricatorPeopleApplication extends PhabricatorApplication { PeopleCreateUsersCapability::CAPABILITY => array( 'default' => PhabricatorPolicies::POLICY_ADMIN, ), + PeopleDisableUsersCapability::CAPABILITY => array( + 'default' => PhabricatorPolicies::POLICY_ADMIN, + ), PeopleBrowseUserDirectoryCapability::CAPABILITY => array(), ); } diff --git a/src/applications/people/capability/PeopleDisableUsersCapability.php b/src/applications/people/capability/PeopleDisableUsersCapability.php new file mode 100644 index 0000000000..bb58ed2e76 --- /dev/null +++ b/src/applications/people/capability/PeopleDisableUsersCapability.php @@ -0,0 +1,16 @@ +requireApplicationCapability( + PeopleDisableUsersCapability::CAPABILITY); + if ($this->getActingAsPHID() === $object->getPHID()) { $errors[] = $this->newInvalidError( pht('You can not enable or disable your own account.')); @@ -69,4 +73,14 @@ final class PhabricatorUserDisableTransaction return $errors; } + public function getRequiredCapabilities( + $object, + PhabricatorApplicationTransaction $xaction) { + + // You do not need to be able to edit users to disable them. Instead, this + // requirement is replaced with a requirement that you have the "Can + // Disable Users" permission. + + return null; + } } From f9192d07f2c798b5f4257c7cdf6cfb566fe6e3cc Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 10:43:40 -0700 Subject: [PATCH 05/27] Align web UI "Disable" and "Approve/Disapprove" flows with new "Can Disable Users" permission Summary: Depends on D19606. Ref T13189. See PHI642. - Disabling/enabling users no longer requires admin. Now, you just need "Can Disable Users". - Update the UI to appropriately show the action in black or grey depending on what clicking the button will do. - For "Approve/Disapprove", fix a couple bugs, then let them go through without respect for "Can Disable Users". This is conceptually a different action, even though it ultimately sets the "Disabled" flag. Test Plan: - Disabled/enabled users from the web UI as various users, including a non-administrator with "Can Disable Users". - Hit permissions errors from the web UI as various users, including an administrator without "Can Disable Users". - Saw the "Disable/Enable" action activate properly based on whether clicking the button would actually work. - Disapproved a user without "Can Disable Users" permission, tried to re-disapprove a user. - Approved a user, tried to reapprove a user. Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19607 --- .../PhabricatorPeopleApproveController.php | 7 ++++ .../PhabricatorPeopleDisableController.php | 34 +++++++++++++++++-- ...abricatorPeopleProfileManageController.php | 19 +++++++---- 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/applications/people/controller/PhabricatorPeopleApproveController.php b/src/applications/people/controller/PhabricatorPeopleApproveController.php index 58cd2e2119..0e97ad6ee6 100644 --- a/src/applications/people/controller/PhabricatorPeopleApproveController.php +++ b/src/applications/people/controller/PhabricatorPeopleApproveController.php @@ -16,6 +16,13 @@ final class PhabricatorPeopleApproveController $done_uri = $this->getApplicationURI('query/approval/'); + if ($user->getIsApproved()) { + return $this->newDialog() + ->setTitle(pht('Already Approved')) + ->appendChild(pht('This user has already been approved.')) + ->addCancelButton($done_uri); + } + if ($request->isFormPost()) { id(new PhabricatorUserEditor()) ->setActor($viewer) diff --git a/src/applications/people/controller/PhabricatorPeopleDisableController.php b/src/applications/people/controller/PhabricatorPeopleDisableController.php index f51f42047b..9f2718086b 100644 --- a/src/applications/people/controller/PhabricatorPeopleDisableController.php +++ b/src/applications/people/controller/PhabricatorPeopleDisableController.php @@ -3,10 +3,14 @@ final class PhabricatorPeopleDisableController extends PhabricatorPeopleController { + public function shouldRequireAdmin() { + return false; + } + public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); $id = $request->getURIData('id'); - $via = $request->getURIData('id'); + $via = $request->getURIData('via'); $user = id(new PhabricatorPeopleQuery()) ->setViewer($viewer) @@ -20,11 +24,36 @@ final class PhabricatorPeopleDisableController // on profiles and also via the "X" action on the approval queue. We do // things slightly differently depending on the context the actor is in. + // In particular, disabling via "Disapprove" requires you be an + // administrator (and bypasses the "Can Disable Users" permission). + // Disabling via "Disable" requires the permission only. + $is_disapprove = ($via == 'disapprove'); if ($is_disapprove) { $done_uri = $this->getApplicationURI('query/approval/'); + + if (!$viewer->getIsAdmin()) { + return $this->newDialog() + ->setTitle(pht('No Permission')) + ->appendParagraph(pht('Only administrators can disapprove users.')) + ->addCancelButton($done_uri); + } + + if ($user->getIsApproved()) { + return $this->newDialog() + ->setTitle(pht('Already Approved')) + ->appendParagraph(pht('This user has already been approved.')) + ->addCancelButton($done_uri); + } + + // On the "Disapprove" flow, bypass the "Can Disable Users" permission. + $actor = PhabricatorUser::getOmnipotentUser(); $should_disable = true; } else { + $this->requireApplicationCapability( + PeopleDisableUsersCapability::CAPABILITY); + + $actor = $viewer; $done_uri = $this->getApplicationURI("manage/{$id}/"); $should_disable = !$user->getIsDisabled(); } @@ -46,7 +75,8 @@ final class PhabricatorPeopleDisableController ->setNewValue($should_disable); id(new PhabricatorUserTransactionEditor()) - ->setActor($viewer) + ->setActor($actor) + ->setActingAsPHID($viewer->getPHID()) ->setContentSourceFromRequest($request) ->setContinueOnMissingFields(true) ->setContinueOnNoEffect(true) diff --git a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php index 2ac3e6de89..9759a375c7 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php @@ -75,11 +75,22 @@ final class PhabricatorPeopleProfileManageController private function buildCurtain(PhabricatorUser $user) { $viewer = $this->getViewer(); + $is_self = ($user->getPHID() === $viewer->getPHID()); + $can_edit = PhabricatorPolicyFilter::hasCapability( $viewer, $user, PhabricatorPolicyCapability::CAN_EDIT); + $is_admin = $viewer->getIsAdmin(); + $can_admin = ($is_admin && !$is_self); + + $has_disable = $this->hasApplicationCapability( + PeopleDisableUsersCapability::CAPABILITY); + $can_disable = ($has_disable && !$is_self); + + $can_welcome = ($is_admin && $user->canEstablishWebSessions()); + $curtain = $this->newCurtainView($user); $curtain->addAction( @@ -114,10 +125,6 @@ final class PhabricatorPeopleProfileManageController $empower_name = pht('Make Administrator'); } - $is_admin = $viewer->getIsAdmin(); - $is_self = ($user->getPHID() === $viewer->getPHID()); - $can_admin = ($is_admin && !$is_self); - $curtain->addAction( id(new PhabricatorActionView()) ->setIcon($empower_icon) @@ -146,7 +153,7 @@ final class PhabricatorPeopleProfileManageController id(new PhabricatorActionView()) ->setIcon($disable_icon) ->setName($disable_name) - ->setDisabled(!$can_admin) + ->setDisabled(!$can_disable) ->setWorkflow(true) ->setHref($this->getApplicationURI('disable/'.$user->getID().'/'))); @@ -158,8 +165,6 @@ final class PhabricatorPeopleProfileManageController ->setWorkflow(true) ->setHref($this->getApplicationURI('delete/'.$user->getID().'/'))); - $can_welcome = ($is_admin && $user->canEstablishWebSessions()); - $curtain->addAction( id(new PhabricatorActionView()) ->setIcon('fa-envelope') From cd8b5b82c8602f64a42efaaaa80ab96f40e6bbea Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 27 Aug 2018 07:52:04 -0700 Subject: [PATCH 06/27] Stop requiring CAN_EDIT to reach the TransactionEditor via "*.edit" in EditEngine Summary: Depends on D19607. Ref T13189. See PHI642. Ref T13186. Some transactions can sometimes be applied to objects you can not edit. Currently, using `*.edit` to edit an object always explicitly requires CAN_EDIT. Now that individual transactions require CAN_EDIT by default and can reduce or replace this requirement, stop requiring CAN_EDIT to reach the editor. The only expected effect of this change is that low-permission edits (like disabling a user, leaving a project, or leaving a thread) can now work via `*.edit`. Test Plan: - Tried to perform a normal edit (changing a task title) against an object with no CAN_EDIT. Still got a permissions error. - As a non-admin, disabled other users while holding the "Can Disable Users" permission. - As a non-admin, got a permissions error while trying to disable other users while not holding the "Can Disable Users" permission. Reviewers: amckinley Maniphest Tasks: T13189, T13186 Differential Revision: https://secure.phabricator.com/D19608 --- .../editengine/PhabricatorEditEngine.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index f8f6f371bc..ad495a3a44 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -2003,7 +2003,19 @@ abstract class PhabricatorEditEngine $identifier = $request->getValue('objectIdentifier'); if ($identifier) { $this->setIsCreate(false); - $object = $this->newObjectFromIdentifier($identifier); + + // After T13186, each transaction can individually weaken or replace the + // capabilities required to apply it, so we no longer need CAN_EDIT to + // attempt to apply transactions to objects. In practice, almost all + // transactions require CAN_EDIT so we won't get very far if we don't + // have it. + $capabilities = array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + + $object = $this->newObjectFromIdentifier( + $identifier, + $capabilities); } else { $this->requireCreateCapability(); From 0295a0022917ab7570cda02df27768558474ae80 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 27 Aug 2018 09:27:32 -0700 Subject: [PATCH 07/27] When there are no setup issues, don't show a weird empty box Summary: Ref T13189. When there are no setup issues, we currently double-render a weird setup issues box underneath the notice. Get rid of it. Test Plan: Viewed page with and without setup issues, saw less awkward UI. Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19609 --- .../PhabricatorConfigIssueListController.php | 34 ++++++++----------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/src/applications/config/controller/PhabricatorConfigIssueListController.php b/src/applications/config/controller/PhabricatorConfigIssueListController.php index 770f79a9f2..0ca94abe04 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueListController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueListController.php @@ -33,27 +33,26 @@ final class PhabricatorConfigIssueListController PhabricatorSetupCheck::GROUP_OTHER, 'fa-question-circle'); - $no_issues = null; - if (empty($issues)) { - $no_issues = id(new PHUIInfoView()) + $title = pht('Setup Issues'); + $header = $this->buildHeaderView($title); + + if (!$issues) { + $issue_list = id(new PHUIInfoView()) ->setTitle(pht('No Issues')) ->appendChild( pht('Your install has no current setup issues to resolve.')) ->setSeverity(PHUIInfoView::SEVERITY_NOTICE); + } else { + $issue_list = array( + $important, + $php, + $mysql, + $other, + ); + + $issue_list = $this->buildConfigBoxView(pht('Issues'), $issue_list); } - $title = pht('Setup Issues'); - $header = $this->buildHeaderView($title); - - $issue_list = array( - $important, - $php, - $mysql, - $other, - ); - - $issue_list = $this->buildConfigBoxView(pht('Issues'), $issue_list); - $crumbs = $this->buildApplicationCrumbs() ->addTextCrumb($title) ->setBorder(true); @@ -62,10 +61,7 @@ final class PhabricatorConfigIssueListController ->setHeader($header) ->setNavigation($nav) ->setFixed(true) - ->setMainColumn(array( - $no_issues, - $issue_list, - )); + ->setMainColumn($issue_list); return $this->newPage() ->setTitle($title) From b87a809b0bfe9a44243c7540841fdb185207cb0e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 27 Aug 2018 09:48:11 -0700 Subject: [PATCH 08/27] Make some remarkup handling in Config cleaner, fixing {{other.option} links Summary: Depends on D19609. Ref T13189. At some point, we switched from RemarkupEngine to RemarkupView and lost this piece of hack-magic. Restore the hack-magic. It's still hack-magic instead of a real rule, but things are at least cleaner than they were before. Test Plan: Viewed `auth.require-approval`, etc. Saw references to other config options linked properly. Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19610 --- src/__phutil_library_map__.php | 5 +-- .../PhabricatorConfigEditController.php | 26 +++-------- .../config/option/PhabricatorConfigOption.php | 43 ++++--------------- 3 files changed, 16 insertions(+), 58 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 05ef3c96fe..b6d71ea81b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -8290,10 +8290,7 @@ phutil_register_library_map(array( 'PhabricatorConfigManualActivity' => 'PhabricatorConfigEntryDAO', 'PhabricatorConfigModule' => 'Phobject', 'PhabricatorConfigModuleController' => 'PhabricatorConfigController', - 'PhabricatorConfigOption' => array( - 'Phobject', - 'PhabricatorMarkupInterface', - ), + 'PhabricatorConfigOption' => 'Phobject', 'PhabricatorConfigOptionType' => 'Phobject', 'PhabricatorConfigPHIDModule' => 'PhabricatorConfigModule', 'PhabricatorConfigProxySource' => 'PhabricatorConfigSource', diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index 37f7db6c7c..224705e181 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -153,30 +153,16 @@ final class PhabricatorConfigEditController $e_value); } - $engine = new PhabricatorMarkupEngine(); - $engine->setViewer($viewer); - $engine->addObject($option, 'description'); - $engine->process(); - $description = phutil_tag( - 'div', - array( - 'class' => 'phabricator-remarkup', - ), - $engine->getOutput($option, 'description')); - $form ->setUser($viewer) ->addHiddenInput('issue', $request->getStr('issue')); - $description = $option->getDescription(); - if (strlen($description)) { - $description_view = new PHUIRemarkupView($viewer, $description); - - $form - ->appendChild( - id(new AphrontFormMarkupControl()) - ->setLabel(pht('Description')) - ->setValue($description_view)); + $description = $option->newDescriptionRemarkupView($viewer); + if ($description) { + $form->appendChild( + id(new AphrontFormMarkupControl()) + ->setLabel(pht('Description')) + ->setValue($description)); } if ($group) { diff --git a/src/applications/config/option/PhabricatorConfigOption.php b/src/applications/config/option/PhabricatorConfigOption.php index 11b8de2617..385af002c7 100644 --- a/src/applications/config/option/PhabricatorConfigOption.php +++ b/src/applications/config/option/PhabricatorConfigOption.php @@ -1,8 +1,7 @@ getKey().':'.$field; - } - - public function newMarkupEngine($field) { - return PhabricatorMarkupEngine::newMarkupEngine(array()); - } - - public function getMarkupText($field) { - switch ($field) { - case 'description': - $text = $this->getDescription(); - break; - case 'summary': - $text = $this->getSummary(); - break; + public function newDescriptionRemarkupView(PhabricatorUser $viewer) { + $description = $this->getDescription(); + if (!strlen($description)) { + return null; } - // TODO: We should probably implement this as a real Markup rule, but - // markup rules are a bit of a mess right now and it doesn't hurt us to - // fake this. - $text = preg_replace( + // TODO: Some day, we should probably implement this as a real rule. + $description = preg_replace( '/{{([^}]+)}}/', '[[/config/edit/\\1/ | \\1]]', - $text); + $description); - return $text; - } - - public function didMarkupText($field, $output, PhutilMarkupEngine $engine) { - return $output; - } - - public function shouldUseMarkupCache($field) { - return false; + return new PHUIRemarkupView($viewer, $description); } } From 415de4ce375a2477be95e37a97be938e3d038e73 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 27 Aug 2018 10:09:13 -0700 Subject: [PATCH 09/27] Repaint filetree more consistently for mobile/device views Summary: Ref T13189. See . We currently call a nonexistent `resetdrag()` which does nothing. Some sequences of interactions can result in a blank left column in mobile/device widths. Repaint the filetree away more consistently on device change. Test Plan: Viewed a revision, toggled filetree off + on, resized to narrow width. Before: bad left margin, JS console error. After: proper repaint at device breakpoint, no JS console error. Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19611 --- resources/celerity/map.php | 26 +++++----- .../rsrc/js/core/behavior-phabricator-nav.js | 51 ++++++++++++------- 2 files changed, 47 insertions(+), 30 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 66a568dabe..d674d399e7 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', 'core.pkg.css' => 'fc4839c8', - 'core.pkg.js' => '2058ec09', + 'core.pkg.js' => 'b5a949ca', 'differential.pkg.css' => '06dc617c', 'differential.pkg.js' => 'c1cfa143', 'diffusion.pkg.css' => 'a2d17c7d', @@ -473,7 +473,7 @@ return array( 'rsrc/js/core/behavior-more.js' => 'a80d0378', 'rsrc/js/core/behavior-object-selector.js' => '77c1f0b0', 'rsrc/js/core/behavior-oncopy.js' => '2926fff2', - 'rsrc/js/core/behavior-phabricator-nav.js' => '94b7c320', + 'rsrc/js/core/behavior-phabricator-nav.js' => '9d32bc88', 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => 'acd29eee', 'rsrc/js/core/behavior-read-only-warning.js' => 'ba158207', 'rsrc/js/core/behavior-redirect.js' => '0213259f', @@ -631,7 +631,7 @@ return array( 'javelin-behavior-phabricator-keyboard-pager' => 'a8da01f0', 'javelin-behavior-phabricator-keyboard-shortcuts' => '01fca1f0', 'javelin-behavior-phabricator-line-linker' => '66a62306', - 'javelin-behavior-phabricator-nav' => '94b7c320', + 'javelin-behavior-phabricator-nav' => '9d32bc88', 'javelin-behavior-phabricator-notification-example' => '8ce821c5', 'javelin-behavior-phabricator-object-selector' => '77c1f0b0', 'javelin-behavior-phabricator-oncopy' => '2926fff2', @@ -1628,16 +1628,6 @@ return array( 'javelin-resource', 'javelin-routable', ), - '94b7c320' => array( - 'javelin-behavior', - 'javelin-behavior-device', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-magical-init', - 'javelin-vector', - 'javelin-request', - 'javelin-util', - ), '960f6a39' => array( 'javelin-behavior', 'javelin-dom', @@ -1658,6 +1648,16 @@ return array( 'javelin-workflow', 'javelin-stratcom', ), + '9d32bc88' => array( + 'javelin-behavior', + 'javelin-behavior-device', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-magical-init', + 'javelin-vector', + 'javelin-request', + 'javelin-util', + ), '9d9685d6' => array( 'phui-oi-list-view-css', ), diff --git a/webroot/rsrc/js/core/behavior-phabricator-nav.js b/webroot/rsrc/js/core/behavior-phabricator-nav.js index cf6dc880bc..8bc4ad04e8 100644 --- a/webroot/rsrc/js/core/behavior-phabricator-nav.js +++ b/webroot/rsrc/js/core/behavior-phabricator-nav.js @@ -20,10 +20,13 @@ JX.behavior('phabricator-nav', function(config) { // - Flexible Navigation Column ------------------------------------------------ - var dragging; var track; + var collapsed = config.collapsed; + var narrowed; + var visible = null; + JX.enableDispatch(document.body, 'mousemove'); JX.DOM.listen(drag, 'mousedown', null, function(e) { @@ -95,6 +98,7 @@ JX.behavior('phabricator-nav', function(config) { if (!dragging) { return; } + JX.DOM.alterClass(document.body, 'jx-drag-col', false); dragging = false; @@ -117,6 +121,29 @@ JX.behavior('phabricator-nav', function(config) { return (JX.$V(drag).x - JX.Vector.getScroll().x); } + function repaint() { + narrowed = !JX.Device.isDesktop(); + + var was_visible = visible; + visible = (!collapsed && !narrowed); + + if (was_visible === visible) { + return; + } + + if (!visible) { + savedrag(); + } + + JX.DOM.alterClass(main, 'has-local-nav', visible); + JX.DOM.alterClass(main, 'has-drag-nav', visible); + JX.DOM.alterClass(main, 'has-closed-nav', !visible); + + if (visible) { + restoredrag(); + } + } + var saved_width = config.width; function savedrag() { saved_width = get_width(); @@ -136,21 +163,10 @@ JX.behavior('phabricator-nav', function(config) { content.style.marginLeft = (saved_width + JX.Vector.getDim(drag).x) + 'px'; } - var collapsed = config.collapsed; JX.Stratcom.listen('differential-filetree-toggle', null, function() { collapsed = !collapsed; - if (collapsed) { - savedrag(); - } - - JX.DOM.alterClass(main, 'has-local-nav', !collapsed); - JX.DOM.alterClass(main, 'has-drag-nav', !collapsed); - JX.DOM.alterClass(main, 'has-closed-nav', collapsed); - - if (!collapsed) { - restoredrag(); - } + repaint(); new JX.Request('/settings/adjust/', JX.bag) .setData({ key : 'nav-collapsed', value : (collapsed ? 1 : 0) }) @@ -168,7 +184,9 @@ JX.behavior('phabricator-nav', function(config) { // of the navigation bar. function onresize() { - if (JX.Device.getDevice() != 'desktop') { + repaint(); + + if (!visible) { return; } @@ -193,14 +211,13 @@ JX.behavior('phabricator-nav', function(config) { local.style.left = 0; JX.Stratcom.listen(['scroll', 'resize'], null, onresize); - onresize(); + repaint(); // - Navigation Reset ---------------------------------------------------------- JX.Stratcom.listen('phabricator-device-change', null, function() { - resetdrag(); - onresize(); + repaint(); }); }); From ee823982a46b3cf8d72628f4a7c9a06febc13340 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 27 Aug 2018 10:48:08 -0700 Subject: [PATCH 10/27] Remove another old remarkup engine callsite in Config Summary: Ref T13189. Summaries do not appear to be meaningfully rendered with Remarkup so just drop the engine. See D19610 for the previous change in this vein. Test Plan: Viewed config list with option summaries. Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19612 --- .../controller/PhabricatorConfigGroupController.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/applications/config/controller/PhabricatorConfigGroupController.php b/src/applications/config/controller/PhabricatorConfigGroupController.php index 7068ddaecc..7a3f77dfea 100644 --- a/src/applications/config/controller/PhabricatorConfigGroupController.php +++ b/src/applications/config/controller/PhabricatorConfigGroupController.php @@ -60,17 +60,10 @@ final class PhabricatorConfigGroupController $db_values = mpull($db_values, null, 'getConfigKey'); } - $engine = id(new PhabricatorMarkupEngine()) - ->setViewer($this->getRequest()->getUser()); - foreach ($options as $option) { - $engine->addObject($option, 'summary'); - } - $engine->process(); - $list = new PHUIObjectItemListView(); $list->setBig(true); foreach ($options as $option) { - $summary = $engine->getOutput($option, 'summary'); + $summary = $option->getSummary(); $item = id(new PHUIObjectItemView()) ->setHeader($option->getKey()) From 2f5c6541fc8efba09a08e7979a08c0c6de060f99 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 27 Aug 2018 10:41:59 -0700 Subject: [PATCH 11/27] Add an "Activated Epoch" and an "Acquired Epoch" to Drydock Leases Summary: Ref T13189. See PHI690. When a lease is first acquired or activated, note the time. This supports better visibility into queue lengths. For now, this is only queryable via DB and visible in the UI, but can be more broadly exposed in the future. Test Plan: Landed a revision, saw the leases get sensible timestamps for acquisition/activation. Reviewers: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19613 --- .../20180827.drydock.01.acquired.sql | 2 ++ .../20180827.drydock.02.activated.sql | 2 ++ .../controller/DrydockLeaseViewController.php | 24 +++++++++++++++++++ .../drydock/storage/DrydockLease.php | 20 ++++++++++++++++ 4 files changed, 48 insertions(+) create mode 100644 resources/sql/autopatches/20180827.drydock.01.acquired.sql create mode 100644 resources/sql/autopatches/20180827.drydock.02.activated.sql diff --git a/resources/sql/autopatches/20180827.drydock.01.acquired.sql b/resources/sql/autopatches/20180827.drydock.01.acquired.sql new file mode 100644 index 0000000000..55948391c9 --- /dev/null +++ b/resources/sql/autopatches/20180827.drydock.01.acquired.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_drydock.drydock_lease + ADD acquiredEpoch INT UNSIGNED; diff --git a/resources/sql/autopatches/20180827.drydock.02.activated.sql b/resources/sql/autopatches/20180827.drydock.02.activated.sql new file mode 100644 index 0000000000..552f7b6b24 --- /dev/null +++ b/resources/sql/autopatches/20180827.drydock.02.activated.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_drydock.drydock_lease + ADD activatedEpoch INT UNSIGNED; diff --git a/src/applications/drydock/controller/DrydockLeaseViewController.php b/src/applications/drydock/controller/DrydockLeaseViewController.php index 91a911277f..18e1d0088d 100644 --- a/src/applications/drydock/controller/DrydockLeaseViewController.php +++ b/src/applications/drydock/controller/DrydockLeaseViewController.php @@ -163,6 +163,30 @@ final class DrydockLeaseViewController extends DrydockLeaseController { } $view->addProperty(pht('Expires'), $until_display); + $acquired_epoch = $lease->getAcquiredEpoch(); + $activated_epoch = $lease->getActivatedEpoch(); + + if ($acquired_epoch) { + $acquired_display = phabricator_datetime($acquired_epoch, $viewer); + } else { + if ($activated_epoch) { + $acquired_display = phutil_tag( + 'em', + array(), + pht('Activated on Acquisition')); + } else { + $acquired_display = phutil_tag('em', array(), pht('Not Acquired')); + } + } + $view->addProperty(pht('Acquired'), $acquired_display); + + if ($activated_epoch) { + $activated_display = phabricator_datetime($activated_epoch, $viewer); + } else { + $activated_display = phutil_tag('em', array(), pht('Not Activated')); + } + $view->addProperty(pht('Activated'), $activated_display); + $attributes = $lease->getAttributes(); if ($attributes) { $view->addSectionHeader( diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php index 4cee7a5f17..866bb21b37 100644 --- a/src/applications/drydock/storage/DrydockLease.php +++ b/src/applications/drydock/storage/DrydockLease.php @@ -10,6 +10,8 @@ final class DrydockLease extends DrydockDAO protected $authorizingPHID; protected $attributes = array(); protected $status = DrydockLeaseStatus::STATUS_PENDING; + protected $acquiredEpoch; + protected $activatedEpoch; private $resource = self::ATTACHABLE; private $unconsumedCommands = self::ATTACHABLE; @@ -62,6 +64,22 @@ final class DrydockLease extends DrydockDAO $this->scheduleUpdate(); } + public function setStatus($status) { + if ($status == DrydockLeaseStatus::STATUS_ACQUIRED) { + if (!$this->getAcquiredEpoch()) { + $this->setAcquiredEpoch(PhabricatorTime::getNow()); + } + } + + if ($status == DrydockLeaseStatus::STATUS_ACTIVE) { + if (!$this->getActivatedEpoch()) { + $this->setActivatedEpoch(PhabricatorTime::getNow()); + } + } + + return parent::setStatus($status); + } + public function getLeaseName() { return pht('Lease %d', $this->getID()); } @@ -78,6 +96,8 @@ final class DrydockLease extends DrydockDAO 'resourceType' => 'text128', 'ownerPHID' => 'phid?', 'resourcePHID' => 'phid?', + 'acquiredEpoch' => 'epoch?', + 'activatedEpoch' => 'epoch?', ), self::CONFIG_KEY_SCHEMA => array( 'key_resource' => array( From 632cafec881bbabcd84ddbe7022c521dea6e81bc Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 27 Aug 2018 12:46:03 -0700 Subject: [PATCH 12/27] Pass commit authorship information to Buildkite Summary: Fixes T12251. Ref T13189. See PHI610. The difficulty here is that we don't want to disclose Phabricator account information to Buildkite. We're comfortable disclosing information from `git`, etc. - For commits, use the Identity to provide authorship information from Git. - For revisions, use the local commit information on the Diff to provide the Git/Mercurial/etc author of the HEAD commit. Test Plan: - Built commits and revisions in Buildkite via Harbormaster. - I can't actually figure out how to see author information on the Buildkite side, but the values look sane when dumped locally. Reviewers: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13189, T12251 Differential Revision: https://secure.phabricator.com/D19614 --- .../DifferentialBuildableEngine.php | 24 +++++++++++++++++++ .../harbormaster/DiffusionBuildableEngine.php | 6 +++++ .../engine/HarbormasterBuildableEngine.php | 3 +++ ...masterBuildkiteBuildStepImplementation.php | 14 +++++++++++ .../storage/PhabricatorRepositoryCommit.php | 19 +++++++++++++++ .../storage/PhabricatorRepositoryIdentity.php | 10 ++++++++ 6 files changed, 76 insertions(+) diff --git a/src/applications/differential/harbormaster/DifferentialBuildableEngine.php b/src/applications/differential/harbormaster/DifferentialBuildableEngine.php index df38885edf..8554f7be25 100644 --- a/src/applications/differential/harbormaster/DifferentialBuildableEngine.php +++ b/src/applications/differential/harbormaster/DifferentialBuildableEngine.php @@ -54,4 +54,28 @@ final class DifferentialBuildableEngine $this->applyTransactions(array($xaction)); } + public function getAuthorIdentity() { + $object = $this->getObject(); + + if ($object instanceof DifferentialRevision) { + $object = $object->loadActiveDiff(); + } + + $authorship = $object->getDiffAuthorshipDict(); + if (!isset($authorship['authorName'])) { + return null; + } + + $name = $authorship['authorName']; + $address = idx($authorship, 'authorEmail'); + + $full = id(new PhutilEmailAddress()) + ->setDisplayName($name) + ->setAddress($address); + + return id(new PhabricatorRepositoryIdentity()) + ->setIdentityName((string)$full) + ->makeEphemeral(); + } + } diff --git a/src/applications/diffusion/harbormaster/DiffusionBuildableEngine.php b/src/applications/diffusion/harbormaster/DiffusionBuildableEngine.php index bb222df28e..748386f1bd 100644 --- a/src/applications/diffusion/harbormaster/DiffusionBuildableEngine.php +++ b/src/applications/diffusion/harbormaster/DiffusionBuildableEngine.php @@ -34,4 +34,10 @@ final class DiffusionBuildableEngine $this->applyTransactions(array($xaction)); } + public function getAuthorIdentity() { + return $this->getObject() + ->loadIdentities($this->getViewer()) + ->getAuthorIdentity(); + } + } diff --git a/src/applications/harbormaster/engine/HarbormasterBuildableEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildableEngine.php index 41d31673d1..25222975b7 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildableEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildableEngine.php @@ -101,5 +101,8 @@ abstract class HarbormasterBuildableEngine $xactions); } + public function getAuthorIdentity() { + return null; + } } diff --git a/src/applications/harbormaster/step/HarbormasterBuildkiteBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterBuildkiteBuildStepImplementation.php index 8a522c4926..ef4cc0daf1 100644 --- a/src/applications/harbormaster/step/HarbormasterBuildkiteBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterBuildkiteBuildStepImplementation.php @@ -117,6 +117,20 @@ EOTEXT ), ); + $engine = HarbormasterBuildableEngine::newForObject( + $object, + $viewer); + + $author_identity = $engine->getAuthorIdentity(); + if ($author_identity) { + $data_structure += array( + 'author' => array( + 'name' => $author_identity->getIdentityDisplayName(), + 'email' => $author_identity->getIdentityEmailAddress(), + ), + ); + } + $json_data = phutil_json_encode($data_structure); $credential_phid = $this->getSetting('token'); diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 4cb9def346..f36869686f 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -200,6 +200,8 @@ final class PhabricatorRepositoryCommit $this->authorIdentity = $author; $this->committerIdentity = $committer; + + return $this; } public function getAuthorIdentity() { @@ -485,6 +487,23 @@ final class PhabricatorRepositoryCommit return null; } + public function loadIdentities(PhabricatorUser $viewer) { + if ($this->authorIdentity !== self::ATTACHABLE) { + return $this; + } + + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withIDs(array($this->getID())) + ->needIdentities(true) + ->executeOne(); + + $author_identity = $commit->getAuthorIdentity(); + $committer_identity = $commit->getCommitterIdentity(); + + return $this->attachIdentities($author_identity, $committer_identity); + } + public function hasCommitterIdentity() { return ($this->getCommitterIdentity() !== null); } diff --git a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php index f801e06b89..416d1a3cd2 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php +++ b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php @@ -65,6 +65,16 @@ final class PhabricatorRepositoryIdentity $this->getIdentityNameEncoding()); } + public function getIdentityEmailAddress() { + $address = new PhutilEmailAddress($this->getIdentityName()); + return $address->getAddress(); + } + + public function getIdentityDisplayName() { + $address = new PhutilEmailAddress($this->getIdentityName()); + return $address->getDisplayName(); + } + public function getIdentityShortName() { // TODO return $this->getIdentityName(); From 614f9ba1fba60c8c971b564c97ef4750575effd3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Aug 2018 13:02:17 -0700 Subject: [PATCH 13/27] Allow unit test results to specify that their details are formatted with remarkup when reporting to "harbormaster.sendmessage" Summary: Ref T13189. See PHI710. Ref T13088. Fixes T9951. Allow callers to `harbormaster.sendmessage` to specify that the test details are remarkup so they can use rich formatting and include links, files, etc. Test Plan: {F5840098} Reviewers: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13189, T13088, T9951 Differential Revision: https://secure.phabricator.com/D19615 --- resources/celerity/map.php | 4 +- .../DifferentialRevisionViewController.php | 2 +- .../HarbormasterBuildableViewController.php | 3 +- .../HarbormasterUnitMessageListController.php | 1 + .../HarbormasterUnitMessageViewController.php | 18 +---- .../build/HarbormasterBuildUnitMessage.php | 75 +++++++++++++++++++ .../view/HarbormasterUnitPropertyView.php | 35 ++------- .../view/HarbormasterUnitSummaryView.php | 1 + .../application/harbormaster/harbormaster.css | 7 +- 9 files changed, 93 insertions(+), 53 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index d674d399e7..3842fbe839 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -75,7 +75,7 @@ return array( 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', 'rsrc/css/application/flag/flag.css' => 'bba8f811', - 'rsrc/css/application/harbormaster/harbormaster.css' => '730a4a3c', + 'rsrc/css/application/harbormaster/harbormaster.css' => '7446ce72', 'rsrc/css/application/herald/herald-test.css' => 'a52e323e', 'rsrc/css/application/herald/herald.css' => 'cd8d0134', 'rsrc/css/application/maniphest/report.css' => '9b9580b7', @@ -554,7 +554,7 @@ return array( 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', 'global-drag-and-drop-css' => 'b556a948', - 'harbormaster-css' => '730a4a3c', + 'harbormaster-css' => '7446ce72', 'herald-css' => 'cd8d0134', 'herald-rule-editor' => 'dca75c0e', 'herald-test-css' => 'a52e323e', diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index b809a8ee80..8216c11557 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -1315,7 +1315,7 @@ final class DifferentialRevisionViewController } return id(new HarbormasterUnitSummaryView()) - ->setUser($viewer) + ->setViewer($viewer) ->setExcuse($excuse) ->setBuildable($diff->getBuildable()) ->setUnitMessages($diff->getUnitMessages()) diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php index 2ebc934605..1e79ad2b46 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php @@ -318,7 +318,7 @@ final class HarbormasterBuildableViewController if ($lint_data) { $lint_table = id(new HarbormasterLintPropertyView()) - ->setUser($viewer) + ->setViewer($viewer) ->setLimit(10) ->setLintMessages($lint_data); @@ -343,6 +343,7 @@ final class HarbormasterBuildableViewController if ($unit_data) { $unit = id(new HarbormasterUnitSummaryView()) + ->setViewer($viewer) ->setBuildable($buildable) ->setUnitMessages($unit_data) ->setShowViewAll(true) diff --git a/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php b/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php index 5f65bca86d..d548ceac98 100644 --- a/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php +++ b/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php @@ -39,6 +39,7 @@ final class HarbormasterUnitMessageListController } $unit = id(new HarbormasterUnitSummaryView()) + ->setViewer($viewer) ->setBuildable($buildable) ->setUnitMessages($unit_data); diff --git a/src/applications/harbormaster/controller/HarbormasterUnitMessageViewController.php b/src/applications/harbormaster/controller/HarbormasterUnitMessageViewController.php index 9881da06c6..5cb33c0c9a 100644 --- a/src/applications/harbormaster/controller/HarbormasterUnitMessageViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterUnitMessageViewController.php @@ -88,23 +88,7 @@ final class HarbormasterUnitMessageViewController pht('Run At'), phabricator_datetime($message->getDateCreated(), $viewer)); - $details = $message->getUnitMessageDetails(); - if (strlen($details)) { - // TODO: Use the log view here, once it gets cleaned up. - // Shenanigans below. - $details = phutil_tag( - 'div', - array( - 'class' => 'PhabricatorMonospaced', - 'style' => - 'white-space: pre-wrap; '. - 'color: #666666; '. - 'overflow-x: auto;', - ), - $details); - } else { - $details = phutil_tag('em', array(), pht('No details provided.')); - } + $details = $message->newUnitMessageDetailsView($viewer); $view->addSectionHeader( pht('Details'), diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php b/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php index 956850bd9f..b2f566c3eb 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php @@ -13,6 +13,9 @@ final class HarbormasterBuildUnitMessage private $buildTarget = self::ATTACHABLE; + const FORMAT_TEXT = 'text'; + const FORMAT_REMARKUP = 'remarkup'; + public static function initializeNewUnitMessage( HarbormasterBuildTarget $build_target) { return id(new HarbormasterBuildUnitMessage()) @@ -66,6 +69,13 @@ final class HarbormasterBuildUnitMessage 'description' => pht( 'Additional human-readable information about the failure.'), ), + 'format' => array( + 'type' => 'optional string', + 'description' => pht( + 'Format for the text provided in "details". Valid values are '. + '"text" (default) or "remarkup". This controls how test details '. + 'are rendered when shown to users.'), + ), ); } @@ -104,6 +114,11 @@ final class HarbormasterBuildUnitMessage $obj->setProperty('details', $details); } + $format = idx($dict, 'format'); + if ($format) { + $obj->setProperty('format', $format); + } + return $obj; } @@ -149,6 +164,66 @@ final class HarbormasterBuildUnitMessage return $this->getProperty('details', ''); } + public function getUnitMessageDetailsFormat() { + return $this->getProperty('format', self::FORMAT_TEXT); + } + + public function newUnitMessageDetailsView( + PhabricatorUser $viewer, + $summarize = false) { + + $format = $this->getUnitMessageDetailsFormat(); + + $is_text = ($format !== self::FORMAT_REMARKUP); + $is_remarkup = ($format === self::FORMAT_REMARKUP); + + $full_details = $this->getUnitMessageDetails(); + + if (!strlen($full_details)) { + if ($summarize) { + return null; + } + $details = phutil_tag('em', array(), pht('No details provided.')); + } else if ($summarize) { + if ($is_text) { + $details = id(new PhutilUTF8StringTruncator()) + ->setMaximumBytes(2048) + ->truncateString($full_details); + $details = phutil_split_lines($details); + + $limit = 3; + if (count($details) > $limit) { + $details = array_slice($details, 0, $limit); + } + + $details = implode('', $details); + } else { + $details = $full_details; + } + } else { + $details = $full_details; + } + + require_celerity_resource('harbormaster-css'); + + $classes = array(); + $classes[] = 'harbormaster-unit-details'; + + if ($is_remarkup) { + $details = new PHUIRemarkupView($viewer, $details); + } else { + $classes[] = 'harbormaster-unit-details-text'; + $classes[] = 'PhabricatorMonospaced'; + } + + return phutil_tag( + 'div', + array( + 'class' => implode(' ', $classes), + ), + $details); + } + public function getUnitMessageDisplayName() { $name = $this->getName(); diff --git a/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php b/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php index 0ce1688027..ee8b80a7a7 100644 --- a/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php +++ b/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php @@ -34,9 +34,8 @@ final class HarbormasterUnitPropertyView extends AphrontView { return $this; } - public function render() { - require_celerity_resource('harbormaster-css'); + $viewer = $this->getViewer(); $messages = $this->unitMessages; $messages = msort($messages, 'getSortKey'); @@ -84,13 +83,10 @@ final class HarbormasterUnitPropertyView extends AphrontView { $name); } - $details = $message->getUnitMessageDetails(); - if (strlen($details)) { - $name = array( - $name, - $this->renderUnitTestDetails($details), - ); - } + $name = array( + $name, + $message->newUnitMessageDetailsView($viewer, true), + ); $rows[] = array( $result_icon, @@ -158,25 +154,4 @@ final class HarbormasterUnitPropertyView extends AphrontView { return $table; } - private function renderUnitTestDetails($full_details) { - $details = id(new PhutilUTF8StringTruncator()) - ->setMaximumBytes(2048) - ->truncateString($full_details); - $details = phutil_split_lines($details); - - $limit = 3; - if (count($details) > $limit) { - $details = array_slice($details, 0, $limit); - } - - $details = implode('', $details); - - return phutil_tag( - 'div', - array( - 'class' => 'PhabricatorMonospaced harbormaster-unit-details', - ), - $details); - } - } diff --git a/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php b/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php index e5a66a3eb8..66ba2307b6 100644 --- a/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php +++ b/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php @@ -77,6 +77,7 @@ final class HarbormasterUnitSummaryView extends AphrontView { ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY); $table = id(new HarbormasterUnitPropertyView()) + ->setViewer($this->getViewer()) ->setUnitMessages($messages); if ($this->showViewAll) { diff --git a/webroot/rsrc/css/application/harbormaster/harbormaster.css b/webroot/rsrc/css/application/harbormaster/harbormaster.css index f3b75a397b..9fdad43a42 100644 --- a/webroot/rsrc/css/application/harbormaster/harbormaster.css +++ b/webroot/rsrc/css/application/harbormaster/harbormaster.css @@ -26,11 +26,14 @@ .harbormaster-unit-details { margin: 8px 0 4px; overflow: hidden; - white-space: pre; - text-overflow: ellipsis; color: {$lightgreytext}; } +.harbormaster-unit-details-text { + white-space: pre-wrap; + text-overflow: ellipsis; +} + .harbormaster-log-view-loading { padding: 8px; text-align: center; From fd0da4c41f822182dc40822a498e03995c4a5485 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Aug 2018 13:57:35 -0700 Subject: [PATCH 14/27] Rename "PHUIDocumentViewPro" to "PHUIDocumentView" Summary: Ref T13077. There is no "PHUIDocumentView" so toss the "Pro" suffix from this classname. Test Plan: Grepped for `PHUIDocumentView` and `PHUIDocumentViewPro`. Reviewers: amckinley Maniphest Tasks: T13077 Differential Revision: https://secure.phabricator.com/D19616 --- src/__phutil_library_map__.php | 4 ++-- .../diffusion/view/DiffusionReadmeView.php | 2 +- .../diviner/controller/DivinerAtomController.php | 2 +- .../diviner/controller/DivinerBookController.php | 2 +- .../diviner/controller/DivinerMainController.php | 2 +- .../guides/module/PhabricatorGuideInstallModule.php | 2 +- .../module/PhabricatorGuideQuickStartModule.php | 2 +- .../controller/LegalpadDocumentSignController.php | 2 +- ...habricatorApplicationEmailCommandsController.php | 2 +- .../phame/controller/PhameHomeController.php | 2 +- .../controller/blog/PhameBlogViewController.php | 2 +- .../controller/post/PhamePostViewController.php | 2 +- .../controller/PhrictionDocumentController.php | 13 +++++++------ .../editengine/PhabricatorEditEngine.php | 2 +- .../PhabricatorTypeaheadFunctionHelpController.php | 2 +- ...PHUIDocumentViewPro.php => PHUIDocumentView.php} | 2 +- src/view/phui/PHUIRemarkupPreviewPanel.php | 2 +- 17 files changed, 24 insertions(+), 23 deletions(-) rename src/view/phui/{PHUIDocumentViewPro.php => PHUIDocumentView.php} (98%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b6d71ea81b..284980bcc5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1912,7 +1912,7 @@ phutil_register_library_map(array( 'PHUIDiffTableOfContentsListView' => 'infrastructure/diff/view/PHUIDiffTableOfContentsListView.php', 'PHUIDiffTwoUpInlineCommentRowScaffold' => 'infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php', 'PHUIDocumentSummaryView' => 'view/phui/PHUIDocumentSummaryView.php', - 'PHUIDocumentViewPro' => 'view/phui/PHUIDocumentViewPro.php', + 'PHUIDocumentView' => 'view/phui/PHUIDocumentView.php', 'PHUIFeedStoryExample' => 'applications/uiexample/examples/PHUIFeedStoryExample.php', 'PHUIFeedStoryView' => 'view/phui/PHUIFeedStoryView.php', 'PHUIFormDividerControl' => 'view/form/control/PHUIFormDividerControl.php', @@ -7453,7 +7453,7 @@ phutil_register_library_map(array( 'PHUIDiffTableOfContentsListView' => 'AphrontView', 'PHUIDiffTwoUpInlineCommentRowScaffold' => 'PHUIDiffInlineCommentRowScaffold', 'PHUIDocumentSummaryView' => 'AphrontTagView', - 'PHUIDocumentViewPro' => 'AphrontTagView', + 'PHUIDocumentView' => 'AphrontTagView', 'PHUIFeedStoryExample' => 'PhabricatorUIExample', 'PHUIFeedStoryView' => 'AphrontView', 'PHUIFormDividerControl' => 'AphrontFormControl', diff --git a/src/applications/diffusion/view/DiffusionReadmeView.php b/src/applications/diffusion/view/DiffusionReadmeView.php index 693333249d..21e97f4718 100644 --- a/src/applications/diffusion/view/DiffusionReadmeView.php +++ b/src/applications/diffusion/view/DiffusionReadmeView.php @@ -94,7 +94,7 @@ final class DiffusionReadmeView extends DiffusionView { } $readme_content = phutil_tag_div($class, $readme_content); - $document = id(new PHUIDocumentViewPro()) + $document = id(new PHUIDocumentView()) ->setFluid(true) ->appendChild($readme_content); diff --git a/src/applications/diviner/controller/DivinerAtomController.php b/src/applications/diviner/controller/DivinerAtomController.php index bf69497b16..0c92bd1738 100644 --- a/src/applications/diviner/controller/DivinerAtomController.php +++ b/src/applications/diviner/controller/DivinerAtomController.php @@ -72,7 +72,7 @@ final class DivinerAtomController extends DivinerController { $prop_list = new PHUIPropertyGroupView(); $prop_list->addPropertyList($properties); - $document = id(new PHUIDocumentViewPro()) + $document = id(new PHUIDocumentView()) ->setBook($book->getTitle(), $group_name) ->setHeader($header) ->addClass('diviner-view'); diff --git a/src/applications/diviner/controller/DivinerBookController.php b/src/applications/diviner/controller/DivinerBookController.php index 1aa8790ffa..4e0ae3524b 100644 --- a/src/applications/diviner/controller/DivinerBookController.php +++ b/src/applications/diviner/controller/DivinerBookController.php @@ -45,7 +45,7 @@ final class DivinerBookController extends DivinerController { ->setName($book->getRepository()->getMonogram())); } - $document = new PHUIDocumentViewPro(); + $document = new PHUIDocumentView(); $document->setHeader($header); $document->addClass('diviner-view'); diff --git a/src/applications/diviner/controller/DivinerMainController.php b/src/applications/diviner/controller/DivinerMainController.php index e7d179c077..400fb48d0f 100644 --- a/src/applications/diviner/controller/DivinerMainController.php +++ b/src/applications/diviner/controller/DivinerMainController.php @@ -27,7 +27,7 @@ final class DivinerMainController extends DivinerController { ->setHeader(pht('Documentation Books')) ->addActionLink($query_button); - $document = new PHUIDocumentViewPro(); + $document = new PHUIDocumentView(); $document->setHeader($header); $document->addClass('diviner-view'); diff --git a/src/applications/guides/module/PhabricatorGuideInstallModule.php b/src/applications/guides/module/PhabricatorGuideInstallModule.php index 7a46b20027..172ff7a17b 100644 --- a/src/applications/guides/module/PhabricatorGuideInstallModule.php +++ b/src/applications/guides/module/PhabricatorGuideInstallModule.php @@ -169,7 +169,7 @@ final class PhabricatorGuideInstallModule extends PhabricatorGuideModule { $intro = new PHUIRemarkupView($viewer, $intro); - $intro = id(new PHUIDocumentViewPro()) + $intro = id(new PHUIDocumentView()) ->appendChild($intro); return array($intro, $guide_items); diff --git a/src/applications/guides/module/PhabricatorGuideQuickStartModule.php b/src/applications/guides/module/PhabricatorGuideQuickStartModule.php index c06e320df7..8c4b4c2606 100644 --- a/src/applications/guides/module/PhabricatorGuideQuickStartModule.php +++ b/src/applications/guides/module/PhabricatorGuideQuickStartModule.php @@ -206,7 +206,7 @@ final class PhabricatorGuideQuickStartModule extends PhabricatorGuideModule { 'these features at your own pace.'); $intro = new PHUIRemarkupView($viewer, $intro); - $intro = id(new PHUIDocumentViewPro()) + $intro = id(new PHUIDocumentView()) ->appendChild($intro); return array($intro, $guide_items); diff --git a/src/applications/legalpad/controller/LegalpadDocumentSignController.php b/src/applications/legalpad/controller/LegalpadDocumentSignController.php index f19cf43d0e..1748dba452 100644 --- a/src/applications/legalpad/controller/LegalpadDocumentSignController.php +++ b/src/applications/legalpad/controller/LegalpadDocumentSignController.php @@ -272,7 +272,7 @@ final class LegalpadDocumentSignController extends LegalpadController { $preamble_box->addPropertyList($preamble); } - $content = id(new PHUIDocumentViewPro()) + $content = id(new PHUIDocumentView()) ->addClass('legalpad') ->setHeader($header) ->appendChild( diff --git a/src/applications/meta/controller/PhabricatorApplicationEmailCommandsController.php b/src/applications/meta/controller/PhabricatorApplicationEmailCommandsController.php index 2979e92229..43ed2e957e 100644 --- a/src/applications/meta/controller/PhabricatorApplicationEmailCommandsController.php +++ b/src/applications/meta/controller/PhabricatorApplicationEmailCommandsController.php @@ -132,7 +132,7 @@ final class PhabricatorApplicationEmailCommandsController $header = id(new PHUIHeaderView()) ->setHeader($title); - $document = id(new PHUIDocumentViewPro()) + $document = id(new PHUIDocumentView()) ->setHeader($header) ->appendChild($info_view) ->appendChild($content_box); diff --git a/src/applications/phame/controller/PhameHomeController.php b/src/applications/phame/controller/PhameHomeController.php index 037026fa80..bf263e97a8 100644 --- a/src/applications/phame/controller/PhameHomeController.php +++ b/src/applications/phame/controller/PhameHomeController.php @@ -84,7 +84,7 @@ final class PhameHomeController extends PhamePostController { pht('Recent Posts'), $this->getApplicationURI('post/')); - $page = id(new PHUIDocumentViewPro()) + $page = id(new PHUIDocumentView()) ->setHeader($header) ->appendChild($post_list); diff --git a/src/applications/phame/controller/blog/PhameBlogViewController.php b/src/applications/phame/controller/blog/PhameBlogViewController.php index 08b23e5a95..e9e422aeb1 100644 --- a/src/applications/phame/controller/blog/PhameBlogViewController.php +++ b/src/applications/phame/controller/blog/PhameBlogViewController.php @@ -85,7 +85,7 @@ final class PhameBlogViewController extends PhameLiveController { } } - $page = id(new PHUIDocumentViewPro()) + $page = id(new PHUIDocumentView()) ->setHeader($header) ->appendChild($post_list); diff --git a/src/applications/phame/controller/post/PhamePostViewController.php b/src/applications/phame/controller/post/PhamePostViewController.php index 63adedb7ae..11d94d2f94 100644 --- a/src/applications/phame/controller/post/PhamePostViewController.php +++ b/src/applications/phame/controller/post/PhamePostViewController.php @@ -30,7 +30,7 @@ final class PhamePostViewController $header->setActionList($actions); } - $document = id(new PHUIDocumentViewPro()) + $document = id(new PHUIDocumentView()) ->setHeader($header); if ($moved) { diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index c8ffb86ce8..57dad25ab5 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -227,7 +227,7 @@ final class PhrictionDocumentController } $prop_list = phutil_tag_div('phui-document-view-pro-box', $prop_list); - $page_content = id(new PHUIDocumentViewPro()) + $page_content = id(new PHUIDocumentView()) ->setHeader($header) ->setToc($toc) ->appendChild( @@ -241,11 +241,12 @@ final class PhrictionDocumentController ->setTitle($page_title) ->setCrumbs($crumbs) ->setPageObjectPHIDs(array($document->getPHID())) - ->appendChild(array( - $page_content, - $prop_list, - $children, - )); + ->appendChild( + array( + $page_content, + $prop_list, + $children, + )); } diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index ad495a3a44..579b53a989 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1679,7 +1679,7 @@ abstract class PhabricatorEditEngine ->setUser($viewer) ->setFields($fields); - $document = id(new PHUIDocumentViewPro()) + $document = id(new PHUIDocumentView()) ->setUser($viewer) ->setHeader($header) ->appendChild($help_view); diff --git a/src/applications/typeahead/controller/PhabricatorTypeaheadFunctionHelpController.php b/src/applications/typeahead/controller/PhabricatorTypeaheadFunctionHelpController.php index 27b30a6278..a7a1f55caa 100644 --- a/src/applications/typeahead/controller/PhabricatorTypeaheadFunctionHelpController.php +++ b/src/applications/typeahead/controller/PhabricatorTypeaheadFunctionHelpController.php @@ -142,7 +142,7 @@ final class PhabricatorTypeaheadFunctionHelpController $header = id(new PHUIHeaderView()) ->setHeader($title); - $document = id(new PHUIDocumentViewPro()) + $document = id(new PHUIDocumentView()) ->setHeader($header) ->appendChild($content_box); diff --git a/src/view/phui/PHUIDocumentViewPro.php b/src/view/phui/PHUIDocumentView.php similarity index 98% rename from src/view/phui/PHUIDocumentViewPro.php rename to src/view/phui/PHUIDocumentView.php index 51ecc3526e..5b6bf9fcf9 100644 --- a/src/view/phui/PHUIDocumentViewPro.php +++ b/src/view/phui/PHUIDocumentView.php @@ -1,6 +1,6 @@ setHeader(pht('%s (Preview)', $this->header)); - $content = id(new PHUIDocumentViewPro()) + $content = id(new PHUIDocumentView()) ->setHeader($header) ->appendChild($preview); } From 4afb6446d97ba4353ec495f65b75bb71ad7b7228 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Aug 2018 14:37:19 -0700 Subject: [PATCH 15/27] Allow DocumentView to render with a curtain, and make Phriction use a curtain Summary: Depends on D19616. Ref T13077. Fixes T8172. In the last round of design updates, a lot of actions got stuffed into "Actions" menus. I never really got used to these and think they're a net usability loss, and broadly agree with the feedback in T8172. I'd generally like to move back toward a state where actions are available on the page, not hidden in a menu. For now, just put a curtain view on these pages. This could be refined later (e.g., stick this menu to the right hand side of the screen) depending on where other Phriction changes go. (Broadly, I'm also not satisfied with where we ended up on the fixed-width pages like Diffusion > Manage, Config, and Instances. In contrast, I //do// like where we ended up with Phortune in terms of overall design. I anticipate revisiting some of this stuff eventually.) Test Plan: - Looked at Phriction pages on desktop/tablet/mobile/printable -- actions are now available on the page. - Looked at other DocumentView pages (like Phame blogs) -- no changes for now. Reviewers: amckinley Maniphest Tasks: T13077, T8172 Differential Revision: https://secure.phabricator.com/D19617 --- resources/celerity/map.php | 14 ++-- .../PhrictionDocumentController.php | 46 +++++------ src/view/phui/PHUIDocumentView.php | 76 ++++++++++++++----- webroot/rsrc/css/phui/phui-document-pro.css | 29 +++++++ webroot/rsrc/css/phui/phui-document.css | 7 +- webroot/rsrc/css/phui/phui-header-view.css | 1 - 6 files changed, 116 insertions(+), 57 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 3842fbe839..609f156e88 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => 'fc4839c8', + 'core.pkg.css' => 'badf3f16', 'core.pkg.js' => 'b5a949ca', 'differential.pkg.css' => '06dc617c', 'differential.pkg.js' => 'c1cfa143', @@ -146,15 +146,15 @@ return array( 'rsrc/css/phui/phui-comment-panel.css' => 'f50152ad', 'rsrc/css/phui/phui-crumbs-view.css' => '10728aaa', 'rsrc/css/phui/phui-curtain-view.css' => '2bdaf026', - 'rsrc/css/phui/phui-document-pro.css' => '8af7ea27', + 'rsrc/css/phui/phui-document-pro.css' => '0e41dd91', 'rsrc/css/phui/phui-document-summary.css' => '9ca48bdf', - 'rsrc/css/phui/phui-document.css' => '878c2f52', + 'rsrc/css/phui/phui-document.css' => '552493fa', 'rsrc/css/phui/phui-feed-story.css' => '44a9c8e9', 'rsrc/css/phui/phui-fontkit.css' => '1320ed01', 'rsrc/css/phui/phui-form-view.css' => 'f808e5be', 'rsrc/css/phui/phui-form.css' => '7aaa04e3', 'rsrc/css/phui/phui-head-thing.css' => 'fd311e5f', - 'rsrc/css/phui/phui-header-view.css' => 'edeb9252', + 'rsrc/css/phui/phui-header-view.css' => '1ba8b707', 'rsrc/css/phui/phui-hovercard.css' => 'f0592bcf', 'rsrc/css/phui/phui-icon-set-selector.css' => '87db8fee', 'rsrc/css/phui/phui-icon.css' => 'cf24ceec', @@ -813,15 +813,15 @@ return array( 'phui-crumbs-view-css' => '10728aaa', 'phui-curtain-view-css' => '2bdaf026', 'phui-document-summary-view-css' => '9ca48bdf', - 'phui-document-view-css' => '878c2f52', - 'phui-document-view-pro-css' => '8af7ea27', + 'phui-document-view-css' => '552493fa', + 'phui-document-view-pro-css' => '0e41dd91', 'phui-feed-story-css' => '44a9c8e9', 'phui-font-icon-base-css' => '870a7360', 'phui-fontkit-css' => '1320ed01', 'phui-form-css' => '7aaa04e3', 'phui-form-view-css' => 'f808e5be', 'phui-head-thing-view-css' => 'fd311e5f', - 'phui-header-view-css' => 'edeb9252', + 'phui-header-view-css' => '1ba8b707', 'phui-hovercard' => '1bd28176', 'phui-hovercard-view-css' => 'f0592bcf', 'phui-icon-set-selector-css' => '87db8fee', diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index 57dad25ab5..fce2c066f7 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -201,7 +201,10 @@ final class PhrictionDocumentController $children = $this->renderDocumentChildren($slug); - $actions = $this->buildActionView($viewer, $document); + $curtain = null; + if ($document->getID()) { + $curtain = $this->buildCurtain($document); + } $crumbs = $this->buildApplicationCrumbs(); $crumbs->setBorder(true); @@ -213,8 +216,7 @@ final class PhrictionDocumentController $header = id(new PHUIHeaderView()) ->setUser($viewer) ->setPolicyObject($document) - ->setHeader($page_title) - ->setActionList($actions); + ->setHeader($page_title); if ($content) { $header->setEpoch($content->getDateCreated()); @@ -237,6 +239,10 @@ final class PhrictionDocumentController $core_content, )); + if ($curtain) { + $page_content->setCurtain($curtain); + } + return $this->newPage() ->setTitle($page_title) ->setCrumbs($crumbs) @@ -258,8 +264,7 @@ final class PhrictionDocumentController $viewer = $this->getViewer(); $view = id(new PHUIPropertyListView()) - ->setUser($viewer) - ->setObject($document); + ->setUser($viewer); $view->addProperty( pht('Last Author'), @@ -272,9 +277,9 @@ final class PhrictionDocumentController return $view; } - private function buildActionView( - PhabricatorUser $viewer, - PhrictionDocument $document) { + private function buildCurtain(PhrictionDocument $document) { + $viewer = $this->getViewer(); + $can_edit = PhabricatorPolicyFilter::hasCapability( $viewer, $document, @@ -282,19 +287,9 @@ final class PhrictionDocumentController $slug = PhabricatorSlug::normalize($this->slug); - $action_view = id(new PhabricatorActionListView()) - ->setUser($viewer) - ->setObject($document); + $curtain = $this->newCurtainView($document); - if (!$document->getID()) { - return $action_view->addAction( - id(new PhabricatorActionView()) - ->setName(pht('Create This Document')) - ->setIcon('fa-plus-square') - ->setHref('/phriction/edit/?slug='.$slug)); - } - - $action_view->addAction( + $curtain->addAction( id(new PhabricatorActionView()) ->setName(pht('Edit Document')) ->setDisabled(!$can_edit) @@ -302,7 +297,7 @@ final class PhrictionDocumentController ->setHref('/phriction/edit/'.$document->getID().'/')); if ($document->getStatus() == PhrictionDocumentStatus::STATUS_EXISTS) { - $action_view->addAction( + $curtain->addAction( id(new PhabricatorActionView()) ->setName(pht('Move Document')) ->setDisabled(!$can_edit) @@ -310,7 +305,7 @@ final class PhrictionDocumentController ->setHref('/phriction/move/'.$document->getID().'/') ->setWorkflow(true)); - $action_view->addAction( + $curtain->addAction( id(new PhabricatorActionView()) ->setName(pht('Delete Document')) ->setDisabled(!$can_edit) @@ -319,7 +314,7 @@ final class PhrictionDocumentController ->setWorkflow(true)); } - $action_view->addAction( + $curtain->addAction( id(new PhabricatorActionView()) ->setName(pht('View History')) ->setIcon('fa-list') @@ -327,15 +322,14 @@ final class PhrictionDocumentController $print_uri = PhrictionDocument::getSlugURI($slug).'?__print__=1'; - $action_view->addAction( + $curtain->addAction( id(new PhabricatorActionView()) ->setName(pht('Printable Page')) ->setIcon('fa-print') ->setOpenInNewWindow(true) ->setHref($print_uri)); - return $action_view; - + return $curtain; } private function renderDocumentChildren($slug) { diff --git a/src/view/phui/PHUIDocumentView.php b/src/view/phui/PHUIDocumentView.php index 5b6bf9fcf9..32324ddb5a 100644 --- a/src/view/phui/PHUIDocumentView.php +++ b/src/view/phui/PHUIDocumentView.php @@ -8,6 +8,7 @@ final class PHUIDocumentView extends AphrontTagView { private $fluid; private $toc; private $foot; + private $curtain; public function setHeader(PHUIHeaderView $header) { $header->setTall(true); @@ -36,6 +37,15 @@ final class PHUIDocumentView extends AphrontTagView { return $this; } + public function setCurtain(PHUICurtainView $curtain) { + $this->curtain = $curtain; + return $this; + } + + public function getCurtain() { + return $this->curtain; + } + protected function getTagAttributes() { $classes = array(); @@ -61,6 +71,17 @@ final class PHUIDocumentView extends AphrontTagView { $classes[] = 'phui-document-view'; $classes[] = 'phui-document-view-pro'; + if ($this->curtain) { + $classes[] = 'has-curtain'; + } else { + $classes[] = 'has-no-curtain'; + } + + if ($this->curtain) { + $action_list = $this->curtain->getActionList(); + $this->header->setActionListID($action_list->getID()); + } + $book = null; if ($this->bookname) { $book = pht('%s (%s)', $this->bookname, $this->bookdescription); @@ -114,32 +135,51 @@ final class PHUIDocumentView extends AphrontTagView { $this->foot); } - $content_inner = phutil_tag( + $curtain = null; + if ($this->curtain) { + $curtain = phutil_tag( 'div', array( - 'class' => 'phui-document-inner', + 'class' => 'phui-document-curtain', ), + $this->curtain); + } + + $main_content = phutil_tag( + 'div', + array( + 'class' => 'phui-document-content-view', + ), + $main_content); + + $content_inner = phutil_tag( + 'div', + array( + 'class' => 'phui-document-inner', + ), + array( + $table_of_contents, + $this->header, array( - $table_of_contents, - $this->header, + $curtain, $main_content, - $foot_content, - )); + ), + $foot_content, + )); $content = phutil_tag( - 'div', - array( - 'class' => 'phui-document-content', - ), - $content_inner); - - return phutil_tag( - 'div', - array( - 'class' => implode(' ', $classes), - ), - $content); + 'div', + array( + 'class' => 'phui-document-content', + ), + $content_inner); + return phutil_tag( + 'div', + array( + 'class' => implode(' ', $classes), + ), + $content); } } diff --git a/webroot/rsrc/css/phui/phui-document-pro.css b/webroot/rsrc/css/phui/phui-document-pro.css index 35c843f81f..29dc3beffa 100644 --- a/webroot/rsrc/css/phui/phui-document-pro.css +++ b/webroot/rsrc/css/phui/phui-document-pro.css @@ -8,6 +8,35 @@ margin: 0 auto; } +.phui-document-view.phui-document-view-pro.has-curtain { + max-width: 1132px; +} + +.printable .phui-document-view.phui-document-view-pro.has-curtain { + max-width: none; +} + +.device-desktop .phui-document-inner { + overflow: hidden; +} + +.device-desktop .has-curtain .phui-document-content-view { + padding-right: 332px; +} + +.printable .phui-document-content-view { + padding-right: 0; +} + +.device-desktop .phui-document-curtain { + float: right; + width: 300px; +} + +.printable .phui-document-curtain { + display: none; +} + .phui-document-container { background-color: {$page.content}; position: relative; diff --git a/webroot/rsrc/css/phui/phui-document.css b/webroot/rsrc/css/phui/phui-document.css index 954a220229..f554930a5f 100644 --- a/webroot/rsrc/css/phui/phui-document.css +++ b/webroot/rsrc/css/phui/phui-document.css @@ -63,7 +63,8 @@ font-size: 14px; } -.phui-document-view .phui-header-action-links .phui-mobile-menu { +.phui-document-view.has-no-curtain + .phui-header-action-links .phui-mobile-menu { display: block; } @@ -71,10 +72,6 @@ padding: 8px; } -.device-desktop .phui-document-content .phabricator-action-list-view { - display: none; -} - .phui-document-content .phabricator-remarkup .remarkup-code-block { clear: both; margin: 16px 0; diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css index 5a8f06281d..6cafb3e330 100644 --- a/webroot/rsrc/css/phui/phui-header-view.css +++ b/webroot/rsrc/css/phui/phui-header-view.css @@ -259,7 +259,6 @@ body .phui-header-shell.phui-bleed-header color: {$lightgreytext}; } - .phui-header-action-links .phui-mobile-menu { display: none; } From 04f8270a746645a045261765d18c50d8f54ce94a Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Aug 2018 15:10:28 -0700 Subject: [PATCH 16/27] Remove some unusual UI policy hints in Phriction Summary: Ref T13077. We currently have these weird policy hints in Phriction that we don't use in other applications. Just remove them for consistency to make the eventual swap to EditEngine a little easier. Also nuke some unreacahble code. Test Plan: Loaded edit page, saw more standard UI. Reviewers: amckinley Maniphest Tasks: T13077 Differential Revision: https://secure.phabricator.com/D19618 --- .../controller/PhrictionEditController.php | 24 +++++++------------ .../form/control/AphrontFormPolicyControl.php | 10 -------- 2 files changed, 8 insertions(+), 26 deletions(-) diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php index ccd473931c..675b1cd630 100644 --- a/src/applications/phriction/controller/PhrictionEditController.php +++ b/src/applications/phriction/controller/PhrictionEditController.php @@ -226,13 +226,7 @@ final class PhrictionEditController ->execute(); $view_capability = PhabricatorPolicyCapability::CAN_VIEW; $edit_capability = PhabricatorPolicyCapability::CAN_EDIT; - $codex = id(PhabricatorPolicyCodex::newFromObject($document, $viewer)) - ->setCapability($view_capability); - $view_capability_description = $codex->getPolicySpecialRuleForCapability( - PhabricatorPolicyCapability::CAN_VIEW)->getDescription(); - $edit_capability_description = $codex->getPolicySpecialRuleForCapability( - PhabricatorPolicyCapability::CAN_EDIT)->getDescription(); $form = id(new AphrontFormView()) ->setUser($viewer) @@ -279,15 +273,13 @@ final class PhrictionEditController ->setSpacePHID($v_space) ->setPolicyObject($document) ->setCapability($view_capability) - ->setPolicies($policies) - ->setCaption($view_capability_description)) + ->setPolicies($policies)) ->appendChild( id(new AphrontFormPolicyControl()) ->setName('editPolicy') ->setPolicyObject($document) ->setCapability($edit_capability) - ->setPolicies($policies) - ->setCaption($edit_capability_description)) + ->setPolicies($policies)) ->appendChild( id(new AphrontFormTextControl()) ->setLabel(pht('Edit Notes')) @@ -323,17 +315,17 @@ final class PhrictionEditController $crumbs->setBorder(true); $view = id(new PHUITwoColumnView()) - ->setFooter(array( - $draft_note, - $form_box, - $preview, - )); + ->setFooter( + array( + $draft_note, + $form_box, + $preview, + )); return $this->newPage() ->setTitle($page_title) ->setCrumbs($crumbs) ->appendChild($view); - } } diff --git a/src/view/form/control/AphrontFormPolicyControl.php b/src/view/form/control/AphrontFormPolicyControl.php index 0b0e608048..53e6f940c9 100644 --- a/src/view/form/control/AphrontFormPolicyControl.php +++ b/src/view/form/control/AphrontFormPolicyControl.php @@ -279,7 +279,6 @@ final class AphrontFormPolicyControl extends AphrontFormControl { ->setIcon($icon); } - if ($this->templatePHIDType) { $context_path = 'template/'.$this->templatePHIDType.'/'; } else { @@ -346,15 +345,6 @@ final class AphrontFormPolicyControl extends AphrontFormControl { )), $input, )); - - return AphrontFormSelectControl::renderSelectTag( - $this->getValue(), - $this->getOptions(), - array( - 'name' => $this->getName(), - 'disabled' => $this->getDisabled() ? 'disabled' : null, - 'id' => $this->getID(), - )); } public static function getSelectCustomKey() { From 64cee4a902dafa5265d6ce82f6b16ddc852bf6e0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Aug 2018 16:09:24 -0700 Subject: [PATCH 17/27] Move Phriction internal document/content references from IDs to PHIDs Summary: Ref T13077. This is mostly just a small cleanup change, even though the actual change is large. We currently reference content and document objects from one another with `contentID` and `documentID`, but this means that `contentID` must be nullable. Switching to PHIDs allows the column to be non-nullable. This also supports reorienting some current and future transactions around PHIDs, which is preferable for the API. In particular, I'm adding a "publish version X" transaction soon, and would rather callers pass a PHID than an ID or version number, since this will make the API more consistent and powerful. Today, `contentID` gets used as a cheaty way to order documents by (content) edit time. Since PHIDs aren't orderable and stuff is going to become actually-revertible soon, replace this with an epoch timestamp. Test Plan: - Created, edited, moved, retitled, and deleted Phriction documents. - Grepped for `documentID` and `contentID`. - This probably breaks //something// but I'll be in this code for a bit and am likely to catch whatever breaks. Reviewers: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13077 Differential Revision: https://secure.phabricator.com/D19619 --- .../20180828.phriction.01.contentphid.sql | 2 + .../20180828.phriction.02.documentphid.sql | 2 + .../20180828.phriction.03.editedepoch.sql | 2 + .../20180828.phriction.04.migrate.php | 57 ++++++++++++ .../20180828.phriction.05.contentid.sql | 2 + .../20180828.phriction.06.documentid.sql | 2 + src/__phutil_library_map__.php | 12 +-- .../controller/PhrictionDiffController.php | 7 +- .../PhrictionDocumentController.php | 2 +- .../controller/PhrictionHistoryController.php | 2 +- .../editor/PhrictionTransactionEditor.php | 87 ++++++++----------- .../phriction/query/PhrictionContentQuery.php | 12 +-- .../query/PhrictionDocumentQuery.php | 20 ++--- .../phriction/storage/PhrictionContent.php | 4 +- .../phriction/storage/PhrictionDocument.php | 25 +++--- .../PhrictionDocumentContentTransaction.php | 7 +- .../PhrictionDocumentDeleteTransaction.php | 11 ++- .../PhrictionDocumentMoveAwayTransaction.php | 14 ++- .../PhrictionDocumentMoveToTransaction.php | 17 ++-- .../PhrictionDocumentTitleTransaction.php | 8 +- .../PhrictionDocumentVersionTransaction.php | 10 +++ 21 files changed, 183 insertions(+), 122 deletions(-) create mode 100644 resources/sql/autopatches/20180828.phriction.01.contentphid.sql create mode 100644 resources/sql/autopatches/20180828.phriction.02.documentphid.sql create mode 100644 resources/sql/autopatches/20180828.phriction.03.editedepoch.sql create mode 100644 resources/sql/autopatches/20180828.phriction.04.migrate.php create mode 100644 resources/sql/autopatches/20180828.phriction.05.contentid.sql create mode 100644 resources/sql/autopatches/20180828.phriction.06.documentid.sql create mode 100644 src/applications/phriction/xaction/PhrictionDocumentVersionTransaction.php diff --git a/resources/sql/autopatches/20180828.phriction.01.contentphid.sql b/resources/sql/autopatches/20180828.phriction.01.contentphid.sql new file mode 100644 index 0000000000..cf3d78ebdf --- /dev/null +++ b/resources/sql/autopatches/20180828.phriction.01.contentphid.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phriction.phriction_document + ADD contentPHID VARBINARY(64) NOT NULL; diff --git a/resources/sql/autopatches/20180828.phriction.02.documentphid.sql b/resources/sql/autopatches/20180828.phriction.02.documentphid.sql new file mode 100644 index 0000000000..c15b4b17b8 --- /dev/null +++ b/resources/sql/autopatches/20180828.phriction.02.documentphid.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phriction.phriction_content + ADD documentPHID VARBINARY(64) NOT NULL; diff --git a/resources/sql/autopatches/20180828.phriction.03.editedepoch.sql b/resources/sql/autopatches/20180828.phriction.03.editedepoch.sql new file mode 100644 index 0000000000..eae31fc0ba --- /dev/null +++ b/resources/sql/autopatches/20180828.phriction.03.editedepoch.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phriction.phriction_document + ADD editedEpoch INT UNSIGNED NOT NULL; diff --git a/resources/sql/autopatches/20180828.phriction.04.migrate.php b/resources/sql/autopatches/20180828.phriction.04.migrate.php new file mode 100644 index 0000000000..461eae2dad --- /dev/null +++ b/resources/sql/autopatches/20180828.phriction.04.migrate.php @@ -0,0 +1,57 @@ +establishConnection('w'); + +$document_iterator = new LiskRawMigrationIterator( + $conn, + $document_table->getTableName()); +foreach ($document_iterator as $row) { + $content_id = $row['contentID']; + + $content_row = queryfx_one( + $conn, + 'SELECT phid, dateCreated FROM %T WHERE id = %d', + $content_table->getTableName(), + $content_id); + + if (!$content_row) { + continue; + } + + queryfx( + $conn, + 'UPDATE %T SET contentPHID = %s, editedEpoch = %d WHERE id = %d', + $document_table->getTableName(), + $content_row['phid'], + $content_row['dateCreated'], + $row['id']); +} + +$content_iterator = new LiskRawMigrationIterator( + $conn, + $content_table->getTableName()); +foreach ($content_iterator as $row) { + $document_id = $row['documentID']; + + $document_row = queryfx_one( + $conn, + 'SELECT phid FROM %T WHERE id = %d', + $document_table->getTableName(), + $document_id); + if (!$document_row) { + continue; + } + + queryfx( + $conn, + 'UPDATE %T SET documentPHID = %s WHERE id = %d', + $content_table->getTableName(), + $document_row['phid'], + $row['id']); +} diff --git a/resources/sql/autopatches/20180828.phriction.05.contentid.sql b/resources/sql/autopatches/20180828.phriction.05.contentid.sql new file mode 100644 index 0000000000..d6cba741a3 --- /dev/null +++ b/resources/sql/autopatches/20180828.phriction.05.contentid.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phriction.phriction_document + DROP contentID; diff --git a/resources/sql/autopatches/20180828.phriction.06.documentid.sql b/resources/sql/autopatches/20180828.phriction.06.documentid.sql new file mode 100644 index 0000000000..2323154b3e --- /dev/null +++ b/resources/sql/autopatches/20180828.phriction.06.documentid.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phriction.phriction_content + DROP documentID; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 284980bcc5..191522d8fb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -5036,6 +5036,7 @@ phutil_register_library_map(array( 'PhrictionDocumentTitleHeraldField' => 'applications/phriction/herald/PhrictionDocumentTitleHeraldField.php', 'PhrictionDocumentTitleTransaction' => 'applications/phriction/xaction/PhrictionDocumentTitleTransaction.php', 'PhrictionDocumentTransactionType' => 'applications/phriction/xaction/PhrictionDocumentTransactionType.php', + 'PhrictionDocumentVersionTransaction' => 'applications/phriction/xaction/PhrictionDocumentVersionTransaction.php', 'PhrictionEditConduitAPIMethod' => 'applications/phriction/conduit/PhrictionEditConduitAPIMethod.php', 'PhrictionEditController' => 'applications/phriction/controller/PhrictionEditController.php', 'PhrictionHistoryConduitAPIMethod' => 'applications/phriction/conduit/PhrictionHistoryConduitAPIMethod.php', @@ -11129,17 +11130,17 @@ phutil_register_library_map(array( ), 'PhrictionDocumentAuthorHeraldField' => 'PhrictionDocumentHeraldField', 'PhrictionDocumentContentHeraldField' => 'PhrictionDocumentHeraldField', - 'PhrictionDocumentContentTransaction' => 'PhrictionDocumentTransactionType', + 'PhrictionDocumentContentTransaction' => 'PhrictionDocumentVersionTransaction', 'PhrictionDocumentController' => 'PhrictionController', 'PhrictionDocumentDatasource' => 'PhabricatorTypeaheadDatasource', - 'PhrictionDocumentDeleteTransaction' => 'PhrictionDocumentTransactionType', + 'PhrictionDocumentDeleteTransaction' => 'PhrictionDocumentVersionTransaction', 'PhrictionDocumentFerretEngine' => 'PhabricatorFerretEngine', 'PhrictionDocumentFulltextEngine' => 'PhabricatorFulltextEngine', 'PhrictionDocumentHeraldAdapter' => 'HeraldAdapter', 'PhrictionDocumentHeraldField' => 'HeraldField', 'PhrictionDocumentHeraldFieldGroup' => 'HeraldFieldGroup', - 'PhrictionDocumentMoveAwayTransaction' => 'PhrictionDocumentTransactionType', - 'PhrictionDocumentMoveToTransaction' => 'PhrictionDocumentTransactionType', + 'PhrictionDocumentMoveAwayTransaction' => 'PhrictionDocumentVersionTransaction', + 'PhrictionDocumentMoveToTransaction' => 'PhrictionDocumentVersionTransaction', 'PhrictionDocumentPHIDType' => 'PhabricatorPHIDType', 'PhrictionDocumentPathHeraldField' => 'PhrictionDocumentHeraldField', 'PhrictionDocumentPolicyCodex' => 'PhabricatorPolicyCodex', @@ -11148,8 +11149,9 @@ phutil_register_library_map(array( 'PhrictionDocumentSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhrictionDocumentStatus' => 'PhabricatorObjectStatus', 'PhrictionDocumentTitleHeraldField' => 'PhrictionDocumentHeraldField', - 'PhrictionDocumentTitleTransaction' => 'PhrictionDocumentTransactionType', + 'PhrictionDocumentTitleTransaction' => 'PhrictionDocumentVersionTransaction', 'PhrictionDocumentTransactionType' => 'PhabricatorModularTransactionType', + 'PhrictionDocumentVersionTransaction' => 'PhrictionDocumentTransactionType', 'PhrictionEditConduitAPIMethod' => 'PhrictionConduitAPIMethod', 'PhrictionEditController' => 'PhrictionController', 'PhrictionHistoryConduitAPIMethod' => 'PhrictionConduitAPIMethod', diff --git a/src/applications/phriction/controller/PhrictionDiffController.php b/src/applications/phriction/controller/PhrictionDiffController.php index f443ad657b..c81a90a3df 100644 --- a/src/applications/phriction/controller/PhrictionDiffController.php +++ b/src/applications/phriction/controller/PhrictionDiffController.php @@ -65,8 +65,8 @@ final class PhrictionDiffController extends PhrictionController { $slug = $document->getSlug(); - $revert_l = $this->renderRevertButton($content_l, $current); - $revert_r = $this->renderRevertButton($content_r, $current); + $revert_l = $this->renderRevertButton($document, $content_l, $current); + $revert_r = $this->renderRevertButton($document, $content_r, $current); $crumbs = $this->buildApplicationCrumbs(); $crumb_views = $this->renderBreadcrumbs($slug); @@ -179,10 +179,11 @@ final class PhrictionDiffController extends PhrictionController { } private function renderRevertButton( + PhrictionDocument $document, PhrictionContent $content, PhrictionContent $current) { - $document_id = $content->getDocumentID(); + $document_id = $document->getID(); $version = $content->getVersion(); $hidden_statuses = array( diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index fce2c066f7..0018ca229a 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -78,7 +78,7 @@ final class PhrictionDocumentController return new Aphront404Response(); } - if ($content->getID() != $document->getContentID()) { + if ($content->getPHID() != $document->getContentPHID()) { $version_note = id(new PHUIInfoView()) ->setSeverity(PHUIInfoView::SEVERITY_NOTICE) ->appendChild( diff --git a/src/applications/phriction/controller/PhrictionHistoryController.php b/src/applications/phriction/controller/PhrictionHistoryController.php index 432b1dfdef..2a3afc9769 100644 --- a/src/applications/phriction/controller/PhrictionHistoryController.php +++ b/src/applications/phriction/controller/PhrictionHistoryController.php @@ -52,7 +52,7 @@ final class PhrictionHistoryController } $vs_head = null; - if ($content->getID() != $document->getContentID()) { + if ($content->getPHID() != $document->getContentPHID()) { $vs_head = $diff_uri ->alter('l', $content->getVersion()) ->alter('r', $current->getVersion()); diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php index b9f2c0e2ca..d91165ab1d 100644 --- a/src/applications/phriction/editor/PhrictionTransactionEditor.php +++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php @@ -93,29 +93,13 @@ final class PhrictionTransactionEditor return $types; } - protected function shouldApplyInitialEffects( - PhabricatorLiskDAO $object, - array $xactions) { - - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case PhrictionDocumentTitleTransaction::TRANSACTIONTYPE: - case PhrictionDocumentContentTransaction::TRANSACTIONTYPE: - case PhrictionDocumentDeleteTransaction::TRANSACTIONTYPE: - case PhrictionDocumentMoveToTransaction::TRANSACTIONTYPE: - case PhrictionDocumentMoveAwayTransaction::TRANSACTIONTYPE: - return true; - } - } - return parent::shouldApplyInitialEffects($object, $xactions); - } - - protected function applyInitialEffects( + protected function expandTransactions( PhabricatorLiskDAO $object, array $xactions) { $this->setOldContent($object->getContent()); - $this->setNewContent($this->buildNewContentTemplate($object)); + + return parent::expandTransactions($object, $xactions); } protected function expandTransaction( @@ -148,7 +132,6 @@ final class PhrictionTransactionEditor break; default: break; - } return $xactions; @@ -158,29 +141,12 @@ final class PhrictionTransactionEditor PhabricatorLiskDAO $object, array $xactions) { - $save_content = false; - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case PhrictionDocumentTitleTransaction::TRANSACTIONTYPE: - case PhrictionDocumentMoveToTransaction::TRANSACTIONTYPE: - case PhrictionDocumentMoveAwayTransaction::TRANSACTIONTYPE: - case PhrictionDocumentDeleteTransaction::TRANSACTIONTYPE: - case PhrictionDocumentContentTransaction::TRANSACTIONTYPE: - $save_content = true; - break; - default: - break; - } - } + if ($this->hasNewDocumentContent()) { + $content = $this->getNewDocumentContent($object); - if ($save_content) { - $content = $this->getNewContent(); - $content->setDocumentID($object->getID()); - $content->save(); - - $object->setContentID($content->getID()); - $object->save(); - $object->attachContent($content); + $content + ->setDocumentPHID($object->getPHID()) + ->save(); } if ($this->getIsNewObject() && !$this->getSkipAncestorCheck()) { @@ -535,24 +501,47 @@ final class PhrictionTransactionEditor ->setDocument($object); } - private function buildNewContentTemplate( - PhrictionDocument $document) { + private function hasNewDocumentContent() { + return (bool)$this->newContent; + } - $new_content = id(new PhrictionContent()) + public function getNewDocumentContent(PhrictionDocument $document) { + if (!$this->hasNewDocumentContent()) { + $content = $this->newDocumentContent($document); + + // Generate a PHID now so we can populate "contentPHID" before saving + // the document to the database: the column is not nullable so we need + // a value. + $content_phid = $content->generatePHID(); + + $content->setPHID($content_phid); + + $document->setContentPHID($content_phid); + $document->attachContent($content); + $document->setEditedEpoch(PhabricatorTime::getNow()); + + $this->newContent = $content; + } + + return $this->newContent; + } + + private function newDocumentContent(PhrictionDocument $document) { + $content = id(new PhrictionContent()) ->setSlug($document->getSlug()) - ->setAuthorPHID($this->getActor()->getPHID()) + ->setAuthorPHID($this->getActingAsPHID()) ->setChangeType(PhrictionChangeType::CHANGE_EDIT) ->setTitle($this->getOldContent()->getTitle()) ->setContent($this->getOldContent()->getContent()) ->setDescription(''); if (strlen($this->getDescription())) { - $new_content->setDescription($this->getDescription()); + $content->setDescription($this->getDescription()); } - $new_content->setVersion($this->getOldContent()->getVersion() + 1); + $content->setVersion($this->getOldContent()->getVersion() + 1); - return $new_content; + return $content; } protected function getCustomWorkerState() { diff --git a/src/applications/phriction/query/PhrictionContentQuery.php b/src/applications/phriction/query/PhrictionContentQuery.php index 053f40cf9c..6efab5e1c6 100644 --- a/src/applications/phriction/query/PhrictionContentQuery.php +++ b/src/applications/phriction/query/PhrictionContentQuery.php @@ -76,7 +76,7 @@ final class PhrictionContentQuery if ($this->shouldJoinDocumentTable()) { $joins[] = qsprintf( $conn, - 'JOIN %T d ON d.id = c.documentID', + 'JOIN %T d ON d.phid = c.documentPHID', id(new PhrictionDocument())->getTableName()); } @@ -84,19 +84,19 @@ final class PhrictionContentQuery } protected function willFilterPage(array $contents) { - $document_ids = mpull($contents, 'getDocumentID'); + $document_phids = mpull($contents, 'getDocumentPHID'); $documents = id(new PhrictionDocumentQuery()) ->setViewer($this->getViewer()) ->setParentQuery($this) - ->withIDs($document_ids) + ->withPHIDs($document_phids) ->execute(); - $documents = mpull($documents, null, 'getID'); + $documents = mpull($documents, null, 'getPHID'); foreach ($contents as $key => $content) { - $document_id = $content->getDocumentID(); + $document_phid = $content->getDocumentPHID(); - $document = idx($documents, $document_id); + $document = idx($documents, $document_phid); if (!$document) { unset($contents[$key]); $this->didRejectResult($content); diff --git a/src/applications/phriction/query/PhrictionDocumentQuery.php b/src/applications/phriction/query/PhrictionDocumentQuery.php index 0c6a2c23bc..8a0fa325ae 100644 --- a/src/applications/phriction/query/PhrictionDocumentQuery.php +++ b/src/applications/phriction/query/PhrictionDocumentQuery.php @@ -151,17 +151,17 @@ final class PhrictionDocumentQuery $contents = id(new PhrictionContentQuery()) ->setViewer($this->getViewer()) ->setParentQuery($this) - ->withIDs(mpull($documents, 'getContentID')) + ->withPHIDs(mpull($documents, 'getContentPHID')) ->execute(); - $contents = mpull($contents, null, 'getID'); + $contents = mpull($contents, null, 'getPHID'); foreach ($documents as $key => $document) { - $content_id = $document->getContentID(); - if (empty($contents[$content_id])) { + $content_phid = $document->getContentPHID(); + if (empty($contents[$content_phid])) { unset($documents[$key]); continue; } - $document->attachContent($contents[$content_id]); + $document->attachContent($contents[$content_phid]); } } @@ -175,7 +175,7 @@ final class PhrictionDocumentQuery $content_dao = new PhrictionContent(); $joins[] = qsprintf( $conn, - 'JOIN %T c ON d.contentID = c.id', + 'JOIN %T c ON d.contentPHID = c.phid', $content_dao->getTableName()); } @@ -321,7 +321,7 @@ final class PhrictionDocumentQuery public function getBuiltinOrders() { return parent::getBuiltinOrders() + array( self::ORDER_HIERARCHY => array( - 'vector' => array('depth', 'title', 'updated'), + 'vector' => array('depth', 'title', 'updated', 'id'), 'name' => pht('Hierarchy'), ), ); @@ -343,9 +343,9 @@ final class PhrictionDocumentQuery ), 'updated' => array( 'table' => 'd', - 'column' => 'contentID', + 'column' => 'editedEpoch', 'type' => 'int', - 'unique' => true, + 'unique' => false, ), ); } @@ -356,7 +356,7 @@ final class PhrictionDocumentQuery $map = array( 'id' => $document->getID(), 'depth' => $document->getDepth(), - 'updated' => $document->getContentID(), + 'updated' => $document->getEditedEpoch(), ); foreach ($keys as $key) { diff --git a/src/applications/phriction/storage/PhrictionContent.php b/src/applications/phriction/storage/PhrictionContent.php index 515fc6b7d5..287974715f 100644 --- a/src/applications/phriction/storage/PhrictionContent.php +++ b/src/applications/phriction/storage/PhrictionContent.php @@ -7,7 +7,7 @@ final class PhrictionContent PhabricatorDestructibleInterface, PhabricatorConduitResultInterface { - protected $documentID; + protected $documentPHID; protected $version; protected $authorPHID; @@ -35,7 +35,7 @@ final class PhrictionContent ), self::CONFIG_KEY_SCHEMA => array( 'documentID' => array( - 'columns' => array('documentID', 'version'), + 'columns' => array('documentPHID', 'version'), 'unique' => true, ), 'authorPHID' => array( diff --git a/src/applications/phriction/storage/PhrictionDocument.php b/src/applications/phriction/storage/PhrictionDocument.php index 6edb516e6e..0e260c2e40 100644 --- a/src/applications/phriction/storage/PhrictionDocument.php +++ b/src/applications/phriction/storage/PhrictionDocument.php @@ -17,12 +17,13 @@ final class PhrictionDocument extends PhrictionDAO protected $slug; protected $depth; - protected $contentID; + protected $contentPHID; protected $status; protected $mailKey; protected $viewPolicy; protected $editPolicy; protected $spacePHID; + protected $editedEpoch; private $contentObject = self::ATTACHABLE; private $ancestors = array(); @@ -34,16 +35,11 @@ final class PhrictionDocument extends PhrictionDAO self::CONFIG_COLUMN_SCHEMA => array( 'slug' => 'sort128', 'depth' => 'uint32', - 'contentID' => 'id?', 'status' => 'text32', 'mailKey' => 'bytes20', + 'editedEpoch' => 'epoch', ), self::CONFIG_KEY_SCHEMA => array( - 'key_phid' => null, - 'phid' => array( - 'columns' => array('phid'), - 'unique' => true, - ), 'slug' => array( 'columns' => array('slug'), 'unique' => true, @@ -56,17 +52,16 @@ final class PhrictionDocument extends PhrictionDAO ) + parent::getConfiguration(); } - public function generatePHID() { - return PhabricatorPHID::generateNewPHID( - PhrictionDocumentPHIDType::TYPECONST); + public function getPHIDType() { + return PhrictionDocumentPHIDType::TYPECONST; } public static function initializeNewDocument(PhabricatorUser $actor, $slug) { - $document = new PhrictionDocument(); - $document->setSlug($slug); + $document = id(new self()) + ->setSlug($slug); - $content = new PhrictionContent(); - $content->setSlug($slug); + $content = id(new PhrictionContent()) + ->setSlug($slug); $default_title = PhabricatorSlug::getDefaultTitle($slug); $content->setTitle($default_title); @@ -95,6 +90,8 @@ final class PhrictionDocument extends PhrictionDAO ->setSpacePHID($actor->getDefaultSpacePHID()); } + $document->setEditedEpoch(PhabricatorTime::getNow()); + return $document; } diff --git a/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php index a051546824..759121dd65 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php @@ -1,7 +1,7 @@ setStatus(PhrictionDocumentStatus::STATUS_EXISTS); - } - public function applyExternalEffects($object, $value) { - $this->getEditor()->getNewContent()->setContent($value); + $content = $this->getNewDocumentContent($object); + $content->setContent($value); } public function shouldHide() { diff --git a/src/applications/phriction/xaction/PhrictionDocumentDeleteTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentDeleteTransaction.php index 25cc7c2b28..b1d894b96c 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentDeleteTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentDeleteTransaction.php @@ -1,7 +1,7 @@ setStatus(PhrictionDocumentStatus::STATUS_DELETED); - } - public function applyExternalEffects($object, $value) { - $this->getEditor()->getNewContent()->setContent(''); - $this->getEditor()->getNewContent()->setChangeType( - PhrictionChangeType::CHANGE_DELETE); + $content = $this->getNewDocumentContent($object); + + $content->setContent(''); + $content->setChangeType(PhrictionChangeType::CHANGE_DELETE); } public function getActionStrength() { diff --git a/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php index 3cd45f73b7..09ec56dd25 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php @@ -1,7 +1,7 @@ setStatus(PhrictionDocumentStatus::STATUS_MOVED); - } - public function applyExternalEffects($object, $value) { - $dict = $value; - $this->getEditor()->getNewContent()->setContent(''); - $this->getEditor()->getNewContent()->setChangeType( - PhrictionChangeType::CHANGE_MOVE_AWAY); - $this->getEditor()->getNewContent()->setChangeRef($dict['id']); + $content = $this->getNewDocumentContent($object); + + $content->setContent(''); + $content->setChangeType(PhrictionChangeType::CHANGE_MOVE_AWAY); + $content->setChangeRef($value['id']); } public function getActionName() { diff --git a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php index bdf2c5d8fc..180c263942 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php @@ -1,7 +1,7 @@ $document->getID(), 'phid' => $document->getPHID(), @@ -26,15 +27,13 @@ final class PhrictionDocumentMoveToTransaction public function applyInternalEffects($object, $value) { $object->setStatus(PhrictionDocumentStatus::STATUS_EXISTS); - } - public function applyExternalEffects($object, $value) { - $dict = $value; - $this->getEditor()->getNewContent()->setContent($dict['content']); - $this->getEditor()->getNewContent()->setTitle($dict['title']); - $this->getEditor()->getNewContent()->setChangeType( - PhrictionChangeType::CHANGE_MOVE_HERE); - $this->getEditor()->getNewContent()->setChangeRef($dict['id']); + $content = $this->getNewDocumentContent($object); + + $content->setContent($value['content']); + $content->setTitle($value['title']); + $content->setChangeType(PhrictionChangeType::CHANGE_MOVE_HERE); + $content->setChangeRef($value['id']); } public function getActionStrength() { diff --git a/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php index 5c97d1bec8..25a77efffe 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php @@ -1,7 +1,7 @@ setStatus(PhrictionDocumentStatus::STATUS_EXISTS); - } - public function applyExternalEffects($object, $value) { - $this->getEditor()->getNewContent()->setTitle($value); + $content = $this->getNewDocumentContent($object); + + $content->setTitle($value); } public function getActionStrength() { diff --git a/src/applications/phriction/xaction/PhrictionDocumentVersionTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentVersionTransaction.php new file mode 100644 index 0000000000..005e617e3b --- /dev/null +++ b/src/applications/phriction/xaction/PhrictionDocumentVersionTransaction.php @@ -0,0 +1,10 @@ +getEditor()->getNewDocumentContent($object); + } + +} From 50f4adef64cfee615f3bc4638e7328e864bca00c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 29 Aug 2018 08:22:53 -0700 Subject: [PATCH 18/27] Remove on-object mailkeys from Phriction Summary: Depends on D19619. Ref T13065. Ref T13077. Migrate Phriction mail keys to the new infrastructure and drop the column. Test Plan: Ran migrations, spot-checked the database. Reviewers: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13077, T13065 Differential Revision: https://secure.phabricator.com/D19620 --- .../20180829.phriction.01.mailkey.php | 26 +++++++++++++++++++ .../20180829.phriction.02.rmkey.sql | 2 ++ .../phriction/storage/PhrictionDocument.php | 15 +++-------- 3 files changed, 31 insertions(+), 12 deletions(-) create mode 100644 resources/sql/autopatches/20180829.phriction.01.mailkey.php create mode 100644 resources/sql/autopatches/20180829.phriction.02.rmkey.sql diff --git a/resources/sql/autopatches/20180829.phriction.01.mailkey.php b/resources/sql/autopatches/20180829.phriction.01.mailkey.php new file mode 100644 index 0000000000..cb85a3c5ef --- /dev/null +++ b/resources/sql/autopatches/20180829.phriction.01.mailkey.php @@ -0,0 +1,26 @@ +establishConnection('w'); +$document_name = $document_table->getTableName(); + +$properties_table = new PhabricatorMetaMTAMailProperties(); +$conn = $properties_table->establishConnection('w'); + +$iterator = new LiskRawMigrationIterator($document_conn, $document_name); +foreach ($iterator as $row) { + queryfx( + $conn, + 'INSERT IGNORE INTO %T + (objectPHID, mailProperties, dateCreated, dateModified) + VALUES + (%s, %s, %d, %d)', + $properties_table->getTableName(), + $row['phid'], + phutil_json_encode( + array( + 'mailKey' => $row['mailKey'], + )), + PhabricatorTime::getNow(), + PhabricatorTime::getNow()); +} diff --git a/resources/sql/autopatches/20180829.phriction.02.rmkey.sql b/resources/sql/autopatches/20180829.phriction.02.rmkey.sql new file mode 100644 index 0000000000..8199287db7 --- /dev/null +++ b/resources/sql/autopatches/20180829.phriction.02.rmkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phriction.phriction_document + DROP mailKey; diff --git a/src/applications/phriction/storage/PhrictionDocument.php b/src/applications/phriction/storage/PhrictionDocument.php index 0e260c2e40..626d02c1db 100644 --- a/src/applications/phriction/storage/PhrictionDocument.php +++ b/src/applications/phriction/storage/PhrictionDocument.php @@ -19,7 +19,6 @@ final class PhrictionDocument extends PhrictionDAO protected $depth; protected $contentPHID; protected $status; - protected $mailKey; protected $viewPolicy; protected $editPolicy; protected $spacePHID; @@ -36,7 +35,6 @@ final class PhrictionDocument extends PhrictionDAO 'slug' => 'sort128', 'depth' => 'uint32', 'status' => 'text32', - 'mailKey' => 'bytes20', 'editedEpoch' => 'epoch', ), self::CONFIG_KEY_SCHEMA => array( @@ -95,13 +93,6 @@ final class PhrictionDocument extends PhrictionDAO return $document; } - public function save() { - if (!$this->getMailKey()) { - $this->setMailKey(Filesystem::readRandomCharacters(20)); - } - return parent::save(); - } - public static function getSlugURI($slug, $type = 'document') { static $types = array( 'document' => '/w/', @@ -329,9 +320,9 @@ final class PhrictionDocument extends PhrictionDAO /* -( PhabricatorPolicyCodexInterface )------------------------------------ */ - public function newPolicyCodex() { - return new PhrictionDocumentPolicyCodex(); - } + public function newPolicyCodex() { + return new PhrictionDocumentPolicyCodex(); + } } From 349686319e3066c954e876ce0d296da0443717bb Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Aug 2018 15:43:40 -0700 Subject: [PATCH 19/27] Allow the published version of a Phriction document to differ from the most recent version Summary: Depends on D19620. Ref T13077. This adds a "Publish" operation which points the current version at some historical version of the document -- not necessarily the most recent version. Newer versions become "drafts". This is still quite rough and missing a lot of hinting in the UI, I'm just making it work so I can start making the UI understand it. Test Plan: Used the "Publish" action to publish older versions of a document, saw the document revert. Many UI hints are missing and this operation is puzzling and not yet usable for normal users. Reviewers: amckinley Maniphest Tasks: T13077 Differential Revision: https://secure.phabricator.com/D19621 --- src/__phutil_library_map__.php | 4 + .../PhabricatorPhrictionApplication.php | 2 + .../PhrictionDocumentController.php | 29 ++++++- .../controller/PhrictionPublishController.php | 86 +++++++++++++++++++ .../PhrictionDocumentPublishTransaction.php | 71 +++++++++++++++ src/view/phui/PHUIDocumentView.php | 11 +++ 6 files changed, 200 insertions(+), 3 deletions(-) create mode 100644 src/applications/phriction/controller/PhrictionPublishController.php create mode 100644 src/applications/phriction/xaction/PhrictionDocumentPublishTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 191522d8fb..a86027bf13 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -5029,6 +5029,7 @@ phutil_register_library_map(array( 'PhrictionDocumentPHIDType' => 'applications/phriction/phid/PhrictionDocumentPHIDType.php', 'PhrictionDocumentPathHeraldField' => 'applications/phriction/herald/PhrictionDocumentPathHeraldField.php', 'PhrictionDocumentPolicyCodex' => 'applications/phriction/codex/PhrictionDocumentPolicyCodex.php', + 'PhrictionDocumentPublishTransaction' => 'applications/phriction/xaction/PhrictionDocumentPublishTransaction.php', 'PhrictionDocumentQuery' => 'applications/phriction/query/PhrictionDocumentQuery.php', 'PhrictionDocumentSearchConduitAPIMethod' => 'applications/phriction/conduit/PhrictionDocumentSearchConduitAPIMethod.php', 'PhrictionDocumentSearchEngine' => 'applications/phriction/query/PhrictionDocumentSearchEngine.php', @@ -5046,6 +5047,7 @@ phutil_register_library_map(array( 'PhrictionMarkupPreviewController' => 'applications/phriction/controller/PhrictionMarkupPreviewController.php', 'PhrictionMoveController' => 'applications/phriction/controller/PhrictionMoveController.php', 'PhrictionNewController' => 'applications/phriction/controller/PhrictionNewController.php', + 'PhrictionPublishController' => 'applications/phriction/controller/PhrictionPublishController.php', 'PhrictionRemarkupRule' => 'applications/phriction/markup/PhrictionRemarkupRule.php', 'PhrictionReplyHandler' => 'applications/phriction/mail/PhrictionReplyHandler.php', 'PhrictionSchemaSpec' => 'applications/phriction/storage/PhrictionSchemaSpec.php', @@ -11144,6 +11146,7 @@ phutil_register_library_map(array( 'PhrictionDocumentPHIDType' => 'PhabricatorPHIDType', 'PhrictionDocumentPathHeraldField' => 'PhrictionDocumentHeraldField', 'PhrictionDocumentPolicyCodex' => 'PhabricatorPolicyCodex', + 'PhrictionDocumentPublishTransaction' => 'PhrictionDocumentTransactionType', 'PhrictionDocumentQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhrictionDocumentSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'PhrictionDocumentSearchEngine' => 'PhabricatorApplicationSearchEngine', @@ -11161,6 +11164,7 @@ phutil_register_library_map(array( 'PhrictionMarkupPreviewController' => 'PhabricatorController', 'PhrictionMoveController' => 'PhrictionController', 'PhrictionNewController' => 'PhrictionController', + 'PhrictionPublishController' => 'PhrictionController', 'PhrictionRemarkupRule' => 'PhutilRemarkupRule', 'PhrictionReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler', 'PhrictionSchemaSpec' => 'PhabricatorConfigSchemaSpec', diff --git a/src/applications/phriction/application/PhabricatorPhrictionApplication.php b/src/applications/phriction/application/PhabricatorPhrictionApplication.php index ea91727c76..d44845c4b9 100644 --- a/src/applications/phriction/application/PhabricatorPhrictionApplication.php +++ b/src/applications/phriction/application/PhabricatorPhrictionApplication.php @@ -56,6 +56,8 @@ final class PhabricatorPhrictionApplication extends PhabricatorApplication { 'edit/(?:(?P[1-9]\d*)/)?' => 'PhrictionEditController', 'delete/(?P[1-9]\d*)/' => 'PhrictionDeleteController', + 'publish/(?P[1-9]\d*)/(?P[1-9]\d*)/' + => 'PhrictionPublishController', 'new/' => 'PhrictionNewController', 'move/(?P[1-9]\d*)/' => 'PhrictionMoveController', diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index 0018ca229a..7ffc00086b 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -203,7 +203,7 @@ final class PhrictionDocumentController $curtain = null; if ($document->getID()) { - $curtain = $this->buildCurtain($document); + $curtain = $this->buildCurtain($document, $content); } $crumbs = $this->buildApplicationCrumbs(); @@ -230,11 +230,11 @@ final class PhrictionDocumentController $prop_list = phutil_tag_div('phui-document-view-pro-box', $prop_list); $page_content = id(new PHUIDocumentView()) + ->setBanner($version_note) ->setHeader($header) ->setToc($toc) ->appendChild( array( - $version_note, $move_notice, $core_content, )); @@ -277,7 +277,9 @@ final class PhrictionDocumentController return $view; } - private function buildCurtain(PhrictionDocument $document) { + private function buildCurtain( + PhrictionDocument $document, + PhrictionContent $content) { $viewer = $this->getViewer(); $can_edit = PhabricatorPolicyFilter::hasCapability( @@ -286,6 +288,7 @@ final class PhrictionDocumentController PhabricatorPolicyCapability::CAN_EDIT); $slug = PhabricatorSlug::normalize($this->slug); + $id = $document->getID(); $curtain = $this->newCurtainView($document); @@ -296,6 +299,26 @@ final class PhrictionDocumentController ->setIcon('fa-pencil') ->setHref('/phriction/edit/'.$document->getID().'/')); + $is_current = false; + $content_id = null; + if ($content) { + if ($content->getPHID() == $document->getContentPHID()) { + $is_current = true; + } + $content_id = $content->getID(); + } + $can_publish = ($can_edit && $content && !$is_current); + + $publish_uri = "/phriction/publish/{$id}/{$content_id}/"; + + $curtain->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Publish')) + ->setIcon('fa-upload') + ->setDisabled(!$can_publish) + ->setWorkflow(true) + ->setHref($publish_uri)); + if ($document->getStatus() == PhrictionDocumentStatus::STATUS_EXISTS) { $curtain->addAction( id(new PhabricatorActionView()) diff --git a/src/applications/phriction/controller/PhrictionPublishController.php b/src/applications/phriction/controller/PhrictionPublishController.php new file mode 100644 index 0000000000..43c90cb1fa --- /dev/null +++ b/src/applications/phriction/controller/PhrictionPublishController.php @@ -0,0 +1,86 @@ +getViewer(); + $id = $request->getURIData('documentID'); + $content_id = $request->getURIData('contentID'); + + $document = id(new PhrictionDocumentQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->needContent(true) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_EDIT, + PhabricatorPolicyCapability::CAN_VIEW, + )) + ->executeOne(); + if (!$document) { + return new Aphront404Response(); + } + + $document_uri = $document->getURI(); + + $content = id(new PhrictionContentQuery()) + ->setViewer($viewer) + ->withIDs(array($content_id)) + ->executeOne(); + if (!$content) { + return new Aphront404Response(); + } + + if ($content->getPHID() == $document->getContentPHID()) { + return $this->newDialog() + ->setTitle(pht('Already Published')) + ->appendChild( + pht( + 'This version of the document is already the published '. + 'version.')) + ->addCancelButton($document_uri); + } + + $content_uri = $document_uri.'?v='.$content->getVersion(); + + if ($request->isFormPost()) { + $xactions = array(); + + $xactions[] = id(new PhrictionTransaction()) + ->setTransactionType( + PhrictionDocumentPublishTransaction::TRANSACTIONTYPE) + ->setNewValue($content->getPHID()); + + id(new PhrictionTransactionEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->applyTransactions($document, $xactions); + + return id(new AphrontRedirectResponse())->setURI($document_uri); + } + + if ($content->getVersion() < $document->getContent()->getVersion()) { + $title = pht('Revert Document?'); + $body = pht( + 'Revert the published version of this document to an older '. + 'version?'); + $button = pht('Revert'); + } else { + $title = pht('Publish Draft?'); + $body = pht( + 'Update the published version of this document to this newer '. + 'version?'); + $button = pht('Publish'); + } + + return $this->newDialog() + ->setTitle($title) + ->appendChild($body) + ->addSubmitButton($button) + ->addCancelButton($content_uri); + } + +} diff --git a/src/applications/phriction/xaction/PhrictionDocumentPublishTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentPublishTransaction.php new file mode 100644 index 0000000000..324e5c2d4d --- /dev/null +++ b/src/applications/phriction/xaction/PhrictionDocumentPublishTransaction.php @@ -0,0 +1,71 @@ +getContentPHID(); + } + + public function applyInternalEffects($object, $value) { + $object->setContentPHID($value); + } + + public function getActionName() { + return pht('Published'); + } + + public function getTitle() { + return pht( + '%s published a new version of this document.', + $this->renderAuthor()); + } + + public function getTitleForFeed() { + return pht( + '%s published a new version of %s.', + $this->renderAuthor(), + $this->renderObject()); + } + + public function validateTransactions($object, array $xactions) { + $actor = $this->getActor(); + $errors = array(); + + foreach ($xactions as $xaction) { + $content_phid = $xaction->getNewValue(); + + // If this isn't changing anything, skip it. + if ($content_phid === $object->getContentPHID()) { + continue; + } + + $content = id(new PhrictionContentQuery()) + ->setViewer($actor) + ->withPHIDs(array($content_phid)) + ->executeOne(); + if (!$content) { + $errors[] = $this->newInvalidError( + pht( + 'Unable to load Content object with PHID "%s".', + $content_phid), + $xaction); + continue; + } + + if ($content->getDocumentPHID() !== $object->getPHID()) { + $errors[] = $this->newInvalidError( + pht( + 'Content object "%s" can not be published because it belongs '. + 'to a different document.', + $content_phid)); + continue; + } + } + + return $errors; + } + +} diff --git a/src/view/phui/PHUIDocumentView.php b/src/view/phui/PHUIDocumentView.php index 32324ddb5a..b29afb5496 100644 --- a/src/view/phui/PHUIDocumentView.php +++ b/src/view/phui/PHUIDocumentView.php @@ -9,6 +9,7 @@ final class PHUIDocumentView extends AphrontTagView { private $toc; private $foot; private $curtain; + private $banner; public function setHeader(PHUIHeaderView $header) { $header->setTall(true); @@ -46,6 +47,15 @@ final class PHUIDocumentView extends AphrontTagView { return $this->curtain; } + public function setBanner($banner) { + $this->banner = $banner; + return $this; + } + + public function getBanner() { + return $this->banner; + } + protected function getTagAttributes() { $classes = array(); @@ -160,6 +170,7 @@ final class PHUIDocumentView extends AphrontTagView { array( $table_of_contents, $this->header, + $this->banner, array( $curtain, $main_content, From 876638e42895bb3f2387b3b68955bb8e667e161f Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 29 Aug 2018 09:31:32 -0700 Subject: [PATCH 20/27] Add a UI element for navigating between versions of a Phriction document Summary: Depends on D19621. Ref T13077. Fixes T4815. This adds previous/current/next/draft buttons and makes navigation between unpublished and published versions of a document more clear. Test Plan: {F5841997} Reviewers: amckinley Maniphest Tasks: T13077, T4815 Differential Revision: https://secure.phabricator.com/D19622 --- resources/celerity/map.php | 4 +- .../PhrictionDocumentController.php | 158 +++++++++++++++--- webroot/rsrc/css/phui/phui-document.css | 6 + 3 files changed, 147 insertions(+), 21 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 609f156e88..743abd38ee 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -148,7 +148,7 @@ return array( 'rsrc/css/phui/phui-curtain-view.css' => '2bdaf026', 'rsrc/css/phui/phui-document-pro.css' => '0e41dd91', 'rsrc/css/phui/phui-document-summary.css' => '9ca48bdf', - 'rsrc/css/phui/phui-document.css' => '552493fa', + 'rsrc/css/phui/phui-document.css' => 'c4ac41f9', 'rsrc/css/phui/phui-feed-story.css' => '44a9c8e9', 'rsrc/css/phui/phui-fontkit.css' => '1320ed01', 'rsrc/css/phui/phui-form-view.css' => 'f808e5be', @@ -813,7 +813,7 @@ return array( 'phui-crumbs-view-css' => '10728aaa', 'phui-curtain-view-css' => '2bdaf026', 'phui-document-summary-view-css' => '9ca48bdf', - 'phui-document-view-css' => '552493fa', + 'phui-document-view-css' => 'c4ac41f9', 'phui-document-view-pro-css' => '0e41dd91', 'phui-feed-story-css' => '44a9c8e9', 'phui-font-icon-base-css' => '870a7360', diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index 7ffc00086b..6e9edc1346 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -20,8 +20,6 @@ final class PhrictionDocumentController return id(new AphrontRedirectResponse())->setURI($uri); } - require_celerity_resource('phriction-document-css'); - $version_note = null; $core_content = ''; $move_notice = ''; @@ -29,6 +27,8 @@ final class PhrictionDocumentController $content = null; $toc = null; + $is_draft = false; + $document = id(new PhrictionDocumentQuery()) ->setViewer($viewer) ->withSlugs(array($slug)) @@ -66,8 +66,14 @@ final class PhrictionDocumentController ->addAction($create_button); } else { - $version = $request->getInt('v'); + $draft_content = id(new PhrictionContentQuery()) + ->setViewer($viewer) + ->withDocumentPHIDs(array($document->getPHID())) + ->setLimit(1) + ->executeOne(); + $max_version = (int)$draft_content->getVersion(); + $version = $request->getInt('v'); if ($version) { $content = id(new PhrictionContentQuery()) ->setViewer($viewer) @@ -78,15 +84,111 @@ final class PhrictionDocumentController return new Aphront404Response(); } - if ($content->getPHID() != $document->getContentPHID()) { - $version_note = id(new PHUIInfoView()) - ->setSeverity(PHUIInfoView::SEVERITY_NOTICE) - ->appendChild( - pht( - 'You are viewing an older version of this document, as it '. - 'appeared on %s.', - phabricator_datetime($content->getDateCreated(), $viewer))); + // When the "v" parameter exists, the user is in history mode so we + // show this header even if they're looking at the current version + // of the document. This keeps the next/previous links working. + + $view_version = (int)$content->getVersion(); + $published_version = (int)$document->getContent()->getVersion(); + + if ($view_version < $published_version) { + $version_note = pht( + 'You are viewing an older version of this document, as it '. + 'appeared on %s.', + phabricator_datetime($content->getDateCreated(), $viewer)); + } else if ($view_version > $published_version) { + $is_draft = true; + $version_note = pht( + 'You are viewing an unpublished draft of this document.'); + } else { + $version_note = pht( + 'You are viewing the current published version of this document.'); } + + $version_note = array( + phutil_tag( + 'strong', + array(), + pht('Version %d of %d: ', $view_version, $max_version)), + ' ', + $version_note, + ); + + $version_note = id(new PHUIInfoView()) + ->setSeverity(PHUIInfoView::SEVERITY_NOTICE) + ->appendChild($version_note); + + $document_uri = new PhutilURI($document->getURI()); + + if ($view_version > 1) { + $previous_uri = $document_uri->alter('v', ($view_version - 1)); + } else { + $previous_uri = null; + } + + if ($view_version !== $published_version) { + $current_uri = $document_uri->alter('v', $published_version); + } else { + $current_uri = null; + } + + if ($view_version < $max_version) { + $next_uri = $document_uri->alter('v', ($view_version + 1)); + } else { + $next_uri = null; + } + + if ($view_version !== $max_version) { + $draft_uri = $document_uri->alter('v', $max_version); + } else { + $draft_uri = null; + } + + $button_bar = id(new PHUIButtonBarView()) + ->addButton( + id(new PHUIButtonView()) + ->setTag('a') + ->setColor('grey') + ->setIcon('fa-backward') + ->setDisabled(!$previous_uri) + ->setHref($previous_uri) + ->setText(pht('Previous'))) + ->addButton( + id(new PHUIButtonView()) + ->setTag('a') + ->setColor('grey') + ->setIcon('fa-file-o') + ->setDisabled(!$current_uri) + ->setHref($current_uri) + ->setText(pht('Published'))) + ->addButton( + id(new PHUIButtonView()) + ->setTag('a') + ->setColor('grey') + ->setIcon('fa-forward', false) + ->setDisabled(!$next_uri) + ->setHref($next_uri) + ->setText(pht('Next'))) + ->addButton( + id(new PHUIButtonView()) + ->setTag('a') + ->setColor('grey') + ->setIcon('fa-fast-forward', false) + ->setDisabled(!$draft_uri) + ->setHref($draft_uri) + ->setText(pht('Draft'))); + + require_celerity_resource('phui-document-view-css'); + + $version_note = array( + $version_note, + phutil_tag( + 'div', + array( + 'class' => 'phui-document-version-navigation', + ), + $button_bar), + ); } else { $content = $document->getContent(); } @@ -218,7 +320,15 @@ final class PhrictionDocumentController ->setPolicyObject($document) ->setHeader($page_title); - if ($content) { + if ($is_draft) { + $draft_tag = id(new PHUITagView()) + ->setName(pht('Draft')) + ->setIcon('fa-spinner') + ->setColor('pink') + ->setType(PHUITagView::TYPE_SHADE); + + $header->addTag($draft_tag); + } else if ($content) { $header->setEpoch($content->getDateCreated()); } @@ -299,21 +409,37 @@ final class PhrictionDocumentController ->setIcon('fa-pencil') ->setHref('/phriction/edit/'.$document->getID().'/')); + $curtain->addAction( + id(new PhabricatorActionView()) + ->setName(pht('View History')) + ->setIcon('fa-history') + ->setHref(PhrictionDocument::getSlugURI($slug, 'history'))); + $is_current = false; $content_id = null; + $is_draft = false; if ($content) { if ($content->getPHID() == $document->getContentPHID()) { $is_current = true; } $content_id = $content->getID(); + + $current_version = $document->getContent()->getVersion(); + $is_draft = ($content->getVersion() >= $current_version); } $can_publish = ($can_edit && $content && !$is_current); + if ($is_draft) { + $publish_name = pht('Publish Draft'); + } else { + $publish_name = pht('Publish Revert'); + } + $publish_uri = "/phriction/publish/{$id}/{$content_id}/"; $curtain->addAction( id(new PhabricatorActionView()) - ->setName(pht('Publish')) + ->setName($publish_name) ->setIcon('fa-upload') ->setDisabled(!$can_publish) ->setWorkflow(true) @@ -337,12 +463,6 @@ final class PhrictionDocumentController ->setWorkflow(true)); } - $curtain->addAction( - id(new PhabricatorActionView()) - ->setName(pht('View History')) - ->setIcon('fa-list') - ->setHref(PhrictionDocument::getSlugURI($slug, 'history'))); - $print_uri = PhrictionDocument::getSlugURI($slug).'?__print__=1'; $curtain->addAction( diff --git a/webroot/rsrc/css/phui/phui-document.css b/webroot/rsrc/css/phui/phui-document.css index f554930a5f..1c5511b0d3 100644 --- a/webroot/rsrc/css/phui/phui-document.css +++ b/webroot/rsrc/css/phui/phui-document.css @@ -100,3 +100,9 @@ .remarkup-code { font: 13px/18px "Menlo", "Consolas", "Monaco", monospace; } + +.phui-document-version-navigation { + text-align: center; + padding: 8px; + background-color: {$lightgreybackground}; +} From 75a04551524f87c9b1b8dc09343a5ee3ce3e98c7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 29 Aug 2018 14:11:35 -0700 Subject: [PATCH 21/27] Add "Revision test plan" as a Herald field; remove test plan from the "Revision summary" field Summary: See PHI844. Ref T13189. Add "Revision test plan" as an available field for Herald. This is a little niche -- and a little odd because it sticks around even if you fully disable test plans -- but probably broadly reasonable. The existing "Revision summary" field counterintuitively included the test plan. Separate this out since it's now a separate field and the behavior was weird historic nonsense. I'll note this in the changelog. Test Plan: Wrote a rule using both fields, verified they generated the expected values. Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19623 --- src/__phutil_library_map__.php | 2 ++ ...DifferentialRevisionSummaryHeraldField.php | 5 +---- ...ifferentialRevisionTestPlanHeraldField.php | 20 +++++++++++++++++++ 3 files changed, 23 insertions(+), 4 deletions(-) create mode 100644 src/applications/differential/herald/DifferentialRevisionTestPlanHeraldField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a86027bf13..3abc10ef49 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -644,6 +644,7 @@ phutil_register_library_map(array( 'DifferentialRevisionStatusTransaction' => 'applications/differential/xaction/DifferentialRevisionStatusTransaction.php', 'DifferentialRevisionSummaryHeraldField' => 'applications/differential/herald/DifferentialRevisionSummaryHeraldField.php', 'DifferentialRevisionSummaryTransaction' => 'applications/differential/xaction/DifferentialRevisionSummaryTransaction.php', + 'DifferentialRevisionTestPlanHeraldField' => 'applications/differential/herald/DifferentialRevisionTestPlanHeraldField.php', 'DifferentialRevisionTestPlanTransaction' => 'applications/differential/xaction/DifferentialRevisionTestPlanTransaction.php', 'DifferentialRevisionTitleHeraldField' => 'applications/differential/herald/DifferentialRevisionTitleHeraldField.php', 'DifferentialRevisionTitleTransaction' => 'applications/differential/xaction/DifferentialRevisionTitleTransaction.php', @@ -6004,6 +6005,7 @@ phutil_register_library_map(array( 'DifferentialRevisionStatusTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionSummaryHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionSummaryTransaction' => 'DifferentialRevisionTransactionType', + 'DifferentialRevisionTestPlanHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionTestPlanTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionTitleHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionTitleTransaction' => 'DifferentialRevisionTransactionType', diff --git a/src/applications/differential/herald/DifferentialRevisionSummaryHeraldField.php b/src/applications/differential/herald/DifferentialRevisionSummaryHeraldField.php index eafc9bc990..587468d549 100644 --- a/src/applications/differential/herald/DifferentialRevisionSummaryHeraldField.php +++ b/src/applications/differential/herald/DifferentialRevisionSummaryHeraldField.php @@ -10,10 +10,7 @@ final class DifferentialRevisionSummaryHeraldField } public function getHeraldFieldValue($object) { - // NOTE: For historical reasons, this field includes the test plan. We - // could maybe try to fix this some day, but it probably aligns reasonably - // well with user expectation without harming anything. - return $object->getSummary()."\n\n".$object->getTestPlan(); + return $object->getSummary(); } protected function getHeraldFieldStandardType() { diff --git a/src/applications/differential/herald/DifferentialRevisionTestPlanHeraldField.php b/src/applications/differential/herald/DifferentialRevisionTestPlanHeraldField.php new file mode 100644 index 0000000000..bb2e6d0910 --- /dev/null +++ b/src/applications/differential/herald/DifferentialRevisionTestPlanHeraldField.php @@ -0,0 +1,20 @@ +getTestPlan(); + } + + protected function getHeraldFieldStandardType() { + return self::STANDARD_TEXT; + } + +} From 0a77b0e53e7f96a46bbf5013489b1de67286f308 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 Aug 2018 06:13:43 -0700 Subject: [PATCH 22/27] Work around an issue in MariaDB where dropping a column from a UNIQUE KEY fails Summary: See T13193. See T13077. If we drop a column which is part of a UNIQUE KEY, MariaDB raises an error. This is probably a bad idea on our side anyway, but in this case it wasn't an obviously bad idea. To get around this: - Drop the unique key, if it exists, before dropping the column. - Explicitly add the new unique key afterward. Test Plan: Ran `bin/storage upgrade` locally without issue, but I'm on MySQL. Will follow up on T13193. Reviewers: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D19624 --- .../20180828.phriction.06.c.documentid.php | 20 +++++++++++++++++++ .../20180828.phriction.07.documentkey.sql | 2 ++ .../phriction/storage/PhrictionContent.php | 2 +- 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 resources/sql/autopatches/20180828.phriction.06.c.documentid.php create mode 100644 resources/sql/autopatches/20180828.phriction.07.documentkey.sql diff --git a/resources/sql/autopatches/20180828.phriction.06.c.documentid.php b/resources/sql/autopatches/20180828.phriction.06.c.documentid.php new file mode 100644 index 0000000000..474643d620 --- /dev/null +++ b/resources/sql/autopatches/20180828.phriction.06.c.documentid.php @@ -0,0 +1,20 @@ +establishConnection('w'); + +try { + queryfx( + $conn, + 'ALTER TABLE %T DROP KEY documentID', + $table->getTableName()); +} catch (AphrontQueryException $ex) { + // Ignore. +} diff --git a/resources/sql/autopatches/20180828.phriction.07.documentkey.sql b/resources/sql/autopatches/20180828.phriction.07.documentkey.sql new file mode 100644 index 0000000000..aea3c97130 --- /dev/null +++ b/resources/sql/autopatches/20180828.phriction.07.documentkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phriction.phriction_content + ADD UNIQUE KEY `key_version` (documentPHID, version); diff --git a/src/applications/phriction/storage/PhrictionContent.php b/src/applications/phriction/storage/PhrictionContent.php index 287974715f..5c597ab885 100644 --- a/src/applications/phriction/storage/PhrictionContent.php +++ b/src/applications/phriction/storage/PhrictionContent.php @@ -34,7 +34,7 @@ final class PhrictionContent 'description' => 'text', ), self::CONFIG_KEY_SCHEMA => array( - 'documentID' => array( + 'key_version' => array( 'columns' => array('documentPHID', 'version'), 'unique' => true, ), From 3b1294cf4599a86aa4789cb0618e004e4210b521 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 Aug 2018 09:53:18 -0700 Subject: [PATCH 23/27] Store Phriction max version on Document, improve editing rules for editing documents with drafts Summary: Ref T13077. We need to know the maximum version of a document in several cases, so denormalize it onto the Document object. Then clean up some behaviors where we edit a document with, e.g., 7 versions but version 5 is currently published. For now, we: edit starting with version 7, save as version 8, and immediately publish the new version. Test Plan: - Ran migration. - Edited a draft page without hitting any weird version errors. - Checked database for sensible `maxVersion` values. Reviewers: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13077 Differential Revision: https://secure.phabricator.com/D19625 --- .../20180830.phriction.01.maxversion.sql | 2 ++ .../20180830.phriction.02.maxes.php | 30 +++++++++++++++++++ .../PhrictionDocumentController.php | 7 +---- .../controller/PhrictionEditController.php | 26 ++++++++++------ .../editor/PhrictionTransactionEditor.php | 5 ++-- .../phriction/storage/PhrictionDocument.php | 3 ++ 6 files changed, 56 insertions(+), 17 deletions(-) create mode 100644 resources/sql/autopatches/20180830.phriction.01.maxversion.sql create mode 100644 resources/sql/autopatches/20180830.phriction.02.maxes.php diff --git a/resources/sql/autopatches/20180830.phriction.01.maxversion.sql b/resources/sql/autopatches/20180830.phriction.01.maxversion.sql new file mode 100644 index 0000000000..f6f24e8333 --- /dev/null +++ b/resources/sql/autopatches/20180830.phriction.01.maxversion.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phriction.phriction_document + ADD maxVersion INT UNSIGNED NOT NULL; diff --git a/resources/sql/autopatches/20180830.phriction.02.maxes.php b/resources/sql/autopatches/20180830.phriction.02.maxes.php new file mode 100644 index 0000000000..97abf010db --- /dev/null +++ b/resources/sql/autopatches/20180830.phriction.02.maxes.php @@ -0,0 +1,30 @@ +establishConnection('w'); + +$iterator = new LiskRawMigrationIterator( + $conn, + $document_table->getTableName()); +foreach ($iterator as $row) { + $content = queryfx_one( + $conn, + 'SELECT MAX(version) max FROM %T WHERE documentPHID = %s', + $content_table->getTableName(), + $row['phid']); + if (!$content) { + continue; + } + + queryfx( + $conn, + 'UPDATE %T SET maxVersion = %d WHERE id = %d', + $document_table->getTableName(), + $content['max'], + $row['id']); +} diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index 6e9edc1346..0a4dfdd070 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -66,12 +66,7 @@ final class PhrictionDocumentController ->addAction($create_button); } else { - $draft_content = id(new PhrictionContentQuery()) - ->setViewer($viewer) - ->withDocumentPHIDs(array($document->getPHID())) - ->setLimit(1) - ->executeOne(); - $max_version = (int)$draft_content->getVersion(); + $max_version = (int)$document->getMaxVersion(); $version = $request->getInt('v'); if ($version) { diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php index 675b1cd630..2c47187007 100644 --- a/src/applications/phriction/controller/PhrictionEditController.php +++ b/src/applications/phriction/controller/PhrictionEditController.php @@ -7,7 +7,7 @@ final class PhrictionEditController $viewer = $request->getViewer(); $id = $request->getURIData('id'); - $current_version = null; + $max_version = null; if ($id) { $is_new = false; $document = id(new PhrictionDocumentQuery()) @@ -24,7 +24,7 @@ final class PhrictionEditController return new Aphront404Response(); } - $current_version = $document->getContent()->getVersion(); + $max_version = $document->getMaxVersion(); $revert = $request->getInt('revert'); if ($revert) { @@ -37,9 +37,12 @@ final class PhrictionEditController return new Aphront404Response(); } } else { - $content = $document->getContent(); + $content = id(new PhrictionContentQuery()) + ->setViewer($viewer) + ->withDocumentPHIDs(array($document->getPHID())) + ->setLimit(1) + ->executeOne(); } - } else { $slug = $request->getStr('slug'); $slug = PhabricatorSlug::normalize($slug); @@ -54,8 +57,13 @@ final class PhrictionEditController ->executeOne(); if ($document) { - $content = $document->getContent(); - $current_version = $content->getVersion(); + $content = id(new PhrictionContentQuery()) + ->setViewer($viewer) + ->withDocumentPHIDs(array($document->getPHID())) + ->setLimit(1) + ->executeOne(); + + $max_version = $document->getMaxVersion(); $is_new = false; } else { $document = PhrictionDocument::initializeNewDocument($viewer, $slug); @@ -128,7 +136,7 @@ final class PhrictionEditController $title = $request->getStr('title'); $content_text = $request->getStr('content'); $notes = $request->getStr('description'); - $current_version = $request->getInt('contentVersion'); + $max_version = $request->getInt('contentVersion'); $v_view = $request->getStr('viewPolicy'); $v_edit = $request->getStr('editPolicy'); $v_cc = $request->getArr('cc'); @@ -168,7 +176,7 @@ final class PhrictionEditController ->setContinueOnNoEffect(true) ->setDescription($notes) ->setProcessContentVersionError(!$request->getBool('overwrite')) - ->setContentVersion($current_version); + ->setContentVersion($max_version); try { $editor->applyTransactions($document, $xactions); @@ -232,7 +240,7 @@ final class PhrictionEditController ->setUser($viewer) ->addHiddenInput('slug', $document->getSlug()) ->addHiddenInput('nodraft', $request->getBool('nodraft')) - ->addHiddenInput('contentVersion', $current_version) + ->addHiddenInput('contentVersion', $max_version) ->addHiddenInput('overwrite', $overwrite) ->appendChild( id(new AphrontFormTextControl()) diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php index d91165ab1d..3c045e16ba 100644 --- a/src/applications/phriction/editor/PhrictionTransactionEditor.php +++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php @@ -468,7 +468,7 @@ final class PhrictionTransactionEditor $error = null; if ($this->getContentVersion() && - ($object->getContent()->getVersion() != $this->getContentVersion())) { + ($object->getMaxVersion() != $this->getContentVersion())) { $error = new PhabricatorApplicationTransactionValidationError( $type, pht('Edit Conflict'), @@ -519,6 +519,7 @@ final class PhrictionTransactionEditor $document->setContentPHID($content_phid); $document->attachContent($content); $document->setEditedEpoch(PhabricatorTime::getNow()); + $document->setMaxVersion($content->getVersion()); $this->newContent = $content; } @@ -539,7 +540,7 @@ final class PhrictionTransactionEditor $content->setDescription($this->getDescription()); } - $content->setVersion($this->getOldContent()->getVersion() + 1); + $content->setVersion($document->getMaxVersion() + 1); return $content; } diff --git a/src/applications/phriction/storage/PhrictionDocument.php b/src/applications/phriction/storage/PhrictionDocument.php index 626d02c1db..b6dcd6d56d 100644 --- a/src/applications/phriction/storage/PhrictionDocument.php +++ b/src/applications/phriction/storage/PhrictionDocument.php @@ -23,6 +23,7 @@ final class PhrictionDocument extends PhrictionDAO protected $editPolicy; protected $spacePHID; protected $editedEpoch; + protected $maxVersion; private $contentObject = self::ATTACHABLE; private $ancestors = array(); @@ -36,6 +37,7 @@ final class PhrictionDocument extends PhrictionDAO 'depth' => 'uint32', 'status' => 'text32', 'editedEpoch' => 'epoch', + 'maxVersion' => 'uint32', ), self::CONFIG_KEY_SCHEMA => array( 'slug' => array( @@ -89,6 +91,7 @@ final class PhrictionDocument extends PhrictionDAO } $document->setEditedEpoch(PhabricatorTime::getNow()); + $document->setMaxVersion(0); return $document; } From 18e8d9a452651aa465e9c87bc3fc28fc38d41c3b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 29 Aug 2018 15:46:13 -0700 Subject: [PATCH 24/27] Make the Phriction "History" view more aware of drafts Summary: Ref T13077. Updates the "History" view to be slightly better organized and draft-aware. Test Plan: Viewed page history in Phriction. Reviewers: amckinley Maniphest Tasks: T13077 Differential Revision: https://secure.phabricator.com/D19626 --- .../controller/PhrictionHistoryController.php | 131 ++++++++++-------- src/view/phui/PHUIObjectItemView.php | 4 +- 2 files changed, 75 insertions(+), 60 deletions(-) diff --git a/src/applications/phriction/controller/PhrictionHistoryController.php b/src/applications/phriction/controller/PhrictionHistoryController.php index 2a3afc9769..3023d7dcce 100644 --- a/src/applications/phriction/controller/PhrictionHistoryController.php +++ b/src/applications/phriction/controller/PhrictionHistoryController.php @@ -31,32 +31,18 @@ final class PhrictionHistoryController ->executeWithCursorPager($pager); $author_phids = mpull($history, 'getAuthorPHID'); - $handles = $this->loadViewerHandles($author_phids); + $handles = $viewer->loadHandles($author_phids); + + $max_version = (int)$document->getMaxVersion(); + $current_version = $document->getContent()->getVersion(); $list = new PHUIObjectItemListView(); $list->setFlush(true); - foreach ($history as $content) { - - $author = $handles[$content->getAuthorPHID()]->renderLink(); $slug_uri = PhrictionDocument::getSlugURI($document->getSlug()); $version = $content->getVersion(); - $diff_uri = new PhutilURI('/phriction/diff/'.$document->getID().'/'); - - $vs_previous = null; - if ($content->getVersion() != 1) { - $vs_previous = $diff_uri - ->alter('l', $content->getVersion() - 1) - ->alter('r', $content->getVersion()); - } - - $vs_head = null; - if ($content->getPHID() != $document->getContentPHID()) { - $vs_head = $diff_uri - ->alter('l', $content->getVersion()) - ->alter('r', $current->getVersion()); - } + $base_uri = new PhutilURI('/phriction/diff/'.$document->getID().'/'); $change_type = PhrictionChangeType::getChangeTypeLabel( $content->getChangeType()); @@ -68,63 +54,90 @@ final class PhrictionHistoryController $color = 'lightbluetext'; break; case PhrictionChangeType::CHANGE_MOVE_HERE: - $color = 'yellow'; + $color = 'yellow'; break; case PhrictionChangeType::CHANGE_MOVE_AWAY: - $color = 'orange'; + $color = 'orange'; break; case PhrictionChangeType::CHANGE_STUB: $color = 'green'; break; default: - throw new Exception(pht('Unknown change type!')); + $color = 'indigo'; break; } + $version_uri = $slug_uri.'?v='.$version; + $item = id(new PHUIObjectItemView()) - ->setHeader(pht('%s by %s', $change_type, $author)) - ->setStatusIcon('fa-file '.$color) - ->addAttribute( - phutil_tag( - 'a', - array( - 'href' => $slug_uri.'?v='.$version, - ), - pht('Version %s', $version))) - ->addAttribute(pht('%s %s', - phabricator_date($content->getDateCreated(), $viewer), - phabricator_time($content->getDateCreated(), $viewer))); + ->setHref($version_uri); - if ($content->getDescription()) { - $item->addAttribute($content->getDescription()); - } - - if ($vs_previous) { - $item->addIcon( - 'fa-reply', - pht('Show Change'), - array( - 'href' => $vs_previous, - )); + if ($version > $current_version) { + $icon = 'fa-spinner'; + $color = 'pink'; + $header = pht('Draft %d', $version); } else { - $item->addIcon( - 'fa-reply grey', - phutil_tag('em', array(), pht('No previous change'))); + $icon = 'fa-file-o'; + $header = pht('Version %d', $version); } - if ($vs_head) { - $item->addIcon( - 'fa-reply-all', - pht('Show Later Changes'), - array( - 'href' => $vs_head, - )); - } else { - $item->addIcon( - 'fa-reply-all grey', - phutil_tag('em', array(), pht('No later changes'))); + if ($version == $current_version) { + $item->setEffect('selected'); } + $item + ->setHeader($header) + ->setStatusIcon($icon.' '.$color); + + $description = $content->getDescription(); + if (strlen($description)) { + $item->addAttribute($description); + } + + $item->addIcon( + null, + phabricator_datetime($content->getDateCreated(), $viewer)); + + $author_phid = $content->getAuthorPHID(); + $item->addByline($viewer->renderHandle($author_phid)); + + $diff_uri = null; + if ($version > 1) { + $diff_uri = $base_uri + ->alter('l', $version - 1) + ->alter('r', $version); + } else { + $diff_uri = null; + } + + if ($content->getVersion() != $max_version) { + $compare_uri = $base_uri + ->alter('l', $version) + ->alter('r', $max_version); + } else { + $compare_uri = null; + } + + $button_bar = id(new PHUIButtonBarView()) + ->addButton( + id(new PHUIButtonView()) + ->setTag('a') + ->setColor('grey') + ->setIcon('fa-chevron-down') + ->setDisabled(!$diff_uri) + ->setHref($diff_uri) + ->setText(pht('Diff'))) + ->addButton( + id(new PHUIButtonView()) + ->setTag('a') + ->setColor('grey') + ->setIcon('fa-chevron-circle-up') + ->setDisabled(!$compare_uri) + ->setHref($compare_uri) + ->setText(pht('Compare'))); + + $item->setSideColumn($button_bar); + $list->addItem($item); } diff --git a/src/view/phui/PHUIObjectItemView.php b/src/view/phui/PHUIObjectItemView.php index c1e57632c4..0657086c49 100644 --- a/src/view/phui/PHUIObjectItemView.php +++ b/src/view/phui/PHUIObjectItemView.php @@ -709,8 +709,9 @@ final class PHUIObjectItemView extends AphrontTagView { } /* Fixed width, right column container. */ + $column3 = null; if ($this->sideColumn) { - $column2 = phutil_tag( + $column3 = phutil_tag( 'div', array( 'class' => 'phui-oi-col2 phui-oi-side-column', @@ -731,6 +732,7 @@ final class PHUIObjectItemView extends AphrontTagView { $column0, $column1, $column2, + $column3, ))); $box = phutil_tag( From 26d9ee456f30805624d6a1ff975ea022e7285b34 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 31 Aug 2018 10:02:28 -0700 Subject: [PATCH 25/27] Hide the new Phriction "Publish" operation behind the "Prototype" toggle Summary: Ref T13077. This is currently a little too confusing to go out into the world, mostly because there's no way to edit documents without auto-publishing them. Keep it out of the spotlight for this release. Test Plan: Viewed Phriction, saw publish operation marked as a prototype. Reviewers: amckinley Maniphest Tasks: T13077 Differential Revision: https://secure.phabricator.com/D19627 --- .../controller/PhrictionDocumentController.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index 0a4dfdd070..f2cf974504 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -432,13 +432,17 @@ final class PhrictionDocumentController $publish_uri = "/phriction/publish/{$id}/{$content_id}/"; - $curtain->addAction( - id(new PhabricatorActionView()) - ->setName($publish_name) - ->setIcon('fa-upload') - ->setDisabled(!$can_publish) - ->setWorkflow(true) - ->setHref($publish_uri)); + if (PhabricatorEnv::getEnvConfig('phabricator.show-prototypes')) { + $publish_name = pht('Publish (Prototype!)'); + + $curtain->addAction( + id(new PhabricatorActionView()) + ->setName($publish_name) + ->setIcon('fa-upload') + ->setDisabled(!$can_publish) + ->setWorkflow(true) + ->setHref($publish_uri)); + } if ($document->getStatus() == PhrictionDocumentStatus::STATUS_EXISTS) { $curtain->addAction( From c5960c71f92995ab9cae5e1f700268ce25c24c3a Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 31 Aug 2018 12:02:41 -0700 Subject: [PATCH 26/27] Splice in a patch to remove Phriction content rows with no document The unique key on may fail to apply if any content rows don't have a valid document. This is rare, but we have some old random garbage rows on "secure.phabricator.com" which prevent the next patch from applying. Just toss these rows, they're junk. --- .../sql/autopatches/20180828.phriction.07.c.documentuniq.sql | 1 + 1 file changed, 1 insertion(+) create mode 100644 resources/sql/autopatches/20180828.phriction.07.c.documentuniq.sql diff --git a/resources/sql/autopatches/20180828.phriction.07.c.documentuniq.sql b/resources/sql/autopatches/20180828.phriction.07.c.documentuniq.sql new file mode 100644 index 0000000000..481935beed --- /dev/null +++ b/resources/sql/autopatches/20180828.phriction.07.c.documentuniq.sql @@ -0,0 +1 @@ +DELETE FROM phriction_content WHERE documentPHID = ''; From dc0924deeddff1a76251ae84e7c2bddd11854073 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 31 Aug 2018 12:07:07 -0700 Subject: [PATCH 27/27] Properly namespace the query in the spliced-in content cleanup patch Yikes. Actually ran it this time! --- .../sql/autopatches/20180828.phriction.07.c.documentuniq.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/sql/autopatches/20180828.phriction.07.c.documentuniq.sql b/resources/sql/autopatches/20180828.phriction.07.c.documentuniq.sql index 481935beed..d086cc6141 100644 --- a/resources/sql/autopatches/20180828.phriction.07.c.documentuniq.sql +++ b/resources/sql/autopatches/20180828.phriction.07.c.documentuniq.sql @@ -1 +1 @@ -DELETE FROM phriction_content WHERE documentPHID = ''; +DELETE FROM {$NAMESPACE}_phriction.phriction_content WHERE documentPHID = '';