From 88227d26bc131647a48353be0a48c3314dd14895 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 21 Feb 2014 11:53:04 -0800 Subject: [PATCH] Allow CustomField to provide ApplicationTransaction change details Summary: Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff. Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs. Test Plan: {F115918} - Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3886, T418 Differential Revision: https://secure.phabricator.com/D8284 --- resources/celerity/map.php | 20 +++--- .../storage/PhabricatorConfigTransaction.php | 10 ++- .../customfield/DifferentialSummaryField.php | 14 +++- .../legalpad/storage/LegalpadTransaction.php | 13 ++-- .../storage/ManiphestTransaction.php | 15 ++-- .../PassphraseCredentialTransaction.php | 13 ++-- .../phlux/storage/PhluxTransaction.php | 13 ++-- .../pholio/storage/PholioTransaction.php | 10 ++- .../storage/PonderAnswerTransaction.php | 13 ++-- .../storage/PonderQuestionTransaction.php | 13 ++-- .../PhabricatorRepositoryTransaction.php | 13 ++-- .../PhabricatorSlowvoteTransaction.php | 10 +-- .../PhabricatorApplicationTransaction.php | 71 +++++++++++++++---- .../PhabricatorApplicationTransactionView.php | 6 +- .../field/PhabricatorCustomField.php | 21 ++++++ .../transactions/behavior-transaction-list.js | 13 ---- 16 files changed, 140 insertions(+), 128 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 2b3d84efae..1f8dd90d58 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -407,7 +407,7 @@ return array( 'rsrc/js/application/search/behavior-reorder-queries.js' => '34397f68', 'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => 'a51fdb2e', 'rsrc/js/application/transactions/behavior-transaction-comment-form.js' => '9084a36f', - 'rsrc/js/application/transactions/behavior-transaction-list.js' => 'bfb45968', + 'rsrc/js/application/transactions/behavior-transaction-list.js' => '3c918aa8', 'rsrc/js/application/uiexample/JavelinViewExample.js' => 'd4a14807', 'rsrc/js/application/uiexample/ReactorButtonExample.js' => '44524435', 'rsrc/js/application/uiexample/ReactorCheckboxExample.js' => '7ba325ee', @@ -598,7 +598,7 @@ return array( 'javelin-behavior-phabricator-show-all-transactions' => '7c273581', 'javelin-behavior-phabricator-tooltips' => 'e5dd1c6d', 'javelin-behavior-phabricator-transaction-comment-form' => '9084a36f', - 'javelin-behavior-phabricator-transaction-list' => 'bfb45968', + 'javelin-behavior-phabricator-transaction-list' => '3c918aa8', 'javelin-behavior-phabricator-watch-anchor' => '06e05112', 'javelin-behavior-phame-post-preview' => '61d927ec', 'javelin-behavior-pholio-mock-edit' => '1e1e8bb0', @@ -1057,6 +1057,14 @@ return array( 2 => 'javelin-util', 3 => 'javelin-request', ), + '3c918aa8' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-stratcom', + 2 => 'javelin-workflow', + 3 => 'javelin-dom', + 4 => 'javelin-fx', + ), '403a3dce' => array( 0 => 'javelin-install', @@ -1592,14 +1600,6 @@ return array( 1 => 'javelin-dom', 2 => 'javelin-reactor-dom', ), - 'bfb45968' => - array( - 0 => 'javelin-behavior', - 1 => 'javelin-stratcom', - 2 => 'javelin-workflow', - 3 => 'javelin-dom', - 4 => 'javelin-fx', - ), 'c01153ea' => array( 0 => 'javelin-behavior', diff --git a/src/applications/config/storage/PhabricatorConfigTransaction.php b/src/applications/config/storage/PhabricatorConfigTransaction.php index ebfd944252..75c2cc34a3 100644 --- a/src/applications/config/storage/PhabricatorConfigTransaction.php +++ b/src/applications/config/storage/PhabricatorConfigTransaction.php @@ -89,12 +89,10 @@ final class PhabricatorConfigTransaction $new_text = PhabricatorConfigJSON::prettyPrintJSON($new['value']); } - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText($old_text) - ->setNewText($new_text); - - return $view->render(); + return $this->renderTextCorpusChangeDetails( + $viewer, + $old_text, + $new_text); } public function getColor() { diff --git a/src/applications/differential/customfield/DifferentialSummaryField.php b/src/applications/differential/customfield/DifferentialSummaryField.php index 88f6cb779d..60aa173841 100644 --- a/src/applications/differential/customfield/DifferentialSummaryField.php +++ b/src/applications/differential/customfield/DifferentialSummaryField.php @@ -64,6 +64,18 @@ final class DifferentialSummaryField $xaction->renderHandleLink($object_phid)); } - // TODO: Support hasChangeDetails() in CustomFields. + public function getApplicationTransactionHasChangeDetails( + PhabricatorApplicationTransaction $xaction) { + return true; + } + + public function getApplicationTransactionChangeDetails( + PhabricatorApplicationTransaction $xaction, + PhabricatorUser $viewer) { + return $xaction->renderTextCorpusChangeDetails( + $viewer, + $xaction->getOldValue(), + $xaction->getNewValue()); + } } diff --git a/src/applications/legalpad/storage/LegalpadTransaction.php b/src/applications/legalpad/storage/LegalpadTransaction.php index 9abf15db2d..26cb8b1572 100644 --- a/src/applications/legalpad/storage/LegalpadTransaction.php +++ b/src/applications/legalpad/storage/LegalpadTransaction.php @@ -68,15 +68,10 @@ final class LegalpadTransaction extends PhabricatorApplicationTransaction { } public function renderChangeDetails(PhabricatorUser $viewer) { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText($old) - ->setNewText($new); - - return $view->render(); + return $this->renderTextCorpusChangeDetails( + $viewer, + $this->getOldValue(), + $this->getNewValue()); } } diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index 72b76714ba..09a3f936e5 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -631,17 +631,10 @@ final class ManiphestTransaction } public function renderChangeDetails(PhabricatorUser $viewer) { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - require_celerity_resource('differential-changeset-view-css'); - - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText($old) - ->setNewText($new); - - return $view->render(); + return $this->renderTextCorpusChangeDetails( + $viewer, + $this->getOldValue(), + $this->getNewValue()); } public function getMailTags() { diff --git a/src/applications/passphrase/storage/PassphraseCredentialTransaction.php b/src/applications/passphrase/storage/PassphraseCredentialTransaction.php index 52739a4808..2303ea39ac 100644 --- a/src/applications/passphrase/storage/PassphraseCredentialTransaction.php +++ b/src/applications/passphrase/storage/PassphraseCredentialTransaction.php @@ -91,15 +91,10 @@ final class PassphraseCredentialTransaction } public function renderChangeDetails(PhabricatorUser $viewer) { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText(json_encode($old)) - ->setNewText(json_encode($new)); - - return $view->render(); + return $this->renderTextCorpusChangeDetails( + $viewer, + json_encode($this->getOldValue()), + json_encode($this->getNewValue())); } diff --git a/src/applications/phlux/storage/PhluxTransaction.php b/src/applications/phlux/storage/PhluxTransaction.php index 1835796620..9cac59fbec 100644 --- a/src/applications/phlux/storage/PhluxTransaction.php +++ b/src/applications/phlux/storage/PhluxTransaction.php @@ -43,15 +43,10 @@ final class PhluxTransaction extends PhabricatorApplicationTransaction { } public function renderChangeDetails(PhabricatorUser $viewer) { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText(json_encode($old)) - ->setNewText(json_encode($new)); - - return $view->render(); + return $this->renderTextCorpusChangeDetails( + $viewer, + json_encode($this->getOldValue()), + json_encode($this->getNewValue())); } diff --git a/src/applications/pholio/storage/PholioTransaction.php b/src/applications/pholio/storage/PholioTransaction.php index c20865b914..d65414f098 100644 --- a/src/applications/pholio/storage/PholioTransaction.php +++ b/src/applications/pholio/storage/PholioTransaction.php @@ -283,12 +283,10 @@ final class PholioTransaction extends PhabricatorApplicationTransaction { $new = reset($new); } - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText($old) - ->setNewText($new); - - return $view->render(); + return $this->renderTextCorpusChangeDetails( + $viewer, + $old, + $new); } public function getColor() { diff --git a/src/applications/ponder/storage/PonderAnswerTransaction.php b/src/applications/ponder/storage/PonderAnswerTransaction.php index aaffe9102f..f12303b3a7 100644 --- a/src/applications/ponder/storage/PonderAnswerTransaction.php +++ b/src/applications/ponder/storage/PonderAnswerTransaction.php @@ -95,15 +95,10 @@ final class PonderAnswerTransaction } public function renderChangeDetails(PhabricatorUser $viewer) { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText($old) - ->setNewText($new); - - return $view->render(); + return $this->renderTextCorpusChangeDetails( + $viewer, + $this->getOldValue(), + $this->getNewValue()); } } diff --git a/src/applications/ponder/storage/PonderQuestionTransaction.php b/src/applications/ponder/storage/PonderQuestionTransaction.php index 3218a01f8a..a1ed5d47cc 100644 --- a/src/applications/ponder/storage/PonderQuestionTransaction.php +++ b/src/applications/ponder/storage/PonderQuestionTransaction.php @@ -135,15 +135,10 @@ final class PonderQuestionTransaction } public function renderChangeDetails(PhabricatorUser $viewer) { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText($old) - ->setNewText($new); - - return $view->render(); + return $this->renderTextCorpusChangeDetails( + $viewer, + $this->getOldValue(), + $this->getNewValue()); } public function getActionStrength() { diff --git a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php index c29aafde9b..446dcb19e6 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php +++ b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php @@ -381,15 +381,10 @@ final class PhabricatorRepositoryTransaction } public function renderChangeDetails(PhabricatorUser $viewer) { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText($old) - ->setNewText($new); - - return $view->render(); + return $this->renderTextCorpusChangeDetails( + $viewer, + $this->getOldValue(), + $this->getNewValue()); } } diff --git a/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php b/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php index bdd04c8540..9370ea361a 100644 --- a/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php +++ b/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php @@ -119,15 +119,7 @@ final class PhabricatorSlowvoteTransaction } public function renderChangeDetails(PhabricatorUser $viewer) { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) - ->setUser($viewer) - ->setOldText($old) - ->setNewText($new); - - return $view->render(); + return $this->renderTextCorpusChangeDetails($viewer); } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index e378860aa5..2b4b282d76 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -250,6 +250,29 @@ abstract class PhabricatorApplicationTransaction return null; } + protected function getTransactionCustomField() { + switch ($this->getTransactionType()) { + case PhabricatorTransactions::TYPE_CUSTOMFIELD: + $key = $this->getMetadataValue('customfield:key'); + if (!$key) { + return null; + } + + $field = PhabricatorCustomField::getObjectField( + $this->getObject(), + PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS, + $key); + if (!$field) { + return null; + } + + $field->setViewer($this->getViewer()); + return $field; + } + + return null; + } + public function shouldHide() { switch ($this->getTransactionType()) { case PhabricatorTransactions::TYPE_VIEW_POLICY: @@ -396,13 +419,8 @@ abstract class PhabricatorApplicationTransaction } case PhabricatorTransactions::TYPE_CUSTOMFIELD: - $key = $this->getMetadataValue('customfield:key'); - $field = PhabricatorCustomField::getObjectField( - $this->getObject(), - PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS, - $key); + $field = $this->getTransactionCustomField(); if ($field) { - $field->setViewer($this->getViewer()); return $field->getApplicationTransactionTitle($this); } else { return pht( @@ -460,13 +478,8 @@ abstract class PhabricatorApplicationTransaction $this->renderHandleLink($author_phid), $this->renderHandleLink($object_phid)); case PhabricatorTransactions::TYPE_CUSTOMFIELD: - $key = $this->getMetadataValue('customfield:key'); - $field = PhabricatorCustomField::getObjectField( - $this->getObject(), - PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS, - $key); + $field = $this->getTransactionCustomField(); if ($field) { - $field->setViewer($this->getViewer()); return $field->getApplicationTransactionTitleForFeed($this, $story); } else { return pht( @@ -524,11 +537,43 @@ abstract class PhabricatorApplicationTransaction } public function hasChangeDetails() { + switch ($this->getTransactionType()) { + case PhabricatorTransactions::TYPE_CUSTOMFIELD: + $field = $this->getTransactionCustomField(); + if ($field) { + return $field->getApplicationTransactionHasChangeDetails($this); + } + break; + } return false; } public function renderChangeDetails(PhabricatorUser $viewer) { - return null; + switch ($this->getTransactionType()) { + case PhabricatorTransactions::TYPE_CUSTOMFIELD: + $field = $this->getTransactionCustomField(); + if ($field) { + return $field->getApplicationTransactionChangeDetails($this, $viewer); + } + break; + } + + return $this->renderTextCorpusChangeDetails(); + } + + public function renderTextCorpusChangeDetails( + PhabricatorUser $viewer, + $old, + $new) { + + require_celerity_resource('differential-changeset-view-css'); + + $view = id(new PhabricatorApplicationTransactionTextDiffDetailView()) + ->setUser($viewer) + ->setOldText($old) + ->setNewText($new); + + return $view->render(); } public function attachTransactionGroup(array $group) { diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php index 2d2a47afdb..38592a2978 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php @@ -175,11 +175,7 @@ class PhabricatorApplicationTransactionView extends AphrontView { 'a', array( 'href' => '/transactions/detail/'.$xaction->getPHID().'/', - 'sigil' => 'transaction-detail', - 'mustcapture' => true, - 'meta' => array( - 'anchor' => $this->anchorOffset, - ), + 'sigil' => 'workflow', ), pht('(Show Details)')); } diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index f4b6ac2b10..f8e9e69454 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -904,6 +904,27 @@ abstract class PhabricatorCustomField { } + public function getApplicationTransactionHasChangeDetails( + PhabricatorApplicationTransaction $xaction) { + if ($this->proxy) { + return $this->proxy->getApplicationTransactionHasChangeDetails( + $xaction); + } + return false; + } + + public function getApplicationTransactionChangeDetails( + PhabricatorApplicationTransaction $xaction, + PhabricatorUser $viewer) { + if ($this->proxy) { + return $this->proxy->getApplicationTransactionChangeDetails( + $xaction, + $viewer); + } + return null; + } + + /* -( Edit View )---------------------------------------------------------- */ diff --git a/webroot/rsrc/js/application/transactions/behavior-transaction-list.js b/webroot/rsrc/js/application/transactions/behavior-transaction-list.js index 6793d6c342..8b93f04f7b 100644 --- a/webroot/rsrc/js/application/transactions/behavior-transaction-list.js +++ b/webroot/rsrc/js/application/transactions/behavior-transaction-list.js @@ -75,19 +75,6 @@ JX.behavior('phabricator-transaction-list', function(config) { e.kill(); }); - JX.DOM.listen(list, 'click', 'transaction-detail', function(e) { - if (!e.isNormalClick()) { - return; - } - - JX.Workflow.newFromLink(e.getTarget()) - .setData({anchor: e.getData('anchor')}) - .setHandler(ontransactions) - .start(); - - e.kill(); - }); - JX.Stratcom.listen( ['submit', 'didSyntheticSubmit'], 'transaction-append',