From c05306d746d7e06e3f2a5eb712af89a616abfdc6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Dec 2016 09:49:24 -0800 Subject: [PATCH] Move Differential to EditEngine comments Summary: Ref T11114. This is a transitional change that breaks a bunch of stuff. I'll hold it until I've restored features. This stuff works: - Commenting. - Subscribers/tags/reviewers. - Pinning. - Drafts. This stuff does not work yet: - Preview of inline comments. - Probably submitting inlines, whatsoever. - Comment-area warnings like "There are failing tests." - All meaningful actions (accept, reject, etc). Test Plan: Commented on a revision. Essentially nothing else works yet. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11114 Differential Revision: https://secure.phabricator.com/D17106 --- src/__phutil_library_map__.php | 4 - .../DifferentialCommentSaveController.php | 143 ------------- .../DifferentialRevisionViewController.php | 190 +---------------- .../editor/DifferentialRevisionEditEngine.php | 5 + .../view/DifferentialAddCommentView.php | 201 ------------------ 5 files changed, 8 insertions(+), 535 deletions(-) delete mode 100644 src/applications/differential/controller/DifferentialCommentSaveController.php delete mode 100644 src/applications/differential/view/DifferentialAddCommentView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b3438afc96..fbef0ef13e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -348,7 +348,6 @@ phutil_register_library_map(array( 'DarkConsoleXHProfPluginAPI' => 'applications/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php', 'DifferentialAction' => 'applications/differential/constants/DifferentialAction.php', 'DifferentialActionEmailCommand' => 'applications/differential/command/DifferentialActionEmailCommand.php', - 'DifferentialAddCommentView' => 'applications/differential/view/DifferentialAddCommentView.php', 'DifferentialAdjustmentMapTestCase' => 'applications/differential/storage/__tests__/DifferentialAdjustmentMapTestCase.php', 'DifferentialAffectedPath' => 'applications/differential/storage/DifferentialAffectedPath.php', 'DifferentialAsanaRepresentationField' => 'applications/differential/customfield/DifferentialAsanaRepresentationField.php', @@ -381,7 +380,6 @@ phutil_register_library_map(array( 'DifferentialChangesetViewController' => 'applications/differential/controller/DifferentialChangesetViewController.php', 'DifferentialCloseConduitAPIMethod' => 'applications/differential/conduit/DifferentialCloseConduitAPIMethod.php', 'DifferentialCommentPreviewController' => 'applications/differential/controller/DifferentialCommentPreviewController.php', - 'DifferentialCommentSaveController' => 'applications/differential/controller/DifferentialCommentSaveController.php', 'DifferentialCommitMessageCustomField' => 'applications/differential/field/DifferentialCommitMessageCustomField.php', 'DifferentialCommitMessageField' => 'applications/differential/field/DifferentialCommitMessageField.php', 'DifferentialCommitMessageParser' => 'applications/differential/parser/DifferentialCommitMessageParser.php', @@ -4982,7 +4980,6 @@ phutil_register_library_map(array( 'DarkConsoleXHProfPluginAPI' => 'Phobject', 'DifferentialAction' => 'Phobject', 'DifferentialActionEmailCommand' => 'MetaMTAEmailTransactionCommand', - 'DifferentialAddCommentView' => 'AphrontView', 'DifferentialAdjustmentMapTestCase' => 'PhutilTestCase', 'DifferentialAffectedPath' => 'DifferentialDAO', 'DifferentialAsanaRepresentationField' => 'DifferentialCustomField', @@ -5018,7 +5015,6 @@ phutil_register_library_map(array( 'DifferentialChangesetViewController' => 'DifferentialController', 'DifferentialCloseConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialCommentPreviewController' => 'DifferentialController', - 'DifferentialCommentSaveController' => 'DifferentialController', 'DifferentialCommitMessageCustomField' => 'DifferentialCommitMessageField', 'DifferentialCommitMessageField' => 'Phobject', 'DifferentialCommitMessageParser' => 'Phobject', diff --git a/src/applications/differential/controller/DifferentialCommentSaveController.php b/src/applications/differential/controller/DifferentialCommentSaveController.php deleted file mode 100644 index 87376bf6d5..0000000000 --- a/src/applications/differential/controller/DifferentialCommentSaveController.php +++ /dev/null @@ -1,143 +0,0 @@ -getViewer(); - $id = $request->getURIData('id'); - - if (!$request->isFormPost()) { - return new Aphront400Response(); - } - - $revision = id(new DifferentialRevisionQuery()) - ->setViewer($viewer) - ->withIDs(array($id)) - ->needReviewerStatus(true) - ->needReviewerAuthority(true) - ->executeOne(); - if (!$revision) { - return new Aphront404Response(); - } - - $type_action = DifferentialTransaction::TYPE_ACTION; - $type_subscribers = PhabricatorTransactions::TYPE_SUBSCRIBERS; - $type_edge = PhabricatorTransactions::TYPE_EDGE; - $type_comment = PhabricatorTransactions::TYPE_COMMENT; - $type_inline = DifferentialTransaction::TYPE_INLINE; - - $edge_reviewer = DifferentialRevisionHasReviewerEdgeType::EDGECONST; - - $xactions = array(); - - $action = $request->getStr('action'); - switch ($action) { - case DifferentialAction::ACTION_COMMENT: - case DifferentialAction::ACTION_ADDREVIEWERS: - case DifferentialAction::ACTION_ADDCCS: - // These transaction types have no direct effect, they just - // accompany other transaction types which can have an effect. - break; - default: - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType($type_action) - ->setNewValue($request->getStr('action')); - break; - } - - $ccs = $request->getArr('ccs'); - if ($ccs) { - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType($type_subscribers) - ->setNewValue(array('+' => $ccs)); - } - - $current_reviewers = mpull( - $revision->getReviewerStatus(), - null, - 'getReviewerPHID'); - - $reviewer_edges = array(); - $add_reviewers = $request->getArr('reviewers'); - foreach ($add_reviewers as $reviewer_phid) { - if (isset($current_reviewers[$reviewer_phid])) { - continue; - } - $reviewer = new DifferentialReviewerProxy( - $reviewer_phid, - array( - 'status' => DifferentialReviewerStatus::STATUS_ADDED, - )); - $reviewer_edges[$reviewer_phid] = array( - 'data' => $reviewer->getEdgeData(), - ); - } - - if ($add_reviewers) { - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType($type_edge) - ->setMetadataValue('edge:type', $edge_reviewer) - ->setNewValue(array('+' => $reviewer_edges)); - } - - $inlines = DifferentialTransactionQuery::loadUnsubmittedInlineComments( - $viewer, - $revision); - foreach ($inlines as $inline) { - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType($type_inline) - ->attachComment($inline); - } - - // NOTE: If there are no other transactions, add an empty comment - // transaction so that we'll raise a more user-friendly error message, - // to the effect of "you can not post an empty comment". - $no_xactions = !$xactions; - - $comment = $request->getStr('comment'); - if (strlen($comment) || $no_xactions) { - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType($type_comment) - ->attachComment( - id(new DifferentialTransactionComment()) - ->setRevisionPHID($revision->getPHID()) - ->setContent($comment)); - } - - - $editor = id(new DifferentialTransactionEditor()) - ->setActor($viewer) - ->setContentSourceFromRequest($request) - ->setContinueOnMissingFields(true) - ->setContinueOnNoEffect($request->isContinueRequest()); - - $revision_uri = '/D'.$revision->getID(); - - try { - $editor->applyTransactions($revision, $xactions); - } catch (PhabricatorApplicationTransactionNoEffectException $ex) { - return id(new PhabricatorApplicationTransactionNoEffectResponse()) - ->setCancelURI($revision_uri) - ->setException($ex); - } catch (PhabricatorApplicationTransactionValidationException $ex) { - return id(new PhabricatorApplicationTransactionValidationResponse()) - ->setCancelURI($revision_uri) - ->setException($ex); - } - - $user = $request->getUser(); - $draft = id(new PhabricatorDraft())->loadOneWhere( - 'authorPHID = %s AND draftKey = %s', - $user->getPHID(), - 'differential-comment-'.$revision->getID()); - if ($draft) { - $draft->delete(); - } - DifferentialDraft::deleteAllDrafts($user->getPHID(), $revision->getPHID()); - - return id(new AphrontRedirectResponse()) - ->setURI('/D'.$revision->getID()); - } - -} diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 390c90b2e9..6b38ed042f 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -390,11 +390,6 @@ final class DifferentialRevisionViewController extends DifferentialController { ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->addTabGroup($tab_group); - $comment_form = null; - if (!$viewer_is_anonymous) { - $comment_form = $this->buildCommentForm($revision, $field_list); - } - $signatures = DifferentialRequiredSignaturesField::loadForRevision( $revision); $missing_signatures = false; @@ -426,14 +421,9 @@ final class DifferentialRevisionViewController extends DifferentialController { ); } - if ($comment_form) { - $footer[] = $comment_form; - } else { - // TODO: For now, just use this to get "Login to Comment". - $footer[] = id(new PhabricatorApplicationTransactionCommentView()) - ->setUser($viewer) - ->setRequestURI($request->getRequestURI()); - } + $footer[] = id(new DifferentialRevisionEditEngine()) + ->setViewer($viewer) + ->buildEditEngineCommentView($revision); $object_id = 'D'.$revision->getID(); $operations_box = $this->buildOperationsBox($revision); @@ -458,20 +448,12 @@ final class DifferentialRevisionViewController extends DifferentialController { ->build($changesets); } - // Haunt Mode - $pane_id = celerity_generate_unique_node_id(); - Javelin::initBehavior( - 'differential-keyboard-navigation', - array( - 'haunt' => $pane_id, - )); Javelin::initBehavior('differential-user-select'); $view = id(new PHUITwoColumnView()) ->setHeader($header) ->setSubheader($subheader) ->setCurtain($curtain) - ->setID($pane_id) ->setMainColumn(array( $operations_box, $info_view, @@ -613,172 +595,6 @@ final class DifferentialRevisionViewController extends DifferentialController { return $curtain; } - private function buildCommentForm( - DifferentialRevision $revision, - $field_list) { - - $viewer = $this->getViewer(); - - $draft = id(new PhabricatorDraft())->loadOneWhere( - 'authorPHID = %s AND draftKey = %s', - $viewer->getPHID(), - 'differential-comment-'.$revision->getID()); - - $reviewers = array(); - $ccs = array(); - if ($draft) { - $reviewers = idx($draft->getMetadata(), 'reviewers', array()); - $ccs = idx($draft->getMetadata(), 'ccs', array()); - if ($reviewers || $ccs) { - $handles = $this->loadViewerHandles(array_merge($reviewers, $ccs)); - $reviewers = array_select_keys($handles, $reviewers); - $ccs = array_select_keys($handles, $ccs); - } - } - - $comment_form = id(new DifferentialAddCommentView()) - ->setRevision($revision); - - $review_warnings = array(); - foreach ($field_list->getFields() as $field) { - $review_warnings[] = $field->getWarningsForDetailView(); - } - $review_warnings = array_mergev($review_warnings); - - if ($review_warnings) { - $review_warnings_panel = id(new PHUIInfoView()) - ->setSeverity(PHUIInfoView::SEVERITY_WARNING) - ->setErrors($review_warnings); - $comment_form->setInfoView($review_warnings_panel); - } - - $action_uri = $this->getApplicationURI( - 'comment/save/'.$revision->getID().'/'); - - $comment_form->setActions($this->getRevisionCommentActions($revision)) - ->setActionURI($action_uri) - ->setUser($viewer) - ->setDraft($draft) - ->setReviewers(mpull($reviewers, 'getFullName', 'getPHID')) - ->setCCs(mpull($ccs, 'getFullName', 'getPHID')); - - // TODO: This just makes the "Z" key work. Generalize this and remove - // it at some point. - $comment_form = phutil_tag( - 'div', - array( - 'class' => 'differential-add-comment-panel', - ), - $comment_form); - return $comment_form; - } - - private function getRevisionCommentActions(DifferentialRevision $revision) { - $actions = array( - DifferentialAction::ACTION_COMMENT => true, - ); - - $viewer = $this->getViewer(); - $viewer_phid = $viewer->getPHID(); - $viewer_is_owner = ($viewer_phid == $revision->getAuthorPHID()); - $viewer_is_reviewer = in_array($viewer_phid, $revision->getReviewers()); - $status = $revision->getStatus(); - - $viewer_has_accepted = false; - $viewer_has_rejected = false; - $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED; - $status_rejected = DifferentialReviewerStatus::STATUS_REJECTED; - foreach ($revision->getReviewerStatus() as $reviewer) { - if ($reviewer->getReviewerPHID() == $viewer_phid) { - if ($reviewer->getStatus() == $status_accepted) { - $viewer_has_accepted = true; - } - if ($reviewer->getStatus() == $status_rejected) { - $viewer_has_rejected = true; - } - break; - } - } - - $allow_self_accept = PhabricatorEnv::getEnvConfig( - 'differential.allow-self-accept'); - $always_allow_abandon = PhabricatorEnv::getEnvConfig( - 'differential.always-allow-abandon'); - $always_allow_close = PhabricatorEnv::getEnvConfig( - 'differential.always-allow-close'); - $allow_reopen = PhabricatorEnv::getEnvConfig( - 'differential.allow-reopen'); - - if ($viewer_is_owner) { - switch ($status) { - case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: - $actions[DifferentialAction::ACTION_ACCEPT] = $allow_self_accept; - $actions[DifferentialAction::ACTION_ABANDON] = true; - $actions[DifferentialAction::ACTION_RETHINK] = true; - break; - case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: - case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: - $actions[DifferentialAction::ACTION_ACCEPT] = $allow_self_accept; - $actions[DifferentialAction::ACTION_ABANDON] = true; - $actions[DifferentialAction::ACTION_REQUEST] = true; - break; - case ArcanistDifferentialRevisionStatus::ACCEPTED: - $actions[DifferentialAction::ACTION_ABANDON] = true; - $actions[DifferentialAction::ACTION_REQUEST] = true; - $actions[DifferentialAction::ACTION_RETHINK] = true; - $actions[DifferentialAction::ACTION_CLOSE] = true; - break; - case ArcanistDifferentialRevisionStatus::CLOSED: - break; - case ArcanistDifferentialRevisionStatus::ABANDONED: - $actions[DifferentialAction::ACTION_RECLAIM] = true; - break; - } - } else { - switch ($status) { - case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: - $actions[DifferentialAction::ACTION_ABANDON] = $always_allow_abandon; - $actions[DifferentialAction::ACTION_ACCEPT] = true; - $actions[DifferentialAction::ACTION_REJECT] = true; - $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer; - break; - case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: - case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: - $actions[DifferentialAction::ACTION_ABANDON] = $always_allow_abandon; - $actions[DifferentialAction::ACTION_ACCEPT] = true; - $actions[DifferentialAction::ACTION_REJECT] = !$viewer_has_rejected; - $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer; - break; - case ArcanistDifferentialRevisionStatus::ACCEPTED: - $actions[DifferentialAction::ACTION_ABANDON] = $always_allow_abandon; - $actions[DifferentialAction::ACTION_ACCEPT] = !$viewer_has_accepted; - $actions[DifferentialAction::ACTION_REJECT] = true; - $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer; - break; - case ArcanistDifferentialRevisionStatus::CLOSED: - case ArcanistDifferentialRevisionStatus::ABANDONED: - break; - } - if ($status != ArcanistDifferentialRevisionStatus::CLOSED) { - $actions[DifferentialAction::ACTION_CLAIM] = true; - $actions[DifferentialAction::ACTION_CLOSE] = $always_allow_close; - } - } - - $actions[DifferentialAction::ACTION_ADDREVIEWERS] = true; - $actions[DifferentialAction::ACTION_ADDCCS] = true; - $actions[DifferentialAction::ACTION_REOPEN] = $allow_reopen && - ($status == ArcanistDifferentialRevisionStatus::CLOSED); - - $actions = array_keys(array_filter($actions)); - $actions_dict = array(); - foreach ($actions as $action) { - $actions_dict[$action] = DifferentialAction::getActionVerb($action); - } - - return $actions_dict; - } - private function loadHistoryDiffStatus(array $diffs) { assert_instances_of($diffs, 'DifferentialDiff'); diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php index 8e15238661..43b3afe91f 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -73,6 +73,10 @@ final class DifferentialRevisionEditEngine return $object->getURI(); } + protected function getEditorURI() { + return $this->getApplication()->getApplicationURI('revision/edit/'); + } + public function setDiff(DifferentialDiff $diff) { $this->diff = $diff; return $this; @@ -176,6 +180,7 @@ final class DifferentialRevisionEditEngine ->setUseEdgeTransactions(true) ->setTransactionType( DifferentialRevisionReviewersTransaction::TRANSACTIONTYPE) + ->setCommentActionLabel(pht('Edit Reviewers')) ->setDescription(pht('Reviewers for this revision.')) ->setConduitDescription(pht('Change the reviewers for this revision.')) ->setConduitTypeDescription(pht('New reviewers.')) diff --git a/src/applications/differential/view/DifferentialAddCommentView.php b/src/applications/differential/view/DifferentialAddCommentView.php deleted file mode 100644 index 1cddab151d..0000000000 --- a/src/applications/differential/view/DifferentialAddCommentView.php +++ /dev/null @@ -1,201 +0,0 @@ -errorView = $error_view; - return $this; - } - - public function getErrorView() { - return $this->errorView; - } - - public function setRevision($revision) { - $this->revision = $revision; - return $this; - } - - public function setActions(array $actions) { - $this->actions = $actions; - return $this; - } - - public function setActionURI($uri) { - $this->actionURI = $uri; - return $this; - } - - public function setDraft(PhabricatorDraft $draft = null) { - $this->draft = $draft; - return $this; - } - - public function setReviewers(array $names) { - $this->reviewers = $names; - return $this; - } - - public function setCCs(array $names) { - $this->ccs = $names; - return $this; - } - - public function render() { - $viewer = $this->getViewer(); - - $this->requireResource('differential-revision-add-comment-css'); - $revision = $this->revision; - - $action = null; - if ($this->draft) { - $action = idx($this->draft->getMetadata(), 'action'); - } - - $enable_reviewers = DifferentialAction::allowReviewers($action); - $enable_ccs = ($action == DifferentialAction::ACTION_ADDCCS); - $add_reviewers_labels = array( - 'add_reviewers' => pht('Add Reviewers'), - 'request_review' => pht('Add Reviewers'), - 'resign' => pht('Suggest Reviewers'), - ); - - $mailable_source = new PhabricatorMetaMTAMailableDatasource(); - - // TODO: This should be a reviewers datasource, but it's a mess. - $reviewer_source = new PhabricatorMetaMTAMailableDatasource(); - - $form = new AphrontFormView(); - $form - ->setWorkflow(true) - ->setViewer($viewer) - ->setAction($this->actionURI) - ->addHiddenInput('revision_id', $revision->getID()) - ->appendChild( - id(new AphrontFormSelectControl()) - ->setLabel(pht('Action')) - ->setName('action') - ->setValue($action) - ->setID('comment-action') - ->setOptions($this->actions)) - ->appendControl( - id(new AphrontFormTokenizerControl()) - ->setLabel($enable_reviewers ? $add_reviewers_labels[$action] : - $add_reviewers_labels['add_reviewers']) - ->setName('reviewers') - ->setControlID('add-reviewers') - ->setControlStyle($enable_reviewers ? null : 'display: none') - ->setID('add-reviewers-tokenizer') - ->setDisableBehavior(true) - ->setDatasource($reviewer_source)) - ->appendControl( - id(new AphrontFormTokenizerControl()) - ->setLabel(pht('Add Subscribers')) - ->setName('ccs') - ->setControlID('add-ccs') - ->setControlStyle($enable_ccs ? null : 'display: none') - ->setID('add-ccs-tokenizer') - ->setDisableBehavior(true) - ->setDatasource($mailable_source)) - ->appendChild( - id(new PhabricatorRemarkupControl()) - ->setName('comment') - ->setID('comment-content') - ->setLabel(pht('Comment')) - ->setValue($this->draft ? $this->draft->getDraft() : null) - ->setViewer($viewer)) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue(pht('Submit'))); - - Javelin::initBehavior( - 'differential-add-reviewers-and-ccs', - array( - 'dynamic' => array( - 'add-reviewers-tokenizer' => array( - 'actions' => array( - 'request_review' => 1, - 'add_reviewers' => 1, - 'resign' => 1, - ), - 'src' => $reviewer_source->getDatasourceURI(), - 'value' => $this->reviewers, - 'row' => 'add-reviewers', - 'labels' => $add_reviewers_labels, - 'placeholder' => $reviewer_source->getPlaceholderText(), - ), - 'add-ccs-tokenizer' => array( - 'actions' => array('add_ccs' => 1), - 'src' => $mailable_source->getDatasourceURI(), - 'value' => $this->ccs, - 'row' => 'add-ccs', - 'placeholder' => $mailable_source->getPlaceholderText(), - ), - ), - 'select' => 'comment-action', - )); - - $diff = $revision->loadActiveDiff(); - $rev_id = $revision->getID(); - - Javelin::initBehavior( - 'differential-feedback-preview', - array( - 'uri' => '/differential/comment/preview/'.$rev_id.'/', - 'preview' => 'comment-preview', - 'action' => 'comment-action', - 'content' => 'comment-content', - 'previewTokenizers' => array( - 'reviewers' => 'add-reviewers-tokenizer', - 'ccs' => 'add-ccs-tokenizer', - ), - - 'inlineuri' => '/differential/comment/inline/preview/'.$rev_id.'/', - 'inline' => 'inline-comment-preview', - )); - - $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); - $header_text = $is_serious - ? pht('Add Comment') - : pht('Leap Into Action!'); - - $header = id(new PHUIHeaderView()) - ->setHeader($header_text); - - $anchor = id(new PhabricatorAnchorView()) - ->setAnchorName('comment') - ->setNavigationMarker(true); - - $loading = phutil_tag( - 'span', - array('class' => 'aphront-panel-preview-loading-text'), - pht('Loading comment preview...')); - - $preview = phutil_tag_div( - 'aphront-panel-preview aphront-panel-flush', - array( - phutil_tag('div', array('id' => 'comment-preview'), $loading), - phutil_tag('div', array('id' => 'inline-comment-preview')), - )); - - - $comment_box = id(new PHUIObjectBoxView()) - ->setHeader($header) - ->appendChild($anchor) - ->appendChild($form); - - if ($this->errorView) { - $comment_box->setInfoView($this->errorView); - } - - return array($comment_box, $preview); - } -}