From 4bbe6f307a7a0800cfd31f5014c0292250703210 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 22 Jun 2016 08:20:50 -0700 Subject: [PATCH] Resolve relationship edit conflicts more naturally Summary: Ref T11179. Ref T4768. Currently, on `master`, if two users open "Edit Revisions" at the same time, then add revisions A and B, only the last state wins (just "B"). Instead, apply these as "add A" and "add B" so they merge in a natural way. Test Plan: - Opened edit dialog in two windows. - Added "A" in one, "B" in the other. - Saved both. - Saw "Added A" and "Added B" transactions, instead of "Added A" and "Removed A, added B". Reviewers: chad Reviewed By: chad Maniphest Tasks: T4768, T11179 Differential Revision: https://secure.phabricator.com/D16164 --- resources/celerity/map.php | 18 +++++++++--------- ...PhabricatorSearchRelationshipController.php | 13 ++++++++----- .../PhabricatorObjectSelectorDialog.php | 16 ++++++++++++++++ .../rsrc/js/core/behavior-object-selector.js | 13 ++++++++++++- 4 files changed, 45 insertions(+), 15 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 2944634334..dee9bd423d 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'core.pkg.js' => 'f2139810', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => 'b3eea3f5', - 'differential.pkg.js' => '4b7d8f19', + 'differential.pkg.js' => '01a010d6', 'diffusion.pkg.css' => '91c5d3a6', 'diffusion.pkg.js' => '3a9a8bfa', 'maniphest.pkg.css' => '4845691a', @@ -495,7 +495,7 @@ return array( 'rsrc/js/core/behavior-lightbox-attachments.js' => 'f8ba29d7', 'rsrc/js/core/behavior-line-linker.js' => '1499a8cb', 'rsrc/js/core/behavior-more.js' => 'a80d0378', - 'rsrc/js/core/behavior-object-selector.js' => '49b73b36', + 'rsrc/js/core/behavior-object-selector.js' => '9030ebef', 'rsrc/js/core/behavior-oncopy.js' => '2926fff2', 'rsrc/js/core/behavior-phabricator-nav.js' => '56a1ca03', 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => '116cf19b', @@ -658,7 +658,7 @@ return array( 'javelin-behavior-phabricator-line-linker' => '1499a8cb', 'javelin-behavior-phabricator-nav' => '56a1ca03', 'javelin-behavior-phabricator-notification-example' => '8ce821c5', - 'javelin-behavior-phabricator-object-selector' => '49b73b36', + 'javelin-behavior-phabricator-object-selector' => '9030ebef', 'javelin-behavior-phabricator-oncopy' => '2926fff2', 'javelin-behavior-phabricator-remarkup-assist' => '116cf19b', 'javelin-behavior-phabricator-reveal-content' => '60821bc7', @@ -1214,12 +1214,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - '49b73b36' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-request', - 'javelin-util', - ), '4b700e9e' => array( 'javelin-behavior', 'javelin-dom', @@ -1608,6 +1602,12 @@ return array( 'javelin-dom', 'javelin-request', ), + '9030ebef' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-request', + 'javelin-util', + ), '9196fb06' => array( 'javelin-install', 'javelin-dom', diff --git a/src/applications/search/controller/PhabricatorSearchRelationshipController.php b/src/applications/search/controller/PhabricatorSearchRelationshipController.php index 12e4d16216..c97c020b0a 100644 --- a/src/applications/search/controller/PhabricatorSearchRelationshipController.php +++ b/src/applications/search/controller/PhabricatorSearchRelationshipController.php @@ -44,18 +44,20 @@ final class PhabricatorSearchRelationshipController $src_handle = $handles[$src_phid]; $done_uri = $src_handle->getURI(); + $initial_phids = $dst_phids; if ($request->isFormPost()) { $phids = explode(';', $request->getStr('phids')); $phids = array_filter($phids); $phids = array_values($phids); - // TODO: Embed these in the form instead, to gracefully resolve - // concurrent edits like we do for subscribers and projects. - $old_phids = $dst_phids; + $initial_phids = $request->getStrList('initialPHIDs'); - $add_phids = $phids; - $rem_phids = array_diff($old_phids, $add_phids); + // Apply the changes as adds and removes relative to the original state + // of the object when the dialog was rendered so that two users adding + // relationships at the same time don't race and overwrite one another. + $add_phids = array_diff($phids, $initial_phids); + $rem_phids = array_diff($initial_phids, $phids); if ($add_phids) { $dst_objects = id(new PhabricatorObjectQuery()) @@ -149,6 +151,7 @@ final class PhabricatorSearchRelationshipController return id(new PhabricatorObjectSelectorDialog()) ->setUser($viewer) + ->setInitialPHIDs($initial_phids) ->setHandles($handles) ->setFilters($filters) ->setSelectedFilter('created') diff --git a/src/view/control/PhabricatorObjectSelectorDialog.php b/src/view/control/PhabricatorObjectSelectorDialog.php index eaa51e5111..52729846c8 100644 --- a/src/view/control/PhabricatorObjectSelectorDialog.php +++ b/src/view/control/PhabricatorObjectSelectorDialog.php @@ -10,6 +10,7 @@ final class PhabricatorObjectSelectorDialog extends Phobject { private $searchURI; private $selectedFilter; private $excluded; + private $initialPHIDs; private $title; private $header; @@ -77,6 +78,15 @@ final class PhabricatorObjectSelectorDialog extends Phobject { return $this; } + public function setInitialPHIDs(array $initial_phids) { + $this->initialPHIDs = $initial_phids; + return $this; + } + + public function getInitialPHIDs() { + return $this->initialPHIDs; + } + public function buildDialog() { $user = $this->user; @@ -171,8 +181,14 @@ final class PhabricatorObjectSelectorDialog extends Phobject { $view = new PhabricatorHandleObjectSelectorDataView($handle); $handle_views[$phid] = $view->renderData(); } + $dialog->addHiddenInput('phids', implode(';', array_keys($this->handles))); + $initial_phids = $this->getInitialPHIDs(); + if ($initial_phids) { + $initial_phids = implode(', ', $initial_phids); + $dialog->addHiddenInput('initialPHIDs', $initial_phids); + } Javelin::initBehavior( 'phabricator-object-selector', diff --git a/webroot/rsrc/js/core/behavior-object-selector.js b/webroot/rsrc/js/core/behavior-object-selector.js index d219883ff7..d4a542c840 100644 --- a/webroot/rsrc/js/core/behavior-object-selector.js +++ b/webroot/rsrc/js/core/behavior-object-selector.js @@ -18,10 +18,21 @@ JX.behavior('phabricator-object-selector', function(config) { var query_timer = null; var query_delay = 50; - var phid_input = JX.DOM.find( + // TODO: This is fairly grotesque, but the dialog has two different forms + // inside it and there's no way to sigil the inputs in the "real" form right + // now. Clean this up when the dialog as a whole gets cleaned up. + + var inputs = JX.DOM.scry( JX.$(config.form), 'input', 'aphront-dialog-application-input'); + var phid_input; + for (var ii = 0; ii < inputs.length; ii++) { + if (inputs[ii].name == 'phids') { + phid_input = inputs[ii]; + break; + } + } var last_value = JX.$(config.query).value;