From 7d41535010aa71408cf656e08f9d61fc940f1718 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Jul 2019 10:14:22 -0700 Subject: [PATCH] When a task card is edited, emit update events for old boards and parent boards Summary: Ref T4900. When a card is edited, we currently emit an update notification for all the projects the task is tagged with. This isn't quite the right set: - We want to emit notifications for projects the task //was previously// tagged with, so it can be removed from boards it should no longer be part of. - We want to emit notifications for ancestors of projects the task is or was tagged with, so parent project boards can be updated. - However, we don't need to emit notifications for projects that don't actually have workboards. Adjust the notification set to align better to these rules. Test Plan: - Removal of Parent Project: Edited a task on board "A > B", removing the "B" project tag. Saw board A update in another window. - Normal Update: Edited a task title on board X, saw board X update in another window. - Used `bin/aphlict debug` to inspect the notification set, saw generally sensible-seeming data going over the wire. Reviewers: amckinley Maniphest Tasks: T4900 Differential Revision: https://secure.phabricator.com/D20680 --- resources/celerity/map.php | 28 +++---- .../editor/ManiphestTransactionEditor.php | 76 ++++++++++++++----- .../client/PhabricatorNotificationClient.php | 4 + .../js/application/projects/WorkboardBoard.js | 2 +- 4 files changed, 78 insertions(+), 32 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 90ef62ed1b..ca09a081e5 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -412,7 +412,7 @@ return array( 'rsrc/js/application/phortune/phortune-credit-card-form.js' => 'd12d214f', 'rsrc/js/application/policy/behavior-policy-control.js' => '0eaa33a9', 'rsrc/js/application/policy/behavior-policy-rule-editor.js' => '9347f172', - 'rsrc/js/application/projects/WorkboardBoard.js' => '75727403', + 'rsrc/js/application/projects/WorkboardBoard.js' => 'b46d88c5', 'rsrc/js/application/projects/WorkboardCard.js' => '0392a5d8', 'rsrc/js/application/projects/WorkboardCardTemplate.js' => '84f82dad', 'rsrc/js/application/projects/WorkboardColumn.js' => 'c3d24e63', @@ -743,7 +743,7 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => '75727403', + 'javelin-workboard-board' => 'b46d88c5', 'javelin-workboard-card' => '0392a5d8', 'javelin-workboard-card-template' => '84f82dad', 'javelin-workboard-column' => 'c3d24e63', @@ -1549,18 +1549,6 @@ return array( 'javelin-uri', 'javelin-request', ), - 75727403 => array( - 'javelin-install', - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - 'javelin-workboard-column', - 'javelin-workboard-header-template', - 'javelin-workboard-card-template', - 'javelin-workboard-order-template', - ), '78bc5d94' => array( 'javelin-behavior', 'javelin-uri', @@ -1900,6 +1888,18 @@ return array( 'b347a301' => array( 'javelin-behavior', ), + 'b46d88c5' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-workflow', + 'phabricator-draggable-list', + 'javelin-workboard-column', + 'javelin-workboard-header-template', + 'javelin-workboard-card-template', + 'javelin-workboard-order-template', + ), 'b49fd60c' => array( 'multirow-row-manager', 'trigger-rule', diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 235c73280b..247f5ce1fe 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -3,6 +3,7 @@ final class ManiphestTransactionEditor extends PhabricatorApplicationTransactionEditor { + private $oldProjectPHIDs; private $moreValidationErrors = array(); public function getEditorApplicationClass() { @@ -378,6 +379,11 @@ final class ManiphestTransactionEditor } } + $send_notifications = PhabricatorNotificationClient::isEnabled(); + if ($send_notifications) { + $this->oldProjectPHIDs = $this->loadProjectPHIDs($object); + } + return $results; } @@ -859,14 +865,61 @@ final class ManiphestTransactionEditor return array_values($phid_list); } - protected function didApplyTransactions($object, array $xactions) { - // TODO: This should include projects which the object was previously - // associated with but no longer is (so it can be removed from those - // boards) but currently does not. + $send_notifications = PhabricatorNotificationClient::isEnabled(); + if ($send_notifications) { + $old_phids = $this->oldProjectPHIDs; + $new_phids = $this->loadProjectPHIDs($object); + + // We want to emit update notifications for all old and new tagged + // projects, and all parents of those projects. For example, if an + // edit removes project "A > B" from a task, the "A" workboard should + // receive an update event. + + $project_phids = array_fuse($old_phids) + array_fuse($new_phids); + $project_phids = array_keys($project_phids); + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($project_phids) + ->execute(); + + $notify_projects = array(); + foreach ($projects as $project) { + $notify_projects[$project->getPHID()] = $project; + foreach ($project->getAncestorProjects() as $ancestor) { + $notify_projects[$ancestor->getPHID()] = $ancestor; + } + } + + foreach ($notify_projects as $key => $project) { + if (!$project->getHasWorkboard()) { + unset($notify_projects[$key]); + } + } + + $notify_phids = array_keys($notify_projects); + + if ($notify_phids) { + $data = array( + 'type' => 'workboards', + 'subscribers' => $notify_phids, + ); + + PhabricatorNotificationClient::tryToPostMessage($data); + } + } + + return $xactions; + } + + private function loadProjectPHIDs(ManiphestTask $task) { + if (!$task->getPHID()) { + return array(); + } $edge_query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs(array($object->getPHID())) + ->withSourcePHIDs(array($task->getPHID())) ->withEdgeTypes( array( PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, @@ -874,18 +927,7 @@ final class ManiphestTransactionEditor $edge_query->execute(); - $project_phids = $edge_query->getDestinationPHIDs(); - - if ($project_phids) { - $data = array( - 'type' => 'workboards', - 'subscribers' => $project_phids, - ); - - PhabricatorNotificationClient::tryToPostMessage($data); - } - - return $xactions; + return $edge_query->getDestinationPHIDs(); } } diff --git a/src/applications/notification/client/PhabricatorNotificationClient.php b/src/applications/notification/client/PhabricatorNotificationClient.php index ff5538dbcf..1cede1498d 100644 --- a/src/applications/notification/client/PhabricatorNotificationClient.php +++ b/src/applications/notification/client/PhabricatorNotificationClient.php @@ -37,4 +37,8 @@ final class PhabricatorNotificationClient extends Phobject { } } + public static function isEnabled() { + return (bool)PhabricatorNotificationServerRef::getEnabledAdminServers(); + } + } diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index 2f904b9f68..cce0ed9f69 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -711,7 +711,7 @@ JX.install('WorkboardBoard', { // Compare the server state to the client state, and add or remove // cards on the client as necessary to synchronize them. - if (update_map[card_phid][column_phid]) { + if (update_map[card_phid] && update_map[card_phid][column_phid]) { if (!card) { column.newCard(card_phid); column.markForRedraw();