From 63e6b2553e0ecdb0d10cde7c1845529a6d53e836 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Oct 2017 11:20:02 -0700 Subject: [PATCH 01/30] Simply how Differential drafts ignore Harbormaster autobuilds Summary: Ref T2543. When a revision is created, we check if any builds are waiting/failed, and submit it for review immediately if we aren't waiting for anything. In doing this, we ignore builds with only autotargets, since these are client-side and failures from local `arc lint` / `arc unit` should not count (the user has already chosen to ignore/skip them). The way we do this has some issues: - Herald may have started builds, but they may still be PENDING and not have any targets yet. In this case, we'll see "no non-autotargets" and ignore the build, which is wrong. - We have to load targets but don't really care about them, which is more work than we really need to do. - And it's kind of complex, too. Instead, just let `BuildQuery` filter out "autobuilds" (builds generated from autoplans) with a JOIN. Test Plan: Ran `arc diff` with builds configured, got a clean "Draft" state instead of an incorrect promotion directly to "Needs Review". Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18721 --- .../storage/DifferentialRevision.php | 25 +-------- .../query/HarbormasterBuildQuery.php | 55 +++++++++++++++++-- 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 76c7c4da95..da7e18ce05 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -721,9 +721,10 @@ final class DifferentialRevision extends DifferentialDAO return array(); } - $builds = id(new HarbormasterBuildQuery()) + return id(new HarbormasterBuildQuery()) ->setViewer($viewer) ->withBuildablePHIDs(mpull($buildables, 'getPHID')) + ->withAutobuilds(false) ->withBuildStatuses( array( HarbormasterBuildStatus::STATUS_INACTIVE, @@ -735,29 +736,7 @@ final class DifferentialRevision extends DifferentialDAO HarbormasterBuildStatus::STATUS_PAUSED, HarbormasterBuildStatus::STATUS_DEADLOCKED, )) - ->needBuildTargets(true) ->execute(); - if (!$builds) { - return array(); - } - - $active = array(); - foreach ($builds as $key => $build) { - foreach ($build->getBuildTargets() as $target) { - if ($target->isAutotarget()) { - // Ignore autotargets when looking for active of failed builds. If - // local tests fail and you continue anyway, you don't need to - // double-confirm them. - continue; - } - - // This build has at least one real target that's doing something. - $active[$key] = $build; - break; - } - } - - return $active; } diff --git a/src/applications/harbormaster/query/HarbormasterBuildQuery.php b/src/applications/harbormaster/query/HarbormasterBuildQuery.php index cf6db6115b..770ca4413b 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildQuery.php @@ -10,6 +10,7 @@ final class HarbormasterBuildQuery private $buildPlanPHIDs; private $initiatorPHIDs; private $needBuildTargets; + private $autobuilds; public function withIDs(array $ids) { $this->ids = $ids; @@ -41,6 +42,11 @@ final class HarbormasterBuildQuery return $this; } + public function withAutobuilds($with_autobuilds) { + $this->autobuilds = $with_autobuilds; + return $this; + } + public function needBuildTargets($need_targets) { $this->needBuildTargets = $need_targets; return $this; @@ -141,50 +147,87 @@ final class HarbormasterBuildQuery if ($this->ids !== null) { $where[] = qsprintf( $conn, - 'id IN (%Ld)', + 'b.id IN (%Ld)', $this->ids); } if ($this->phids !== null) { $where[] = qsprintf( $conn, - 'phid in (%Ls)', + 'b.phid in (%Ls)', $this->phids); } if ($this->buildStatuses !== null) { $where[] = qsprintf( $conn, - 'buildStatus in (%Ls)', + 'b.buildStatus in (%Ls)', $this->buildStatuses); } if ($this->buildablePHIDs !== null) { $where[] = qsprintf( $conn, - 'buildablePHID IN (%Ls)', + 'b.buildablePHID IN (%Ls)', $this->buildablePHIDs); } if ($this->buildPlanPHIDs !== null) { $where[] = qsprintf( $conn, - 'buildPlanPHID IN (%Ls)', + 'b.buildPlanPHID IN (%Ls)', $this->buildPlanPHIDs); } if ($this->initiatorPHIDs !== null) { $where[] = qsprintf( $conn, - 'initiatorPHID IN (%Ls)', + 'b.initiatorPHID IN (%Ls)', $this->initiatorPHIDs); } + if ($this->autobuilds !== null) { + if ($this->autobuilds) { + $where[] = qsprintf( + $conn, + 'p.planAutoKey IS NOT NULL'); + } else { + $where[] = qsprintf( + $conn, + 'p.planAutoKey IS NULL'); + } + } + return $where; } + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { + $joins = parent::buildJoinClauseParts($conn); + + if ($this->shouldJoinPlanTable()) { + $joins[] = qsprintf( + $conn, + 'JOIN %T p ON b.buildPlanPHID = p.phid', + id(new HarbormasterBuildPlan())->getTableName()); + } + + return $joins; + } + + private function shouldJoinPlanTable() { + if ($this->autobuilds !== null) { + return true; + } + + return false; + } + public function getQueryApplicationClass() { return 'PhabricatorHarbormasterApplication'; } + protected function getPrimaryTableAlias() { + return 'b'; + } + } From 157f47cd149c1a811c12850bbf76cc05dd805ad9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Oct 2017 15:37:19 -0700 Subject: [PATCH 02/30] Rewrite CommitQuery to use UNION for performance Summary: Ref T12680. See PHI167. See that task for discussion. Rewrite `DiffusionCommitQuery` to work more like `DifferentialRevisionQuery`, and use a UNION to find "all revisions you need to audit OR respond to". I tried to get this working a little more cleanly than RevisionQuery does, and can probably simplify that now. Test Plan: Poked at the UI locally without hitting any apparent issues, but my local data is pretty garbage at this point. I'll take a look at how the query plans work on `secure`. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12680 Differential Revision: https://secure.phabricator.com/D18722 --- .../diffusion/query/DiffusionCommitQuery.php | 83 +++++++++++++------ ...PhabricatorCursorPagedPolicyAwareQuery.php | 35 ++++++-- 2 files changed, 85 insertions(+), 33 deletions(-) diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index da8937910e..8996bab628 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -177,7 +177,63 @@ final class DiffusionCommitQuery } protected function loadPage() { - return $this->loadStandardPage($this->newResultObject()); + $table = $this->newResultObject(); + $conn = $table->establishConnection('r'); + + $subqueries = array(); + if ($this->responsiblePHIDs) { + $base_authors = $this->authorPHIDs; + $base_auditors = $this->auditorPHIDs; + + $responsible_phids = $this->responsiblePHIDs; + if ($base_authors) { + $all_authors = array_merge($base_authors, $responsible_phids); + } else { + $all_authors = $responsible_phids; + } + + if ($base_auditors) { + $all_auditors = array_merge($base_auditors, $responsible_phids); + } else { + $all_auditors = $responsible_phids; + } + + $this->authorPHIDs = $all_authors; + $this->auditorPHIDs = $base_auditors; + $subqueries[] = $this->buildStandardPageQuery( + $conn, + $table->getTableName()); + + $this->authorPHIDs = $base_authors; + $this->auditorPHIDs = $all_auditors; + $subqueries[] = $this->buildStandardPageQuery( + $conn, + $table->getTableName()); + } else { + $subqueries[] = $this->buildStandardPageQuery( + $conn, + $table->getTableName()); + } + + if (count($subqueries) > 1) { + foreach ($subqueries as $key => $subquery) { + $subqueries[$key] = '('.$subquery.')'; + } + + $query = qsprintf( + $conn, + '%Q %Q %Q', + implode(' UNION DISTINCT ', $subqueries), + $this->buildOrderClause($conn, true), + $this->buildLimitClause($conn)); + } else { + $query = head($subqueries); + } + + $rows = queryfx_all($conn, '%Q', $query); + $rows = $this->didLoadRawRows($rows); + + return $table->loadAllFromArray($rows); } protected function willFilterPage(array $commits) { @@ -487,18 +543,10 @@ final class DiffusionCommitQuery $this->auditorPHIDs); } - if ($this->responsiblePHIDs !== null) { - $where[] = qsprintf( - $conn, - '(audit.auditorPHID IN (%Ls) OR commit.authorPHID IN (%Ls))', - $this->responsiblePHIDs, - $this->responsiblePHIDs); - } - if ($this->statuses !== null) { $where[] = qsprintf( $conn, - 'commit.auditStatus IN (%Ls)', + 'commit.auditStatus IN (%Ld)', $this->statuses); } @@ -541,10 +589,6 @@ final class DiffusionCommitQuery return ($this->auditIDs || $this->auditorPHIDs); } - private function shouldJoinAudit() { - return (bool)$this->responsiblePHIDs; - } - private function shouldJoinOwners() { return (bool)$this->packagePHIDs; } @@ -560,13 +604,6 @@ final class DiffusionCommitQuery $audit_request->getTableName()); } - if ($this->shouldJoinAudit()) { - $join[] = qsprintf( - $conn, - 'LEFT JOIN %T audit ON commit.phid = audit.commitPHID', - $audit_request->getTableName()); - } - if ($this->shouldJoinOwners()) { $join[] = qsprintf( $conn, @@ -584,10 +621,6 @@ final class DiffusionCommitQuery return true; } - if ($this->shouldJoinAudit()) { - return true; - } - if ($this->shouldJoinOwners()) { return true; } diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 9bebfe6957..19d6ba07f4 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -111,7 +111,19 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery AphrontDatabaseConnection $conn, $table_name) { - $rows = queryfx_all( + $query = $this->buildStandardPageQuery($conn, $table_name); + + $rows = queryfx_all($conn, '%Q', $query); + $rows = $this->didLoadRawRows($rows); + + return $rows; + } + + protected function buildStandardPageQuery( + AphrontDatabaseConnection $conn, + $table_name) { + + return qsprintf( $conn, '%Q FROM %T %Q %Q %Q %Q %Q %Q %Q', $this->buildSelectClause($conn), @@ -123,10 +135,6 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $this->buildHavingClause($conn), $this->buildOrderClause($conn), $this->buildLimitClause($conn)); - - $rows = $this->didLoadRawRows($rows); - - return $rows; } protected function didLoadRawRows(array $rows) { @@ -1032,7 +1040,10 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery /** * @task order */ - final protected function buildOrderClause(AphrontDatabaseConnection $conn) { + final protected function buildOrderClause( + AphrontDatabaseConnection $conn, + $for_union = false) { + $orderable = $this->getOrderableColumns(); $vector = $this->getOrderVector(); @@ -1045,7 +1056,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $parts[] = $part; } - return $this->formatOrderClause($conn, $parts); + return $this->formatOrderClause($conn, $parts, $for_union); } @@ -1054,7 +1065,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery */ protected function formatOrderClause( AphrontDatabaseConnection $conn, - array $parts) { + array $parts, + $for_union = false) { $is_query_reversed = false; if ($this->getBeforeID()) { @@ -1075,6 +1087,13 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } $table = idx($part, 'table'); + + // When we're building an ORDER BY clause for a sequence of UNION + // statements, we can't refer to tables from the subqueries. + if ($for_union) { + $table = null; + } + $column = $part['column']; if ($table !== null) { From e3a48dde1d5b95df4eb0ea1763a6baa7576e418f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 23 Oct 2017 10:03:56 -0700 Subject: [PATCH 03/30] Correct a method signature in DifferentialDraftField Summary: Ref T12190. See . (I have a followup to fix the root issue.) Test Plan: Loaded Differential with an eye on the error log in PHP7, no longer saw warnings. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12190 Differential Revision: https://secure.phabricator.com/D18723 --- .../differential/customfield/DifferentialDraftField.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/differential/customfield/DifferentialDraftField.php b/src/applications/differential/customfield/DifferentialDraftField.php index e37d622a33..e7ed2bedb2 100644 --- a/src/applications/differential/customfield/DifferentialDraftField.php +++ b/src/applications/differential/customfield/DifferentialDraftField.php @@ -24,7 +24,7 @@ final class DifferentialDraftField return true; } - public function renderPropertyViewValue() { + public function renderPropertyViewValue(array $handles) { return null; } From 683df399e7f4753dcea7ce03961e36956d6e790d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 23 Oct 2017 14:59:32 -0700 Subject: [PATCH 04/30] Simplify UNION/ORDER query construction in DifferentialRevisionQuery Summary: Ref T12680. Use the slightly sleeker construction from D18722 in Differential. Test Plan: Viewed revision list, reordered by date modified. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12680 Differential Revision: https://secure.phabricator.com/D18727 --- .../query/DifferentialRevisionQuery.php | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 8fd91cbd7e..727a4ca184 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -38,8 +38,6 @@ final class DifferentialRevisionQuery private $needDrafts; private $needFlags; - private $buildingGlobalOrder; - /* -( Query Configuration )------------------------------------------------ */ @@ -484,12 +482,11 @@ final class DifferentialRevisionQuery } if (count($selects) > 1) { - $this->buildingGlobalOrder = true; $query = qsprintf( $conn_r, '%Q %Q %Q', implode(' UNION DISTINCT ', $selects), - $this->buildOrderClause($conn_r), + $this->buildOrderClause($conn_r, true), $this->buildLimitClause($conn_r)); } else { $query = head($selects); @@ -513,7 +510,6 @@ final class DifferentialRevisionQuery $group_by = $this->buildGroupClause($conn_r); $having = $this->buildHavingClause($conn_r); - $this->buildingGlobalOrder = false; $order_by = $this->buildOrderClause($conn_r); $limit = $this->buildLimitClause($conn_r); @@ -758,17 +754,9 @@ final class DifferentialRevisionQuery } public function getOrderableColumns() { - $primary = ($this->buildingGlobalOrder ? null : 'r'); - return array( - 'id' => array( - 'table' => $primary, - 'column' => 'id', - 'type' => 'int', - 'unique' => true, - ), 'updated' => array( - 'table' => $primary, + 'table' => $this->getPrimaryTableAlias(), 'column' => 'dateModified', 'type' => 'int', ), From 1d213dc1fa4b45280196457c61a1c36c396a602c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 23 Oct 2017 15:04:06 -0700 Subject: [PATCH 05/30] Clean up virtual "_ft_rank" column for query construction of Ferret objects Summary: Ref T12974. Ferret object queries SELECT a virtual "_ft_rank" column for relevance ordering. Currently, they always SELECT this column. That's fine and doesn't hurt anything, but makes developing and debugging things kind of a pain since every query has this `, blah blah _ft_rank` junk. Instead, construct this column only if we're actually going to use it. Mostly, this cleans up DarkConsole / query logs a bit. Test Plan: Viewed normal query results on various pages, viewed global search results, ordered Maniphest tasks by normal stuff and by "Relevance". Viewed DarkConsole, saw no more "_ft_rank" junk on normal pages. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12974 Differential Revision: https://secure.phabricator.com/D18728 --- .../policy/PhabricatorCursorPagedPolicyAwareQuery.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 19d6ba07f4..63daa6df79 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -1565,6 +1565,13 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return $select; } + $vector = $this->getOrderVector(); + if (!$vector->containsKey('rank')) { + // We only need to SELECT the virtual "_ft_rank" column if we're + // actually sorting the results by rank. + return $select; + } + if (!$this->ferretEngine) { $select[] = '0 _ft_rank'; return $select; From 28a24c333fdd4a8def13e624552eeaa3e37f7e08 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 24 Oct 2017 05:57:15 -0700 Subject: [PATCH 06/30] Fix a couple of other missing getApplicationTransactionCommentObject() implementations Summary: See PHI165. See D18715. These objects (projects, blogs) also need implementations now. (I thought about making this method `abstract` or doing try/catch to maybe make this more robust, but I think this should be the end of it, and those changes have mild complexity/compatibility/risk issues.) Test Plan: Changed `bin/search index` to index only one document of each type, ran `bin/search index --all --force`, saw no more comment-related errors. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18729 --- src/applications/phame/storage/PhameBlogTransaction.php | 4 ++++ .../project/storage/PhabricatorProjectTransaction.php | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/applications/phame/storage/PhameBlogTransaction.php b/src/applications/phame/storage/PhameBlogTransaction.php index c605510d7d..d3d6a79d0a 100644 --- a/src/applications/phame/storage/PhameBlogTransaction.php +++ b/src/applications/phame/storage/PhameBlogTransaction.php @@ -15,6 +15,10 @@ final class PhameBlogTransaction return PhabricatorPhameBlogPHIDType::TYPECONST; } + public function getApplicationTransactionCommentObject() { + return null; + } + public function getBaseTransactionClass() { return 'PhameBlogTransactionType'; } diff --git a/src/applications/project/storage/PhabricatorProjectTransaction.php b/src/applications/project/storage/PhabricatorProjectTransaction.php index 158c2480c0..a8b2bb0d4a 100644 --- a/src/applications/project/storage/PhabricatorProjectTransaction.php +++ b/src/applications/project/storage/PhabricatorProjectTransaction.php @@ -19,6 +19,10 @@ final class PhabricatorProjectTransaction return PhabricatorProjectProjectPHIDType::TYPECONST; } + public function getApplicationTransactionCommentObject() { + return null; + } + public function getBaseTransactionClass() { return 'PhabricatorProjectTransactionType'; } From beaf0ad9a6360154d5c3f79e2e7eb62b553dcf5b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 24 Oct 2017 09:10:48 -0700 Subject: [PATCH 07/30] Attribute revision promotion from "Draft" to "Needs Review" to the author Summary: Ref T2543. When Harbormaster finishes builds and promotes a draft revision to review, we currently publish "Harbormaster requested review of...". Instead, attribute this action to the author, since that's more natural and more useful. Test Plan: Promoted a diff locally, saw it attributed to me rather than Harbormaster. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18730 --- .../differential/editor/DifferentialTransactionEditor.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 7c9bb01b07..5e013eff43 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1537,7 +1537,13 @@ final class DifferentialTransactionEditor if ($object->isDraft() && $auto_undraft) { $active_builds = $this->hasActiveBuilds($object); if (!$active_builds) { + // When Harbormaster moves a revision out of the draft state, we + // attribute the action to the revision author since this is more + // natural and more useful. + $author_phid = $object->getAuthorPHID(); + $xaction = $object->getApplicationTransactionTemplate() + ->setAuthorPHID($author_phid) ->setTransactionType( DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE) ->setOldValue(false) From f1204c8c45683d86afa5624a6ef7f88f88f8a6c7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 26 Oct 2017 13:38:12 -0700 Subject: [PATCH 08/30] Convert Ponder Questions to Ferret engine Summary: See PHI177. Ref T12974. PonderQuestion was overlooked during the Ferret engine conversions. Test Plan: Ran migrations, searched for questions, got results: {F5241185} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12974 Differential Revision: https://secure.phabricator.com/D18736 --- .../20171026.ferret.01.ponder.doc.sql | 9 +++++++++ .../20171026.ferret.02.ponder.field.sql | 8 ++++++++ .../20171026.ferret.03.ponder.ngrams.sql | 5 +++++ .../20171026.ferret.04.ponder.cngrams.sql | 7 +++++++ .../20171026.ferret.05.ponder.index.php | 11 +++++++++++ src/__phutil_library_map__.php | 3 +++ .../search/PonderQuestionFerretEngine.php | 18 ++++++++++++++++++ .../ponder/storage/PonderQuestion.php | 12 +++++++++++- 8 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 resources/sql/autopatches/20171026.ferret.01.ponder.doc.sql create mode 100644 resources/sql/autopatches/20171026.ferret.02.ponder.field.sql create mode 100644 resources/sql/autopatches/20171026.ferret.03.ponder.ngrams.sql create mode 100644 resources/sql/autopatches/20171026.ferret.04.ponder.cngrams.sql create mode 100644 resources/sql/autopatches/20171026.ferret.05.ponder.index.php create mode 100644 src/applications/ponder/search/PonderQuestionFerretEngine.php diff --git a/resources/sql/autopatches/20171026.ferret.01.ponder.doc.sql b/resources/sql/autopatches/20171026.ferret.01.ponder.doc.sql new file mode 100644 index 0000000000..38c86a4134 --- /dev/null +++ b/resources/sql/autopatches/20171026.ferret.01.ponder.doc.sql @@ -0,0 +1,9 @@ +CREATE TABLE {$NAMESPACE}_ponder.ponder_question_fdocument ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + objectPHID VARBINARY(64) NOT NULL, + isClosed BOOL NOT NULL, + authorPHID VARBINARY(64), + ownerPHID VARBINARY(64), + epochCreated INT UNSIGNED NOT NULL, + epochModified INT UNSIGNED NOT NULL +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20171026.ferret.02.ponder.field.sql b/resources/sql/autopatches/20171026.ferret.02.ponder.field.sql new file mode 100644 index 0000000000..871f0d8f5b --- /dev/null +++ b/resources/sql/autopatches/20171026.ferret.02.ponder.field.sql @@ -0,0 +1,8 @@ +CREATE TABLE {$NAMESPACE}_ponder.ponder_question_ffield ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + documentID INT UNSIGNED NOT NULL, + fieldKey VARCHAR(4) NOT NULL COLLATE {$COLLATE_TEXT}, + rawCorpus LONGTEXT NOT NULL COLLATE {$COLLATE_SORT}, + termCorpus LONGTEXT NOT NULL COLLATE {$COLLATE_SORT}, + normalCorpus LONGTEXT NOT NULL COLLATE {$COLLATE_SORT} +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20171026.ferret.03.ponder.ngrams.sql b/resources/sql/autopatches/20171026.ferret.03.ponder.ngrams.sql new file mode 100644 index 0000000000..3d2a3024b8 --- /dev/null +++ b/resources/sql/autopatches/20171026.ferret.03.ponder.ngrams.sql @@ -0,0 +1,5 @@ +CREATE TABLE {$NAMESPACE}_ponder.ponder_question_fngrams ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + documentID INT UNSIGNED NOT NULL, + ngram CHAR(3) NOT NULL COLLATE {$COLLATE_TEXT} +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20171026.ferret.04.ponder.cngrams.sql b/resources/sql/autopatches/20171026.ferret.04.ponder.cngrams.sql new file mode 100644 index 0000000000..49b66e0d39 --- /dev/null +++ b/resources/sql/autopatches/20171026.ferret.04.ponder.cngrams.sql @@ -0,0 +1,7 @@ +CREATE TABLE {$NAMESPACE}_ponder.ponder_question_fngrams_common ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + ngram CHAR(3) NOT NULL COLLATE {$COLLATE_TEXT}, + needsCollection BOOL NOT NULL, + UNIQUE KEY `key_ngram` (ngram), + KEY `key_collect` (needsCollection) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20171026.ferret.05.ponder.index.php b/resources/sql/autopatches/20171026.ferret.05.ponder.index.php new file mode 100644 index 0000000000..20489846d2 --- /dev/null +++ b/resources/sql/autopatches/20171026.ferret.05.ponder.index.php @@ -0,0 +1,11 @@ +getPHID(), + array( + 'force' => true, + )); +} diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 93efcc65e1..91d5a3cc7b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4774,6 +4774,7 @@ phutil_register_library_map(array( 'PonderQuestionEditController' => 'applications/ponder/controller/PonderQuestionEditController.php', 'PonderQuestionEditEngine' => 'applications/ponder/editor/PonderQuestionEditEngine.php', 'PonderQuestionEditor' => 'applications/ponder/editor/PonderQuestionEditor.php', + 'PonderQuestionFerretEngine' => 'applications/ponder/search/PonderQuestionFerretEngine.php', 'PonderQuestionFulltextEngine' => 'applications/ponder/search/PonderQuestionFulltextEngine.php', 'PonderQuestionHistoryController' => 'applications/ponder/controller/PonderQuestionHistoryController.php', 'PonderQuestionListController' => 'applications/ponder/controller/PonderQuestionListController.php', @@ -10539,6 +10540,7 @@ phutil_register_library_map(array( 'PhabricatorDestructibleInterface', 'PhabricatorSpacesInterface', 'PhabricatorFulltextInterface', + 'PhabricatorFerretInterface', ), 'PonderQuestionAnswerTransaction' => 'PonderQuestionTransactionType', 'PonderQuestionAnswerWikiTransaction' => 'PonderQuestionTransactionType', @@ -10548,6 +10550,7 @@ phutil_register_library_map(array( 'PonderQuestionEditController' => 'PonderController', 'PonderQuestionEditEngine' => 'PhabricatorEditEngine', 'PonderQuestionEditor' => 'PonderEditor', + 'PonderQuestionFerretEngine' => 'PhabricatorFerretEngine', 'PonderQuestionFulltextEngine' => 'PhabricatorFulltextEngine', 'PonderQuestionHistoryController' => 'PonderController', 'PonderQuestionListController' => 'PonderController', diff --git a/src/applications/ponder/search/PonderQuestionFerretEngine.php b/src/applications/ponder/search/PonderQuestionFerretEngine.php new file mode 100644 index 0000000000..72345f60c0 --- /dev/null +++ b/src/applications/ponder/search/PonderQuestionFerretEngine.php @@ -0,0 +1,18 @@ + Date: Thu, 26 Oct 2017 13:00:09 -0700 Subject: [PATCH 09/30] Add a missing `artifactIndex` key to HarbormasterBuildArtifact Summary: See PHI176. We issue a query with only `artifactIndex` from `BuildTarget`, but don't have an applicable key. Test Plan: This isn't on the normal Harbormaster execution path so I'm not 100% sure I have a local repro, but will confirm with customer. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D18732 --- .../harbormaster/storage/build/HarbormasterBuildArtifact.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php index 6825bf6646..461ef4b06f 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php @@ -43,6 +43,9 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO 'key_target' => array( 'columns' => array('buildTargetPHID', 'artifactType'), ), + 'key_index' => array( + 'columns' => array('artifactIndex'), + ), ), ) + parent::getConfiguration(); } From fbfed82efddee9732909036f358bc48c272136e4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 26 Oct 2017 13:05:31 -0700 Subject: [PATCH 10/30] Add a missing DaemonLogEvent key for garbage collection Summary: See PHI176. This is issued periodically by the garbage collector. Normally this table is relatively small-ish so this missing key isn't hugely noticeable. Test Plan: Ran `./bin/garbage collect --collector daemon.processes --trace` to get the query the GC runs. Ran ##DELETE FROM `daemon_logevent` WHERE epoch < 1508443504 LIMIT 100## before and after the key, saw a much better query plan afterward: Before: ``` mysql> explain DELETE FROM `daemon_logevent` WHERE epoch < 1508443504 LIMIT 100; +----+-------------+-----------------+------+---------------+------+---------+------+-------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +----+-------------+-----------------+------+---------------+------+---------+------+-------+-------------+ | 1 | SIMPLE | daemon_logevent | ALL | NULL | NULL | NULL | NULL | 19325 | Using where | +----+-------------+-----------------+------+---------------+------+---------+------+-------+-------------+ 1 row in set (0.00 sec) ``` After: ``` mysql> explain DELETE FROM `daemon_logevent` WHERE epoch < 1508443504 LIMIT 100; +----+-------------+-----------------+-------+---------------+-----------+---------+-------+------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +----+-------------+-----------------+-------+---------------+-----------+---------+-------+------+-------------+ | 1 | SIMPLE | daemon_logevent | range | key_epoch | key_epoch | 4 | const | 1 | Using where | +----+-------------+-----------------+-------+---------------+-----------+---------+-------+------+-------------+ 1 row in set (0.00 sec) ``` Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18733 --- src/applications/daemon/storage/PhabricatorDaemonLogEvent.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/applications/daemon/storage/PhabricatorDaemonLogEvent.php b/src/applications/daemon/storage/PhabricatorDaemonLogEvent.php index afdbd1f621..c5d2cbd1d4 100644 --- a/src/applications/daemon/storage/PhabricatorDaemonLogEvent.php +++ b/src/applications/daemon/storage/PhabricatorDaemonLogEvent.php @@ -18,6 +18,9 @@ final class PhabricatorDaemonLogEvent extends PhabricatorDaemonDAO { 'logID' => array( 'columns' => array('logID', 'epoch'), ), + 'key_epoch' => array( + 'columns' => array('epoch'), + ), ), ) + parent::getConfiguration(); } From b04d9fdc0ba875d1fd0ff61152a712e163135e18 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 26 Oct 2017 13:11:51 -0700 Subject: [PATCH 11/30] Add a missing key to PhabricatorFile for destroying Files Summary: See PHI176. Depends on D18733. We issue a query when deleting files that currently doesn't hit any keys. Test Plan: Ran `./bin/remove destroy --force --trace F56376` to get the query. Ran ##SELECT * FROM `file` WHERE storageEngine = 'blob' AND storageHandle = '23366' LIMIT 1## before and after the change. Before: ``` mysql> explain SELECT * FROM `file` WHERE storageEngine = 'blob' AND storageHandle = '23366' LIMIT 1; +----+-------------+-------+------+---------------+------+---------+------+-------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +----+-------------+-------+------+---------------+------+---------+------+-------+-------------+ | 1 | SIMPLE | file | ALL | NULL | NULL | NULL | NULL | 33866 | Using where | +----+-------------+-------+------+---------------+------+---------+------+-------+-------------+ 1 row in set (0.01 sec) ``` After: ``` mysql> explain SELECT * FROM `file` WHERE storageEngine = 'blob' AND storageHandle = '23366' LIMIT 1; +----+-------------+-------+------+---------------+------------+---------+-------------+------+------------------------------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +----+-------------+-------+------+---------------+------------+---------+-------------+------+------------------------------------+ | 1 | SIMPLE | file | ref | key_engine | key_engine | 388 | const,const | 190 | Using index condition; Using where | +----+-------------+-------+------+---------------+------------+---------+-------------+------+------------------------------------+ 1 row in set (0.00 sec) ``` Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18734 --- src/applications/files/storage/PhabricatorFile.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 9636b5b017..c87623c68c 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -134,6 +134,9 @@ final class PhabricatorFile extends PhabricatorFileDAO 'columns' => array('builtinKey'), 'unique' => true, ), + 'key_engine' => array( + 'columns' => array('storageEngine', 'storageHandle(64)'), + ), ), ) + parent::getConfiguration(); } From d1b4c9f50b99b08fd77fbf6554834b61839813eb Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 26 Oct 2017 13:17:33 -0700 Subject: [PATCH 12/30] Add a missing key on DrydockLease Summary: Depends on D18734. See PHI176. We run this query on the main Drydock lease web UI, among other places. There is currently no `status` key which can satisfy it. Test Plan: Viewed Drydock lease page to get the query. Ran ##explain SELECT * FROM `drydock_lease` WHERE (status IN ('pending', 'acquired', 'active')) ORDER BY `id` DESC LIMIT 101;## before and after the change. I don't have a ton of leases locally so the un-key'd EXPLAIN isn't //that// bad, but still shows that we're getting a better key. Before: ``` mysql> explain SELECT * FROM `drydock_lease` WHERE (status IN ('pending', 'acquired', 'active')) ORDER BY `id` DESC LIMIT 101; +----+-------------+---------------+-------+---------------+---------+---------+------+------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +----+-------------+---------------+-------+---------------+---------+---------+------+------+-------------+ | 1 | SIMPLE | drydock_lease | index | NULL | PRIMARY | 4 | NULL | 101 | Using where | +----+-------------+---------------+-------+---------------+---------+---------+------+------+-------------+ 1 row in set (0.00 sec) ``` After: ``` mysql> explain SELECT * FROM `drydock_lease` WHERE (status IN ('pending', 'acquired', 'active')) ORDER BY `id` DESC LIMIT 101; +----+-------------+---------------+-------+---------------+------------+---------+------+------+---------------------------------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +----+-------------+---------------+-------+---------------+------------+---------+------+------+---------------------------------------+ | 1 | SIMPLE | drydock_lease | range | key_status | key_status | 130 | NULL | 5 | Using index condition; Using filesort | +----+-------------+---------------+-------+---------------+------------+---------+------+------+---------------------------------------+ 1 row in set (0.00 sec) ``` Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D18735 --- src/applications/drydock/storage/DrydockLease.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php index 0dd4e36b48..e60529afe4 100644 --- a/src/applications/drydock/storage/DrydockLease.php +++ b/src/applications/drydock/storage/DrydockLease.php @@ -83,6 +83,9 @@ final class DrydockLease extends DrydockDAO 'key_resource' => array( 'columns' => array('resourcePHID', 'status'), ), + 'key_status' => array( + 'columns' => array('status'), + ), ), ) + parent::getConfiguration(); } From f7f3dd5b20848c221e16bc34b3ce61ddbdbdde94 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 24 Oct 2017 14:04:06 -0700 Subject: [PATCH 13/30] Don't run Herald build and mail rules when they don't make sense Summary: Ref T2543. Fixes T10109. Currently, Herald only runs in Differential when a change updates the diff. This is partly for historical reasons, and partly because we don't want to restart builds every time someone makes a comment. However, this behavior is inconsistent with other applications (which always trigger on any change), and occasionally confusing to users (in T10109, for example) or otherwise undesirable. A similar issue is that T2543 has introduced a "Draft" state, where revisions don't send normal mail until builds finish. This interacts poorly with "Send me an email" rules (which shouldn't do anything here) and particularly with "Send me an email + only run these actions the first time the rule matches", since that might have an effect like "do nothing when the revision is created, then never anything again since you already did nothing once". To navigate both of these issues, let objects tell Herald that certain actions (like mail or builds) are currently forbidden. If a rule uses a field or action which is currently forbidden, the whole rule automatically fails before it executes, but doesn't count toward "only the first time" as far as Herald's tracking of rule execution is concerned. Then, forbid mail for draft revisions, and forbid builds for revisions which didn't just get updated. Forbidding mail fixes the issues with "Send me an email" that were created by the introduction of the draft state. Finally, make Herald run on every revision update, not just substantive updates to the diff. This resolves T10109. Test Plan: Created revisions via the draft -> submit workflow, saw different transcripts. Here's a mail action being forbidden for a draft: {F5237324} Here's a build action being forbidden for a "mundane" update: {F5237326} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T10109, T2543 Differential Revision: https://secure.phabricator.com/D18731 --- src/__phutil_library_map__.php | 10 + .../editor/DifferentialTransactionEditor.php | 48 +++-- .../herald/DifferentialHeraldStateReasons.php | 22 ++ .../HarbormasterRunBuildPlansHeraldAction.php | 6 + .../herald/action/HeraldAction.php | 12 ++ .../herald/adapter/HeraldAdapter.php | 35 +++ .../controller/HeraldTranscriptController.php | 20 +- .../herald/engine/HeraldEngine.php | 204 ++++++++++++++---- src/applications/herald/field/HeraldField.php | 4 + .../herald/state/HeraldBuildableState.php | 7 + .../herald/state/HeraldMailableState.php | 7 + src/applications/herald/state/HeraldState.php | 3 + .../herald/state/HeraldStateReasons.php | 26 +++ .../transcript/HeraldConditionTranscript.php | 7 + .../transcript/HeraldRuleTranscript.php | 6 + .../PhabricatorMetaMTAEmailHeraldAction.php | 6 + 16 files changed, 359 insertions(+), 64 deletions(-) create mode 100644 src/applications/differential/herald/DifferentialHeraldStateReasons.php create mode 100644 src/applications/herald/state/HeraldBuildableState.php create mode 100644 src/applications/herald/state/HeraldMailableState.php create mode 100644 src/applications/herald/state/HeraldState.php create mode 100644 src/applications/herald/state/HeraldStateReasons.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 91d5a3cc7b..2040568496 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -458,6 +458,7 @@ phutil_register_library_map(array( 'DifferentialGetWorkingCopy' => 'applications/differential/DifferentialGetWorkingCopy.php', 'DifferentialGitSVNIDCommitMessageField' => 'applications/differential/field/DifferentialGitSVNIDCommitMessageField.php', 'DifferentialHarbormasterField' => 'applications/differential/customfield/DifferentialHarbormasterField.php', + 'DifferentialHeraldStateReasons' => 'applications/differential/herald/DifferentialHeraldStateReasons.php', 'DifferentialHiddenComment' => 'applications/differential/storage/DifferentialHiddenComment.php', 'DifferentialHostField' => 'applications/differential/customfield/DifferentialHostField.php', 'DifferentialHovercardEngineExtension' => 'applications/differential/engineextension/DifferentialHovercardEngineExtension.php', @@ -1328,6 +1329,7 @@ phutil_register_library_map(array( 'HeraldApplicationActionGroup' => 'applications/herald/action/HeraldApplicationActionGroup.php', 'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php', 'HeraldBasicFieldGroup' => 'applications/herald/field/HeraldBasicFieldGroup.php', + 'HeraldBuildableState' => 'applications/herald/state/HeraldBuildableState.php', 'HeraldCommitAdapter' => 'applications/diffusion/herald/HeraldCommitAdapter.php', 'HeraldCondition' => 'applications/herald/storage/HeraldCondition.php', 'HeraldConditionTranscript' => 'applications/herald/storage/transcript/HeraldConditionTranscript.php', @@ -1351,6 +1353,7 @@ phutil_register_library_map(array( 'HeraldGroup' => 'applications/herald/group/HeraldGroup.php', 'HeraldInvalidActionException' => 'applications/herald/engine/exception/HeraldInvalidActionException.php', 'HeraldInvalidConditionException' => 'applications/herald/engine/exception/HeraldInvalidConditionException.php', + 'HeraldMailableState' => 'applications/herald/state/HeraldMailableState.php', 'HeraldManageGlobalRulesCapability' => 'applications/herald/capability/HeraldManageGlobalRulesCapability.php', 'HeraldManiphestTaskAdapter' => 'applications/maniphest/herald/HeraldManiphestTaskAdapter.php', 'HeraldNewController' => 'applications/herald/controller/HeraldNewController.php', @@ -1388,6 +1391,8 @@ phutil_register_library_map(array( 'HeraldSchemaSpec' => 'applications/herald/storage/HeraldSchemaSpec.php', 'HeraldSelectFieldValue' => 'applications/herald/value/HeraldSelectFieldValue.php', 'HeraldSpaceField' => 'applications/spaces/herald/HeraldSpaceField.php', + 'HeraldState' => 'applications/herald/state/HeraldState.php', + 'HeraldStateReasons' => 'applications/herald/state/HeraldStateReasons.php', 'HeraldSubscribersField' => 'applications/subscriptions/herald/HeraldSubscribersField.php', 'HeraldSupportActionGroup' => 'applications/herald/action/HeraldSupportActionGroup.php', 'HeraldSupportFieldGroup' => 'applications/herald/field/HeraldSupportFieldGroup.php', @@ -5467,6 +5472,7 @@ phutil_register_library_map(array( 'DifferentialGetWorkingCopy' => 'Phobject', 'DifferentialGitSVNIDCommitMessageField' => 'DifferentialCommitMessageField', 'DifferentialHarbormasterField' => 'DifferentialCustomField', + 'DifferentialHeraldStateReasons' => 'HeraldStateReasons', 'DifferentialHiddenComment' => 'DifferentialDAO', 'DifferentialHostField' => 'DifferentialCustomField', 'DifferentialHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', @@ -6458,6 +6464,7 @@ phutil_register_library_map(array( 'HeraldApplicationActionGroup' => 'HeraldActionGroup', 'HeraldApplyTranscript' => 'Phobject', 'HeraldBasicFieldGroup' => 'HeraldFieldGroup', + 'HeraldBuildableState' => 'HeraldState', 'HeraldCommitAdapter' => array( 'HeraldAdapter', 'HarbormasterBuildableAdapterInterface', @@ -6487,6 +6494,7 @@ phutil_register_library_map(array( 'HeraldGroup' => 'Phobject', 'HeraldInvalidActionException' => 'Exception', 'HeraldInvalidConditionException' => 'Exception', + 'HeraldMailableState' => 'HeraldState', 'HeraldManageGlobalRulesCapability' => 'PhabricatorPolicyCapability', 'HeraldManiphestTaskAdapter' => 'HeraldAdapter', 'HeraldNewController' => 'HeraldController', @@ -6531,6 +6539,8 @@ phutil_register_library_map(array( 'HeraldSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'HeraldSelectFieldValue' => 'HeraldFieldValue', 'HeraldSpaceField' => 'HeraldField', + 'HeraldState' => 'Phobject', + 'HeraldStateReasons' => 'Phobject', 'HeraldSubscribersField' => 'HeraldField', 'HeraldSupportActionGroup' => 'HeraldActionGroup', 'HeraldSupportFieldGroup' => 'HeraldFieldGroup', diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 5e013eff43..9ab30c3f06 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1003,26 +1003,7 @@ final class DifferentialTransactionEditor protected function shouldApplyHeraldRules( PhabricatorLiskDAO $object, array $xactions) { - - if ($this->getIsNewObject()) { - return true; - } - - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_UPDATE: - if (!$this->getIsCloseByCommit()) { - return true; - } - break; - case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE: - // When users commandeer revisions, we may need to trigger - // signatures or author-based rules. - return true; - } - } - - return parent::shouldApplyHeraldRules($object, $xactions); + return true; } protected function didApplyHeraldRules( @@ -1211,6 +1192,33 @@ final class DifferentialTransactionEditor $revision, $revision->getActiveDiff()); + // If the object is still a draft, prevent "Send me an email" and other + // similar rules from acting yet. + if (!$object->shouldBroadcast()) { + $adapter->setForbiddenAction( + HeraldMailableState::STATECONST, + DifferentialHeraldStateReasons::REASON_DRAFT); + } + + // If this edit didn't actually change the diff (for example, a user + // edited the title or changed subscribers), prevent "Run build plan" + // and other similar rules from acting yet, since the build results will + // not (or, at least, should not) change unless the actual source changes. + $has_update = false; + $type_update = DifferentialTransaction::TYPE_UPDATE; + foreach ($xactions as $xaction) { + if ($xaction->getTransactionType() == $type_update) { + $has_update = true; + break; + } + } + + if (!$has_update) { + $adapter->setForbiddenAction( + HeraldBuildableState::STATECONST, + DifferentialHeraldStateReasons::REASON_UNCHANGED); + } + return $adapter; } diff --git a/src/applications/differential/herald/DifferentialHeraldStateReasons.php b/src/applications/differential/herald/DifferentialHeraldStateReasons.php new file mode 100644 index 0000000000..d3259560d5 --- /dev/null +++ b/src/applications/differential/herald/DifferentialHeraldStateReasons.php @@ -0,0 +1,22 @@ + pht( + 'This revision is still an unsubmitted draft, so mail will not '. + 'be sent yet.'), + self::REASON_UNCHANGED => pht( + 'The update which triggered Herald did not update the diff for '. + 'this revision, so builds will not run.'), + ); + + return idx($reasons, $reason); + } + +} diff --git a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php index 76bdf9ccc3..8c718e5f5d 100644 --- a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php +++ b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php @@ -7,6 +7,12 @@ final class HarbormasterRunBuildPlansHeraldAction const ACTIONCONST = 'harbormaster.build'; + public function getRequiredAdapterStates() { + return array( + HeraldBuildableState::STATECONST, + ); + } + public function getActionGroupKey() { return HeraldSupportActionGroup::ACTIONGROUPKEY; } diff --git a/src/applications/herald/action/HeraldAction.php b/src/applications/herald/action/HeraldAction.php index 091f3e9bc2..a2d589f9f2 100644 --- a/src/applications/herald/action/HeraldAction.php +++ b/src/applications/herald/action/HeraldAction.php @@ -17,6 +17,7 @@ abstract class HeraldAction extends Phobject { const DO_STANDARD_PERMISSION = 'do.standard.permission'; const DO_STANDARD_INVALID_ACTION = 'do.standard.invalid-action'; const DO_STANDARD_WRONG_RULE_TYPE = 'do.standard.wrong-rule-type'; + const DO_STANDARD_FORBIDDEN = 'do.standard.forbidden'; abstract public function getHeraldActionName(); abstract public function supportsObject($object); @@ -25,6 +26,10 @@ abstract class HeraldAction extends Phobject { abstract public function renderActionDescription($value); + public function getRequiredAdapterStates() { + return array(); + } + protected function renderActionEffectDescription($type, $data) { return null; } @@ -336,6 +341,11 @@ abstract class HeraldAction extends Phobject { 'color' => 'red', 'name' => pht('Wrong Rule Type'), ), + self::DO_STANDARD_FORBIDDEN => array( + 'icon' => 'fa-ban', + 'color' => 'violet', + 'name' => pht('Forbidden'), + ), ); } @@ -381,6 +391,8 @@ abstract class HeraldAction extends Phobject { return pht( 'This action does not support rules of type "%s".', $data); + case self::DO_STANDARD_FORBIDDEN: + return HeraldStateReasons::getExplanation($data); } return null; diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 78ce862945..1c50c0b5ee 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -37,6 +37,7 @@ abstract class HeraldAdapter extends Phobject { private $fieldMap; private $actionMap; private $edgeCache = array(); + private $forbiddenActions = array(); public function getEmailPHIDs() { return array_values($this->emailPHIDs); @@ -1116,4 +1117,38 @@ abstract class HeraldAdapter extends Phobject { return $this->edgeCache[$type]; } + +/* -( Forbidden Actions )-------------------------------------------------- */ + + + final public function getForbiddenActions() { + return array_keys($this->forbiddenActions); + } + + final public function setForbiddenAction($action, $reason) { + $this->forbiddenActions[$action] = $reason; + return $this; + } + + final public function getRequiredFieldStates($field_key) { + return $this->requireFieldImplementation($field_key) + ->getRequiredAdapterStates(); + } + + final public function getRequiredActionStates($action_key) { + return $this->requireActionImplementation($action_key) + ->getRequiredAdapterStates(); + } + + final public function getForbiddenReason($action) { + if (!isset($this->forbiddenActions[$action])) { + throw new Exception( + pht( + 'Action "%s" is not forbidden!', + $action)); + } + + return $this->forbiddenActions[$action]; + } + } diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php index a9509beb33..dc7ca676e3 100644 --- a/src/applications/herald/controller/HeraldTranscriptController.php +++ b/src/applications/herald/controller/HeraldTranscriptController.php @@ -273,7 +273,11 @@ final class HeraldTranscriptController extends HeraldController { ->setTarget(phutil_tag('strong', array(), pht('Conditions')))); foreach ($cond_xscripts as $cond_xscript) { - if ($cond_xscript->getResult()) { + if ($cond_xscript->isForbidden()) { + $icon = 'fa-ban'; + $color = 'indigo'; + $result = pht('Forbidden'); + } else if ($cond_xscript->getResult()) { $icon = 'fa-check'; $color = 'green'; $result = pht('Passed'); @@ -284,12 +288,17 @@ final class HeraldTranscriptController extends HeraldController { } if ($cond_xscript->getNote()) { + $note_text = $cond_xscript->getNote(); + if ($cond_xscript->isForbidden()) { + $note_text = HeraldStateReasons::getExplanation($note_text); + } + $note = phutil_tag( 'div', array( 'class' => 'herald-condition-note', ), - $cond_xscript->getNote()); + $note_text); } else { $note = null; } @@ -310,7 +319,12 @@ final class HeraldTranscriptController extends HeraldController { $cond_list->addItem($cond_item); } - if ($rule_xscript->getResult()) { + if ($rule_xscript->isForbidden()) { + $last_icon = 'fa-ban'; + $last_color = 'indigo'; + $last_result = pht('Forbidden'); + $last_note = pht('Object state prevented rule evaluation.'); + } else if ($rule_xscript->getResult()) { $last_icon = 'fa-check-circle'; $last_color = 'green'; $last_result = pht('Passed'); diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index df81117b80..b8e1db5626 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -12,6 +12,9 @@ final class HeraldEngine extends Phobject { protected $object; private $dryRun; + private $forbiddenFields = array(); + private $forbiddenActions = array(); + public function setDryRun($dry_run) { $this->dryRun = $dry_run; return $this; @@ -76,39 +79,42 @@ final class HeraldEngine extends Phobject { // This is not a dry run, and this rule is only supposed to be // applied a single time, and it's already been applied... // That means automatic failure. - $xscript = id(new HeraldRuleTranscript()) - ->setRuleID($rule->getID()) + $this->newRuleTranscript($rule) ->setResult(false) - ->setRuleName($rule->getName()) - ->setRuleOwner($rule->getAuthorPHID()) ->setReason( pht( 'This rule is only supposed to be repeated a single time, '. 'and it has already been applied.')); - $this->transcript->addRuleTranscript($xscript); + $rule_matches = false; } else { - $rule_matches = $this->doesRuleMatch($rule, $object); + if ($this->isForbidden($rule, $object)) { + $this->newRuleTranscript($rule) + ->setResult(HeraldRuleTranscript::RESULT_FORBIDDEN) + ->setReason( + pht( + 'Object state is not compatible with rule.')); + + $rule_matches = false; + } else { + $rule_matches = $this->doesRuleMatch($rule, $object); + } } } catch (HeraldRecursiveConditionsException $ex) { $names = array(); - foreach ($this->stack as $rule_id => $ignored) { - $names[] = '"'.$rules[$rule_id]->getName().'"'; + foreach ($this->stack as $rule_phid => $ignored) { + $names[] = '"'.$rules[$rule_phid]->getName().'"'; } $names = implode(', ', $names); - foreach ($this->stack as $rule_id => $ignored) { - $xscript = new HeraldRuleTranscript(); - $xscript->setRuleID($rule_id); - $xscript->setResult(false); - $xscript->setReason( - pht( - "Rules %s are recursively dependent upon one another! ". - "Don't do this! You have formed an unresolvable cycle in the ". - "dependency graph!", - $names)); - $xscript->setRuleName($rules[$rule_id]->getName()); - $xscript->setRuleOwner($rules[$rule_id]->getAuthorPHID()); - $this->transcript->addRuleTranscript($xscript); + foreach ($this->stack as $rule_phid => $ignored) { + $this->newRuleTranscript($rules[$rule_phid]) + ->setResult(false) + ->setReason( + pht( + "Rules %s are recursively dependent upon one another! ". + "Don't do this! You have formed an unresolvable cycle in the ". + "dependency graph!", + $names)); } $rule_matches = false; } @@ -309,14 +315,9 @@ final class HeraldEngine extends Phobject { } } - $rule_transcript = new HeraldRuleTranscript(); - $rule_transcript->setRuleID($rule->getID()); - $rule_transcript->setResult($result); - $rule_transcript->setReason($reason); - $rule_transcript->setRuleName($rule->getName()); - $rule_transcript->setRuleOwner($rule->getAuthorPHID()); - - $this->transcript->addRuleTranscript($rule_transcript); + $this->newRuleTranscript($rule) + ->setResult($result) + ->setReason($reason); return $result; } @@ -327,16 +328,7 @@ final class HeraldEngine extends Phobject { HeraldAdapter $object) { $object_value = $this->getConditionObjectValue($condition, $object); - $test_value = $condition->getValue(); - - $cond = $condition->getFieldCondition(); - - $transcript = new HeraldConditionTranscript(); - $transcript->setRuleID($rule->getID()); - $transcript->setConditionID($condition->getID()); - $transcript->setFieldName($condition->getFieldName()); - $transcript->setCondition($cond); - $transcript->setTestValue($test_value); + $transcript = $this->newConditionTranscript($rule, $condition); try { $result = $object->doesConditionMatch( @@ -351,8 +343,6 @@ final class HeraldEngine extends Phobject { $transcript->setResult($result); - $this->transcript->addConditionTranscript($transcript); - return $result; } @@ -446,4 +436,136 @@ final class HeraldEngine extends Phobject { return false; } + private function newRuleTranscript(HeraldRule $rule) { + $xscript = id(new HeraldRuleTranscript()) + ->setRuleID($rule->getID()) + ->setRuleName($rule->getName()) + ->setRuleOwner($rule->getAuthorPHID()); + + $this->transcript->addRuleTranscript($xscript); + + return $xscript; + } + + private function newConditionTranscript( + HeraldRule $rule, + HeraldCondition $condition) { + + $xscript = id(new HeraldConditionTranscript()) + ->setRuleID($rule->getID()) + ->setConditionID($condition->getID()) + ->setFieldName($condition->getFieldName()) + ->setCondition($condition->getFieldCondition()) + ->setTestValue($condition->getValue()); + + $this->transcript->addConditionTranscript($xscript); + + return $xscript; + } + + private function newApplyTranscript( + HeraldAdapter $adapter, + HeraldRule $rule, + HeraldActionRecord $action) { + + $effect = id(new HeraldEffect()) + ->setObjectPHID($adapter->getPHID()) + ->setAction($action->getAction()) + ->setTarget($action->getTarget()) + ->setRule($rule); + + $xscript = new HeraldApplyTranscript($effect, false); + + $this->transcript->addApplyTranscript($xscript); + + return $xscript; + } + + private function isForbidden( + HeraldRule $rule, + HeraldAdapter $adapter) { + + $forbidden = $adapter->getForbiddenActions(); + if (!$forbidden) { + return false; + } + + $forbidden = array_fuse($forbidden); + + $is_forbidden = false; + + foreach ($rule->getConditions() as $condition) { + $field_key = $condition->getFieldName(); + + if (!isset($this->forbiddenFields[$field_key])) { + $reason = null; + + try { + $states = $adapter->getRequiredFieldStates($field_key); + } catch (Exception $ex) { + $states = array(); + } + + foreach ($states as $state) { + if (!isset($forbidden[$state])) { + continue; + } + $reason = $adapter->getForbiddenReason($state); + break; + } + + $this->forbiddenFields[$field_key] = $reason; + } + + $forbidden_reason = $this->forbiddenFields[$field_key]; + if ($forbidden_reason !== null) { + $this->newConditionTranscript($rule, $condition) + ->setResult(HeraldConditionTranscript::RESULT_FORBIDDEN) + ->setNote($forbidden_reason); + + $is_forbidden = true; + } + } + + foreach ($rule->getActions() as $action_record) { + $action_key = $action_record->getAction(); + + if (!isset($this->forbiddenActions[$action_key])) { + $reason = null; + + try { + $states = $adapter->getRequiredActionStates($action_key); + } catch (Exception $ex) { + $states = array(); + } + + foreach ($states as $state) { + if (!isset($forbidden[$state])) { + continue; + } + $reason = $adapter->getForbiddenReason($state); + break; + } + + $this->forbiddenActions[$action_key] = $reason; + } + + $forbidden_reason = $this->forbiddenActions[$action_key]; + if ($forbidden_reason !== null) { + $this->newApplyTranscript($adapter, $rule, $action_record) + ->setAppliedReason( + array( + array( + 'type' => HeraldAction::DO_STANDARD_FORBIDDEN, + 'data' => $forbidden_reason, + ), + )); + + $is_forbidden = true; + } + } + + return $is_forbidden; + } + } diff --git a/src/applications/herald/field/HeraldField.php b/src/applications/herald/field/HeraldField.php index 2abed0ff1d..408a76f7fb 100644 --- a/src/applications/herald/field/HeraldField.php +++ b/src/applications/herald/field/HeraldField.php @@ -20,6 +20,10 @@ abstract class HeraldField extends Phobject { return null; } + public function getRequiredAdapterStates() { + return array(); + } + protected function getHeraldFieldStandardType() { throw new PhutilMethodNotImplementedException(); } diff --git a/src/applications/herald/state/HeraldBuildableState.php b/src/applications/herald/state/HeraldBuildableState.php new file mode 100644 index 0000000000..d19ae87c36 --- /dev/null +++ b/src/applications/herald/state/HeraldBuildableState.php @@ -0,0 +1,7 @@ +setAncestorClass(__CLASS__) + ->execute(); + } + + final public static function getExplanation($reason) { + $reasons = self::getAllReasons(); + + foreach ($reasons as $reason_implementation) { + $explanation = $reason_implementation->explainReason($reason); + if ($explanation !== null) { + return $explanation; + } + } + + return pht('Unknown reason ("%s").', $reason); + } + +} diff --git a/src/applications/herald/storage/transcript/HeraldConditionTranscript.php b/src/applications/herald/storage/transcript/HeraldConditionTranscript.php index 36a534e6b0..a7096b6880 100644 --- a/src/applications/herald/storage/transcript/HeraldConditionTranscript.php +++ b/src/applications/herald/storage/transcript/HeraldConditionTranscript.php @@ -10,6 +10,8 @@ final class HeraldConditionTranscript extends Phobject { protected $note; protected $result; + const RESULT_FORBIDDEN = 'forbidden'; + public function setRuleID($rule_id) { $this->ruleID = $rule_id; return $this; @@ -72,4 +74,9 @@ final class HeraldConditionTranscript extends Phobject { public function getResult() { return $this->result; } + + public function isForbidden() { + return ($this->getResult() === self::RESULT_FORBIDDEN); + } + } diff --git a/src/applications/herald/storage/transcript/HeraldRuleTranscript.php b/src/applications/herald/storage/transcript/HeraldRuleTranscript.php index d12f1f9dd9..8928a74cf6 100644 --- a/src/applications/herald/storage/transcript/HeraldRuleTranscript.php +++ b/src/applications/herald/storage/transcript/HeraldRuleTranscript.php @@ -9,6 +9,12 @@ final class HeraldRuleTranscript extends Phobject { protected $ruleName; protected $ruleOwner; + const RESULT_FORBIDDEN = 'forbidden'; + + public function isForbidden() { + return ($this->getResult() === self::RESULT_FORBIDDEN); + } + public function setResult($result) { $this->result = $result; return $this; diff --git a/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php b/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php index 2b29fe2443..74fb879fe7 100644 --- a/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php +++ b/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php @@ -6,6 +6,12 @@ abstract class PhabricatorMetaMTAEmailHeraldAction const DO_SEND = 'do.send'; const DO_FORCE = 'do.force'; + public function getRequiredAdapterStates() { + return array( + HeraldMailableState::STATECONST, + ); + } + public function supportsObject($object) { // NOTE: This implementation lacks generality, but there's no great way to // figure out if something generates email right now. From 0da3f34728e313b0a2074984911cf320ef82c106 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 Oct 2017 11:27:53 -0700 Subject: [PATCH 14/30] Provide "differential.diff.search" Summary: See PHI90. For now, this only provides a limited amount of information, but should satisfy the use case in PHI90 and build toward a more complete version in the future. Test Plan: Used new Conduit method to retrieve information about diffs. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18744 --- src/__phutil_library_map__.php | 5 ++ ...DifferentialDiffSearchConduitAPIMethod.php | 18 +++++ ...DifferentialQueryDiffsConduitAPIMethod.php | 10 +++ .../query/DifferentialDiffQuery.php | 25 ++++++ .../query/DifferentialDiffSearchEngine.php | 79 ++++++++++++++++++ .../differential/storage/DifferentialDiff.php | 81 ++++++++++++++++++- 6 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 src/applications/differential/conduit/DifferentialDiffSearchConduitAPIMethod.php create mode 100644 src/applications/differential/query/DifferentialDiffSearchEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2040568496..ff1f00c821 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -439,6 +439,8 @@ phutil_register_library_map(array( 'DifferentialDiffQuery' => 'applications/differential/query/DifferentialDiffQuery.php', 'DifferentialDiffRepositoryHeraldField' => 'applications/differential/herald/DifferentialDiffRepositoryHeraldField.php', 'DifferentialDiffRepositoryProjectsHeraldField' => 'applications/differential/herald/DifferentialDiffRepositoryProjectsHeraldField.php', + 'DifferentialDiffSearchConduitAPIMethod' => 'applications/differential/conduit/DifferentialDiffSearchConduitAPIMethod.php', + 'DifferentialDiffSearchEngine' => 'applications/differential/query/DifferentialDiffSearchEngine.php', 'DifferentialDiffTestCase' => 'applications/differential/storage/__tests__/DifferentialDiffTestCase.php', 'DifferentialDiffTransaction' => 'applications/differential/storage/DifferentialDiffTransaction.php', 'DifferentialDiffTransactionQuery' => 'applications/differential/query/DifferentialDiffTransactionQuery.php', @@ -5435,6 +5437,7 @@ phutil_register_library_map(array( 'HarbormasterBuildkiteBuildableInterface', 'PhabricatorApplicationTransactionInterface', 'PhabricatorDestructibleInterface', + 'PhabricatorConduitResultInterface', ), 'DifferentialDiffAffectedFilesHeraldField' => 'DifferentialDiffHeraldField', 'DifferentialDiffAuthorHeraldField' => 'DifferentialDiffHeraldField', @@ -5453,6 +5456,8 @@ phutil_register_library_map(array( 'DifferentialDiffQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialDiffRepositoryHeraldField' => 'DifferentialDiffHeraldField', 'DifferentialDiffRepositoryProjectsHeraldField' => 'DifferentialDiffHeraldField', + 'DifferentialDiffSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', + 'DifferentialDiffSearchEngine' => 'PhabricatorApplicationSearchEngine', 'DifferentialDiffTestCase' => 'PhutilTestCase', 'DifferentialDiffTransaction' => 'PhabricatorApplicationTransaction', 'DifferentialDiffTransactionQuery' => 'PhabricatorApplicationTransactionQuery', diff --git a/src/applications/differential/conduit/DifferentialDiffSearchConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialDiffSearchConduitAPIMethod.php new file mode 100644 index 0000000000..d4222510d7 --- /dev/null +++ b/src/applications/differential/conduit/DifferentialDiffSearchConduitAPIMethod.php @@ -0,0 +1,18 @@ + 'optional list', diff --git a/src/applications/differential/query/DifferentialDiffQuery.php b/src/applications/differential/query/DifferentialDiffQuery.php index f779e40c26..3b1dd3ae62 100644 --- a/src/applications/differential/query/DifferentialDiffQuery.php +++ b/src/applications/differential/query/DifferentialDiffQuery.php @@ -6,6 +6,7 @@ final class DifferentialDiffQuery private $ids; private $phids; private $revisionIDs; + private $revisionPHIDs; private $commitPHIDs; private $hasRevision; @@ -27,6 +28,11 @@ final class DifferentialDiffQuery return $this; } + public function withRevisionPHIDs(array $revision_phids) { + $this->revisionPHIDs = $revision_phids; + return $this; + } + public function withCommitPHIDs(array $phids) { $this->commitPHIDs = $phids; return $this; @@ -160,6 +166,25 @@ final class DifferentialDiffQuery } } + if ($this->revisionPHIDs !== null) { + $viewer = $this->getViewer(); + + $revisions = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->setParentQuery($this) + ->withPHIDs($this->revisionPHIDs) + ->execute(); + $revision_ids = mpull($revisions, 'getID'); + if (!$revision_ids) { + throw new PhabricatorEmptyQueryException(); + } + + $where[] = qsprintf( + $conn, + 'revisionID IN (%Ls)', + $revision_ids); + } + return $where; } diff --git a/src/applications/differential/query/DifferentialDiffSearchEngine.php b/src/applications/differential/query/DifferentialDiffSearchEngine.php new file mode 100644 index 0000000000..89feeb5c3e --- /dev/null +++ b/src/applications/differential/query/DifferentialDiffSearchEngine.php @@ -0,0 +1,79 @@ +newQuery(); + + if ($map['revisionPHIDs']) { + $query->withRevisionPHIDs($map['revisionPHIDs']); + } + + return $query; + } + + protected function buildCustomSearchFields() { + return array( + id(new PhabricatorPHIDsSearchField()) + ->setLabel(pht('Revisions')) + ->setKey('revisionPHIDs') + ->setAliases(array('revision', 'revisions', 'revisionPHID')) + ->setDescription( + pht('Find diffs attached to a particular revision.')), + ); + } + + protected function getURI($path) { + return '/differential/diff/'.$path; + } + + protected function getBuiltinQueryNames() { + $names = array(); + + $names['all'] = pht('All Diffs'); + + return $names; + } + + public function buildSavedQueryFromBuiltin($query_key) { + $query = $this->newSavedQuery(); + $query->setQueryKey($query_key); + + $viewer = $this->requireViewer(); + + switch ($query_key) { + case 'all': + return $query; + } + + return parent::buildSavedQueryFromBuiltin($query_key); + } + + protected function renderResultList( + array $revisions, + PhabricatorSavedQuery $query, + array $handles) { + assert_instances_of($revisions, 'DifferentialDiff'); + + $viewer = $this->requireViewer(); + + // NOTE: This is only exposed to Conduit, so we don't currently render + // results. + + return id(new PhabricatorApplicationSearchResultView()); + } + +} diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index f7e85cd835..d70cdbbdf5 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -9,7 +9,8 @@ final class DifferentialDiff HarbormasterCircleCIBuildableInterface, HarbormasterBuildkiteBuildableInterface, PhabricatorApplicationTransactionInterface, - PhabricatorDestructibleInterface { + PhabricatorDestructibleInterface, + PhabricatorConduitResultInterface { protected $revisionID; protected $authorPHID; @@ -740,4 +741,82 @@ final class DifferentialDiff $this->saveTransaction(); } + +/* -( PhabricatorConduitResultInterface )---------------------------------- */ + + + public function getFieldSpecificationsForConduit() { + return array( + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('revisionPHID') + ->setType('phid') + ->setDescription(pht('Associated revision PHID.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('authorPHID') + ->setType('phid') + ->setDescription(pht('Revision author PHID.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('repositoryPHID') + ->setType('phid') + ->setDescription(pht('Associated repository PHID.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('refs') + ->setType('map') + ->setDescription(pht('List of related VCS references.')), + ); + } + + public function getFieldValuesForConduit() { + $refs = array(); + + $branch = $this->getBranch(); + if (strlen($branch)) { + $refs[] = array( + 'type' => 'branch', + 'name' => $branch, + ); + } + + $onto = $this->loadTargetBranch(); + if (strlen($onto)) { + $refs[] = array( + 'type' => 'onto', + 'name' => $onto, + ); + } + + $base = $this->getSourceControlBaseRevision(); + if (strlen($base)) { + $refs[] = array( + 'type' => 'base', + 'identifier' => $base, + ); + } + + $bookmark = $this->getBookmark(); + if (strlen($bookmark)) { + $refs[] = array( + 'type' => 'bookmark', + 'name' => $bookmark, + ); + } + + $revision_phid = null; + if ($this->getRevisionID()) { + $revision_phid = $this->getRevision()->getPHID(); + } + + return array( + 'revisionPHID' => $revision_phid, + 'authorPHID' => $this->getAuthorPHID(), + 'repositoryPHID' => $this->getRepositoryPHID(), + 'refs' => $refs, + ); + } + + public function getConduitSearchAttachments() { + return array(); + } + + } From f5336cd6e760e6089f009130bb25f2a457c50c4e Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 Oct 2017 10:00:34 -0700 Subject: [PATCH 15/30] Return transactions from "differential.parsecommitmessage" Summary: Depends on D18740. Prepares `arc` to receive a `--draft` flag by letting us switch to "differential.revision.edit" instead of "differential.createrevision". To "differential.revision.edit", we need a transaction list, but we can't automatically construct this list from a field map. Return the transaction list alongside the field map. The next change uses this list (if available) to switch us to the modern API method. Test Plan: Ran `arc diff` on the experiemntal branch with followup changes, got a new revision. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18741 --- ...tialParseCommitMessageConduitAPIMethod.php | 2 ++ .../DifferentialCommitMessageParser.php | 23 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php index 59401862f0..d1ad6b67e7 100644 --- a/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php @@ -36,6 +36,7 @@ final class DifferentialParseCommitMessageConduitAPIMethod $field_map = $parser->parseFields($corpus); $errors = $parser->getErrors(); + $xactions = $parser->getTransactions(); $revision_id_value = idx( $field_map, @@ -49,6 +50,7 @@ final class DifferentialParseCommitMessageConduitAPIMethod 'value' => $revision_id_value, 'validDomain' => $revision_id_valid_domain, ), + 'transactions' => $xactions, ); } diff --git a/src/applications/differential/parser/DifferentialCommitMessageParser.php b/src/applications/differential/parser/DifferentialCommitMessageParser.php index 7e5a50e61f..3a73428e4b 100644 --- a/src/applications/differential/parser/DifferentialCommitMessageParser.php +++ b/src/applications/differential/parser/DifferentialCommitMessageParser.php @@ -28,6 +28,7 @@ final class DifferentialCommitMessageParser extends Phobject { private $errors; private $commitMessageFields; private $raiseMissingFieldErrors = true; + private $xactions; public static function newStandardParser(PhabricatorUser $viewer) { $key_title = DifferentialTitleCommitMessageField::FIELDKEY; @@ -134,6 +135,7 @@ final class DifferentialCommitMessageParser extends Phobject { */ public function parseCorpus($corpus) { $this->errors = array(); + $this->xactions = array(); $label_map = $this->getLabelMap(); $key_title = $this->titleKey; @@ -284,12 +286,25 @@ final class DifferentialCommitMessageParser extends Phobject { try { $result = $field->parseFieldValue($text_value); $result_map[$field_key] = $result; + + try { + $xactions = $field->getFieldTransactions($result); + foreach ($xactions as $xaction) { + $this->xactions[] = $xaction; + } + } catch (Exception $ex) { + $this->errors[] = pht( + 'Error extracting field transactions from "%s": %s', + $field->getFieldName(), + $ex->getMessage()); + } } catch (DifferentialFieldParseException $ex) { $this->errors[] = pht( 'Error parsing field "%s": %s', $field->getFieldName(), $ex->getMessage()); } + } if ($this->getRaiseMissingFieldErrors()) { @@ -317,6 +332,14 @@ final class DifferentialCommitMessageParser extends Phobject { } + /** + * @task parse + */ + public function getTransactions() { + return $this->xactions; + } + + /* -( Support Methods )---------------------------------------------------- */ From 28cec2f8a2bf96df572c04360c412c8436356421 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 Oct 2017 08:55:56 -0700 Subject: [PATCH 16/30] Allow revisions to be held as drafts, even after builds finish Summary: Ref T2543. Instead of autosubmitting revisions to "Needs Review" when builds finish, allow them to be held in "Draft" indefinitely. There's currently no UI for this. I plan to just expose it as `arc diff --draft` for now, in a followup change. Test Plan: - Created a revision (via Conduit) with "hold as draft", saw it hold as draft after builds finished. - Created a revision (normally), saw it autosubmit after builds finished. - Requested review of a "hold as draft" revision to kick it out of draft state. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18737 --- src/__phutil_library_map__.php | 2 + .../customfield/DifferentialDraftField.php | 6 +++ .../editor/DifferentialRevisionEditEngine.php | 16 +++++++ .../editor/DifferentialTransactionEditor.php | 2 +- .../storage/DifferentialRevision.php | 9 ++++ ...fferentialRevisionHoldDraftTransaction.php | 47 +++++++++++++++++++ 6 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 src/applications/differential/xaction/DifferentialRevisionHoldDraftTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ff1f00c821..7180a7ae04 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -550,6 +550,7 @@ phutil_register_library_map(array( 'DifferentialRevisionHasTaskRelationship' => 'applications/differential/relationships/DifferentialRevisionHasTaskRelationship.php', 'DifferentialRevisionHeraldField' => 'applications/differential/herald/DifferentialRevisionHeraldField.php', 'DifferentialRevisionHeraldFieldGroup' => 'applications/differential/herald/DifferentialRevisionHeraldFieldGroup.php', + 'DifferentialRevisionHoldDraftTransaction' => 'applications/differential/xaction/DifferentialRevisionHoldDraftTransaction.php', 'DifferentialRevisionIDCommitMessageField' => 'applications/differential/field/DifferentialRevisionIDCommitMessageField.php', 'DifferentialRevisionInlineTransaction' => 'applications/differential/xaction/DifferentialRevisionInlineTransaction.php', 'DifferentialRevisionInlinesController' => 'applications/differential/controller/DifferentialRevisionInlinesController.php', @@ -5592,6 +5593,7 @@ phutil_register_library_map(array( 'DifferentialRevisionHasTaskRelationship' => 'DifferentialRevisionRelationship', 'DifferentialRevisionHeraldField' => 'HeraldField', 'DifferentialRevisionHeraldFieldGroup' => 'HeraldFieldGroup', + 'DifferentialRevisionHoldDraftTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionIDCommitMessageField' => 'DifferentialCommitMessageField', 'DifferentialRevisionInlineTransaction' => 'PhabricatorModularTransactionType', 'DifferentialRevisionInlinesController' => 'DifferentialController', diff --git a/src/applications/differential/customfield/DifferentialDraftField.php b/src/applications/differential/customfield/DifferentialDraftField.php index e7ed2bedb2..5d625e3ce9 100644 --- a/src/applications/differential/customfield/DifferentialDraftField.php +++ b/src/applications/differential/customfield/DifferentialDraftField.php @@ -36,6 +36,12 @@ final class DifferentialDraftField return array(); } + // If the author has held this revision as a draft explicitly, don't + // show any misleading messages about it autosubmitting later. + if ($revision->getHoldAsDraft()) { + return array(); + } + $warnings = array(); $blocking_map = array( diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php index 9aad031a7c..2bd3a5cdc3 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -235,6 +235,22 @@ final class DifferentialRevisionEditEngine $fields[] = $action->newEditField($object, $viewer); } + $fields[] = id(new PhabricatorBoolEditField()) + ->setKey('draft') + ->setLabel(pht('Hold as Draft')) + ->setIsConduitOnly(true) + ->setOptions( + pht('Autosubmit Once Builds Finish'), + pht('Hold as Draft')) + ->setTransactionType( + DifferentialRevisionHoldDraftTransaction::TRANSACTIONTYPE) + ->setDescription(pht('Hold revision as as draft.')) + ->setConduitDescription( + pht( + 'Change autosubmission from draft state after builds finish.')) + ->setConduitTypeDescription(pht('New "Hold as Draft" setting.')) + ->setValue($object->getHoldAsDraft()); + return $fields; } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 9ab30c3f06..edc8fd0ad3 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1540,7 +1540,7 @@ final class DifferentialTransactionEditor protected function didApplyTransactions($object, array $xactions) { // If a draft revision has no outstanding builds and we're automatically // making drafts public after builds finish, make the revision public. - $auto_undraft = true; + $auto_undraft = !$object->getHoldAsDraft(); if ($object->isDraft() && $auto_undraft) { $active_builds = $this->hasActiveBuilds($object); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index da7e18ce05..467e503f6f 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -57,6 +57,7 @@ final class DifferentialRevision extends DifferentialDAO const RELATION_SUBSCRIBED = 'subd'; const PROPERTY_CLOSED_FROM_ACCEPTED = 'wasAcceptedBeforeClose'; + const PROPERTY_DRAFT_HOLD = 'draft.hold'; public static function initializeNewRevision(PhabricatorUser $actor) { $app = id(new PhabricatorApplicationQuery()) @@ -708,6 +709,14 @@ final class DifferentialRevision extends DifferentialDAO return false; } + public function getHoldAsDraft() { + return $this->getProperty(self::PROPERTY_DRAFT_HOLD, false); + } + + public function setHoldAsDraft($hold) { + return $this->setProperty(self::PROPERTY_DRAFT_HOLD, $hold); + } + public function loadActiveBuilds(PhabricatorUser $viewer) { $diff = $this->getActiveDiff(); diff --git a/src/applications/differential/xaction/DifferentialRevisionHoldDraftTransaction.php b/src/applications/differential/xaction/DifferentialRevisionHoldDraftTransaction.php new file mode 100644 index 0000000000..5bc257ab62 --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionHoldDraftTransaction.php @@ -0,0 +1,47 @@ +getHoldAsDraft(); + } + + public function generateNewValue($object, $value) { + return (bool)$value; + } + + public function applyInternalEffects($object, $value) { + $object->setHoldAsDraft($value); + } + + public function getTitle() { + if ($this->getNewValue()) { + return pht( + '%s held this revision as a draft.', + $this->renderAuthor()); + } else { + return pht( + '%s set this revision to automatically submit once builds complete.', + $this->renderAuthor()); + } + } + + public function getTitleForFeed() { + if ($this->getNewValue()) { + return pht( + '%s held %s as a draft.', + $this->renderAuthor(), + $this->renderObject()); + } else { + return pht( + '%s set %s to automatically submit once builds complete.', + $this->renderAuthor(), + $this->renderObject()); + } + } + +} From 7fa0d066bc459e47e7381476c6fd708355cb9104 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 Oct 2017 10:29:33 -0700 Subject: [PATCH 17/30] Don't run Herald build rules when Differential revisions are updated automatically Summary: Ref T2543. After D18731, Herald build rules run more often, but now incorrectly try to run builds when Diffusion closes a revision because a commit landed. Test Plan: Made some mundane updates locally; this is tricky to test comprehensively locally so I'm mostly planning to just push it to `secure`. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18745 --- .../editor/DifferentialTransactionEditor.php | 24 +++++++++++++++---- .../herald/DifferentialHeraldStateReasons.php | 4 ++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index edc8fd0ad3..70c3272a83 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1204,16 +1204,32 @@ final class DifferentialTransactionEditor // edited the title or changed subscribers), prevent "Run build plan" // and other similar rules from acting yet, since the build results will // not (or, at least, should not) change unless the actual source changes. + // We also don't run Differential builds if the update was caused by + // discovering a commit, as the expectation is that Diffusion builds take + // over once things land. $has_update = false; + $has_commit = false; + $type_update = DifferentialTransaction::TYPE_UPDATE; foreach ($xactions as $xaction) { - if ($xaction->getTransactionType() == $type_update) { - $has_update = true; - break; + if ($xaction->getTransactionType() != $type_update) { + continue; } + + if ($xaction->getMetadataValue('isCommitUpdate')) { + $has_commit = true; + } else { + $has_update = true; + } + + break; } - if (!$has_update) { + if ($has_commit) { + $adapter->setForbiddenAction( + HeraldBuildableState::STATECONST, + DifferentialHeraldStateReasons::REASON_LANDED); + } else if (!$has_update) { $adapter->setForbiddenAction( HeraldBuildableState::STATECONST, DifferentialHeraldStateReasons::REASON_UNCHANGED); diff --git a/src/applications/differential/herald/DifferentialHeraldStateReasons.php b/src/applications/differential/herald/DifferentialHeraldStateReasons.php index d3259560d5..92d2ca3067 100644 --- a/src/applications/differential/herald/DifferentialHeraldStateReasons.php +++ b/src/applications/differential/herald/DifferentialHeraldStateReasons.php @@ -5,6 +5,7 @@ final class DifferentialHeraldStateReasons const REASON_DRAFT = 'differential.draft'; const REASON_UNCHANGED = 'differential.unchanged'; + const REASON_LANDED = 'differential.landed'; public function explainReason($reason) { $reasons = array( @@ -14,6 +15,9 @@ final class DifferentialHeraldStateReasons self::REASON_UNCHANGED => pht( 'The update which triggered Herald did not update the diff for '. 'this revision, so builds will not run.'), + self::REASON_LANDED => pht( + 'The update which triggered Herald was an automatic update in '. + 'response to discovering a commit, so builds will not run.'), ); return idx($reasons, $reason); From 90d0f8ac6cf5eb12406f330a624bbf87c156f517 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 Oct 2017 10:49:19 -0700 Subject: [PATCH 18/30] Revert changes to Diffusion blame view Summary: Ref PHI174. This reverts most of these changes: - 37843127e94a878a7f5bf2c65c8e7004bc65c68a / D18481 - 94cad30ac3f052a711ececf7e370bf5c0071827f / D18474 - 12ae08b6b1a1b7c330593e76c32817f7cdbc87dd / D18473 - 0a013341721f8b1fc249047fe6db26062138b562 / D18462 - ac91ab1ef9196eee0deabfd70157ccc0d53d666e / D18452 These changes made the Diffusion blame view very similar to GitHub's blame view. See D18452 for a before/after of the bulk of these changes; the other revisions are bugfixes. I think this was generally a step backward, and not motivated by solving a specific problem. I've found the new UI less usable than the old one, and at least one install (see PHI174) also has. In particular, the revision/commit titles are very bulky and not terribly useful; the date column also isn't terribly useful; the "age" color actually IS pretty useful and was heavily de-emphasized. I've kept one bugfix here (missing `'a'` tag type) and kept the upgraded icon for "Skip Past This Commit". I'm going to follow this up with some additional changes: - Show a small author profile icon, similar to GitHub, to address PHI174 more directly. - Try a zebra-stripe on blocks of rows to make it more clear where changes affected by a particular commit begin and end. - Try a hue shift, not just a brightness/saturation shift, to make the "age" color more distinct. - Try computing colors as even steps, not based purely on age. Currently, if a file has one long-distant commit and several recent commits, all the recent ones show up as very bright green. I think this would probably be more useful if they were distributed more evenly across the available color bands. Test Plan: Viewed blame views in Diffusion, saw a more compact UI similar to the old UI. {F5251019} Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18746 --- resources/celerity/map.php | 4 +- .../controller/DiffusionBrowseController.php | 109 +++++++++++------- .../diffusion/diffusion-source.css | 59 +++------- 3 files changed, 88 insertions(+), 84 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index ce069dbd11..517827a900 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -73,7 +73,7 @@ return array( 'rsrc/css/application/diffusion/diffusion-icons.css' => '0c15255e', 'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec', - 'rsrc/css/application/diffusion/diffusion-source.css' => '69ac9399', + 'rsrc/css/application/diffusion/diffusion-source.css' => '3a1056d8', 'rsrc/css/application/diffusion/diffusion.css' => '45727264', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', @@ -572,7 +572,7 @@ return array( 'diffusion-icons-css' => '0c15255e', 'diffusion-readme-css' => '419dd5b6', 'diffusion-repository-css' => 'ee6f20ec', - 'diffusion-source-css' => '69ac9399', + 'diffusion-source-css' => '3a1056d8', 'diviner-shared-css' => '896f1d43', 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index b4c0352d53..be9d4fbb9b 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1127,6 +1127,10 @@ final class DiffusionBrowseController extends DiffusionController { )); } + $skip_text = pht('Skip Past This Commit'); + $skip_icon = id(new PHUIIconView()) + ->setIcon('fa-caret-square-o-left'); + foreach ($display as $line_index => $line) { $row = array(); @@ -1142,14 +1146,12 @@ final class DiffusionBrowseController extends DiffusionController { $revision_link = null; $commit_link = null; $before_link = null; - $commit_date = null; - $style = 'border-right: 3px solid '.$line['color'].';'; + $style = 'background: '.$line['color'].';'; if ($identifier && !$line['duplicate']) { if (isset($commit_links[$identifier])) { - $commit_link = $commit_links[$identifier]['link']; - $commit_date = $commit_links[$identifier]['date']; + $commit_link = $commit_links[$identifier]; } if (isset($revision_map[$identifier])) { @@ -1160,10 +1162,6 @@ final class DiffusionBrowseController extends DiffusionController { } $skip_href = $line_href.'?before='.$identifier.'&view=blame'; - $skip_text = pht('Skip Past This Commit'); - $icon = id(new PHUIIconView()) - ->setIcon('fa-caret-square-o-left'); - $before_link = javelin_tag( 'a', array( @@ -1175,7 +1173,7 @@ final class DiffusionBrowseController extends DiffusionController { 'size' => 300, ), ), - $icon); + $skip_icon); } if ($show_blame) { @@ -1186,41 +1184,33 @@ final class DiffusionBrowseController extends DiffusionController { ), $before_link); - $row[] = phutil_tag( - 'th', - array( - 'class' => 'diffusion-rev-link', - ), - $commit_link); - - if ($revision_map) { - $row[] = phutil_tag( - 'th', - array( - 'class' => 'diffusion-blame-revision', - ), - $revision_link); + $object_links = array(); + $object_links[] = $commit_link; + if ($revision_link) { + $object_links[] = phutil_tag('span', array(), '/'); + $object_links[] = $revision_link; } $row[] = phutil_tag( 'th', array( - 'class' => 'diffusion-blame-date', + 'class' => 'diffusion-rev-link', ), - $commit_date); + $object_links); } $line_link = phutil_tag( 'a', array( 'href' => $line_href, + 'style' => $style, ), $line_number); $row[] = javelin_tag( 'th', array( - 'class' => 'diffusion-line-link ', + 'class' => 'diffusion-line-link', 'sigil' => 'phabricator-source-line', 'style' => $style, ), @@ -1536,6 +1526,33 @@ final class DiffusionBrowseController extends DiffusionController { return head($parents); } + private function renderRevisionTooltip( + DifferentialRevision $revision, + $handles) { + $viewer = $this->getRequest()->getUser(); + + $date = phabricator_date($revision->getDateModified(), $viewer); + $id = $revision->getID(); + $title = $revision->getTitle(); + $header = "D{$id} {$title}"; + + $author = $handles[$revision->getAuthorPHID()]->getName(); + + return "{$header}\n{$date} \xC2\xB7 {$author}"; + } + + private function renderCommitTooltip( + PhabricatorRepositoryCommit $commit, + $author) { + + $viewer = $this->getRequest()->getUser(); + + $date = phabricator_date($commit->getEpoch(), $viewer); + $summary = trim($commit->getSummary()); + + return "{$summary}\n{$date} \xC2\xB7 {$author}"; + } + protected function markupText($text) { $engine = PhabricatorMarkupEngine::newDiffusionMarkupEngine(); $engine->setConfig('viewer', $this->getRequest()->getUser()); @@ -1743,6 +1760,9 @@ final class DiffusionBrowseController extends DiffusionController { ->setViewer($viewer) ->withRepository($repository) ->withIdentifiers($identifiers) + // TODO: We only fetch this to improve author display behavior, but + // shouldn't really need to? + ->needCommitData(true) ->execute(); $commits = mpull($commits, null, 'getCommitIdentifier'); } else { @@ -1754,27 +1774,25 @@ final class DiffusionBrowseController extends DiffusionController { private function renderCommitLinks(array $commits, $handles) { $links = array(); - $viewer = $this->getViewer(); foreach ($commits as $identifier => $commit) { - $date = phabricator_date($commit->getEpoch(), $viewer); - $summary = trim($commit->getSummary()); + $tooltip = $this->renderCommitTooltip( + $commit, + $commit->renderAuthorShortName($handles)); - $commit_link = phutil_tag( + $commit_link = javelin_tag( 'a', array( 'href' => $commit->getURI(), + 'sigil' => 'has-tooltip', + 'meta' => array( + 'tip' => $tooltip, + 'align' => 'E', + 'size' => 600, + ), ), - $summary); + $commit->getLocalName()); - $commit_date = phutil_tag( - 'a', - array( - 'href' => $commit->getURI(), - ), - $date); - - $links[$identifier]['link'] = $commit_link; - $links[$identifier]['date'] = $commit_date; + $links[$identifier] = $commit_link; } return $links; @@ -1785,10 +1803,19 @@ final class DiffusionBrowseController extends DiffusionController { foreach ($revisions as $revision) { $revision_id = $revision->getID(); - $revision_link = phutil_tag( + + $tooltip = $this->renderRevisionTooltip($revision, $handles); + + $revision_link = javelin_tag( 'a', array( 'href' => '/'.$revision->getMonogram(), + 'sigil' => 'has-tooltip', + 'meta' => array( + 'tip' => $tooltip, + 'align' => 'E', + 'size' => 600, + ), ), $revision->getMonogram()); diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index 3b8b9a8a16..9ade2d3022 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -13,22 +13,24 @@ -webkit-overflow-scrolling: touch; } -.diffusion-source tr.phabricator-source-highlight th, -.diffusion-source tr.phabricator-source-highlight td { - background: {$gentle.highlight}; +.diffusion-source tr.phabricator-source-highlight { + background: {$sh-yellowbackground}; } .diffusion-source th { text-align: right; vertical-align: top; - color: {$darkbluetext}; + background: {$lightgreybackground}; + color: {$bluetext}; border-right: 1px solid {$thinblueborder}; } .diffusion-source td { vertical-align: top; white-space: pre-wrap; - padding: 3px 12px; + padding-top: 1px; + padding-bottom: 1px; + padding-left: 8px; width: 100%; word-break: break-all; } @@ -43,18 +45,12 @@ } .diffusion-blame-link, -.diffusion-rev-link, -.diffusion-blame-date { +.diffusion-rev-link { white-space: nowrap; } -.diffusion-blame-date, -.diffusion-blame-link, -.diffusion-blame-revision, -.diffusion-rev-link { - background: {$lightgreybackground}; - font: {$basefont}; - font-size: {$smallerfontsize}; +.diffusion-blame-link { + min-width: 28px; } .diffusion-source th.diffusion-rev-link { @@ -62,23 +58,16 @@ min-width: 130px; } -.diffusion-source a { +.diffusion-blame-link a, +.diffusion-rev-link a, +.diffusion-line-link a { color: {$darkbluetext}; } -.diffusion-rev-link a { - max-width: 300px; - overflow: hidden; - white-space: nowrap; - text-overflow: ellipsis; - margin: 3px 8px; - display: block; -} - -.diffusion-blame-date a, -.diffusion-blame-revision a { - float: right; - margin: 3px 8px; +.diffusion-rev-link a, +.diffusion-rev-link span { + margin: 2px 8px 0; + display: inline-block; } .diffusion-rev-link span { @@ -91,19 +80,7 @@ .diffusion-line-link a { /* Give the user a larger click target. */ display: block; - padding: 4px 8px 3px; -} - -.diffusion-line-link a { - color: {$lightgreytext}; -} - -.diffusion-blame-link a .phui-icon-view { - color: {$bluetext}; -} - -.diffusion-blame-link a:hover .phui-icon-view { - color: {$sky}; + padding: 2px 8px; } .diffusion-line-link { From bde71324f82ba0eaeabeeff8eb6db84505b1614f Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 Oct 2017 11:28:31 -0700 Subject: [PATCH 19/30] Show small author portraits in Diffusion blame view Summary: Depends on D18746. See PHI174. Adds small author portraits next to each blame line (this is similar to GitHub). Test Plan: My local test data isn't that great since I don't have commits from a lot of accounts, but looks functional: {F5251056} Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18747 --- resources/celerity/map.php | 4 +- .../controller/DiffusionBrowseController.php | 49 +++++++++++++++++++ .../diffusion/diffusion-source.css | 12 ++++- 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 517827a900..b7cb976a16 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -73,7 +73,7 @@ return array( 'rsrc/css/application/diffusion/diffusion-icons.css' => '0c15255e', 'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec', - 'rsrc/css/application/diffusion/diffusion-source.css' => '3a1056d8', + 'rsrc/css/application/diffusion/diffusion-source.css' => 'd96b3f9f', 'rsrc/css/application/diffusion/diffusion.css' => '45727264', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', @@ -572,7 +572,7 @@ return array( 'diffusion-icons-css' => '0c15255e', 'diffusion-readme-css' => '419dd5b6', 'diffusion-repository-css' => 'ee6f20ec', - 'diffusion-source-css' => '3a1056d8', + 'diffusion-source-css' => 'd96b3f9f', 'diviner-shared-css' => '896f1d43', 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index be9d4fbb9b..f4bd2bdebf 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -969,6 +969,24 @@ final class DiffusionBrowseController extends DiffusionController { $handles = $viewer->loadHandles($phids); + $author_phids = array(); + $author_map = array(); + foreach ($blame_commits as $commit) { + $commit_identifier = $commit->getCommitIdentifier(); + + $author_phid = ''; + if (isset($revision_map[$commit_identifier])) { + $revision_id = $revision_map[$commit_identifier]; + $revision = $revisions[$revision_id]; + $author_phid = $revision->getAuthorPHID(); + } else { + $author_phid = $commit->getAuthorPHID(); + } + + $author_map[$commit_identifier] = $author_phid; + $author_phids[$author_phid] = $author_phid; + } + $colors = array(); if ($blame_commits) { $epochs = array(); @@ -1113,6 +1131,7 @@ final class DiffusionBrowseController extends DiffusionController { // blame outputs. $commit_links = $this->renderCommitLinks($blame_commits, $handles); $revision_links = $this->renderRevisionLinks($revisions, $handles); + $author_links = $this->renderAuthorLinks($author_map, $handles); if ($this->coverage) { require_celerity_resource('differential-changeset-view-css'); @@ -1145,6 +1164,7 @@ final class DiffusionBrowseController extends DiffusionController { $revision_link = null; $commit_link = null; + $author_link = null; $before_link = null; $style = 'background: '.$line['color'].';'; @@ -1152,6 +1172,7 @@ final class DiffusionBrowseController extends DiffusionController { if ($identifier && !$line['duplicate']) { if (isset($commit_links[$identifier])) { $commit_link = $commit_links[$identifier]; + $author_link = $author_links[$author_map[$identifier]]; } if (isset($revision_map[$identifier])) { @@ -1185,6 +1206,7 @@ final class DiffusionBrowseController extends DiffusionController { $before_link); $object_links = array(); + $object_links[] = $author_link; $object_links[] = $commit_link; if ($revision_link) { $object_links[] = phutil_tag('span', array(), '/'); @@ -1772,6 +1794,33 @@ final class DiffusionBrowseController extends DiffusionController { return array($identifiers, $commits); } + private function renderAuthorLinks(array $authors, $handles) { + $links = array(); + + foreach ($authors as $phid) { + if (!strlen($phid)) { + // This means we couldn't identify an author for the commit or the + // revision. We just render a blank for alignment. + $style = null; + $href = null; + } else { + $src = $handles[$phid]->getImageURI(); + $style = 'background-image: url('.$src.');'; + $href = $handles[$phid]->getURI(); + } + + $links[$phid] = javelin_tag( + $href ? 'a' : 'span', + array( + 'class' => 'diffusion-author-link', + 'style' => $style, + 'href' => $href, + )); + } + + return $links; + } + private function renderCommitLinks(array $commits, $handles) { $links = array(); foreach ($commits as $identifier => $commit) { diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index 9ade2d3022..06e7d74477 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -66,7 +66,7 @@ .diffusion-rev-link a, .diffusion-rev-link span { - margin: 2px 8px 0; + margin: 0 8px 0 0; display: inline-block; } @@ -90,3 +90,13 @@ -ms-user-select: none; user-select: none; } + +.diffusion-rev-link .diffusion-author-link { + display: inline-block; + padding: 0; + margin: 2px 4px -4px 4px; + width: 16px; + height: 16px; + background-size: 100% 100%; + background-repeat: no-repeat; +} From a4b934cad2b520fb2de57a5f08eeffa6aff28ffe Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 Oct 2017 11:45:38 -0700 Subject: [PATCH 20/30] Clean up Differential draft mail behaviors Summary: Ref T2543. Fixes two relatively minor things: - When builds finish in Harbormaster, send mail "From" the author. - Set the `firstBroadcast` flag so that initial mail picks up earlier history (notably, the "reviewers" line). For now, I'm not setting `firstBroadcast` on explicit "Request Review" (but maybe we should), and not trying to deal with weird cases where you leave a bunch of comments on a draft. Those might be fine as-is or may get tweaked later. Test Plan: Created a revision with Harbormaster builds, ran builds, saw initial email come "From" the right user with more metadata. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18748 --- .../editor/DifferentialTransactionEditor.php | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 70c3272a83..79a582a43f 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -9,6 +9,7 @@ final class DifferentialTransactionEditor private $didExpandInlineState = false; private $hasReviewTransaction = false; private $affectedPaths; + private $firstBroadcast = false; public function getEditorApplicationClass() { return 'PhabricatorDifferentialApplication'; @@ -27,7 +28,7 @@ final class DifferentialTransactionEditor } public function isFirstBroadcast() { - return $this->getIsNewObject(); + return $this->firstBroadcast; } public function getDiffUpdateTransaction(array $xactions) { @@ -1449,11 +1450,13 @@ final class DifferentialTransactionEditor protected function getCustomWorkerState() { return array( 'changedPriorToCommitURI' => $this->changedPriorToCommitURI, + 'firstBroadcast' => $this->firstBroadcast, ); } protected function loadCustomWorkerState(array $state) { $this->changedPriorToCommitURI = idx($state, 'changedPriorToCommitURI'); + $this->firstBroadcast = idx($state, 'firstBroadcast'); return $this; } @@ -1566,6 +1569,19 @@ final class DifferentialTransactionEditor // natural and more useful. $author_phid = $object->getAuthorPHID(); + // Additionally, we change the acting PHID for the transaction set + // to the author if it isn't already a user so that mail comes from + // the natural author. + $acting_phid = $this->getActingAsPHID(); + $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; + if (phid_get_type($acting_phid) != $user_type) { + $this->setActingAsPHID($author_phid); + } + + // Mark this as the first broadcast we're sending about the revision + // so mail can generate specially. + $this->firstBroadcast = true; + $xaction = $object->getApplicationTransactionTemplate() ->setAuthorPHID($author_phid) ->setTransactionType( From 80ebe401e5849799cbc5568f25cca927b7b1b621 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 Oct 2017 12:23:16 -0700 Subject: [PATCH 21/30] Tweak padding/spacing on Diffusion blame view for profile pictures Summary: Give profile images a little more space, fix "/" spacing, add a tooltip. Test Plan: {F5251205} Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18749 --- resources/celerity/map.php | 4 ++-- .../diffusion/controller/DiffusionBrowseController.php | 5 +++++ .../rsrc/css/application/diffusion/diffusion-source.css | 8 ++++---- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index b7cb976a16..c9e1a5b351 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -73,7 +73,7 @@ return array( 'rsrc/css/application/diffusion/diffusion-icons.css' => '0c15255e', 'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec', - 'rsrc/css/application/diffusion/diffusion-source.css' => 'd96b3f9f', + 'rsrc/css/application/diffusion/diffusion-source.css' => '5f35a3bd', 'rsrc/css/application/diffusion/diffusion.css' => '45727264', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', @@ -572,7 +572,7 @@ return array( 'diffusion-icons-css' => '0c15255e', 'diffusion-readme-css' => '419dd5b6', 'diffusion-repository-css' => 'ee6f20ec', - 'diffusion-source-css' => 'd96b3f9f', + 'diffusion-source-css' => '5f35a3bd', 'diviner-shared-css' => '896f1d43', 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index f4bd2bdebf..4985ac2bb7 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1815,6 +1815,11 @@ final class DiffusionBrowseController extends DiffusionController { 'class' => 'diffusion-author-link', 'style' => $style, 'href' => $href, + 'sigil' => 'has-tooltip', + 'meta' => array( + 'tip' => $handles[$phid]->getName(), + 'align' => 'E', + ), )); } diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index 06e7d74477..a2c67cf16d 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -64,14 +64,14 @@ color: {$darkbluetext}; } -.diffusion-rev-link a, -.diffusion-rev-link span { +.diffusion-rev-link a { margin: 0 8px 0 0; display: inline-block; } .diffusion-rev-link span { - margin-right: -4px; + display: inline-block; + margin-right: 4px; margin-left: -4px; color: {$lightgreytext}; } @@ -94,7 +94,7 @@ .diffusion-rev-link .diffusion-author-link { display: inline-block; padding: 0; - margin: 2px 4px -4px 4px; + margin: 2px 6px -4px 8px; width: 16px; height: 16px; background-size: 100% 100%; From 2a7cdcf740ac4948b25a88fda3db9fb9d3f7337e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 Oct 2017 13:52:23 -0700 Subject: [PATCH 22/30] Fix an issue where the repository symbol index would incorrectly activate inside inline comments Summary: See PHI185. When looking at a revision, you can Command-Click (Mac) symbols to jump to their definitions (provided the symbol index has been built). Currently, the code works on any node inside the changeset list, so it activates when clicking links inside inline comments and opening them in a new window. To avoid this, don't activate if we're inside an inline comment. This technically prevents you from doing a symbol lookup on a symbol inside a codeblock inside an inline, but that seems fine/reasonable. Test Plan: Wrote `Dxxx` in an inline, command-clicked it. Before: got a symbol lookup. After: just a new tab with the revision. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18753 --- resources/celerity/map.php | 18 +++++++++--------- .../repository/repository-crossreference.js | 15 +++++++++++++-- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index c9e1a5b351..30c04ffc92 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '4c79d74f', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', - 'differential.pkg.js' => 'b71b8c5d', + 'differential.pkg.js' => 'ae6460e0', 'diffusion.pkg.css' => 'a2d17c7d', 'diffusion.pkg.js' => '6134c5a1', 'favicon.ico' => '30672e08', @@ -444,7 +444,7 @@ return array( 'rsrc/js/application/releeph/releeph-preview-branch.js' => 'b2b4fbaf', 'rsrc/js/application/releeph/releeph-request-state-change.js' => 'a0b57eb8', 'rsrc/js/application/releeph/releeph-request-typeahead.js' => 'de2e896f', - 'rsrc/js/application/repository/repository-crossreference.js' => 'e5339c43', + 'rsrc/js/application/repository/repository-crossreference.js' => '7fe9bc12', 'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e2e0a072', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08', 'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => '887ad43f', @@ -690,7 +690,7 @@ return array( 'javelin-behavior-reorder-applications' => '76b9fc3e', 'javelin-behavior-reorder-columns' => 'e1d25dfb', 'javelin-behavior-reorder-profile-menu-items' => 'e2e0a072', - 'javelin-behavior-repository-crossreference' => 'e5339c43', + 'javelin-behavior-repository-crossreference' => '7fe9bc12', 'javelin-behavior-scrollbar' => '834a1173', 'javelin-behavior-search-reorder-queries' => 'e9581f08', 'javelin-behavior-select-content' => 'bf5374ef', @@ -1546,6 +1546,12 @@ return array( '7f243deb' => array( 'javelin-install', ), + '7fe9bc12' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-uri', + ), '834a1173' => array( 'javelin-behavior', 'javelin-scrollbar', @@ -2073,12 +2079,6 @@ return array( 'javelin-behavior', 'javelin-dom', ), - 'e5339c43' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-uri', - ), 'e5822781' => array( 'javelin-behavior', 'javelin-dom', diff --git a/webroot/rsrc/js/application/repository/repository-crossreference.js b/webroot/rsrc/js/application/repository/repository-crossreference.js index bfb849c759..138ce4bf3e 100644 --- a/webroot/rsrc/js/application/repository/repository-crossreference.js +++ b/webroot/rsrc/js/application/repository/repository-crossreference.js @@ -46,8 +46,19 @@ JX.behavior('repository-crossreference', function(config, statics) { if (!isSignalkey(e)) { return; } + + var target = e.getTarget(); + + try { + // If we're in an inline comment, don't link symbols. + if (JX.DOM.findAbove(target, 'div', 'differential-inline-comment')) { + return; + } + } catch (ex) { + // Continue if we're not inside an inline comment. + } + if (e.getType() === 'mouseover') { - var target = e.getTarget(); while (target !== document.body) { if (JX.DOM.isNode(target, 'span') && (target.className in class_map)) { @@ -58,7 +69,7 @@ JX.behavior('repository-crossreference', function(config, statics) { target = target.parentNode; } } else if (e.getType() === 'click') { - openSearch(e.getTarget(), lang); + openSearch(target, lang); } }); } From 6d36eb911309d4741ab26c7f7212b642826752b1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Nov 2017 10:42:27 -0700 Subject: [PATCH 23/30] Denormalize Diff PHIDs onto Revisions Summary: Ref T12539. See PHI190. Currently, each Diff has a `revisionID`, but Revisions do not point at the current active diff. To find the active diff for a given revision, we need to issue a separate query. Furthermore, this query is inefficient for bulk loads: if we have a lot of revisions, we end up querying for all diff IDs for all those revisions first, then selecting the largest ones and querying again to get the actual diff objects. This strategy could likely be optimized but the query is a mess in any case. In several cases, it's useful to have the active diff PHID without needing to do a second query -- sometimes for convenience, and sometimes for performance. T12539 is an example of such a case: it would be nice to refine the bucketing logic (which only depends on active diff PHIDs), but it feels bad to make the page heavier to do it. For now, this is unused. I'll start using it to fix the bucketing issue, and then we can expand it gradually to address other performance/convenience issues. Test Plan: - Ran migrations, inspected database, saw sensible values. - Created a new revision, saw a sensible database value. - Updated an existing revision, saw database update properly. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12539 Differential Revision: https://secure.phabricator.com/D18756 --- .../autopatches/20171101.diff.01.active.sql | 2 ++ .../autopatches/20171101.diff.02.populate.php | 24 +++++++++++++++++++ .../editor/DifferentialTransactionEditor.php | 3 +-- .../storage/DifferentialRevision.php | 2 ++ 4 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 resources/sql/autopatches/20171101.diff.01.active.sql create mode 100644 resources/sql/autopatches/20171101.diff.02.populate.php diff --git a/resources/sql/autopatches/20171101.diff.01.active.sql b/resources/sql/autopatches/20171101.diff.01.active.sql new file mode 100644 index 0000000000..aee8c5aa13 --- /dev/null +++ b/resources/sql/autopatches/20171101.diff.01.active.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_revision + ADD activeDiffPHID VARBINARY(64) NOT NULL; diff --git a/resources/sql/autopatches/20171101.diff.02.populate.php b/resources/sql/autopatches/20171101.diff.02.populate.php new file mode 100644 index 0000000000..b41b0aa51d --- /dev/null +++ b/resources/sql/autopatches/20171101.diff.02.populate.php @@ -0,0 +1,24 @@ +establishConnection('w'); +$diff_table = new DifferentialDiff(); + +foreach (new LiskMigrationIterator($table) as $revision) { + $revision_id = $revision->getID(); + + $diff_row = queryfx_one( + $conn, + 'SELECT phid FROM %T WHERE revisionID = %d ORDER BY id DESC LIMIT 1', + $diff_table->getTableName(), + $revision_id); + + if ($diff_row) { + queryfx( + $conn, + 'UPDATE %T SET activeDiffPHID = %s WHERE id = %d', + $table->getTableName(), + $diff_row['phid'], + $revision_id); + } +} diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 79a582a43f..3e6cca1edb 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -138,8 +138,7 @@ final class DifferentialTransactionEditor $object->setRepositoryPHID($diff->getRepositoryPHID()); } $object->attachActiveDiff($diff); - - // TODO: Update the `diffPHID` once we add that. + $object->setActiveDiffPHID($diff->getPHID()); return; } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 467e503f6f..61f934f13b 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -35,6 +35,8 @@ final class DifferentialRevision extends DifferentialDAO protected $mailKey; protected $branchName; protected $repositoryPHID; + protected $activeDiffPHID; + protected $viewPolicy = PhabricatorPolicies::POLICY_USER; protected $editPolicy = PhabricatorPolicies::POLICY_USER; protected $properties = array(); From 6ecdadb76a955854875fa25515fba266bb34cfa9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Nov 2017 10:54:23 -0700 Subject: [PATCH 24/30] After "Request Review", move revisions with voided "Accepts" into "Ready to Review", not "Waiting on Other Reviewers" Summary: Depends on D18756. Fixes T12539. See PHI190. Currently, when this occurs: - Alice accepts. - Bailey requests review. - Alice views her dashboard. ...the revision appears in "Waiting on Other Reviewers" (regardless of whether other reviewers actually exist or not). Instead, ignore these voided/non-current accepts and let the revisions appear in "Ready to Review", which is more natural. Test Plan: Went through the steps above. On `master`, saw revision in "Waiting on Other Reviewers". After patch, saw it in "Ready to Review". Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12539 Differential Revision: https://secure.phabricator.com/D18757 --- ...fferentialRevisionRequiredActionResultBucket.php | 10 +++++++++- .../query/DifferentialRevisionResultBucket.php | 13 ++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php index f3971f8571..5f5f4008db 100644 --- a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php @@ -123,6 +123,14 @@ final class DifferentialRevisionRequiredActionResultBucket $reviewing = array( DifferentialReviewerStatus::STATUS_ADDED, DifferentialReviewerStatus::STATUS_COMMENTED, + + // If an author has used "Request Review" to put an accepted revision + // back into the "Needs Review" state, include "Accepted" reviewers + // whose reviews have been voided in the "Should Review" bucket. + + // If we don't do this, they end up in "Waiting on Other Reviewers", + // even if there are no other reviewers. + DifferentialReviewerStatus::STATUS_ACCEPTED, ); $reviewing = array_fuse($reviewing); @@ -130,7 +138,7 @@ final class DifferentialRevisionRequiredActionResultBucket $results = array(); foreach ($objects as $key => $object) { - if (!$this->hasReviewersWithStatus($object, $phids, $reviewing)) { + if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, false)) { continue; } diff --git a/src/applications/differential/query/DifferentialRevisionResultBucket.php b/src/applications/differential/query/DifferentialRevisionResultBucket.php index 54705649eb..a0769ac349 100644 --- a/src/applications/differential/query/DifferentialRevisionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionResultBucket.php @@ -53,7 +53,8 @@ abstract class DifferentialRevisionResultBucket protected function hasReviewersWithStatus( DifferentialRevision $revision, array $phids, - array $statuses) { + array $statuses, + $current = null) { foreach ($revision->getReviewers() as $reviewer) { $reviewer_phid = $reviewer->getReviewerPHID(); @@ -66,6 +67,16 @@ abstract class DifferentialRevisionResultBucket continue; } + if ($current !== null) { + if ($status == DifferentialReviewerStatus::STATUS_ACCEPTED) { + $diff_phid = $revision->getActiveDiffPHID(); + $is_current = $reviewer->isAccepted($diff_phid); + if ($is_current !== $current) { + continue; + } + } + } + return true; } From 03d059dd26e27077de6c58847926cfe9d2e6590b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Nov 2017 11:08:19 -0700 Subject: [PATCH 25/30] Don't include resigned reviewers in the Differential "To" list Summary: Ref T12689. See PHI178. This isn't a complete solution (you may still get mailed via packages/projects) but should fix the obvious issue, where "Resigned" reviewers are incorrectly always sent mail directly. Test Plan: Had Alice resign, interacted as Bailey, no mail to Alice. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12689 Differential Revision: https://secure.phabricator.com/D18758 --- .../differential/editor/DifferentialTransactionEditor.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 3e6cca1edb..26c440e866 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -630,6 +630,10 @@ final class DifferentialTransactionEditor $phids = array(); $phids[] = $object->getAuthorPHID(); foreach ($object->getReviewers() as $reviewer) { + if ($reviewer->isResigned()) { + continue; + } + $phids[] = $reviewer->getReviewerPHID(); } return $phids; From cb98b60033260998a51821f71add07285fa0d3ef Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Nov 2017 10:25:37 -0800 Subject: [PATCH 26/30] Fill in some straightforward Maniphest transactions for `transaction.search` Summary: See PHI197. Populates "status" transactions and a few other obvious types where there's no security/performance/payload/formatting issue I can come up with. The names here are the same as the names for editing with `maniphest.edit`. Test Plan: Used `transaction.search` to retrieve transactions of all new types. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18761 --- .../DifferentialRevisionStatusTransaction.php | 6 +++--- .../xaction/ManiphestTaskDescriptionTransaction.php | 10 ++++++++++ .../xaction/ManiphestTaskOwnerTransaction.php | 10 ++++++++++ .../xaction/ManiphestTaskPointsTransaction.php | 12 ++++++++++++ .../xaction/ManiphestTaskStatusTransaction.php | 11 +++++++++++ .../xaction/ManiphestTaskTitleTransaction.php | 11 +++++++++++ 6 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/applications/differential/xaction/DifferentialRevisionStatusTransaction.php b/src/applications/differential/xaction/DifferentialRevisionStatusTransaction.php index 615ce38bcf..9c8b3767df 100644 --- a/src/applications/differential/xaction/DifferentialRevisionStatusTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionStatusTransaction.php @@ -74,10 +74,10 @@ final class DifferentialRevisionStatusTransaction return 'status'; } - public function getFieldValuesForConduit($object, $data) { + public function getFieldValuesForConduit($xaction, $data) { return array( - 'old' => $object->getOldValue(), - 'new' => $object->getNewValue(), + 'old' => $xaction->getOldValue(), + 'new' => $xaction->getNewValue(), ); } diff --git a/src/applications/maniphest/xaction/ManiphestTaskDescriptionTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskDescriptionTransaction.php index 009327ed9b..fec1a87604 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskDescriptionTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskDescriptionTransaction.php @@ -57,5 +57,15 @@ final class ManiphestTaskDescriptionTransaction return $changes; } + public function getTransactionTypeForConduit($xaction) { + return 'description'; + } + + public function getFieldValuesForConduit($xaction, $data) { + return array( + 'old' => $xaction->getOldValue(), + 'new' => $xaction->getNewValue(), + ); + } } diff --git a/src/applications/maniphest/xaction/ManiphestTaskOwnerTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskOwnerTransaction.php index d510fe8fbc..caaf84f542 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskOwnerTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskOwnerTransaction.php @@ -154,5 +154,15 @@ final class ManiphestTaskOwnerTransaction } + public function getTransactionTypeForConduit($xaction) { + return 'owner'; + } + + public function getFieldValuesForConduit($xaction, $data) { + return array( + 'old' => $xaction->getOldValue(), + 'new' => $xaction->getNewValue(), + ); + } } diff --git a/src/applications/maniphest/xaction/ManiphestTaskPointsTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskPointsTransaction.php index 8953664f27..5a69199874 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskPointsTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskPointsTransaction.php @@ -107,4 +107,16 @@ final class ManiphestTaskPointsTransaction return $value; } + public function getTransactionTypeForConduit($xaction) { + return 'points'; + } + + public function getFieldValuesForConduit($xaction, $data) { + return array( + 'old' => $xaction->getOldValue(), + 'new' => $xaction->getNewValue(), + ); + } + + } diff --git a/src/applications/maniphest/xaction/ManiphestTaskStatusTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskStatusTransaction.php index 5e1cd44611..a3780e81b9 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskStatusTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskStatusTransaction.php @@ -229,4 +229,15 @@ final class ManiphestTaskStatusTransaction } + public function getTransactionTypeForConduit($xaction) { + return 'status'; + } + + public function getFieldValuesForConduit($xaction, $data) { + return array( + 'old' => $xaction->getOldValue(), + 'new' => $xaction->getNewValue(), + ); + } + } diff --git a/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php index 506817b0fc..e4ec2a132f 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php @@ -72,4 +72,15 @@ final class ManiphestTaskTitleTransaction return $errors; } + public function getTransactionTypeForConduit($xaction) { + return 'title'; + } + + public function getFieldValuesForConduit($xaction, $data) { + return array( + 'old' => $xaction->getOldValue(), + 'new' => $xaction->getNewValue(), + ); + } + } From cc865e549bcddb68544c6ae7415c4e19a7db095d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Nov 2017 10:38:43 -0800 Subject: [PATCH 27/30] Provide revision parent/child edges in `edge.search`, and more information in `differential.revision.search` Summary: See PHI195. This bulks out these API methods since all the requests are pretty straightforward. Test Plan: Ran `edge.search` and `differential.revision.search`. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18762 --- ...entialRevisionDependedOnByRevisionEdgeType.php | 14 ++++++++++++++ ...ferentialRevisionDependsOnRevisionEdgeType.php | 13 +++++++++++++ .../differential/storage/DifferentialRevision.php | 15 +++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/src/applications/differential/edge/DifferentialRevisionDependedOnByRevisionEdgeType.php b/src/applications/differential/edge/DifferentialRevisionDependedOnByRevisionEdgeType.php index 02a24effcf..a6940b536e 100644 --- a/src/applications/differential/edge/DifferentialRevisionDependedOnByRevisionEdgeType.php +++ b/src/applications/differential/edge/DifferentialRevisionDependedOnByRevisionEdgeType.php @@ -13,6 +13,20 @@ final class DifferentialRevisionDependedOnByRevisionEdgeType return true; } + public function getConduitKey() { + return 'revision.child'; + } + + public function getConduitName() { + return pht('Revision Has Child'); + } + + public function getConduitDescription() { + return pht( + 'The source revision makes changes required by the destination '. + 'revision.'); + } + public function getTransactionAddString( $actor, $add_count, diff --git a/src/applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php b/src/applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php index 4613ad8a34..e826f554c6 100644 --- a/src/applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php +++ b/src/applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php @@ -17,6 +17,19 @@ final class DifferentialRevisionDependsOnRevisionEdgeType return true; } + public function getConduitKey() { + return 'revision.parent'; + } + + public function getConduitName() { + return pht('Revision Has Parent'); + } + + public function getConduitDescription() { + return pht( + 'The source revision depends on changes in the destination revision.'); + } + public function getTransactionAddString( $actor, $add_count, diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 61f934f13b..bf4bec0abc 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -986,6 +986,18 @@ final class DifferentialRevision extends DifferentialDAO ->setKey('status') ->setType('map') ->setDescription(pht('Information about revision status.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('repositoryPHID') + ->setType('phid?') + ->setDescription(pht('Revision repository PHID.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('diffPHID') + ->setType('phid') + ->setDescription(pht('Active diff PHID.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('summary') + ->setType('string') + ->setDescription(pht('Revision summary.')), ); } @@ -1002,6 +1014,9 @@ final class DifferentialRevision extends DifferentialDAO 'title' => $this->getTitle(), 'authorPHID' => $this->getAuthorPHID(), 'status' => $status_info, + 'repositoryPHID' => $this->getRepositoryPHID(), + 'diffPHID' => $this->getActiveDiffPHID(), + 'summary' => $this->getSummary(), ); } From 587faa6f677c8d7e914051e5307ade00e2bf43b0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Nov 2017 10:53:14 -0800 Subject: [PATCH 28/30] Remove some defunct old-style transaction policy checks Summary: Ref PHI193. This method of enforcing policy checks is now (mostly) obsolete, and they're generally checked at the Controller/API level instead. Notably, this method does not call `adjustObjectForPolicyChecks(...)` properly, so it can not handle special cases like "creating a project and taking its newly created members into account" for object policies like "Project Members". Just remove these checks, which are redundant with checks elsewhere. Test Plan: - Set Project application default edit policy to "Administrators and Project Members". - Tried to create a project as a non-administrator, adding myself. - Before patch: policy fatal on a VOID object (the project with no PHID generated yet). - After patch: object created properly. Got a sensible policy error if I didn't include myself as a member. - Also verified that other edit rules are still enforced/respected (I can't edit stuff I shouldn't be able to edit). - There's at least a bit of unit test coverage of this, too, which I updated to work via API (which hits the new broad capability checks) instead of via low-level transactions (which enforce only a subset of policy operations now). Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18763 --- .../PhabricatorProjectCoreTestCase.php | 20 ++++++++++++------- .../PhabricatorProjectTransactionEditor.php | 10 ---------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index d9356357b3..f34f224521 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -146,8 +146,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { $user = $this->createUser(); $user->save(); - $user2 = $this->createUser(); - $user2->save(); + $user->setAllowInlineCacheGeneration(true); $proj = $this->createProject($user); @@ -1289,12 +1288,19 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { $new_name = $proj->getName().' '.mt_rand(); - $xaction = new PhabricatorProjectTransaction(); - $xaction->setTransactionType( - PhabricatorProjectNameTransaction::TRANSACTIONTYPE); - $xaction->setNewValue($new_name); + $params = array( + 'objectIdentifier' => $proj->getID(), + 'transactions' => array( + array( + 'type' => 'name', + 'value' => $new_name, + ), + ), + ); - $this->applyTransactions($proj, $user, array($xaction)); + id(new ConduitCall('project.edit', $params)) + ->setUser($user) + ->execute(); return true; } diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index adf4a3138b..2764ce6322 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -120,16 +120,6 @@ final class PhabricatorProjectTransactionEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case PhabricatorProjectNameTransaction::TRANSACTIONTYPE: - case PhabricatorProjectStatusTransaction::TRANSACTIONTYPE: - case PhabricatorProjectImageTransaction::TRANSACTIONTYPE: - case PhabricatorProjectIconTransaction::TRANSACTIONTYPE: - case PhabricatorProjectColorTransaction::TRANSACTIONTYPE: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - return; case PhabricatorProjectLockTransaction::TRANSACTIONTYPE: PhabricatorPolicyFilter::requireCapability( $this->requireActor(), From 12e6106a5907c5160dfca8b997e7b6796d707260 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Nov 2017 11:51:59 -0800 Subject: [PATCH 29/30] Refine bucketing and display rules for voided "Accepts" in Differential Summary: See PHI190. This clarifies the ruleset a bit: - If you accepted, then the author used "Request Review" explicitly, we now show "Accepted Earlier" instead of "Accepted" in the "Reviewers" list on the main revision page. This makes it sligthly more clear why the revision is back in your review queue without picking through the transaction log. - Instead of moving all non-current accepts into "Ready to Review", move only voided accepts into "Ready to Review". This stops us from pulling older accepts which haven't been voided (which could have been incorrectly pulled) and correctly pulls older, voided accepts from before an update (for example: accept, then request review, then update) and generally aligns better with intent/expectation. Test Plan: - Accepted, requested review. - Saw reviewer as "Accepted Earlier". - Saw review in "Ready to Review" bucket. - Accepted, updated (with sticky accept). - Saw reviewer as "Accepted Prior Diff". - Saw review as "Waiting on Authors". Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18764 --- .../DifferentialRevisionRequiredActionResultBucket.php | 2 +- .../query/DifferentialRevisionResultBucket.php | 9 ++++----- .../differential/view/DifferentialReviewersView.php | 10 ++++++++-- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php index 5f5f4008db..711f70afb3 100644 --- a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php @@ -138,7 +138,7 @@ final class DifferentialRevisionRequiredActionResultBucket $results = array(); foreach ($objects as $key => $object) { - if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, false)) { + if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, true)) { continue; } diff --git a/src/applications/differential/query/DifferentialRevisionResultBucket.php b/src/applications/differential/query/DifferentialRevisionResultBucket.php index a0769ac349..c5cc5c0e6c 100644 --- a/src/applications/differential/query/DifferentialRevisionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionResultBucket.php @@ -54,7 +54,7 @@ abstract class DifferentialRevisionResultBucket DifferentialRevision $revision, array $phids, array $statuses, - $current = null) { + $include_voided = null) { foreach ($revision->getReviewers() as $reviewer) { $reviewer_phid = $reviewer->getReviewerPHID(); @@ -67,11 +67,10 @@ abstract class DifferentialRevisionResultBucket continue; } - if ($current !== null) { + if ($include_voided !== null) { if ($status == DifferentialReviewerStatus::STATUS_ACCEPTED) { - $diff_phid = $revision->getActiveDiffPHID(); - $is_current = $reviewer->isAccepted($diff_phid); - if ($is_current !== $current) { + $is_voided = (bool)$reviewer->getVoidedPHID(); + if ($is_voided !== $include_voided) { continue; } } diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php index 33aad25289..f88669e539 100644 --- a/src/applications/differential/view/DifferentialReviewersView.php +++ b/src/applications/differential/view/DifferentialReviewersView.php @@ -47,6 +47,7 @@ final class DifferentialReviewersView extends AphrontView { $action_phid = $reviewer->getLastActionDiffPHID(); $is_current_action = $this->isCurrent($action_phid); + $is_voided = (bool)$reviewer->getVoidedPHID(); $comment_phid = $reviewer->getLastCommentDiffPHID(); $is_current_comment = $this->isCurrent($comment_phid); @@ -86,7 +87,7 @@ final class DifferentialReviewersView extends AphrontView { break; case DifferentialReviewerStatus::STATUS_ACCEPTED: - if ($is_current_action) { + if ($is_current_action && !$is_voided) { $icon = PHUIStatusItemView::ICON_ACCEPT; $color = 'green'; if ($authority_name !== null) { @@ -97,7 +98,12 @@ final class DifferentialReviewersView extends AphrontView { } else { $icon = 'fa-check-circle-o'; $color = 'bluegrey'; - if ($authority_name !== null) { + + if (!$is_current_action && $is_voided) { + // The reviewer accepted the revision, but later the author + // used "Request Review" to request an updated review. + $label = pht('Accepted Earlier'); + } else if ($authority_name !== null) { $label = pht('Accepted Prior Diff (by %s)', $authority_name); } else { $label = pht('Accepted Prior Diff'); From 759c757264c84b35abe603026a29191a2ddf3def Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 7 Nov 2017 05:12:11 -0800 Subject: [PATCH 30/30] Include "Draft" revisions in Differential legacy status queries Summary: See PHI199. Ref T2543. When you run a RevisionQuery with a legacy status constraint (via `differential.query`), we currently don't match "Draft" revisions. Use the actual complete map from `DifferentialRevisionStatus` instead of hard coding the status list so "Draft" is included. Test Plan: - Ran `differential.query` with `ids` and `status` for a draft revision. - Before patch: revision not returned in results. - After patch: revision returned in results. (Note that it returns as "Needs Review", for compatibility.) Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18765 --- .../constants/DifferentialLegacyQuery.php | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/applications/differential/constants/DifferentialLegacyQuery.php b/src/applications/differential/constants/DifferentialLegacyQuery.php index 26d2c4aee2..ff7944afe9 100644 --- a/src/applications/differential/constants/DifferentialLegacyQuery.php +++ b/src/applications/differential/constants/DifferentialLegacyQuery.php @@ -32,14 +32,7 @@ final class DifferentialLegacyQuery } private static function getMap() { - $all = array( - DifferentialRevisionStatus::NEEDS_REVIEW, - DifferentialRevisionStatus::NEEDS_REVISION, - DifferentialRevisionStatus::CHANGES_PLANNED, - DifferentialRevisionStatus::ACCEPTED, - DifferentialRevisionStatus::PUBLISHED, - DifferentialRevisionStatus::ABANDONED, - ); + $all = array_keys(DifferentialRevisionStatus::getAll()); $open = array(); $closed = array(); @@ -61,6 +54,9 @@ final class DifferentialLegacyQuery ), self::STATUS_NEEDS_REVIEW => array( DifferentialRevisionStatus::NEEDS_REVIEW, + + // For legacy callers, "Draft" is treated as "Needs Review". + DifferentialRevisionStatus::DRAFT, ), self::STATUS_NEEDS_REVISION => array( DifferentialRevisionStatus::NEEDS_REVISION,