From 70fd3dd54f56a653f41f74c00bc1d6c8d1d6502f Mon Sep 17 00:00:00 2001 From: Juan Pablo Civile Date: Wed, 10 Jul 2013 13:45:14 -0700 Subject: [PATCH] Store revision reviewer state as edges Summary: Keep track of the state of a reviewer in an edge between reviewer and revision. The edge stores the state of the review, added or rejected. And if the revision was accepted by that reviewer the id of the diff accepted. Test Plan: Create diffs and clowncopterize reviewer list changes. This includes: * Adding new reviewers * Resigning * Commandering a revision Reviewers: epriestley CC: aran, Korvin Maniphest Tasks: T1279 Differential Revision: https://secure.phabricator.com/D6372 Conflicts: src/applications/differential/editor/DifferentialCommentEditor.php --- src/__celerity_resource_map__.php | 46 ++++---- src/__phutil_library_map__.php | 1 + .../constants/DifferentialReviewerStatus.php | 8 ++ .../editor/DifferentialCommentEditor.php | 43 +++----- .../editor/DifferentialRevisionEditor.php | 103 +++++++++++++++++- .../edges/constants/PhabricatorEdgeConfig.php | 5 + 6 files changed, 154 insertions(+), 52 deletions(-) create mode 100644 src/applications/differential/constants/DifferentialReviewerStatus.php diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 7d4e0e2cc3..8467f23eb5 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -1459,7 +1459,7 @@ celerity_register_resource_map(array( ), 'javelin-behavior-differential-add-reviewers-and-ccs' => array( - 'uri' => '/res/27be3f81/rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js', + 'uri' => '/res/fd9f2c1c/rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js', 'type' => 'js', 'requires' => array( @@ -4279,7 +4279,7 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/dd27a69b/differential.pkg.css', 'type' => 'css', ), - '4ad86dee' => + '504ca7d2' => array( 'name' => 'differential.pkg.js', 'symbols' => @@ -4305,7 +4305,7 @@ celerity_register_resource_map(array( 18 => 'javelin-behavior-differential-toggle-files', 19 => 'javelin-behavior-differential-user-select', ), - 'uri' => '/res/pkg/4ad86dee/differential.pkg.js', + 'uri' => '/res/pkg/504ca7d2/differential.pkg.js', 'type' => 'js', ), 'c8ce2d88' => @@ -4403,7 +4403,7 @@ celerity_register_resource_map(array( 'aphront-typeahead-control-css' => '6e2d527c', 'differential-changeset-view-css' => 'dd27a69b', 'differential-core-view-css' => 'dd27a69b', - 'differential-inline-comment-editor' => '4ad86dee', + 'differential-inline-comment-editor' => '504ca7d2', 'differential-local-commits-view-css' => 'dd27a69b', 'differential-results-table-css' => 'dd27a69b', 'differential-revision-add-comment-css' => 'dd27a69b', @@ -4421,24 +4421,24 @@ celerity_register_resource_map(array( 'javelin-behavior-aphlict-dropdown' => '75ccea43', 'javelin-behavior-aphlict-listen' => '75ccea43', 'javelin-behavior-aphront-basic-tokenizer' => '75ccea43', - 'javelin-behavior-aphront-drag-and-drop' => '4ad86dee', - 'javelin-behavior-aphront-drag-and-drop-textarea' => '4ad86dee', + 'javelin-behavior-aphront-drag-and-drop' => '504ca7d2', + 'javelin-behavior-aphront-drag-and-drop-textarea' => '504ca7d2', 'javelin-behavior-aphront-form-disable-on-submit' => '75ccea43', 'javelin-behavior-audit-preview' => '96909266', 'javelin-behavior-dark-console' => '4ccfeb47', 'javelin-behavior-device' => '75ccea43', - 'javelin-behavior-differential-accept-with-errors' => '4ad86dee', - 'javelin-behavior-differential-add-reviewers-and-ccs' => '4ad86dee', - 'javelin-behavior-differential-comment-jump' => '4ad86dee', - 'javelin-behavior-differential-diff-radios' => '4ad86dee', - 'javelin-behavior-differential-dropdown-menus' => '4ad86dee', - 'javelin-behavior-differential-edit-inline-comments' => '4ad86dee', - 'javelin-behavior-differential-feedback-preview' => '4ad86dee', - 'javelin-behavior-differential-keyboard-navigation' => '4ad86dee', - 'javelin-behavior-differential-populate' => '4ad86dee', - 'javelin-behavior-differential-show-more' => '4ad86dee', - 'javelin-behavior-differential-toggle-files' => '4ad86dee', - 'javelin-behavior-differential-user-select' => '4ad86dee', + 'javelin-behavior-differential-accept-with-errors' => '504ca7d2', + 'javelin-behavior-differential-add-reviewers-and-ccs' => '504ca7d2', + 'javelin-behavior-differential-comment-jump' => '504ca7d2', + 'javelin-behavior-differential-diff-radios' => '504ca7d2', + 'javelin-behavior-differential-dropdown-menus' => '504ca7d2', + 'javelin-behavior-differential-edit-inline-comments' => '504ca7d2', + 'javelin-behavior-differential-feedback-preview' => '504ca7d2', + 'javelin-behavior-differential-keyboard-navigation' => '504ca7d2', + 'javelin-behavior-differential-populate' => '504ca7d2', + 'javelin-behavior-differential-show-more' => '504ca7d2', + 'javelin-behavior-differential-toggle-files' => '504ca7d2', + 'javelin-behavior-differential-user-select' => '504ca7d2', 'javelin-behavior-diffusion-commit-graph' => '96909266', 'javelin-behavior-diffusion-pull-lastmodified' => '96909266', 'javelin-behavior-error-log' => '4ccfeb47', @@ -4446,7 +4446,7 @@ celerity_register_resource_map(array( 'javelin-behavior-history-install' => '75ccea43', 'javelin-behavior-konami' => '75ccea43', 'javelin-behavior-lightbox-attachments' => '75ccea43', - 'javelin-behavior-load-blame' => '4ad86dee', + 'javelin-behavior-load-blame' => '504ca7d2', 'javelin-behavior-maniphest-batch-selector' => '98f64f07', 'javelin-behavior-maniphest-subpriority-editor' => '98f64f07', 'javelin-behavior-maniphest-transaction-controls' => '98f64f07', @@ -4458,7 +4458,7 @@ celerity_register_resource_map(array( 'javelin-behavior-phabricator-hovercards' => '75ccea43', 'javelin-behavior-phabricator-keyboard-shortcuts' => '75ccea43', 'javelin-behavior-phabricator-nav' => '75ccea43', - 'javelin-behavior-phabricator-object-selector' => '4ad86dee', + 'javelin-behavior-phabricator-object-selector' => '504ca7d2', 'javelin-behavior-phabricator-oncopy' => '75ccea43', 'javelin-behavior-phabricator-remarkup-assist' => '75ccea43', 'javelin-behavior-phabricator-reveal-content' => '75ccea43', @@ -4466,7 +4466,7 @@ celerity_register_resource_map(array( 'javelin-behavior-phabricator-tooltips' => '75ccea43', 'javelin-behavior-phabricator-watch-anchor' => '75ccea43', 'javelin-behavior-refresh-csrf' => '75ccea43', - 'javelin-behavior-repository-crossreference' => '4ad86dee', + 'javelin-behavior-repository-crossreference' => '504ca7d2', 'javelin-behavior-toggle-class' => '75ccea43', 'javelin-behavior-workflow' => '75ccea43', 'javelin-dom' => 'a9f14d76', @@ -4497,7 +4497,7 @@ celerity_register_resource_map(array( 'phabricator-content-source-view-css' => 'dd27a69b', 'phabricator-core-css' => '6e2d527c', 'phabricator-crumbs-view-css' => '6e2d527c', - 'phabricator-drag-and-drop-file-upload' => '4ad86dee', + 'phabricator-drag-and-drop-file-upload' => '504ca7d2', 'phabricator-dropdown-menu' => '75ccea43', 'phabricator-file-upload' => '75ccea43', 'phabricator-filetree-view-css' => '6e2d527c', @@ -4521,7 +4521,7 @@ celerity_register_resource_map(array( 'phabricator-project-tag-css' => 'adc3c36d', 'phabricator-property-list-view-css' => '6e2d527c', 'phabricator-remarkup-css' => '6e2d527c', - 'phabricator-shaped-request' => '4ad86dee', + 'phabricator-shaped-request' => '504ca7d2', 'phabricator-side-menu-view-css' => '6e2d527c', 'phabricator-standard-page-view' => '6e2d527c', 'phabricator-tag-view-css' => '6e2d527c', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 87e10869b9..d1307936c3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -395,6 +395,7 @@ phutil_register_library_map(array( 'DifferentialRevertPlanFieldSpecification' => 'applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php', 'DifferentialReviewRequestMail' => 'applications/differential/mail/DifferentialReviewRequestMail.php', 'DifferentialReviewedByFieldSpecification' => 'applications/differential/field/specification/DifferentialReviewedByFieldSpecification.php', + 'DifferentialReviewerStatus' => 'applications/differential/constants/DifferentialReviewerStatus.php', 'DifferentialReviewersFieldSpecification' => 'applications/differential/field/specification/DifferentialReviewersFieldSpecification.php', 'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php', 'DifferentialRevisionCommentListView' => 'applications/differential/view/DifferentialRevisionCommentListView.php', diff --git a/src/applications/differential/constants/DifferentialReviewerStatus.php b/src/applications/differential/constants/DifferentialReviewerStatus.php new file mode 100644 index 0000000000..23fc393c93 --- /dev/null +++ b/src/applications/differential/constants/DifferentialReviewerStatus.php @@ -0,0 +1,8 @@ +setStatus(ArcanistDifferentialRevisionStatus::ACCEPTED); - if (!isset($reviewer_phids[$actor_phid])) { - DifferentialRevisionEditor::alterReviewers( - $revision, - $reviewer_phids, - $rem = array(), - $add = array($actor_phid), - $actor_phid); - } + DifferentialRevisionEditor::updateReviewerStatus( + $revision, + $this->getActor(), + $actor_phid, + DifferentialReviewerStatus::STATUS_ADDED); break; case DifferentialAction::ACTION_REQUEST: @@ -278,14 +275,11 @@ final class DifferentialCommentEditor extends PhabricatorEditor { "Unexpected revision state '{$revision_status}'!"); } - if (!isset($reviewer_phids[$actor_phid])) { - DifferentialRevisionEditor::alterReviewers( - $revision, - $reviewer_phids, - $rem = array(), - $add = array($actor_phid), - $actor_phid); - } + DifferentialRevisionEditor::updateReviewerStatus( + $revision, + $this->getActor(), + $actor_phid, + DifferentialReviewerStatus::STATUS_REJECTED); $revision ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVISION); @@ -699,12 +693,11 @@ final class DifferentialCommentEditor extends PhabricatorEditor { $removed_reviewers = array_unique($removed_reviewers); if ($added_reviewers) { - DifferentialRevisionEditor::alterReviewers( + DifferentialRevisionEditor::updateReviewers( $revision, - $reviewer_phids, - $removed_reviewers, + $this->getActor(), $added_reviewers, - $actor_phid); + $removed_reviewers); } return array($added_reviewers, $removed_reviewers); diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php index 76f8b50d0c..15e63265df 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/DifferentialRevisionEditor.php @@ -283,11 +283,11 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { $stable[$key] = array_diff_key($old[$key], $add[$key] + $rem[$key]); } - self::alterReviewers( + self::updateReviewers( $revision, - $this->reviewers, - array_keys($rem['rev']), + $this->getActor(), array_keys($add['rev']), + array_keys($rem['rev']), $this->getActorPHID()); // We want to attribute new CCs to a "reasonPHID", representing the reason @@ -571,7 +571,102 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { array($revision->getAuthorPHID())); } - public static function alterReviewers( + public static function updateReviewers( + DifferentialRevision $revision, + PhabricatorUser $actor, + array $add_phids, + array $remove_phids) { + + $reviewers = $revision->getReviewers(); + + // This is here until the new way proves stable enough + // See https://secure.phabricator.com/T1279 + self::alterReviewers( + $revision, + $reviewers, + $remove_phids, + $add_phids, + $actor->getPHID()); + + $editor = id(new PhabricatorEdgeEditor()) + ->setActor($actor); + + $options = array( + 'data' => array( + 'state' => DifferentialReviewerStatus::STATUS_ADDED + ) + ); + + $reviewer_phids_map = array_fill_keys($reviewers, true); + + foreach ($add_phids as $phid) { + + // Adding an already existing edge again would have cause memory loss + // That is, the previous state for that reviewer would be lost + if (isset($reviewer_phids_map[$phid])) { + continue; + } + + $editor->addEdge( + $revision->getPHID(), + PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER, + $phid, + $options); + } + + foreach ($remove_phids as $phid) { + $editor->removeEdge( + $revision->getPHID(), + PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER, + $phid); + } + + $editor->save(); + } + + public static function updateReviewerStatus( + DifferentialRevision $revision, + PhabricatorUser $actor, + $reviewer_phid, + $status) { + + $reviewers = $revision->getReviewers(); + if (!in_array($reviewer_phid, $reviewers)) { + // This is here until the new way proves stable enough + // See https://secure.phabricator.com/T1279 + self::alterReviewers( + $revision, + $reviewers, + array(), + array($reviewer_phid), + $actor->getPHID()); + } + + $options = array( + 'data' => array( + 'status' => $status + ) + ); + + $active_diff = $revision->loadActiveDiff(); + if ($active_diff) { + $options['data']['diff'] = $active_diff->getID(); + } + + id(new PhabricatorEdgeEditor()) + ->setActor($actor) + ->addEdge( + $revision->getPHID(), + PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER, + $reviewer_phid, + $options) + ->save(); + } + + /** + * @deprecated + */ + private static function alterReviewers( DifferentialRevision $revision, array $stable_phids, array $rem_phids, diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 841d106fe6..0d402541de 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -54,6 +54,9 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { const TYPE_OBJECT_HAS_CONTRIBUTOR = 33; const TYPE_CONTRIBUTED_TO_OBJECT = 34; + const TYPE_DREV_HAS_REVIEWER = 35; + const TYPE_REVIEWER_FOR_DREV = 36; + const TYPE_TEST_NO_CYCLE = 9000; const TYPE_PHOB_HAS_ASANATASK = 80001; @@ -118,6 +121,8 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { self::TYPE_PHOB_HAS_ASANASUBTASK => self::TYPE_ASANASUBTASK_HAS_PHOB, self::TYPE_ASANASUBTASK_HAS_PHOB => self::TYPE_PHOB_HAS_ASANASUBTASK, + self::TYPE_DREV_HAS_REVIEWER => self::TYPE_REVIEWER_FOR_DREV, + self::TYPE_REVIEWER_FOR_DREV => self::TYPE_DREV_HAS_REVIEWER, ); return idx($map, $edge_type);