mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-30 01:10:58 +01:00
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
This commit is contained in:
Juan Pablo Civile
epriestley
parent
c05e026e65
commit
70fd3dd54f
6 changed files with 154 additions and 52 deletions
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -0,0 +1,8 @@
|
|||
<?php
|
||||
|
||||
final class DifferentialReviewerStatus {
|
||||
|
||||
const STATUS_ADDED = 'added';
|
||||
const STATUS_REJECTED = 'rejected';
|
||||
|
||||
}
|
||||
|
|
@ -146,12 +146,12 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
|
|||
$metadata[$key] = $added_reviewers;
|
||||
}
|
||||
|
||||
DifferentialRevisionEditor::alterReviewers(
|
||||
DifferentialRevisionEditor::updateReviewers(
|
||||
$revision,
|
||||
$reviewer_phids,
|
||||
$rem = array($actor_phid),
|
||||
$add = array(),
|
||||
$actor_phid);
|
||||
$actor,
|
||||
array(),
|
||||
array($actor_phid));
|
||||
|
||||
break;
|
||||
|
||||
case DifferentialAction::ACTION_ABANDON:
|
||||
|
|
@ -205,14 +205,11 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
|
|||
$revision
|
||||
->setStatus(ArcanistDifferentialRevisionStatus::ACCEPTED);
|
||||
|
||||
if (!isset($reviewer_phids[$actor_phid])) {
|
||||
DifferentialRevisionEditor::alterReviewers(
|
||||
DifferentialRevisionEditor::updateReviewerStatus(
|
||||
$revision,
|
||||
$reviewer_phids,
|
||||
$rem = array(),
|
||||
$add = array($actor_phid),
|
||||
$actor_phid);
|
||||
}
|
||||
$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(
|
||||
DifferentialRevisionEditor::updateReviewerStatus(
|
||||
$revision,
|
||||
$reviewer_phids,
|
||||
$rem = array(),
|
||||
$add = array($actor_phid),
|
||||
$actor_phid);
|
||||
}
|
||||
$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);
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
Loading…
Reference in a new issue