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
2025-02-05 11:28:25 +01:00 · 2012-04-23 13:50:04 -07:00 · 2012-04-23 13:50:04 -07:00 · 42b1c73f41
commit 42b1c73f41
parent 8069694640
15 changed files with 252 additions and 9 deletions
--- a/resources/sql/patches/137.auditmetadata.sql
+++ 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 = '';
--- 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');
  }
--- 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;
--- 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();

--- 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);

--- 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');
--- 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();

--- 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');
--- 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();
  }
--- 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
--- 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');
--- 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] = '<div>'.$action.'</div>';
@ -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;

--- 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');
--- 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;
--- 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');