Enable herald rule for commits that need auditing

Summary: enable herald commit rules to have access to auditing info. Note that the new herald condition I added contains info for the packages. I thought about using a simpler herald condition like "Requires audit is true or false" and let it work together with the existing "Affected package contains any of the package". It doesn't work because we need the info about the package to decide if the commit requires audit, but the herald conditions work separately. Test Plan: - A commit requiring auditing was detected by a herald rule that checks the auditing status - A commit not requiring auditing was not detected by a herald rule which checks auditing status, but was detected by a rule which doesn't check the auditing status Reviewers: epriestley, nh Reviewed By: epriestley CC: aran, epriestley Differential Revision: https://secure.phabricator.com/D1399
2024-11-10 08:52:39 +01:00 · 2012-01-13 23:08:19 -08:00 · 2012-01-13 23:08:19 -08:00 · 4faab06c3c
commit 4faab06c3c
parent 05ee317555
9 changed files with 73 additions and 44 deletions
--- a/src/applications/herald/adapter/commit/HeraldCommitAdapter.php
+++ b/src/applications/herald/adapter/commit/HeraldCommitAdapter.php
@ -1,7 +1,7 @@
 <?php

 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@ -30,6 +30,7 @@ class HeraldCommitAdapter extends HeraldObjectAdapter {
  protected $affectedPaths;
  protected $affectedRevision;
  protected $affectedPackages;
+  protected $auditNeededPackages;

  public function __construct(
    PhabricatorRepository $repository,
@ -79,6 +80,24 @@ class HeraldCommitAdapter extends HeraldObjectAdapter {
    return $this->affectedPackages;
  }

+  public function loadAuditNeededPackage() {
+    if ($this->auditNeededPackages === null) {
+      $status_arr = array (
+        PhabricatorAuditStatusConstants::AUDIT_REQUIRED,
+          PhabricatorAuditStatusConstants::CONCERNED,
+        );
+      $relationships = id(new PhabricatorOwnersPackageCommitRelationship())
+          ->loadAllWhere(
+        "commitPHID = %s AND auditStatus IN (%Ls)",
+        $this->commit->getPHID(),
+        $status_arr);
+
+      $packages = mpull($relationships, 'getPackagePHID');
+      $this->auditNeededPackages = $packages;
+    }
+    return $this->auditNeededPackages;
+  }
+
  public function loadDifferentialRevision() {
    if ($this->affectedRevision === null) {
      $this->affectedRevision = false;
@ -140,6 +159,8 @@ class HeraldCommitAdapter extends HeraldObjectAdapter {
        $packages = $this->loadAffectedPackages();
        $owners = PhabricatorOwnersOwner::loadAllForPackages($packages);
        return mpull($owners, 'getUserPHID');
+      case HeraldFieldConfig::FIELD_NEED_AUDIT_FOR_PACKAGE:
+        return $this->loadAuditNeededPackage();
      case HeraldFieldConfig::FIELD_DIFFERENTIAL_REVISION:
        $revision = $this->loadDifferentialRevision();
        if (!$revision) {
--- a/src/applications/herald/adapter/commit/init.php
+++ b/src/applications/herald/adapter/commit/init.php
@ -6,6 +6,7 @@



+phutil_require_module('phabricator', 'applications/audit/constants/status');
 phutil_require_module('phabricator', 'applications/differential/storage/revision');
 phutil_require_module('phabricator', 'applications/herald/adapter/base');
 phutil_require_module('phabricator', 'applications/herald/config/action');
@ -15,6 +16,7 @@ phutil_require_module('phabricator', 'applications/herald/storage/transcript/app
 phutil_require_module('phabricator', 'applications/owners/query/path');
 phutil_require_module('phabricator', 'applications/owners/storage/owner');
 phutil_require_module('phabricator', 'applications/owners/storage/package');
+phutil_require_module('phabricator', 'applications/owners/storage/packagecommitrelationship');

 phutil_require_module('phutil', 'utils');

--- a/src/applications/herald/config/condition/HeraldConditionConfig.php
+++ b/src/applications/herald/config/condition/HeraldConditionConfig.php
@ -1,7 +1,7 @@
 <?php

 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@ -120,6 +120,7 @@ class HeraldConditionConfig {
          ));
      case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE:
      case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE_OWNER:
+      case HeraldFieldConfig::FIELD_NEED_AUDIT_FOR_PACKAGE:
        return array_select_keys(
          $map,
          array(
--- a/src/applications/herald/config/field/HeraldFieldConfig.php
+++ b/src/applications/herald/config/field/HeraldFieldConfig.php
@ -1,7 +1,7 @@
 <?php

 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@ -31,6 +31,7 @@ class HeraldFieldConfig {
  const FIELD_RULE                   = 'rule';
  const FIELD_AFFECTED_PACKAGE       = 'affected-package';
  const FIELD_AFFECTED_PACKAGE_OWNER = 'affected-package-owner';
+  const FIELD_NEED_AUDIT_FOR_PACKAGE  = 'need-audit-for-package';
  const FIELD_DIFFERENTIAL_REVISION  = 'differential-revision';
  const FIELD_DIFFERENTIAL_REVIEWERS = 'differential-reviewers';
  const FIELD_DIFFERENTIAL_CCS       = 'differential-ccs';
@ -51,6 +52,8 @@ class HeraldFieldConfig {
      self::FIELD_RULE                   => 'Another Herald rule',
      self::FIELD_AFFECTED_PACKAGE       => 'Any affected package',
      self::FIELD_AFFECTED_PACKAGE_OWNER => "Any affected package's owner",
+      self::FIELD_NEED_AUDIT_FOR_PACKAGE =>
+                                          'Affected packages that need audit',
      self::FIELD_DIFFERENTIAL_REVISION  => 'Differential revision',
      self::FIELD_DIFFERENTIAL_REVIEWERS => 'Differential reviewers',
      self::FIELD_DIFFERENTIAL_CCS       => 'Differential CCs',
@ -93,6 +96,7 @@ class HeraldFieldConfig {
            self::FIELD_RULE,
            self::FIELD_AFFECTED_PACKAGE,
            self::FIELD_AFFECTED_PACKAGE_OWNER,
+            self::FIELD_NEED_AUDIT_FOR_PACKAGE,
            self::FIELD_DIFFERENTIAL_REVISION,
            self::FIELD_DIFFERENTIAL_REVIEWERS,
            self::FIELD_DIFFERENTIAL_CCS,
--- a/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php
+++ b/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php
@ -1,7 +1,7 @@
 <?php

 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@ -57,6 +57,7 @@ class HeraldValueTypeConfig {
          case HeraldFieldConfig::FIELD_TAGS:
            return self::VALUE_TAG;
          case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE:
+          case HeraldFieldConfig::FIELD_NEED_AUDIT_FOR_PACKAGE:
            return self::VALUE_OWNERS_PACKAGE;
          default:
            return self::VALUE_USER;
--- a/src/applications/herald/engine/engine/HeraldEngine.php
+++ b/src/applications/herald/engine/engine/HeraldEngine.php
@ -1,7 +1,7 @@
 <?php

 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@ -436,6 +436,7 @@ class HeraldEngine {
        break;
      case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE:
      case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE_OWNER:
+      case HeraldFieldConfig::FIELD_NEED_AUDIT_FOR_PACKAGE:
        $result = $this->object->getHeraldField($field);
        if (!is_array($result)) {
          throw new HeraldInvalidFieldException(
--- a/src/applications/repository/worker/commitchangeparser/base/PhabricatorRepositoryCommitChangeParserWorker.php
+++ b/src/applications/repository/worker/commitchangeparser/base/PhabricatorRepositoryCommitChangeParserWorker.php
@ -1,7 +1,7 @@
 <?php

 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@ -76,14 +76,6 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker
    PhabricatorSearchCommitIndexer::indexCommit($commit);

    if ($this->shouldQueueFollowupTasks()) {
-      $herald_task = new PhabricatorWorkerTask();
-      $herald_task->setTaskClass('PhabricatorRepositoryCommitHeraldWorker');
-      $herald_task->setData(
-        array(
-          'commitID' => $commit->getID(),
-        ));
-      $herald_task->save();
-
      $owner_task = new PhabricatorWorkerTask();
      $owner_task->setTaskClass('PhabricatorRepositoryCommitOwnersWorker');
      $owner_task->setData(
--- a/src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php
+++ b/src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php
@ -1,7 +1,7 @@
 <?php

 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@ -29,41 +29,47 @@ class PhabricatorRepositoryCommitOwnersWorker
      $repository,
      $affected_paths);

-    if (!$affected_packages) {
-      return;
-    }
+    if ($affected_packages) {
+      foreach ($affected_packages as $package) {
+        $relationship = id(new PhabricatorOwnersPackageCommitRelationship())
+          ->loadOneWhere('packagePHID=%s AND commitPHID=%s',
+          $package->getPHID(),
+          $commit->getPHID());

-    foreach ($affected_packages as $package) {
-      $relationship = id(new PhabricatorOwnersPackageCommitRelationship())
-        ->loadOneWhere('packagePHID=%s AND commitPHID=%s',
-        $package->getPHID(),
-        $commit->getPHID());
-
-      // Don't update relationship if  it exists  already
-      if (!$relationship) {
-        if ($package->getAuditingEnabled()) {
-          $reasons = $this->checkAuditReasons($commit, $package);
-          if ($reasons) {
-            $audit_status =
-              PhabricatorAuditStatusConstants::AUDIT_REQUIRED;
+        // Don't update relationship if  it exists  already
+        if (!$relationship) {
+          if ($package->getAuditingEnabled()) {
+            $reasons = $this->checkAuditReasons($commit, $package);
+            if ($reasons) {
+              $audit_status =
+                PhabricatorAuditStatusConstants::AUDIT_REQUIRED;
+            } else {
+              $audit_status =
+                PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED;
+            }
          } else {
-            $audit_status =
-              PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED;
+            $reasons = array();
+            $audit_status = PhabricatorAuditStatusConstants::NONE;
          }
-        } else {
-          $reasons = array();
-          $audit_status = PhabricatorAuditStatusConstants::NONE;
+
+          $relationship = new PhabricatorOwnersPackageCommitRelationship();
+          $relationship->setPackagePHID($package->getPHID());
+          $relationship->setCommitPHID($commit->getPHID());
+          $relationship->setAuditReasons($reasons);
+          $relationship->setAuditStatus($audit_status);
+
+          $relationship->save();
        }
-
-        $relationship = new PhabricatorOwnersPackageCommitRelationship();
-        $relationship->setPackagePHID($package->getPHID());
-        $relationship->setCommitPHID($commit->getPHID());
-        $relationship->setAuditReasons($reasons);
-        $relationship->setAuditStatus($audit_status);
-
-        $relationship->save();
      }
    }
+
+    $herald_task = new PhabricatorWorkerTask();
+    $herald_task->setTaskClass('PhabricatorRepositoryCommitHeraldWorker');
+    $herald_task->setData(
+      array(
+        'commitID' => $commit->getID(),
+      ));
+    $herald_task->save();
  }

  private function checkAuditReasons(
--- a/src/applications/repository/worker/owner/init.php
+++ b/src/applications/repository/worker/owner/init.php
@ -14,6 +14,7 @@ phutil_require_module('phabricator', 'applications/owners/storage/package');
 phutil_require_module('phabricator', 'applications/owners/storage/packagecommitrelationship');
 phutil_require_module('phabricator', 'applications/repository/storage/commitdata');
 phutil_require_module('phabricator', 'applications/repository/worker/base');
+phutil_require_module('phabricator', 'infrastructure/daemon/workers/storage/task');

 phutil_require_module('phutil', 'utils');