From 42b1c73f411fa098259213fbbb9c815811aec721 Mon Sep 17 00:00:00 2001 From: David Reuss Date: Mon, 23 Apr 2012 13:50:04 -0700 Subject: [PATCH] Allow CC's/Auditors added to audits Test Plan: Added CC's/Auditors, clicked the form elements, and saw correct behaviour. Verified that metadata was present in the detail table. Reviewers: epriestley Reviewed By: epriestley CC: aran, epriestley, 20after4, Koolvin Maniphest Tasks: T904 Differential Revision: https://secure.phabricator.com/D2002 --- resources/sql/patches/137.auditmetadata.sql | 5 + .../PhabricatorAuditActionConstants.php | 6 + .../PhabricatorAuditStatusConstants.php | 2 + .../PhabricatorAuditAddCommentController.php | 11 +- .../PhabricatorAuditPreviewController.php | 26 +++- .../audit/controller/preview/__init__.php | 1 + .../comment/PhabricatorAuditCommentEditor.php | 112 +++++++++++++++++- .../audit/editor/comment/__init__.php | 1 + .../auditcomment/PhabricatorAuditComment.php | 7 ++ .../commit/DiffusionCommitController.php | 46 ++++++- .../diffusion/controller/commit/__init__.php | 1 + .../view/comment/DiffusionCommentView.php | 29 ++++- .../diffusion/view/comment/__init__.php | 1 + .../commentlist/DiffusionCommentListView.php | 12 ++ .../diffusion/view/commentlist/__init__.php | 1 + 15 files changed, 252 insertions(+), 9 deletions(-) create mode 100644 resources/sql/patches/137.auditmetadata.sql diff --git a/resources/sql/patches/137.auditmetadata.sql b/resources/sql/patches/137.auditmetadata.sql new file mode 100644 index 0000000000..2f03fc1d7c --- /dev/null +++ b/resources/sql/patches/137.auditmetadata.sql @@ -0,0 +1,5 @@ +ALTER TABLE phabricator_audit.audit_comment + ADD metadata LONGTEXT COLLATE utf8_bin NOT NULL; + +UPDATE phabricator_audit.audit_comment + SET metadata = '{}' WHERE metadata = ''; diff --git a/src/applications/audit/constants/action/PhabricatorAuditActionConstants.php b/src/applications/audit/constants/action/PhabricatorAuditActionConstants.php index 7fc4db86f1..2d4a51b161 100644 --- a/src/applications/audit/constants/action/PhabricatorAuditActionConstants.php +++ b/src/applications/audit/constants/action/PhabricatorAuditActionConstants.php @@ -23,6 +23,8 @@ final class PhabricatorAuditActionConstants { const COMMENT = 'comment'; const RESIGN = 'resign'; const CLOSE = 'close'; + const ADD_CCS = 'add_ccs'; + const ADD_AUDITORS = 'add_auditors'; public static function getActionNameMap() { static $map = array( @@ -31,6 +33,8 @@ final class PhabricatorAuditActionConstants { self::ACCEPT => 'Accept Commit', self::RESIGN => 'Resign from Audit', self::CLOSE => 'Close Audit', + self::ADD_CCS => 'Add CCs', + self::ADD_AUDITORS => 'Add Auditors', ); return $map; @@ -48,6 +52,8 @@ final class PhabricatorAuditActionConstants { self::ACCEPT => 'accepted', self::RESIGN => 'resigned from', self::CLOSE => 'closed', + self::ADD_CCS => 'added CCs to', + self::ADD_AUDITORS => 'added auditors to', ); return idx($map, $action, 'updated'); } diff --git a/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php b/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php index 02e334f195..b90c0542c1 100644 --- a/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php +++ b/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php @@ -26,6 +26,7 @@ final class PhabricatorAuditStatusConstants { const AUDIT_REQUESTED = 'requested'; const RESIGNED = 'resigned'; const CLOSED = 'closed'; + const CC = 'cc'; public static function getStatusNameMap() { static $map = array( @@ -37,6 +38,7 @@ final class PhabricatorAuditStatusConstants { self::AUDIT_REQUESTED => 'Audit Requested', self::RESIGNED => 'Resigned', self::CLOSED => 'Closed', + self::CC => "Was CC'd", ); return $map; diff --git a/src/applications/audit/controller/addcomment/PhabricatorAuditAddCommentController.php b/src/applications/audit/controller/addcomment/PhabricatorAuditAddCommentController.php index 2227249641..07becae652 100644 --- a/src/applications/audit/controller/addcomment/PhabricatorAuditAddCommentController.php +++ b/src/applications/audit/controller/addcomment/PhabricatorAuditAddCommentController.php @@ -36,17 +36,24 @@ final class PhabricatorAuditAddCommentController return new Aphront404Response(); } + $phids = array($commit_phid); + + $action = $request->getStr('action'); + $comment = id(new PhabricatorAuditComment()) - ->setAction($request->getStr('action')) + ->setAction($action) ->setContent($request->getStr('content')); + $auditors = $request->getArr('auditors'); + $ccs = $request->getArr('ccs'); id(new PhabricatorAuditCommentEditor($commit)) ->setUser($user) ->setAttachInlineComments(true) + ->addAuditors($auditors) + ->addCCs($ccs) ->addComment($comment); - $phids = array($commit_phid); $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); $uri = $handles[$commit_phid]->getURI(); diff --git a/src/applications/audit/controller/preview/PhabricatorAuditPreviewController.php b/src/applications/audit/controller/preview/PhabricatorAuditPreviewController.php index 2a82dee89c..523b7337ad 100644 --- a/src/applications/audit/controller/preview/PhabricatorAuditPreviewController.php +++ b/src/applications/audit/controller/preview/PhabricatorAuditPreviewController.php @@ -34,18 +34,40 @@ final class PhabricatorAuditPreviewController return new Aphront404Response(); } + $action = $request->getStr('action'); + $comment = id(new PhabricatorAuditComment()) ->setActorPHID($user->getPHID()) ->setTargetPHID($commit->getPHID()) - ->setAction($request->getStr('action')) + ->setAction($action) ->setContent($request->getStr('content')); + $phids = array( + $user->getPHID(), + $commit->getPHID(), + ); + + $auditors = $request->getStrList('auditors'); + if ($action == PhabricatorAuditActionConstants::ADD_AUDITORS && $auditors) { + $comment->setMetadata(array( + PhabricatorAuditComment::METADATA_ADDED_AUDITORS => $auditors)); + $phids = array_merge($phids, $auditors); + } + + $ccs = $request->getStrList('ccs'); + if ($action == PhabricatorAuditActionConstants::ADD_CCS && $ccs) { + $comment->setMetadata(array( + PhabricatorAuditComment::METADATA_ADDED_CCS => $ccs)); + $phids = array_merge($phids, $ccs); + } + $view = id(new DiffusionCommentView()) ->setUser($user) ->setComment($comment) ->setIsPreview(true); - $phids = $view->getRequiredHandlePHIDs(); + $phids = array_merge($phids, $view->getRequiredHandlePHIDs()); + $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); $view->setHandles($handles); diff --git a/src/applications/audit/controller/preview/__init__.php b/src/applications/audit/controller/preview/__init__.php index 29abbc7cb5..fd54ac86a4 100644 --- a/src/applications/audit/controller/preview/__init__.php +++ b/src/applications/audit/controller/preview/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'aphront/response/404'); phutil_require_module('phabricator', 'aphront/response/ajax'); +phutil_require_module('phabricator', 'applications/audit/constants/action'); phutil_require_module('phabricator', 'applications/audit/controller/base'); phutil_require_module('phabricator', 'applications/audit/storage/auditcomment'); phutil_require_module('phabricator', 'applications/diffusion/view/comment'); diff --git a/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php index 4dd7ee3718..a503774c9f 100644 --- a/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php @@ -22,6 +22,8 @@ final class PhabricatorAuditCommentEditor { private $user; private $attachInlineComments; + private $auditors = array(); + private $ccs = array(); public function __construct(PhabricatorRepositoryCommit $commit) { $this->commit = $commit; @@ -33,6 +35,16 @@ final class PhabricatorAuditCommentEditor { return $this; } + public function addAuditors(array $auditor_phids) { + $this->auditors = array_merge($this->auditors, $auditor_phids); + return $this; + } + + public function addCCs(array $cc_phids) { + $this->ccs = array_merge($this->ccs, $cc_phids); + return $this; + } + public function setAttachInlineComments($attach_inline_comments) { $this->attachInlineComments = $attach_inline_comments; return $this; @@ -61,13 +73,39 @@ final class PhabricatorAuditCommentEditor { ->setTargetPHID($commit->getPHID()) ->save(); + $content_blocks = array($comment->getContent()); + if ($inline_comments) { foreach ($inline_comments as $inline) { $inline->setAuditCommentID($comment->getID()); $inline->save(); + $content_blocks[] = $inline->getContent(); } } + $ccs = $this->ccs; + $auditors = $this->auditors; + + $metadata = $comment->getMetadata(); + $metacc = array(); + + // Find any "@mentions" in the content blocks. + $mention_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions( + $content_blocks); + if ($mention_ccs) { + $metacc = idx( + $metadata, + PhabricatorAuditComment::METADATA_ADDED_CCS, + array()); + foreach ($mention_ccs as $cc_phid) { + $metacc[] = $cc_phid; + } + } + + if ($metacc) { + $ccs = array_merge($ccs, $metacc); + } + // When a user submits an audit comment, we update all the audit requests // they have authority over to reflect the most recent status. The general // idea here is that if audit has triggered for, e.g., several packages, but @@ -114,7 +152,9 @@ final class PhabricatorAuditCommentEditor { $new_status = null; switch ($action) { case PhabricatorAuditActionConstants::COMMENT: - // Comments don't change audit statuses. + case PhabricatorAuditActionConstants::ADD_CCS: + case PhabricatorAuditActionConstants::ADD_AUDITORS: + // Commenting or adding cc's/auditors doesn't change status. break; case PhabricatorAuditActionConstants::ACCEPT: if (!$user_is_author || $request_is_for_user) { @@ -152,6 +192,8 @@ final class PhabricatorAuditCommentEditor { $new_status = null; switch ($action) { case PhabricatorAuditActionConstants::COMMENT: + case PhabricatorAuditActionConstants::ADD_CCS: + case PhabricatorAuditActionConstants::ADD_AUDITORS: $new_status = PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED; break; case PhabricatorAuditActionConstants::ACCEPT: @@ -181,12 +223,65 @@ final class PhabricatorAuditCommentEditor { } } + $requests_by_auditor = mpull($requests, null, 'getAuditorPHID'); + $requests_phids = array_keys($requests_by_auditor); + + $ccs = array_diff($ccs, $requests_phids); + $auditors = array_diff($auditors, $requests_phids); + + if ($action == PhabricatorAuditActionConstants::ADD_CCS) { + if ($ccs) { + $metadata[PhabricatorAuditComment::METADATA_ADDED_CCS] = $ccs; + $comment->setMetaData($metadata); + } else { + $comment->setAction(PhabricatorAuditActionConstants::COMMENT); + } + } + + if ($action == PhabricatorAuditActionConstants::ADD_AUDITORS) { + if ($auditors) { + $metadata[PhabricatorAuditComment::METADATA_ADDED_AUDITORS] + = $auditors; + $comment->setMetaData($metadata); + } else { + $comment->setAction(PhabricatorAuditActionConstants::COMMENT); + } + } + + $comment->save(); + + if ($auditors) { + foreach ($auditors as $auditor_phid) { + $audit_requested = PhabricatorAuditStatusConstants::AUDIT_REQUESTED; + $requests[] = id (new PhabricatorRepositoryAuditRequest()) + ->setCommitPHID($commit->getPHID()) + ->setAuditorPHID($auditor_phid) + ->setAuditStatus($audit_requested) + ->setAuditReasons( + array('Added by ' . $user->getUsername())) + ->save(); + } + } + + if ($ccs) { + foreach ($ccs as $cc_phid) { + $audit_cc = PhabricatorAuditStatusConstants::CC; + $requests[] = id (new PhabricatorRepositoryAuditRequest()) + ->setCommitPHID($commit->getPHID()) + ->setAuditorPHID($cc_phid) + ->setAuditStatus($audit_cc) + ->setAuditReasons( + array('Added by ' . $user->getUsername())) + ->save(); + } + } + $commit->updateAuditStatus($requests); $commit->save(); $this->publishFeedStory($comment, array_keys($audit_phids)); PhabricatorSearchCommitIndexer::indexCommit($commit); - $this->sendMail($comment, $other_comments, $inline_comments); + $this->sendMail($comment, $other_comments, $inline_comments, $requests); } @@ -256,7 +351,9 @@ final class PhabricatorAuditCommentEditor { private function sendMail( PhabricatorAuditComment $comment, array $other_comments, - array $inline_comments) { + array $inline_comments, + array $requests) { + assert_instances_of($other_comments, 'PhabricatorAuditComment'); assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface'); @@ -277,6 +374,8 @@ final class PhabricatorAuditCommentEditor { PhabricatorAuditActionConstants::ACCEPT => 'Accepted', PhabricatorAuditActionConstants::RESIGN => 'Resigned', PhabricatorAuditActionConstants::CLOSE => 'Closed', + PhabricatorAuditActionConstants::ADD_CCS => 'Added CCs', + PhabricatorAuditActionConstants::ADD_AUDITORS => 'Added Auditors', ); $verb = idx($map, $comment->getAction(), 'Commented On'); @@ -297,6 +396,7 @@ final class PhabricatorAuditCommentEditor { $inline_comments); $email_to = array(); + $email_cc = array(); $author_phid = $data->getCommitDetail('authorPHID'); if ($author_phid) { @@ -308,6 +408,12 @@ final class PhabricatorAuditCommentEditor { $email_cc[] = $other_comment->getActorPHID(); } + foreach ($requests as $request) { + if ($request->getAuditStatus() == PhabricatorAuditStatusConstants::CC) { + $email_cc[] = $request->getAuditorPHID(); + } + } + $phids = array_merge($email_to, $email_cc); $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); diff --git a/src/applications/audit/editor/comment/__init__.php b/src/applications/audit/editor/comment/__init__.php index 2610bb9286..ac18064b6f 100644 --- a/src/applications/audit/editor/comment/__init__.php +++ b/src/applications/audit/editor/comment/__init__.php @@ -13,6 +13,7 @@ phutil_require_module('phabricator', 'applications/audit/storage/inlinecommment' phutil_require_module('phabricator', 'applications/diffusion/query/path'); phutil_require_module('phabricator', 'applications/feed/constants/story'); phutil_require_module('phabricator', 'applications/feed/publisher'); +phutil_require_module('phabricator', 'applications/markup/engine'); phutil_require_module('phabricator', 'applications/metamta/storage/mail'); phutil_require_module('phabricator', 'applications/owners/storage/owner'); phutil_require_module('phabricator', 'applications/owners/storage/package'); diff --git a/src/applications/audit/storage/auditcomment/PhabricatorAuditComment.php b/src/applications/audit/storage/auditcomment/PhabricatorAuditComment.php index 9207ba6bf3..1a02343fc4 100644 --- a/src/applications/audit/storage/auditcomment/PhabricatorAuditComment.php +++ b/src/applications/audit/storage/auditcomment/PhabricatorAuditComment.php @@ -18,14 +18,21 @@ final class PhabricatorAuditComment extends PhabricatorAuditDAO { + const METADATA_ADDED_AUDITORS = 'added-auditors'; + const METADATA_ADDED_CCS = 'added-ccs'; + protected $phid; protected $actorPHID; protected $targetPHID; protected $action; protected $content; + protected $metadata = array(); public function getConfiguration() { return array( + self::CONFIG_SERIALIZATION => array( + 'metadata' => self::SERIALIZATION_JSON, + ), self::CONFIG_AUX_PHID => true, ) + parent::getConfiguration(); } diff --git a/src/applications/diffusion/controller/commit/DiffusionCommitController.php b/src/applications/diffusion/controller/commit/DiffusionCommitController.php index 2ab79be19a..fd2ba880c7 100644 --- a/src/applications/diffusion/controller/commit/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/commit/DiffusionCommitController.php @@ -458,6 +458,22 @@ final class DiffusionCommitController extends DiffusionController { ->setName('action') ->setID('audit-action') ->setOptions($actions)) + ->appendChild( + id(new AphrontFormTokenizerControl()) + ->setLabel('Add Auditors') + ->setName('auditors') + ->setControlID('add-auditors') + ->setControlStyle('display: none') + ->setID('add-auditors-tokenizer') + ->setDisableBehavior(true)) + ->appendChild( + id(new AphrontFormTokenizerControl()) + ->setLabel('Add CCs') + ->setName('ccs') + ->setControlID('add-ccs') + ->setControlStyle('display: none') + ->setID('add-ccs-tokenizer') + ->setDisableBehavior(true)) ->appendChild( id(new AphrontFormTextAreaControl()) ->setLabel('Comments') @@ -483,11 +499,37 @@ final class DiffusionCommitController extends DiffusionController { require_celerity_resource('phabricator-transaction-view-css'); - Javelin::initBehavior('audit-preview', array( + Javelin::initBehavior( + 'differential-add-reviewers-and-ccs', + array( + 'dynamic' => array( + 'add-auditors-tokenizer' => array( + 'actions' => array('add_auditors' => 1), + 'src' => '/typeahead/common/users/', + 'row' => 'add-auditors', + 'ondemand' => PhabricatorEnv::getEnvConfig('tokenizer.ondemand'), + 'placeholder' => 'Type a user name...', + ), + 'add-ccs-tokenizer' => array( + 'actions' => array('add_ccs' => 1), + 'src' => '/typeahead/common/mailable/', + 'row' => 'add-ccs', + 'ondemand' => PhabricatorEnv::getEnvConfig('tokenizer.ondemand'), + 'placeholder' => 'Type a user or mailing list...', + ), + ), + 'select' => 'audit-action', + )); + + Javelin::initBehavior('differential-feedback-preview', array( 'uri' => '/audit/preview/'.$commit->getID().'/', 'preview' => 'audit-preview', 'content' => 'audit-content', 'action' => 'audit-action', + 'previewTokenizers' => array( + 'auditors' => 'add-auditors-tokenizer', + 'ccs' => 'add-ccs-tokenizer', + ), )); $preview_panel = @@ -530,6 +572,8 @@ final class DiffusionCommitController extends DiffusionController { $actions = array(); $actions[PhabricatorAuditActionConstants::COMMENT] = true; + $actions[PhabricatorAuditActionConstants::ADD_CCS] = true; + $actions[PhabricatorAuditActionConstants::ADD_AUDITORS] = true; // We allow you to accept your own commits. A use case here is that you // notice an issue with your own commit and "Raise Concern" as an indicator diff --git a/src/applications/diffusion/controller/commit/__init__.php b/src/applications/diffusion/controller/commit/__init__.php index cfcfa786cb..8cfd49ef45 100644 --- a/src/applications/diffusion/controller/commit/__init__.php +++ b/src/applications/diffusion/controller/commit/__init__.php @@ -45,6 +45,7 @@ phutil_require_module('phabricator', 'view/form/base'); phutil_require_module('phabricator', 'view/form/control/select'); phutil_require_module('phabricator', 'view/form/control/submit'); phutil_require_module('phabricator', 'view/form/control/textarea'); +phutil_require_module('phabricator', 'view/form/control/tokenizer'); phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/headsup/action'); phutil_require_module('phabricator', 'view/layout/headsup/actionlist'); diff --git a/src/applications/diffusion/view/comment/DiffusionCommentView.php b/src/applications/diffusion/view/comment/DiffusionCommentView.php index 8e1a8c611d..46aedc8219 100644 --- a/src/applications/diffusion/view/comment/DiffusionCommentView.php +++ b/src/applications/diffusion/view/comment/DiffusionCommentView.php @@ -115,8 +115,27 @@ final class DiffusionCommentView extends AphrontView { $action = $comment->getAction(); $verb = PhabricatorAuditActionConstants::getActionPastTenseVerb($action); + $metadata = $comment->getMetadata(); + $added_auditors = idx( + $metadata, + PhabricatorAuditComment::METADATA_ADDED_AUDITORS, + array()); + $added_ccs = idx( + $metadata, + PhabricatorAuditComment::METADATA_ADDED_CCS, + array()); + $actions = array(); - $actions[] = "{$author_link} ".phutil_escape_html($verb)." this commit."; + if ($action == PhabricatorAuditActionConstants::ADD_CCS) { + $rendered_ccs = $this->renderHandleList($added_ccs); + $actions[] = "{$author_link} added CCs: {$rendered_ccs}."; + } else if ($action == PhabricatorAuditActionConstants::ADD_AUDITORS) { + $rendered_auditors = $this->renderHandleList($added_auditors); + $actions[] = "{$author_link} added auditors: ". + "{$rendered_auditors}."; + } else { + $actions[] = "{$author_link} ".phutil_escape_html($verb)." this commit."; + } foreach ($actions as $key => $action) { $actions[$key] = '
'.$action.'
'; @@ -179,6 +198,14 @@ final class DiffusionCommentView extends AphrontView { return $this->engine; } + private function renderHandleList(array $phids) { + $result = array(); + foreach ($phids as $phid) { + $result[] = $this->handles[$phid]->renderLink(); + } + return implode(', ', $result); + } + private function renderClasses() { $comment = $this->comment; diff --git a/src/applications/diffusion/view/comment/__init__.php b/src/applications/diffusion/view/comment/__init__.php index 5e87e7e83d..b8d9fe82d0 100644 --- a/src/applications/diffusion/view/comment/__init__.php +++ b/src/applications/diffusion/view/comment/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('phabricator', 'applications/audit/constants/action'); +phutil_require_module('phabricator', 'applications/audit/storage/auditcomment'); phutil_require_module('phabricator', 'applications/markup/engine'); phutil_require_module('phabricator', 'infrastructure/diff/view/inline'); phutil_require_module('phabricator', 'view/base'); diff --git a/src/applications/diffusion/view/commentlist/DiffusionCommentListView.php b/src/applications/diffusion/view/commentlist/DiffusionCommentListView.php index fec0b549d5..6c3aa00b69 100644 --- a/src/applications/diffusion/view/commentlist/DiffusionCommentListView.php +++ b/src/applications/diffusion/view/commentlist/DiffusionCommentListView.php @@ -49,6 +49,18 @@ final class DiffusionCommentListView extends AphrontView { $phids = array(); foreach ($this->comments as $comment) { $phids[$comment->getActorPHID()] = true; + $metadata = $comment->getMetaData(); + + $ccs_key = PhabricatorAuditComment::METADATA_ADDED_CCS; + $added_ccs = idx($metadata, $ccs_key, array()); + foreach ($added_ccs as $cc) { + $phids[$cc] = true; + } + $auditors_key = PhabricatorAuditComment::METADATA_ADDED_AUDITORS; + $added_auditors = idx($metadata, $auditors_key, array()); + foreach ($added_auditors as $auditor) { + $phids[$auditor] = true; + } } foreach ($this->inlineComments as $comment) { $phids[$comment->getAuthorPHID()] = true; diff --git a/src/applications/diffusion/view/commentlist/__init__.php b/src/applications/diffusion/view/commentlist/__init__.php index 740dae8035..aa7060f441 100644 --- a/src/applications/diffusion/view/commentlist/__init__.php +++ b/src/applications/diffusion/view/commentlist/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/audit/storage/auditcomment'); phutil_require_module('phabricator', 'applications/diffusion/view/comment'); phutil_require_module('phabricator', 'view/base');