From 1fc2dfd54baf6113512d2ab54d5a07eb16825d6f Mon Sep 17 00:00:00 2001 From: vrana Date: Tue, 24 Jul 2012 23:50:25 -0700 Subject: [PATCH] Load reviewer stats Summary: This allows getting stats for any arbitrary period, e.g. - everything - last week - week before last week Test Plan: Ran the script for last week. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Differential Revision: https://secure.phabricator.com/D3064 --- .../stats/DifferentialReviewerStats.php | 119 ++++++++++++++++-- .../DifferentialReviewerStatsTestCase.php | 29 ++--- 2 files changed, 124 insertions(+), 24 deletions(-) diff --git a/src/applications/differential/stats/DifferentialReviewerStats.php b/src/applications/differential/stats/DifferentialReviewerStats.php index 003def828d..564cbbc907 100644 --- a/src/applications/differential/stats/DifferentialReviewerStats.php +++ b/src/applications/differential/stats/DifferentialReviewerStats.php @@ -18,18 +18,21 @@ final class DifferentialReviewerStats { private $since = 0; - private $now; + private $until; public function setSince($value) { $this->since = $value; return $this; } - public function setNow($value) { - $this->now = $value; + public function setUntil($value) { + $this->until = $value; return $this; } + /** + * @return array($reviewed, $not_reviewed) + */ public function computeTimes( DifferentialRevision $revision, array $comments) { @@ -103,7 +106,7 @@ final class DifferentialReviewerStats { } } - // TODO: Respect workdays. + // TODO: Respect workdays and status away. if ($old_status != $status) { if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) { @@ -123,10 +126,10 @@ final class DifferentialReviewerStats { } if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) { - $now = ($this->now !== null ? $this->now : time()); - if ($now >= $this->since) { + $date = ($this->until !== null ? $this->until : time()); + if ($date >= $this->since) { foreach ($reviewers as $phid => $start) { - $not_reviewed[$phid][] = $now - $start; + $not_reviewed[$phid][] = $date - $start; } } } @@ -134,4 +137,106 @@ final class DifferentialReviewerStats { return array($reviewed, $not_reviewed); } + public function loadAvgs() { + $limit = 1000; + $conn_r = id(new DifferentialRevision())->establishConnection('r'); + + $sums = array(); + $counts = array(); + $all_not_reviewed = array(); + + $last_id = 0; + do { + $where = ''; + if ($this->until !== null) { + $where .= qsprintf( + $conn_r, + ' AND dateCreated < %d', + $this->until); + } + if ($this->since) { + $where .= qsprintf( + $conn_r, + ' AND (dateModified > %d OR status = %s)', + $this->since, + ArcanistDifferentialRevisionStatus::NEEDS_REVIEW); + } + $revisions = id(new DifferentialRevision())->loadAllWhere( + 'id > %d%Q ORDER BY id LIMIT %d', + $last_id, + $where, + $limit); + + if (!$revisions) { + break; + } + $last_id = last_key($revisions); + + $relations = queryfx_all( + $conn_r, + 'SELECT * FROM %T WHERE revisionID IN (%Ld) AND relation = %s', + DifferentialRevision::RELATIONSHIP_TABLE, + array_keys($revisions), + DifferentialRevision::RELATION_REVIEWER); + $relations = igroup($relations, 'revisionID'); + + $where = ''; + if ($this->until !== null) { + $where = qsprintf( + $conn_r, + ' AND dateCreated < %d', + $this->until); + } + $all_comments = id(new DifferentialComment())->loadAllWhere( + 'revisionID IN (%Ld)%Q ORDER BY revisionID, id', + array_keys($revisions), + $where); + $all_comments = mgroup($all_comments, 'getRevisionID'); + + foreach ($revisions as $id => $revision) { + $revision->attachRelationships(idx($relations, $id, array())); + $comments = idx($all_comments, $id, array()); + + list($reviewed, $not_reviewed) = + $this->computeTimes($revision, $comments); + + foreach ($reviewed as $phid => $times) { + $sums[$phid] = idx($sums, $phid, 0) + array_sum($times); + $counts[$phid] = idx($counts, $phid, 0) + count($times); + } + + foreach ($not_reviewed as $phid => $times) { + $all_not_reviewed[$phid][] = $times; + } + } + } while (count($revisions) >= $limit); + + foreach ($all_not_reviewed as $phid => $not_reviewed) { + if (!array_key_exists($phid, $counts)) { + // If the person didn't make any reviews than take maximum time because + // he is at least that slow. + $sums[$phid] = max(array_map('max', $not_reviewed)); + $counts[$phid] = 1; + continue; + } + $avg = $sums[$phid] / $counts[$phid]; + foreach ($not_reviewed as $times) { + foreach ($times as $time) { + // Don't shorten the average time just because the reviewer was lucky + // to be in a group with someone faster. + if ($time > $avg) { + $sums[$phid] += $time; + $counts[$phid]++; + } + } + } + } + + $avgs = array(); + foreach ($sums as $phid => $sum) { + $avgs[$phid] = $sum / $counts[$phid]; + } + return $avgs; + } + } diff --git a/src/applications/differential/stats/__tests__/DifferentialReviewerStatsTestCase.php b/src/applications/differential/stats/__tests__/DifferentialReviewerStatsTestCase.php index 532eb11dae..e8792aa2cb 100644 --- a/src/applications/differential/stats/__tests__/DifferentialReviewerStatsTestCase.php +++ b/src/applications/differential/stats/__tests__/DifferentialReviewerStatsTestCase.php @@ -19,13 +19,11 @@ 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'), + $this->newReviewer('R1'), + $this->newReviewer('R3'), )); $comments = array( @@ -69,12 +67,7 @@ final class DifferentialReviewerStatsTestCase extends PhabricatorTestCase { public function testReviewerStatsSince() { $revision = new DifferentialRevision(); $revision->setDateCreated(1); - $revision->attachRelationships(array( - array( - 'relation' => DifferentialRevision::RELATION_REVIEWER, - 'objectPHID' => 'R', - ), - )); + $revision->attachRelationships(array($this->newReviewer('R'))); $comments = array( $this->newComment(2, 'R', DifferentialAction::ACTION_REJECT), @@ -93,23 +86,25 @@ final class DifferentialReviewerStatsTestCase extends PhabricatorTestCase { public function testReviewerStatsRequiresReview() { $revision = new DifferentialRevision(); $revision->setDateCreated(1); - $revision->attachRelationships(array( - array( - 'relation' => DifferentialRevision::RELATION_REVIEWER, - 'objectPHID' => 'R', - ), - )); + $revision->attachRelationships(array($this->newReviewer('R'))); $comments = array(); $stats = new DifferentialReviewerStats(); - $stats->setNow(2); + $stats->setUntil(2); list($reviewed, $not_reviewed) = $stats->computeTimes($revision, $comments); $this->assertEqual(array(), $reviewed); $this->assertEqual(array('R' => array(2 - 1)), $not_reviewed); } + private function newReviewer($phid) { + return array( + 'relation' => DifferentialRevision::RELATION_REVIEWER, + 'objectPHID' => $phid, + ); + } + private function newComment($date, $author, $action, $metadata = array()) { return id(new DifferentialComment()) ->setDateCreated($date)