From a2b2c391a1f151f3720324b8c141b96b7a931c96 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Nov 2019 19:24:48 -0800 Subject: [PATCH] Distinguish between "Assigned" and "Effective" identity PHIDs more clearly and consistently Summary: Ref T13444. You can currently explicitly unassign an identity (useful if the matching algorithm is misfiring). However, this populates the "currentEffectiveUserPHID" with the "unassigned()" token, which mostly makes things more difficult. When an identity is explicitly unassigned, convert that into an explicit `null` in the effective user PHID. Then, realign "assigned" / "effective" language a bit. Previously, `withAssigneePHIDs(...)` actualy queried effective users, which was misleading. Finally, bulk up the list view a little bit to make testing slightly easier. Test Plan: - Unassigned an identity, ran migration, saw `currentEffectiveUserPHID` become `NULL` for the identity. - Unassigned a fresh identity, saw NULL. - Queried for various identities under the modified constraints. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13444 Differential Revision: https://secure.phabricator.com/D20908 --- .../20191113.identity.03.unassigned.sql | 3 + .../DiffusionIdentityViewController.php | 3 + ...iffusionRepositoryIdentitySearchEngine.php | 77 ++++++++++++++++--- .../PhabricatorRepositoryIdentityQuery.php | 23 ++++-- .../storage/PhabricatorRepositoryIdentity.php | 11 ++- 5 files changed, 100 insertions(+), 17 deletions(-) create mode 100644 resources/sql/autopatches/20191113.identity.03.unassigned.sql diff --git a/resources/sql/autopatches/20191113.identity.03.unassigned.sql b/resources/sql/autopatches/20191113.identity.03.unassigned.sql new file mode 100644 index 0000000000..768ca1d909 --- /dev/null +++ b/resources/sql/autopatches/20191113.identity.03.unassigned.sql @@ -0,0 +1,3 @@ +UPDATE {$NAMESPACE}_repository.repository_identity + SET currentEffectiveUserPHID = NULL + WHERE currentEffectiveUserPHID = 'unassigned()'; diff --git a/src/applications/diffusion/controller/DiffusionIdentityViewController.php b/src/applications/diffusion/controller/DiffusionIdentityViewController.php index 0ac131daf1..1c78ec5992 100644 --- a/src/applications/diffusion/controller/DiffusionIdentityViewController.php +++ b/src/applications/diffusion/controller/DiffusionIdentityViewController.php @@ -25,6 +25,9 @@ final class DiffusionIdentityViewController ->setHeaderIcon('fa-globe'); $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb( + pht('Identities'), + $this->getApplicationURI('identity/')); $crumbs->addTextCrumb($identity->getObjectName()); $crumbs->setBorder(true); diff --git a/src/applications/diffusion/query/DiffusionRepositoryIdentitySearchEngine.php b/src/applications/diffusion/query/DiffusionRepositoryIdentitySearchEngine.php index 4d10ee44e0..f41b6b89a4 100644 --- a/src/applications/diffusion/query/DiffusionRepositoryIdentitySearchEngine.php +++ b/src/applications/diffusion/query/DiffusionRepositoryIdentitySearchEngine.php @@ -17,21 +17,35 @@ final class DiffusionRepositoryIdentitySearchEngine protected function buildCustomSearchFields() { return array( + id(new PhabricatorUsersSearchField()) + ->setLabel(pht('Matching Users')) + ->setKey('effectivePHIDs') + ->setAliases( + array( + 'effective', + 'effectivePHID', + )) + ->setDescription(pht('Search for identities by effective user.')), id(new DiffusionIdentityAssigneeSearchField()) ->setLabel(pht('Assigned To')) - ->setKey('assignee') - ->setDescription(pht('Search for identities by assignee.')), + ->setKey('assignedPHIDs') + ->setAliases( + array( + 'assigned', + 'assignedPHID', + )) + ->setDescription(pht('Search for identities by explicit assignee.')), id(new PhabricatorSearchTextField()) ->setLabel(pht('Identity Contains')) ->setKey('match') ->setDescription(pht('Search for identities by substring.')), id(new PhabricatorSearchThreeStateField()) - ->setLabel(pht('Is Assigned')) + ->setLabel(pht('Has Matching User')) ->setKey('hasEffectivePHID') ->setOptions( pht('(Show All)'), - pht('Show Only Assigned Identities'), - pht('Show Only Unassigned Identities')), + pht('Show Identities With Matching Users'), + pht('Show Identities Without Matching Users')), ); } @@ -46,8 +60,12 @@ final class DiffusionRepositoryIdentitySearchEngine $query->withIdentityNameLike($map['match']); } - if ($map['assignee']) { - $query->withAssigneePHIDs($map['assignee']); + if ($map['assignedPHIDs']) { + $query->withAssignedPHIDs($map['assignedPHIDs']); + } + + if ($map['effectivePHIDs']) { + $query->withEffectivePHIDs($map['effectivePHIDs']); } return $query; @@ -86,15 +104,54 @@ final class DiffusionRepositoryIdentitySearchEngine $viewer = $this->requireViewer(); - $list = new PHUIObjectItemListView(); - $list->setUser($viewer); + $list = id(new PHUIObjectItemListView()) + ->setViewer($viewer); + + $phids = array(); + foreach ($identities as $identity) { + $phids[] = $identity->getCurrentEffectiveUserPHID(); + $phids[] = $identity->getManuallySetUserPHID(); + } + + $handles = $viewer->loadHandles($phids); + + $unassigned = DiffusionIdentityUnassignedDatasource::FUNCTION_TOKEN; + foreach ($identities as $identity) { $item = id(new PHUIObjectItemView()) - ->setObjectName(pht('Identity %d', $identity->getID())) + ->setObjectName($identity->getObjectName()) ->setHeader($identity->getIdentityShortName()) ->setHref($identity->getURI()) ->setObject($identity); + $status_icon = 'fa-circle-o grey'; + + $effective_phid = $identity->getCurrentEffectiveUserPHID(); + if ($effective_phid) { + $item->addIcon( + 'fa-id-badge', + pht('Matches User: %s', $handles[$effective_phid]->getName())); + + $status_icon = 'fa-id-badge'; + } + + $assigned_phid = $identity->getManuallySetUserPHID(); + if ($assigned_phid) { + if ($assigned_phid === $unassigned) { + $item->addIcon( + 'fa-ban', + pht('Explicitly Unassigned')); + $status_icon = 'fa-ban'; + } else { + $item->addIcon( + 'fa-user', + pht('Assigned To: %s', $handles[$assigned_phid]->getName())); + $status_icon = 'fa-user'; + } + } + + $item->setStatusIcon($status_icon); + $list->addItem($item); } diff --git a/src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php b/src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php index 6ddf8d57c1..14c2a75f80 100644 --- a/src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php @@ -7,7 +7,8 @@ final class PhabricatorRepositoryIdentityQuery private $phids; private $identityNames; private $emailAddresses; - private $assigneePHIDs; + private $assignedPHIDs; + private $effectivePHIDs; private $identityNameLike; private $hasEffectivePHID; @@ -36,8 +37,13 @@ final class PhabricatorRepositoryIdentityQuery return $this; } - public function withAssigneePHIDs(array $assignees) { - $this->assigneePHIDs = $assignees; + public function withAssignedPHIDs(array $assigned) { + $this->assignedPHIDs = $assigned; + return $this; + } + + public function withEffectivePHIDs(array $effective) { + $this->effectivePHIDs = $effective; return $this; } @@ -75,11 +81,18 @@ final class PhabricatorRepositoryIdentityQuery $this->phids); } - if ($this->assigneePHIDs !== null) { + if ($this->assignedPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'repository_identity.manuallySetUserPHID IN (%Ls)', + $this->assignedPHIDs); + } + + if ($this->effectivePHIDs !== null) { $where[] = qsprintf( $conn, 'repository_identity.currentEffectiveUserPHID IN (%Ls)', - $this->assigneePHIDs); + $this->effectivePHIDs); } if ($this->hasEffectivePHID !== null) { diff --git a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php index c33b296fdc..74fbd06544 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php +++ b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php @@ -96,11 +96,18 @@ final class PhabricatorRepositoryIdentity public function save() { if ($this->manuallySetUserPHID) { - $this->currentEffectiveUserPHID = $this->manuallySetUserPHID; + $unassigned = DiffusionIdentityUnassignedDatasource::FUNCTION_TOKEN; + if ($this->manuallySetUserPHID === $unassigned) { + $effective_phid = null; + } else { + $effective_phid = $this->manuallySetUserPHID; + } } else { - $this->currentEffectiveUserPHID = $this->automaticGuessedUserPHID; + $effective_phid = $this->automaticGuessedUserPHID; } + $this->setCurrentEffectiveUserPHID($effective_phid); + $email_address = $this->getIdentityEmailAddress(); // Raw identities are unrestricted binary data, and may consequently -- 2.51.2