Move "BoardResponseEngine" toward a more comprehensive update model

Summary:
Depends on D20639. Ref T4900. Currently, "BoardResponseEngine" has a `setObjectPHID()` method. This is called after edit operations to mean "we just edited object X, so we know it needs to be updated".

Move toward `setUpdatePHIDs(...)` in all cases, with `setUpdatePHIDs(array(the-object-we-just-edited))` as a special case of that. After this change, callers pass:

- An optional list of PHIDs they know need to be updated on the client. Today, this is always be a card we just edited (on edit/move flows), or a sort of made-up list of PHIDs for the moment (when you press "R"). In the future, the "R" endpoint will do a better job of figuring out a more realistic update set.
- An optional list of PHIDs currently visible on the client. This is used to update ordering details and mark cards for removal. This is currently passed by edit/move, but not by pressing "R" (it will be in the future).
- An optional list of objects. The "R" workflow has to load these anyway, so we can save a couple queries by letting callers pass them. For now, the edit/move flows still rely on the engine to figure out what it needs to load.

This does very little to actually change client behavior, it mostly just paves the way for the next update to the "R" workflow to make it handle add/remove cases properly.

Test Plan:
- Edited and moved cards on a workboard.
- Pressed "R" to reload a workboard.

Neither of these operations seem any worse off than they were before. They still don't fully work:

- When you edit a card and delete the current workboard project from it, it remains visible. This is also the behavior on `master`. This is sort of intentional since we don't necessarily want to make these cards suddenly disappear? Ideally, we would probably have some kind of "tombstone" state where the card can still be edited but can't be dragged, and the next explicit user interaction would clean up old tombstones. This interaction is very rare and I don't think it's particularly important to specialize.
- When a card is removed from the board, "R" can't currently figure out that it should be removed from the client. This is because the client does not yet pass a "visiblePHIDs" state. It will in an upcoming change.
- The "R" flow always sends a full set of card updates, and can not yet detect that some cards have not changed.
- There's a TODO, but some ordering stuff isn't handled yet.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20652

epriestley 6 years ago 1ee6ecf3 db696869

+91 -63

6 changed files

expand all

resources

celerity

map.php

src

applications

maniphest

editor

ManiphestEditEngine.php

project

controller

PhabricatorProjectBoardReloadController.php

PhabricatorProjectController.php

engine

PhabricatorBoardResponseEngine.php

webroot

rsrc

application

projects

WorkboardBoard.js

+14 -14

resources/celerity/map.php

