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

epriestley 9 years ago 794b4565 77b3efaf

+78

5 changed files

expand all

resources

sql

autopatches

20170320.reviewers.01.lastaction.sql

20170320.reviewers.02.lastcomment.sql

src

applications

differential

editor

DifferentialTransactionEditor.php

storage

DifferentialReviewer.php

xaction

DifferentialRevisionReviewTransaction.php

resources/sql/autopatches/20170320.reviewers.01.lastaction.sql

··· 1 + ALTER TABLE {$NAMESPACE}_differential.differential_reviewer 2 + ADD lastActionDiffPHID VARBINARY(64);

resources/sql/autopatches/20170320.reviewers.02.lastcomment.sql

··· 1 + ALTER TABLE {$NAMESPACE}_differential.differential_reviewer 2 + ADD lastCommentDiffPHID VARBINARY(64);

+58

src/applications/differential/editor/DifferentialTransactionEditor.php

··· 718 718 break; 719 719 } 720 720 721 + 722 + $this->markReviewerComments($object, $xactions); 723 + 721 724 return $xactions; 722 725 } 723 726 ··· 1835 1838 ->setIsCommandeerSideEffect(true) 1836 1839 ->setNewValue($edits); 1837 1840 } 1841 + 1842 + public function getActiveDiff($object) { 1843 + if ($this->getIsNewObject()) { 1844 + return null; 1845 + } else { 1846 + return $object->getActiveDiff(); 1847 + } 1848 + } 1849 + 1850 + /** 1851 + * When a reviewer makes a comment, mark the last revision they commented 1852 + * on. 1853 + * 1854 + * This allows us to show a hint to help authors and other reviewers quickly 1855 + * distinguish between reviewers who have participated in the discussion and 1856 + * reviewers who haven't been part of it. 1857 + */ 1858 + private function markReviewerComments($object, array $xactions) { 1859 + $acting_phid = $this->getActingAsPHID(); 1860 + if (!$acting_phid) { 1861 + return; 1862 + } 1863 + 1864 + $diff = $this->getActiveDiff($object); 1865 + if (!$diff) { 1866 + return; 1867 + } 1868 + 1869 + $has_comment = false; 1870 + foreach ($xactions as $xaction) { 1871 + if ($xaction->hasComment()) { 1872 + $has_comment = true; 1873 + break; 1874 + } 1875 + } 1876 + 1877 + if (!$has_comment) { 1878 + return; 1879 + } 1880 + 1881 + $reviewer_table = new DifferentialReviewer(); 1882 + $conn = $reviewer_table->establishConnection('w'); 1883 + 1884 + queryfx( 1885 + $conn, 1886 + 'UPDATE %T SET lastCommentDiffPHID = %s 1887 + WHERE revisionPHID = %s 1888 + AND reviewerPHID = %s', 1889 + $reviewer_table->getTableName(), 1890 + $diff->getPHID(), 1891 + $object->getPHID(), 1892 + $acting_phid); 1893 + } 1894 + 1895 + 1838 1896 1839 1897 }

src/applications/differential/storage/DifferentialReviewer.php

··· 7 7 protected $reviewerPHID; 8 8 protected $reviewerStatus; 9 9 10 + protected $lastActionDiffPHID; 11 + protected $lastCommentDiffPHID; 12 + 10 13 protected function getConfiguration() { 11 14 return array( 12 15 self::CONFIG_COLUMN_SCHEMA => array( 13 16 'reviewerStatus' => 'text64', 17 + 'lastActionDiffPHID' => 'phid?', 18 + 'lastCommentDiffPHID' => 'phid?', 14 19 ), 15 20 self::CONFIG_KEY_SCHEMA => array( 16 21 'key_revision' => array(

+11

src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php

··· 138 138 // Now, do the new write. 139 139 140 140 if ($map) { 141 + $diff = $this->getEditor()->getActiveDiff($revision); 142 + if ($diff) { 143 + $diff_phid = $diff->getPHID(); 144 + } else { 145 + $diff_phid = null; 146 + } 147 + 141 148 $table = new DifferentialReviewer(); 142 149 143 150 $reviewers = $table->loadAllWhere( ··· 155 162 } 156 163 157 164 $reviewer->setReviewerStatus($status); 165 + 166 + if ($diff_phid) { 167 + $reviewer->setLastActionDiffPHID($diff_phid); 168 + } 158 169 159 170 if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) { 160 171 if ($reviewer->getID()) {

Configure Feed

Configure Feed