mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-23 14:00:56 +01:00
Compute reviewer stats
Summary: The final goal is to display reviewers response time on homepage. This is a building block for it. The algorithm is quite strict - it doesn't count simple comment as response because reviewers would be able to cheat with comments like "I'm overwhelmed right now and will review next week". We are more liberate in Phabricator where reviewers response with comments without changing the status quite often but I'm not trying to improve response times in Phabricator so this is irrelevant. Reviewers in Facebook changes status more often (to clean their queue) so I follow this approach. There is currently no way to track reviewers silently added and removed in Edit Revision but it's not a big deal. The algorithm doesn't track commandeered revision, there's a TODO for it. Response times are put in two buckets: `$reviewed` and `$not_reviewed`. `$reviewed` contains reviewers who took action, `$not_reviewed` contains reviewers who didn't respond on time. I will probably compute average time from `$reviewed` and raise it for those `$not_reviewed` that are higher than this average. The idea is to not favor reviewers who were only lucky for being in a group with someone fast. Test Plan: Passed test. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Differential Revision: https://secure.phabricator.com/D3062
This commit is contained in:
vrana
parent
02594c2c22
commit
1970ceefe3
3 changed files with 261 additions and 0 deletions
|
|
@ -270,6 +270,8 @@ 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',
|
||||
'DifferentialReviewerStats' => 'applications/differential/stats/DifferentialReviewerStats.php',
|
||||
'DifferentialReviewerStatsTestCase' => 'applications/differential/stats/__tests__/DifferentialReviewerStatsTestCase.php',
|
||||
'DifferentialReviewersFieldSpecification' => 'applications/differential/field/specification/DifferentialReviewersFieldSpecification.php',
|
||||
'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php',
|
||||
'DifferentialRevisionCommentListView' => 'applications/differential/view/DifferentialRevisionCommentListView.php',
|
||||
|
|
@ -1340,6 +1342,7 @@ phutil_register_library_map(array(
|
|||
'DifferentialRevertPlanFieldSpecification' => 'DifferentialFieldSpecification',
|
||||
'DifferentialReviewRequestMail' => 'DifferentialMail',
|
||||
'DifferentialReviewedByFieldSpecification' => 'DifferentialFieldSpecification',
|
||||
'DifferentialReviewerStatsTestCase' => 'PhabricatorTestCase',
|
||||
'DifferentialReviewersFieldSpecification' => 'DifferentialFieldSpecification',
|
||||
'DifferentialRevision' => 'DifferentialDAO',
|
||||
'DifferentialRevisionCommentListView' => 'AphrontView',
|
||||
|
|
|
|||
|
|
@ -0,0 +1,137 @@
|
|||
<?php
|
||||
|
||||
/*
|
||||
* Copyright 2012 Facebook, Inc.
|
||||
*
|
||||
* Licensed under the Apache License, Version 2.0 (the "License");
|
||||
* you may not use this file except in compliance with the License.
|
||||
* You may obtain a copy of the License at
|
||||
*
|
||||
* http://www.apache.org/licenses/LICENSE-2.0
|
||||
*
|
||||
* Unless required by applicable law or agreed to in writing, software
|
||||
* distributed under the License is distributed on an "AS IS" BASIS,
|
||||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
* See the License for the specific language governing permissions and
|
||||
* limitations under the License.
|
||||
*/
|
||||
|
||||
final class DifferentialReviewerStats {
|
||||
private $since = 0;
|
||||
private $now;
|
||||
|
||||
public function setSince($value) {
|
||||
$this->since = $value;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function setNow($value) {
|
||||
$this->now = $value;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function computeTimes(
|
||||
DifferentialRevision $revision,
|
||||
array $comments) {
|
||||
assert_instances_of($comments, 'DifferentialComment');
|
||||
|
||||
$add_rev = DifferentialComment::METADATA_ADDED_REVIEWERS;
|
||||
$rem_rev = DifferentialComment::METADATA_REMOVED_REVIEWERS;
|
||||
|
||||
$date = $revision->getDateCreated();
|
||||
|
||||
// Find out original reviewers.
|
||||
$reviewers = array_fill_keys($revision->getReviewers(), $date);
|
||||
foreach (array_reverse($comments) as $comment) {
|
||||
$metadata = $comment->getMetadata();
|
||||
foreach (idx($metadata, $add_rev, array()) as $phid) {
|
||||
unset($reviewers[$phid]);
|
||||
}
|
||||
foreach (idx($metadata, $rem_rev, array()) as $phid) {
|
||||
$reviewers[$phid] = $date;
|
||||
}
|
||||
}
|
||||
|
||||
$reviewed = array();
|
||||
$not_reviewed = array();
|
||||
$status = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
|
||||
|
||||
foreach ($comments as $comment) {
|
||||
$date = $comment->getDateCreated();
|
||||
$old_status = $status;
|
||||
|
||||
switch ($comment->getAction()) {
|
||||
case DifferentialAction::ACTION_UPDATE:
|
||||
if ($status != ArcanistDifferentialRevisionStatus::CLOSED &&
|
||||
$status != ArcanistDifferentialRevisionStatus::ACCEPTED) {
|
||||
$status = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
|
||||
}
|
||||
break;
|
||||
case DifferentialAction::ACTION_REQUEST:
|
||||
case DifferentialAction::ACTION_RECLAIM:
|
||||
$status = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
|
||||
break;
|
||||
case DifferentialAction::ACTION_REJECT:
|
||||
case DifferentialAction::ACTION_RETHINK:
|
||||
$status = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
|
||||
break;
|
||||
case DifferentialAction::ACTION_ACCEPT:
|
||||
$status = ArcanistDifferentialRevisionStatus::ACCEPTED;
|
||||
break;
|
||||
case DifferentialAction::ACTION_CLOSE:
|
||||
$status = ArcanistDifferentialRevisionStatus::CLOSED;
|
||||
break;
|
||||
case DifferentialAction::ACTION_ABANDON:
|
||||
$status = ArcanistDifferentialRevisionStatus::ABANDONED;
|
||||
break;
|
||||
}
|
||||
|
||||
// Update current reviewers.
|
||||
$metadata = $comment->getMetadata();
|
||||
foreach (idx($metadata, $add_rev, array()) as $phid) {
|
||||
// If someone reviewed a revision without being its reviewer then give
|
||||
// him zero response time.
|
||||
$reviewers[$phid] = $date;
|
||||
}
|
||||
foreach (idx($metadata, $rem_rev, array()) as $phid) {
|
||||
$start = idx($reviewers, $phid);
|
||||
if ($start !== null) {
|
||||
if ($date >= $this->since) {
|
||||
$reviewed[$phid][] = $date - $start;
|
||||
}
|
||||
unset($reviewers[$phid]);
|
||||
}
|
||||
}
|
||||
|
||||
// TODO: Respect workdays.
|
||||
|
||||
if ($old_status != $status) {
|
||||
if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) {
|
||||
$reviewers = array_fill_keys(array_keys($reviewers), $date);
|
||||
} else if ($date >= $this->since) {
|
||||
if ($old_status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) {
|
||||
foreach ($reviewers as $phid => $start) {
|
||||
if ($phid == $comment->getAuthorPHID()) {
|
||||
$reviewed[$phid][] = $date - $start;
|
||||
} else {
|
||||
$not_reviewed[$phid][] = $date - $start;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) {
|
||||
$now = ($this->now !== null ? $this->now : time());
|
||||
if ($now >= $this->since) {
|
||||
foreach ($reviewers as $phid => $start) {
|
||||
$not_reviewed[$phid][] = $now - $start;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return array($reviewed, $not_reviewed);
|
||||
}
|
||||
|
||||
}
|
||||
|
|
@ -0,0 +1,121 @@
|
|||
<?php
|
||||
|
||||
/*
|
||||
* Copyright 2012 Facebook, Inc.
|
||||
*
|
||||
* Licensed under the Apache License, Version 2.0 (the "License");
|
||||
* you may not use this file except in compliance with the License.
|
||||
* You may obtain a copy of the License at
|
||||
*
|
||||
* http://www.apache.org/licenses/LICENSE-2.0
|
||||
*
|
||||
* Unless required by applicable law or agreed to in writing, software
|
||||
* distributed under the License is distributed on an "AS IS" BASIS,
|
||||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
* See the License for the specific language governing permissions and
|
||||
* limitations under the License.
|
||||
*/
|
||||
|
||||
final class DifferentialReviewerStatsTestCase extends PhabricatorTestCase {
|
||||
|
||||
public function testReviewerStats() {
|
||||
$revw = DifferentialRevision::RELATION_REVIEWER;
|
||||
|
||||
$revision = new DifferentialRevision();
|
||||
$revision->setDateCreated(1);
|
||||
$revision->attachRelationships(array(
|
||||
array('relation' => $revw, 'objectPHID' => 'R1'),
|
||||
array('relation' => $revw, 'objectPHID' => 'R3'),
|
||||
));
|
||||
|
||||
$comments = array(
|
||||
$this->newComment(2, 'A', DifferentialAction::ACTION_COMMENT),
|
||||
$this->newComment(4, 'A', DifferentialAction::ACTION_ADDREVIEWERS,
|
||||
array(DifferentialComment::METADATA_ADDED_REVIEWERS => array('R3'))),
|
||||
$this->newComment(8, 'R1', DifferentialAction::ACTION_REJECT),
|
||||
$this->newComment(16, 'A', DifferentialAction::ACTION_COMMENT),
|
||||
$this->newComment(32, 'A', DifferentialAction::ACTION_UPDATE),
|
||||
$this->newComment(64, 'A', DifferentialAction::ACTION_UPDATE),
|
||||
$this->newComment(128, 'A', DifferentialAction::ACTION_COMMENT),
|
||||
$this->newComment(256, 'R2', DifferentialAction::ACTION_RESIGN,
|
||||
array(DifferentialComment::METADATA_REMOVED_REVIEWERS => array('R2'))),
|
||||
$this->newComment(512, 'R3', DifferentialAction::ACTION_ACCEPT),
|
||||
$this->newComment(1024, 'A', DifferentialAction::ACTION_UPDATE),
|
||||
// TODO: claim, abandon, reclaim
|
||||
);
|
||||
|
||||
$stats = new DifferentialReviewerStats();
|
||||
list($reviewed, $not_reviewed) = $stats->computeTimes($revision, $comments);
|
||||
|
||||
ksort($reviewed);
|
||||
$this->assertEqual(
|
||||
array(
|
||||
'R1' => array(8 - 1),
|
||||
'R2' => array(256 - 32),
|
||||
'R3' => array(512 - 32),
|
||||
),
|
||||
$reviewed);
|
||||
|
||||
ksort($not_reviewed);
|
||||
$this->assertEqual(
|
||||
array(
|
||||
'R1' => array(512 - 32),
|
||||
'R2' => array(8 - 1),
|
||||
'R3' => array(8 - 4),
|
||||
),
|
||||
$not_reviewed);
|
||||
}
|
||||
|
||||
public function testReviewerStatsSince() {
|
||||
$revision = new DifferentialRevision();
|
||||
$revision->setDateCreated(1);
|
||||
$revision->attachRelationships(array(
|
||||
array(
|
||||
'relation' => DifferentialRevision::RELATION_REVIEWER,
|
||||
'objectPHID' => 'R',
|
||||
),
|
||||
));
|
||||
|
||||
$comments = array(
|
||||
$this->newComment(2, 'R', DifferentialAction::ACTION_REJECT),
|
||||
$this->newComment(4, 'A', DifferentialAction::ACTION_REQUEST),
|
||||
$this->newComment(8, 'R', DifferentialAction::ACTION_ACCEPT),
|
||||
);
|
||||
|
||||
$stats = new DifferentialReviewerStats();
|
||||
$stats->setSince(4);
|
||||
list($reviewed, $not_reviewed) = $stats->computeTimes($revision, $comments);
|
||||
|
||||
$this->assertEqual(array('R' => array(8 - 4)), $reviewed);
|
||||
$this->assertEqual(array(), $not_reviewed);
|
||||
}
|
||||
|
||||
public function testReviewerStatsRequiresReview() {
|
||||
$revision = new DifferentialRevision();
|
||||
$revision->setDateCreated(1);
|
||||
$revision->attachRelationships(array(
|
||||
array(
|
||||
'relation' => DifferentialRevision::RELATION_REVIEWER,
|
||||
'objectPHID' => 'R',
|
||||
),
|
||||
));
|
||||
|
||||
$comments = array();
|
||||
|
||||
$stats = new DifferentialReviewerStats();
|
||||
$stats->setNow(2);
|
||||
list($reviewed, $not_reviewed) = $stats->computeTimes($revision, $comments);
|
||||
|
||||
$this->assertEqual(array(), $reviewed);
|
||||
$this->assertEqual(array('R' => array(2 - 1)), $not_reviewed);
|
||||
}
|
||||
|
||||
private function newComment($date, $author, $action, $metadata = array()) {
|
||||
return id(new DifferentialComment())
|
||||
->setDateCreated($date)
|
||||
->setAuthorPHID($author)
|
||||
->setAction($action)
|
||||
->setMetadata($metadata);
|
||||
}
|
||||
|
||||
}
|
||||
Loading…
Reference in a new issue