From df1e9ce646c03a9e23be5b1f254a1b67a0b24c0e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Mar 2018 20:07:32 -0800 Subject: [PATCH] Treat Owners paths like "/src/backend" and "/src/backend/" identically Summary: Depends on D19183. Ref T11015. Currently, adding a trailing slash works great and omitting it mysteriously doesn't work. Store a normalized version with an unconditional trailing slash for the lookup logic to operate on, and a separate display version which tracks what the user actually typed. Test Plan: - Entered "/src/main.c", "/src/main.c/", saw them de-duplicate. - Entered "/src/main.c", saw it stay that way in the UI but appear as "/src/main.c/" internally. - Added a rule for "/src/applications/owners" (no slash), created a revision touching paths in that directory, saw Owners fire for it. - Changed the display value of a path only ("/src/main.c" to "/src/main.c/"), saw the update reflected in the UI without any beahvioral change. Maniphest Tasks: T11015 Differential Revision: https://secure.phabricator.com/D19184 --- resources/celerity/map.php | 20 ++++++------ .../PhabricatorOwnersDetailController.php | 4 +-- ...catorOwnersPathsSearchEngineAttachment.php | 2 +- .../owners/storage/PhabricatorOwnersPath.php | 1 + ...abricatorOwnersPackagePathsTransaction.php | 32 +++++++++++++++++++ .../js/application/owners/OwnersPathEditor.js | 2 +- 6 files changed, 47 insertions(+), 14 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index f90e3bd511..a6435d6afd 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -424,7 +424,7 @@ return array( 'rsrc/js/application/maniphest/behavior-line-chart.js' => 'e4232876', 'rsrc/js/application/maniphest/behavior-list-edit.js' => 'a9f88de2', 'rsrc/js/application/maniphest/behavior-subpriorityeditor.js' => '71237763', - 'rsrc/js/application/owners/OwnersPathEditor.js' => 'aa1733d0', + 'rsrc/js/application/owners/OwnersPathEditor.js' => '996d62b9', 'rsrc/js/application/owners/owners-path-editor.js' => '7a68dda3', 'rsrc/js/application/passphrase/passphrase-credential-control.js' => '3cb0b2fc', 'rsrc/js/application/pholio/behavior-pholio-mock-edit.js' => 'bee502c8', @@ -764,7 +764,7 @@ return array( 'maniphest-task-edit-css' => 'fda62a9b', 'maniphest-task-summary-css' => '11cc5344', 'multirow-row-manager' => 'b5d57730', - 'owners-path-editor' => 'aa1733d0', + 'owners-path-editor' => '996d62b9', 'owners-path-editor-css' => '2f00933b', 'paste-css' => '9fcc9773', 'path-typeahead' => 'f7fc67ec', @@ -1676,6 +1676,14 @@ return array( 'javelin-mask', 'phabricator-drag-and-drop-file-upload', ), + '996d62b9' => array( + 'multirow-row-manager', + 'javelin-install', + 'path-typeahead', + 'javelin-dom', + 'javelin-util', + 'phabricator-prefab', + ), '9a6dd75c' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1766,14 +1774,6 @@ return array( 'javelin-fx', 'javelin-util', ), - 'aa1733d0' => array( - 'multirow-row-manager', - 'javelin-install', - 'path-typeahead', - 'javelin-dom', - 'javelin-util', - 'phabricator-prefab', - ), 'ab2f381b' => array( 'javelin-request', 'javelin-behavior', diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php index 3c6ae3836c..c7e96594f3 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -279,7 +279,7 @@ final class PhabricatorOwnersDetailController $href = $repo->generateURI( array( 'branch' => $repo->getDefaultBranch(), - 'path' => $path->getPath(), + 'path' => $path->getPathDisplay(), 'action' => 'browse', )); @@ -288,7 +288,7 @@ final class PhabricatorOwnersDetailController array( 'href' => (string)$href, ), - $path->getPath()); + $path->getPathDisplay()); $rows[] = array( ($path->getExcluded() ? '-' : '+'), diff --git a/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php b/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php index ad5415ef69..19e699e8ff 100644 --- a/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php +++ b/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php @@ -22,7 +22,7 @@ final class PhabricatorOwnersPathsSearchEngineAttachment foreach ($paths as $path) { $list[] = array( 'repositoryPHID' => $path->getRepositoryPHID(), - 'path' => $path->getPath(), + 'path' => $path->getPathDisplay(), 'excluded' => (bool)$path->getExcluded(), ); } diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php index 6c49023e69..0ef7fce59b 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPath.php +++ b/src/applications/owners/storage/PhabricatorOwnersPath.php @@ -49,6 +49,7 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO { return array( 'repositoryPHID' => $this->getRepositoryPHID(), 'path' => $this->getPath(), + 'display' => $this->getPathDisplay(), 'excluded' => (int)$this->getExcluded(), ); } diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php index 5952b78aed..984b26b8ac 100644 --- a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php +++ b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php @@ -103,6 +103,26 @@ final class PhabricatorOwnersPackagePathsTransaction $paths = $object->getPaths(); + // We store paths in a normalized format with a trailing slash, regardless + // of whether the user enters "path/to/file.c" or "src/backend/". Normalize + // paths now. + + $display_map = array(); + foreach ($new as $key => $spec) { + $display_path = $spec['path']; + $raw_path = rtrim($display_path, '/').'/'; + + // If the user entered two paths which normalize to the same value + // (like "src/main.c" and "src/main.c/"), discard the duplicates. + if (isset($display_map[$raw_path])) { + unset($new[$key]); + continue; + } + + $new[$key]['path'] = $raw_path; + $display_map[$raw_path] = $display_path; + } + $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new); list($rem, $add) = $diffs; @@ -111,12 +131,24 @@ final class PhabricatorOwnersPackagePathsTransaction $ref = $path->getRef(); if (PhabricatorOwnersPath::isRefInSet($ref, $set)) { $path->delete(); + continue; + } + + // If the user has changed the display value for a path but the raw + // storage value hasn't changed, update the display value. + + if (isset($display_map[$path->getPath()])) { + $path + ->setPathDisplay($display_map[$path->getPath()]) + ->save(); + continue; } } foreach ($add as $ref) { $path = PhabricatorOwnersPath::newFromRef($ref) ->setPackageID($object->getID()) + ->setPathDisplay($display_map[$ref['path']]) ->save(); } } diff --git a/webroot/rsrc/js/application/owners/OwnersPathEditor.js b/webroot/rsrc/js/application/owners/OwnersPathEditor.js index 3199ac4e2a..f1d47a7908 100644 --- a/webroot/rsrc/js/application/owners/OwnersPathEditor.js +++ b/webroot/rsrc/js/application/owners/OwnersPathEditor.js @@ -115,7 +115,7 @@ JX.install('OwnersPathEditor', { JX.copy( path_input, { - value : path_ref.path || '', + value : path_ref.display || '', name : 'path[' + this._count + ']' });