··· 412 412 'rsrc/js/application/phortune/phortune-credit-card-form.js' => 'd12d214f', 413 413 'rsrc/js/application/policy/behavior-policy-control.js' => '0eaa33a9', 414 414 'rsrc/js/application/policy/behavior-policy-rule-editor.js' => '9347f172', 415 - 'rsrc/js/application/projects/WorkboardBoard.js' => '34c2f539', 415 + 'rsrc/js/application/projects/WorkboardBoard.js' => '46573d65', 416 416 'rsrc/js/application/projects/WorkboardCard.js' => '0392a5d8', 417 417 'rsrc/js/application/projects/WorkboardCardTemplate.js' => '2a61f8d4', 418 418 'rsrc/js/application/projects/WorkboardColumn.js' => 'c3d24e63', ··· 743 743 'javelin-view-renderer' => '9aae2b66', 744 744 'javelin-view-visitor' => '308f9fe4', 745 745 'javelin-websocket' => 'fdc13e4e', 746 - 'javelin-workboard-board' => '34c2f539', 746 + 'javelin-workboard-board' => '46573d65', 747 747 'javelin-workboard-card' => '0392a5d8', 748 748 'javelin-workboard-card-template' => '2a61f8d4', 749 749 'javelin-workboard-column' => 'c3d24e63', ··· 1202 1202 'javelin-install', 1203 1203 'javelin-util', 1204 1204 ), 1205 - '34c2f539' => array( 1206 - 'javelin-install', 1207 - 'javelin-dom', 1208 - 'javelin-util', 1209 - 'javelin-stratcom', 1210 - 'javelin-workflow', 1211 - 'phabricator-draggable-list', 1212 - 'javelin-workboard-column', 1213 - 'javelin-workboard-header-template', 1214 - 'javelin-workboard-card-template', 1215 - 'javelin-workboard-order-template', 1216 - ), 1217 1205 '34c53422' => array( 1218 1206 'javelin-behavior', 1219 1207 'javelin-dom', ··· 1303 1291 'javelin-router', 1304 1292 'javelin-util', 1305 1293 'phabricator-busy', 1294 + ), 1295 + '46573d65' => array( 1296 + 'javelin-install', 1297 + 'javelin-dom', 1298 + 'javelin-util', 1299 + 'javelin-stratcom', 1300 + 'javelin-workflow', 1301 + 'phabricator-draggable-list', 1302 + 'javelin-workboard-column', 1303 + 'javelin-workboard-header-template', 1304 + 'javelin-workboard-card-template', 1305 + 'javelin-workboard-order-template', 1306 1306 ), 1307 1307 '47a0728b' => array( 1308 1308 'javelin-behavior',

+1 -1

src/applications/maniphest/editor/ManiphestEditEngine.php

··· 434 434 $engine = id(new PhabricatorBoardResponseEngine()) 435 435 ->setViewer($viewer) 436 436 ->setBoardPHID($board_phid) 437 - ->setObjectPHID($object_phid) 437 + ->setUpdatePHIDs(array($object_phid)) 438 438 ->setVisiblePHIDs($visible_phids); 439 439 440 440 if ($ordering) {

src/applications/project/controller/PhabricatorProjectBoardReloadController.php

··· 25 25 $engine = id(new PhabricatorBoardResponseEngine()) 26 26 ->setViewer($viewer) 27 27 ->setBoardPHID($board_phid) 28 + ->setObjects($objects) 28 29 ->setUpdatePHIDs($object_phids); 29 30 30 31 // TODO: We don't currently process "order" properly. If a user is viewing

+1 -1

src/applications/project/controller/PhabricatorProjectController.php

··· 184 184 $engine = id(new PhabricatorBoardResponseEngine()) 185 185 ->setViewer($viewer) 186 186 ->setBoardPHID($board_phid) 187 - ->setObjectPHID($object_phid) 187 + ->setUpdatePHIDs(array($object_phid)) 188 188 ->setVisiblePHIDs($visible_phids) 189 189 ->setSounds($sounds); 190 190

+49 -46

src/applications/project/engine/PhabricatorBoardResponseEngine.php

··· 3 3 final class PhabricatorBoardResponseEngine extends Phobject { 4 4 5 5 private $viewer; 6 + private $objects; 6 7 private $boardPHID; 7 - private $objectPHID; 8 - private $visiblePHIDs; 8 + private $visiblePHIDs = array(); 9 9 private $updatePHIDs = array(); 10 10 private $ordering; 11 11 private $sounds; ··· 28 28 return $this->boardPHID; 29 29 } 30 30 31 - public function setObjectPHID($object_phid) { 32 - $this->objectPHID = $object_phid; 31 + public function setObjects(array $objects) { 32 + $this->objects = $objects; 33 33 return $this; 34 34 } 35 35 36 - public function getObjectPHID() { 37 - return $this->objectPHID; 36 + public function getObjects() { 37 + return $this->objects; 38 38 } 39 39 40 40 public function setVisiblePHIDs(array $visible_phids) { ··· 75 75 76 76 public function buildResponse() { 77 77 $viewer = $this->getViewer(); 78 - $object_phid = $this->getObjectPHID(); 79 78 $board_phid = $this->getBoardPHID(); 80 79 $ordering = $this->getOrdering(); 81 80 81 + $update_phids = $this->getUpdatePHIDs(); 82 + $update_phids = array_fuse($update_phids); 83 + 84 + $visible_phids = $this->getVisiblePHIDs(); 85 + $visible_phids = array_fuse($visible_phids); 86 + 87 + $all_phids = $update_phids + $visible_phids; 88 + 82 89 // Load all the other tasks that are visible in the affected columns and 83 90 // perform layout for them. 84 - $all_phids = $this->getAllVisiblePHIDs(); 91 + 92 + if ($this->objects !== null) { 93 + $all_objects = $this->getObjects(); 94 + $all_objects = mpull($all_objects, null, 'getPHID'); 95 + } else { 96 + $all_objects = id(new ManiphestTaskQuery()) 97 + ->setViewer($viewer) 98 + ->withPHIDs($all_phids) 99 + ->execute(); 100 + $all_objects = mpull($all_objects, null, 'getPHID'); 101 + } 85 102 86 103 $layout_engine = id(new PhabricatorBoardLayoutEngine()) 87 104 ->setViewer($viewer) ··· 91 108 92 109 $natural = array(); 93 110 94 - $update_phids = $this->getAllUpdatePHIDs(); 95 111 $update_columns = array(); 96 112 foreach ($update_phids as $update_phid) { 97 113 $update_columns += $layout_engine->getObjectColumns( ··· 105 121 $column_phid); 106 122 $natural[$column_phid] = array_values($column_object_phids); 107 123 } 108 - 109 - $all_objects = id(new ManiphestTaskQuery()) 110 - ->setViewer($viewer) 111 - ->withPHIDs($all_phids) 112 - ->execute(); 113 - $all_objects = mpull($all_objects, null, 'getPHID'); 114 124 115 125 if ($ordering) { 116 126 $vectors = $ordering->getSortVectorsForObjects($all_objects); ··· 164 174 $cards[$card_phid] = $card; 165 175 } 166 176 177 + // Mark cards which are currently visible on the client but not visible 178 + // on the board on the server for removal from the client view of the 179 + // board state. 180 + foreach ($visible_phids as $card_phid) { 181 + if (!isset($cards[$card_phid])) { 182 + $cards[$card_phid] = array( 183 + 'remove' => true, 184 + ); 185 + } 186 + } 187 + 167 188 $payload = array( 168 189 'columnMaps' => $natural, 169 190 'cards' => $cards, ··· 202 223 return array_fuse($exclude_phids); 203 224 } 204 225 205 - private function getAllVisiblePHIDs() { 206 - $phids = $this->getAllUpdatePHIDs(); 207 - 208 - foreach ($this->getVisiblePHIDs() as $phid) { 209 - $phids[] = $phid; 210 - } 211 - 212 - $phids = array_fuse($phids); 213 - 214 - return $phids; 215 - } 216 - 217 - private function getAllUpdatePHIDs() { 218 - $phids = $this->getUpdatePHIDs(); 219 - 220 - $object_phid = $this->getObjectPHID(); 221 - if ($object_phid) { 222 - $phids[] = $object_phid; 223 - } 224 - 225 - $phids = array_fuse($phids); 226 - 227 - return $phids; 228 - } 229 - 230 226 private function newCardTemplates() { 231 227 $viewer = $this->getViewer(); 232 228 233 - $update_phids = $this->getAllUpdatePHIDs(); 229 + $update_phids = $this->getUpdatePHIDs(); 234 230 if (!$update_phids) { 235 231 return array(); 236 232 } 233 + $update_phids = array_fuse($update_phids); 237 234 238 - $objects = id(new ManiphestTaskQuery()) 239 - ->setViewer($viewer) 240 - ->withPHIDs($update_phids) 241 - ->needProjectPHIDs(true) 242 - ->execute(); 235 + if ($this->objects === null) { 236 + $objects = id(new ManiphestTaskQuery()) 237 + ->setViewer($viewer) 238 + ->withPHIDs($update_phids) 239 + ->needProjectPHIDs(true) 240 + ->execute(); 241 + } else { 242 + $objects = $this->getObjects(); 243 + $objects = mpull($objects, null, 'getPHID'); 244 + $objects = array_select_keys($objects, $update_phids); 245 + } 243 246 244 247 if (!$objects) { 245 248 return array();

+25 -1

webroot/rsrc/js/application/projects/WorkboardBoard.js

··· 611 611 } 612 612 } 613 613 614 + // Process card removals. These are cases where the client still sees 615 + // a particular card on a board but it has been removed on the server. 616 + for (card_phid in response.cards) { 617 + card_data = response.cards[card_phid]; 618 + 619 + if (!card_data.remove) { 620 + continue; 621 + } 622 + 623 + for (column_phid in columns) { 624 + var column = columns[column_phid]; 625 + 626 + var card = column.getCard(card_phid); 627 + if (card) { 628 + column.removeCard(card_phid); 629 + column.markForRedraw(); 630 + } 631 + } 632 + } 633 + 614 634 // Process partial updates for cards. This is supplemental data which 615 635 // we can just merge in without any special handling. 616 636 for (card_phid in response.cards) { 617 637 card_data = response.cards[card_phid]; 638 + 639 + if (card_data.remove) { 640 + continue; 641 + } 642 + 618 643 var card_template = this.getCardTemplate(card_phid); 619 644 620 645 if (card_data.nodeHTMLTemplate) { ··· 634 659 card_template.setObjectProperty(key, card_data.properties[key]); 635 660 } 636 661 } 637 - 638 662 639 663 // Process full updates for cards which we have a full update for. This 640 664 // may involve moving them between columns.

Configure Feed

Configure Feed