From 4452239d6190921f055e6131bc2e357bb6cc6b4b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 9 Jul 2011 16:17:36 -0700 Subject: [PATCH] Make maniphest add CCs when users are @mentioned Summary: We don't currently add CCs, but should (similar to how Differential works). This also fixes some problems where you can get no-op CC transactions, and makes mentions a little less aggressively colored. Test Plan: Applied a bunch of CC/mention transactions to tasks and observed the behavior. Reviewed By: jungejason Reviewers: jungejason, tuomaspelkonen, aran, codeblock CC: aran, jungejason Differential Revision: 634 --- src/__celerity_resource_map__.php | 72 +++++++++---------- .../ManiphestTransactionSaveController.php | 34 ++++++++- .../controller/transactionsave/__init__.php | 1 + .../ManiphestTransactionEditor.php | 33 ++++++++- webroot/rsrc/css/core/remarkup.css | 4 +- 5 files changed, 101 insertions(+), 43 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 833f7e2b76..ceaed7a676 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -1026,7 +1026,7 @@ celerity_register_resource_map(array( ), 'phabricator-feed-css' => array( - 'uri' => '/res/80952af2/rsrc/css/application/feed/feed.css', + 'uri' => '/res/617811ab/rsrc/css/application/feed/feed.css', 'type' => 'css', 'requires' => array( @@ -1080,7 +1080,7 @@ celerity_register_resource_map(array( ), 'phabricator-remarkup-css' => array( - 'uri' => '/res/e0c44a00/rsrc/css/core/remarkup.css', + 'uri' => '/res/7ca89111/rsrc/css/core/remarkup.css', 'type' => 'css', 'requires' => array( @@ -1192,7 +1192,24 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/307df223/javelin.pkg.js', 'type' => 'js', ), - '8e9024dc' => + '95b66c1a' => + array ( + 'name' => 'differential.pkg.css', + 'symbols' => + array ( + 0 => 'differential-core-view-css', + 1 => 'differential-changeset-view-css', + 2 => 'differential-revision-detail-css', + 3 => 'differential-revision-history-css', + 4 => 'differential-table-of-contents-css', + 5 => 'differential-revision-comment-css', + 6 => 'differential-revision-add-comment-css', + 7 => 'differential-revision-comment-list-css', + ), + 'uri' => '/res/pkg/95b66c1a/differential.pkg.css', + 'type' => 'css', + ), + 'a06761eb' => array ( 'name' => 'core.pkg.css', 'symbols' => @@ -1213,24 +1230,7 @@ celerity_register_resource_map(array( 13 => 'phabricator-remarkup-css', 14 => 'syntax-highlighting-css', ), - 'uri' => '/res/pkg/8e9024dc/core.pkg.css', - 'type' => 'css', - ), - '95b66c1a' => - array ( - 'name' => 'differential.pkg.css', - 'symbols' => - array ( - 0 => 'differential-core-view-css', - 1 => 'differential-changeset-view-css', - 2 => 'differential-revision-detail-css', - 3 => 'differential-revision-history-css', - 4 => 'differential-table-of-contents-css', - 5 => 'differential-revision-comment-css', - 6 => 'differential-revision-add-comment-css', - 7 => 'differential-revision-comment-list-css', - ), - 'uri' => '/res/pkg/95b66c1a/differential.pkg.css', + 'uri' => '/res/pkg/a06761eb/core.pkg.css', 'type' => 'css', ), 'd0713563' => @@ -1266,15 +1266,15 @@ celerity_register_resource_map(array( ), 'reverse' => array ( - 'aphront-crumbs-view-css' => '8e9024dc', - 'aphront-dialog-view-css' => '8e9024dc', - 'aphront-form-view-css' => '8e9024dc', - 'aphront-list-filter-view-css' => '8e9024dc', - 'aphront-panel-view-css' => '8e9024dc', - 'aphront-side-nav-view-css' => '8e9024dc', - 'aphront-table-view-css' => '8e9024dc', - 'aphront-tokenizer-control-css' => '8e9024dc', - 'aphront-typeahead-control-css' => '8e9024dc', + 'aphront-crumbs-view-css' => 'a06761eb', + 'aphront-dialog-view-css' => 'a06761eb', + 'aphront-form-view-css' => 'a06761eb', + 'aphront-list-filter-view-css' => 'a06761eb', + 'aphront-panel-view-css' => 'a06761eb', + 'aphront-side-nav-view-css' => 'a06761eb', + 'aphront-table-view-css' => 'a06761eb', + 'aphront-tokenizer-control-css' => 'a06761eb', + 'aphront-typeahead-control-css' => 'a06761eb', 'differential-changeset-view-css' => '95b66c1a', 'differential-core-view-css' => '95b66c1a', 'differential-revision-add-comment-css' => '95b66c1a', @@ -1311,13 +1311,13 @@ celerity_register_resource_map(array( 'javelin-util' => '307df223', 'javelin-vector' => '307df223', 'javelin-workflow' => 'd0713563', - 'phabricator-core-buttons-css' => '8e9024dc', - 'phabricator-core-css' => '8e9024dc', - 'phabricator-directory-css' => '8e9024dc', + 'phabricator-core-buttons-css' => 'a06761eb', + 'phabricator-core-css' => 'a06761eb', + 'phabricator-directory-css' => 'a06761eb', 'phabricator-keyboard-shortcut' => 'd0713563', 'phabricator-keyboard-shortcut-manager' => 'd0713563', - 'phabricator-remarkup-css' => '8e9024dc', - 'phabricator-standard-page-view' => '8e9024dc', - 'syntax-highlighting-css' => '8e9024dc', + 'phabricator-remarkup-css' => 'a06761eb', + 'phabricator-standard-page-view' => 'a06761eb', + 'syntax-highlighting-css' => 'a06761eb', ), )); diff --git a/src/applications/maniphest/controller/transactionsave/ManiphestTransactionSaveController.php b/src/applications/maniphest/controller/transactionsave/ManiphestTransactionSaveController.php index 49bbc6a065..1b3706cc02 100644 --- a/src/applications/maniphest/controller/transactionsave/ManiphestTransactionSaveController.php +++ b/src/applications/maniphest/controller/transactionsave/ManiphestTransactionSaveController.php @@ -84,6 +84,14 @@ class ManiphestTransactionSaveController extends ManiphestController { $file_transaction = $transaction; } + // Compute new CCs added by @mentions. We're going to try to add them to + // another CC transaction if we can. If there aren't any CC transactions, + // we'll create a new CC transaction after we handle everything else. + $mention_ccs = DifferentialMarkupEngineFactory::extractPHIDsFromMentions( + array( + $request->getStr('comments'), + )); + $transaction = new ManiphestTransaction(); $transaction ->setAuthorPHID($user->getPHID()) @@ -109,6 +117,10 @@ class ManiphestTransactionSaveController extends ManiphestController { case ManiphestTransactionType::TYPE_CCS: $ccs = $request->getArr('ccs'); $ccs = array_merge($ccs, $task->getCCPHIDs()); + if ($mention_ccs) { + $ccs = array_merge($ccs, $mention_ccs); + $mention_ccs = array(); + } $ccs = array_filter($ccs); $ccs = array_unique($ccs); $transaction->setNewValue($ccs); @@ -143,9 +155,11 @@ class ManiphestTransactionSaveController extends ManiphestController { $old = $task->getCCPHIDs(); $new = array_merge( $old, - array($task->getOwnerPHID())); + array($task->getOwnerPHID()), + $mention_ccs); $new = array_unique(array_filter($new)); if ($old != $new) { + $mention_ccs = null; $cc = new ManiphestTransaction(); $cc->setAuthorPHID($user->getPHID()); $cc->setTransactionType(ManiphestTransactionType::TYPE_CCS); @@ -165,7 +179,7 @@ class ManiphestTransactionSaveController extends ManiphestController { $transactions[] = $assign; } break; - case ManiphestTransactionType::TYPE_NONE: + default: $ccs = $task->getCCPHIDs(); $owner = $task->getOwnerPHID(); @@ -173,17 +187,31 @@ class ManiphestTransactionSaveController extends ManiphestController { // Current user, who is commenting, is not the owner or in ccs. // Add him to ccs $ccs[] = $user->getPHID(); + if ($mention_ccs) { + $ccs = array_merge($ccs, $mention_ccs); + $mention_ccs = null; + } $cc = new ManiphestTransaction(); $cc->setAuthorPHID($user->getPHID()); $cc->setTransactionType(ManiphestTransactionType::TYPE_CCS); $cc->setNewValue($ccs); $transactions[] = $cc; } - default: break; } + if ($mention_ccs) { + // We have mention CCs and didn't pick up a CC transaction anywhere that + // we could shove them into, create a new CC transaction. + $cc = new ManiphestTransaction(); + $cc->setAuthorPHID($user->getPHID()); + $cc->setTransactionType(ManiphestTransactionType::TYPE_CCS); + $ccs = array_merge($task->getCCPHIDs(), $mention_ccs); + $cc->setNewValue($ccs); + + $transactions[] = $cc; + } $editor = new ManiphestTransactionEditor(); $editor->applyTransactions($task, $transactions); diff --git a/src/applications/maniphest/controller/transactionsave/__init__.php b/src/applications/maniphest/controller/transactionsave/__init__.php index b06f03b2db..fc69d9bba9 100644 --- a/src/applications/maniphest/controller/transactionsave/__init__.php +++ b/src/applications/maniphest/controller/transactionsave/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'aphront/response/404'); phutil_require_module('phabricator', 'aphront/response/redirect'); +phutil_require_module('phabricator', 'applications/differential/parser/markup'); phutil_require_module('phabricator', 'applications/draft/storage/draft'); phutil_require_module('phabricator', 'applications/files/storage/file'); phutil_require_module('phabricator', 'applications/maniphest/constants/status'); diff --git a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php index 825d4936d5..c43f85ed39 100644 --- a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php @@ -40,6 +40,8 @@ class ManiphestTransactionEditor { $new = $transaction->getNewValue(); $email_to[] = $transaction->getAuthorPHID(); + $value_is_phid_set = false; + switch ($type) { case ManiphestTransactionType::TYPE_NONE: $old = null; @@ -52,6 +54,7 @@ class ManiphestTransactionEditor { break; case ManiphestTransactionType::TYPE_CCS: $old = $task->getCCPHIDs(); + $value_is_phid_set = true; break; case ManiphestTransactionType::TYPE_PRIORITY: $old = $task->getPriority(); @@ -67,12 +70,40 @@ class ManiphestTransactionEditor { break; case ManiphestTransactionType::TYPE_PROJECTS: $old = $task->getProjectPHIDs(); + $value_is_phid_set = true; break; default: throw new Exception('Unknown action type.'); } - if (($old !== null) && ($old == $new)) { + $old_cmp = $old; + $new_cmp = $new; + if ($value_is_phid_set) { + + // Normalize the old and new values if they are PHID sets so we don't + // get any no-op transactions where the values differ only by keys, + // order, duplicates, etc. + + if (is_array($old)) { + $old = array_filter($old); + $old = array_unique($old); + sort($old); + $old = array_values($old); + $old_cmp = $old; + } + + if (is_array($new)) { + $new = array_filter($new); + $new = array_unique($new); + $transaction->setNewValue($new); + + $new_cmp = $new; + sort($new_cmp); + $new_cmp = array_values($new_cmp); + } + } + + if (($old !== null) && ($old_cmp == $new_cmp)) { if (count($transactions) > 1 && !$transaction->hasComments()) { // If we have at least one other transaction and this one isn't // doing anything and doesn't have any comments, just throw it diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css index 343a44e07c..da16cab510 100644 --- a/webroot/rsrc/css/core/remarkup.css +++ b/webroot/rsrc/css/core/remarkup.css @@ -62,12 +62,10 @@ .phabricator-remarkup-mention-exists { font-weight: bold; - padding: 0 .25em; - background: #aaffaa; + background: #e6f3ff; } .aphront-panel-preview .phabricator-remarkup-mention-unknown { font-weight: bold; - padding: 0 .25em; background: #ffaaaa; }