From 794b456530bc67dd6bb88c50eaac1ca23042fa16 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 20 Mar 2017 09:54:48 -0700 Subject: [PATCH] Store "last comment" and "last action" diffs on reviewers Summary: Ref T10967. We have a "commented" state to help reviewers get a better sense of who is part of a discussion, and a "last action" state to help distinguish between "accept" and "accepted an older version", for the purposes of sticky accepts and as a UI hint. Currently, these are first-class states, partly beacuse we were somewhat limited in what we could do with edges. However, a more flexible way to represent them is as flags separate from the primary state flag. In the new storage, write them as separate state information: `lastActionDiffPHID` stores the Diff PHID of the last review action (accept, reject, etc). `lastCommentDiffPHID` stores the Diff PHID of the last comment (top-level or inline). Test Plan: Applied storage changes, commented and acted on a revision. Saw appropriate state reflected in the database. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10967 Differential Revision: https://secure.phabricator.com/D17514 --- .../20170320.reviewers.01.lastaction.sql | 2 + .../20170320.reviewers.02.lastcomment.sql | 2 + .../editor/DifferentialTransactionEditor.php | 58 +++++++++++++++++++ .../storage/DifferentialReviewer.php | 5 ++ .../DifferentialRevisionReviewTransaction.php | 11 ++++ 5 files changed, 78 insertions(+) create mode 100644 resources/sql/autopatches/20170320.reviewers.01.lastaction.sql create mode 100644 resources/sql/autopatches/20170320.reviewers.02.lastcomment.sql diff --git a/resources/sql/autopatches/20170320.reviewers.01.lastaction.sql b/resources/sql/autopatches/20170320.reviewers.01.lastaction.sql new file mode 100644 index 0000000000..41b8051275 --- /dev/null +++ b/resources/sql/autopatches/20170320.reviewers.01.lastaction.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_reviewer + ADD lastActionDiffPHID VARBINARY(64); diff --git a/resources/sql/autopatches/20170320.reviewers.02.lastcomment.sql b/resources/sql/autopatches/20170320.reviewers.02.lastcomment.sql new file mode 100644 index 0000000000..c430d86064 --- /dev/null +++ b/resources/sql/autopatches/20170320.reviewers.02.lastcomment.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_reviewer + ADD lastCommentDiffPHID VARBINARY(64); diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 59bd746ccd..cfaff0cbef 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -718,6 +718,9 @@ final class DifferentialTransactionEditor break; } + + $this->markReviewerComments($object, $xactions); + return $xactions; } @@ -1836,4 +1839,59 @@ final class DifferentialTransactionEditor ->setNewValue($edits); } + public function getActiveDiff($object) { + if ($this->getIsNewObject()) { + return null; + } else { + return $object->getActiveDiff(); + } + } + + /** + * When a reviewer makes a comment, mark the last revision they commented + * on. + * + * This allows us to show a hint to help authors and other reviewers quickly + * distinguish between reviewers who have participated in the discussion and + * reviewers who haven't been part of it. + */ + private function markReviewerComments($object, array $xactions) { + $acting_phid = $this->getActingAsPHID(); + if (!$acting_phid) { + return; + } + + $diff = $this->getActiveDiff($object); + if (!$diff) { + return; + } + + $has_comment = false; + foreach ($xactions as $xaction) { + if ($xaction->hasComment()) { + $has_comment = true; + break; + } + } + + if (!$has_comment) { + return; + } + + $reviewer_table = new DifferentialReviewer(); + $conn = $reviewer_table->establishConnection('w'); + + queryfx( + $conn, + 'UPDATE %T SET lastCommentDiffPHID = %s + WHERE revisionPHID = %s + AND reviewerPHID = %s', + $reviewer_table->getTableName(), + $diff->getPHID(), + $object->getPHID(), + $acting_phid); + } + + + } diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index 72317a0a4c..731fc9be14 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -7,10 +7,15 @@ final class DifferentialReviewer protected $reviewerPHID; protected $reviewerStatus; + protected $lastActionDiffPHID; + protected $lastCommentDiffPHID; + protected function getConfiguration() { return array( self::CONFIG_COLUMN_SCHEMA => array( 'reviewerStatus' => 'text64', + 'lastActionDiffPHID' => 'phid?', + 'lastCommentDiffPHID' => 'phid?', ), self::CONFIG_KEY_SCHEMA => array( 'key_revision' => array( diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index 9ac731fea9..de38f67bc0 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -138,6 +138,13 @@ abstract class DifferentialRevisionReviewTransaction // Now, do the new write. if ($map) { + $diff = $this->getEditor()->getActiveDiff($revision); + if ($diff) { + $diff_phid = $diff->getPHID(); + } else { + $diff_phid = null; + } + $table = new DifferentialReviewer(); $reviewers = $table->loadAllWhere( @@ -156,6 +163,10 @@ abstract class DifferentialRevisionReviewTransaction $reviewer->setReviewerStatus($status); + if ($diff_phid) { + $reviewer->setLastActionDiffPHID($diff_phid); + } + if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) { if ($reviewer->getID()) { $reviewer->delete();