Remove needRecipients and needAwards from Badges

Summary: Fixes T10798. Separates these two since they don't need to be combined and it allows for more flexibility / scalability.

Test Plan:
- Add Badge
- Edit Badge
- Add myself as Recipient
- Remove myself
- Go to my profile
- Award Badge from there
- Assign myself a badge, try to re-assign it, see validation error.

Also, validation errors on dialog forms are ugly.

{F3495630}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10798, T12270

Differential Revision: https://secure.phabricator.com/D17447

src/applications/badges/controller/PhabricatorBadgesAwardController.php

@@ -22,7 +22,6 @@ final class PhabricatorBadgesAwardController
       $badges = id(new PhabricatorBadgesQuery())
         ->setViewer($viewer)
         ->withPHIDs($badge_phids)
-        ->needRecipients(true)
         ->requireCapabilities(
           array(
             PhabricatorPolicyCapability::CAN_EDIT,

src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php

@@ -11,7 +11,6 @@ final class PhabricatorBadgesEditRecipientsController
     $badge = id(new PhabricatorBadgesQuery())
       ->setViewer($viewer)
       ->withIDs(array($id))
-      ->needRecipients(true)
       ->requireCapabilities(
         array(
           PhabricatorPolicyCapability::CAN_EDIT,
@@ -23,8 +22,6 @@ final class PhabricatorBadgesEditRecipientsController
     }
 
     $view_uri = $this->getApplicationURI('recipients/'.$badge->getID().'/');
-    $awards = $badge->getAwards();
-    $recipient_phids = mpull($awards, 'getRecipientPHID');
 
     if ($request->isFormPost()) {
       $award_phids = array();
@@ -52,17 +49,6 @@ final class PhabricatorBadgesEditRecipientsController
         ->setURI($view_uri);
     }
 
-    $recipient_phids = array_reverse($recipient_phids);
-    $handles = $this->loadViewerHandles($recipient_phids);
-
-    $state = array();
-    foreach ($handles as $handle) {
-      $state[] = array(
-        'phid' => $handle->getPHID(),
-        'name' => $handle->getFullName(),
-      );
-    }
-
     $can_edit = PhabricatorPolicyFilter::hasCapability(
       $viewer,
       $badge,

src/applications/badges/controller/PhabricatorBadgesRecipientsController.php

@@ -14,14 +14,21 @@ final class PhabricatorBadgesRecipientsController
     $badge = id(new PhabricatorBadgesQuery())
       ->setViewer($viewer)
       ->withIDs(array($id))
-      ->needRecipients(true)
       ->executeOne();
     if (!$badge) {
       return new Aphront404Response();
     }
-
     $this->setBadge($badge);
 
+    $awards = id(new PhabricatorBadgesAwardQuery())
+      ->setViewer($viewer)
+      ->withBadgePHIDs(array($badge->getPHID()))
+      ->execute();
+
+    $recipient_phids = mpull($awards, 'getRecipientPHID');
+    $recipient_phids = array_reverse($recipient_phids);
+    $handles = $this->loadViewerHandles($recipient_phids);
+
     $crumbs = $this->buildApplicationCrumbs();
     $crumbs->addTextCrumb(pht('Recipients'));
     $crumbs->setBorder(true);
@@ -29,13 +36,9 @@ final class PhabricatorBadgesRecipientsController
 
     $header = $this->buildHeaderView();
 
-    $awards = $badge->getAwards();
-    $recipient_phids = mpull($awards, 'getRecipientPHID');
-    $recipient_phids = array_reverse($recipient_phids);
-    $handles = $this->loadViewerHandles($recipient_phids);
-
     $recipient_list = id(new PhabricatorBadgesRecipientsListView())
       ->setBadge($badge)
+      ->setAwards($awards)
       ->setHandles($handles)
       ->setUser($viewer);
 

src/applications/badges/controller/PhabricatorBadgesRemoveRecipientsController.php

@@ -10,7 +10,6 @@ final class PhabricatorBadgesRemoveRecipientsController
     $badge = id(new PhabricatorBadgesQuery())
       ->setViewer($viewer)
       ->withIDs(array($id))
-      ->needRecipients(true)
       ->requireCapabilities(
         array(
           PhabricatorPolicyCapability::CAN_VIEW,
@@ -21,14 +20,7 @@ final class PhabricatorBadgesRemoveRecipientsController
       return new Aphront404Response();
     }
 
-    $awards = $badge->getAwards();
-    $recipient_phids = mpull($awards, 'getRecipientPHID');
     $remove_phid = $request->getStr('phid');
-
-    if (!in_array($remove_phid, $recipient_phids)) {
-      return new Aphront404Response();
-    }
-
     $view_uri = $this->getApplicationURI('recipients/'.$badge->getID().'/');
 
     if ($request->isFormPost()) {

src/applications/badges/query/PhabricatorBadgesAwardQuery.php

@@ -9,21 +9,19 @@ final class PhabricatorBadgesAwardQuery
 
 
   protected function willFilterPage(array $awards) {
+    $badge_phids = array();
+    foreach ($awards as $key => $award) {
+      $badge_phids[] = $award->getBadgePHID();
+    }
+
     $badges = id(new PhabricatorBadgesQuery())
       ->setViewer($this->getViewer())
-      ->withRecipientPHIDs(mpull($awards, null, 'getRecipientPHID'))
+      ->withPHIDs($badge_phids)
       ->execute();
 
     $badges = mpull($badges, null, 'getPHID');
-
     foreach ($awards as $key => $award) {
       $award_badge = idx($badges, $award->getBadgePHID());
-      if ($award_badge === null) {
-        $this->didRejectResult($award);
-        unset($awards[$key]);
-        continue;
-      }
-
       $award->attachBadge($award_badge);
     }
 

src/applications/badges/query/PhabricatorBadgesQuery.php

@@ -7,9 +7,6 @@ final class PhabricatorBadgesQuery
   private $phids;
   private $qualities;
   private $statuses;
-  private $recipientPHIDs;
-
-  private $needRecipients;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -31,22 +28,12 @@ final class PhabricatorBadgesQuery
     return $this;
   }
 
-  public function withRecipientPHIDs(array $recipient_phids) {
-    $this->recipientPHIDs = $recipient_phids;
-    return $this;
-  }
-
   public function withNameNgrams($ngrams) {
     return $this->withNgramsConstraint(
       id(new PhabricatorBadgesBadgeNameNgrams()),
       $ngrams);
   }
 
-  public function needRecipients($need_recipients) {
-    $this->needRecipients = $need_recipients;
-    return $this;
-  }
-
   protected function loadPage() {
     return $this->loadStandardPage($this->newResultObject());
   }
@@ -59,24 +46,6 @@ final class PhabricatorBadgesQuery
     return new PhabricatorBadgesBadge();
   }
 
-  protected function didFilterPage(array $badges) {
-    if ($this->needRecipients) {
-      $query = id(new PhabricatorBadgesAwardQuery())
-        ->setViewer($this->getViewer())
-        ->withBadgePHIDs(mpull($badges, 'getPHID'))
-        ->execute();
-
-      $awards = mgroup($query, 'getBadgePHID');
-
-      foreach ($badges as $badge) {
-        $badge_awards = idx($awards, $badge->getPHID(), array());
-        $badge->attachAwards($badge_awards);
-      }
-    }
-
-    return $badges;
-  }
-
   protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
     $where = parent::buildWhereClauseParts($conn);
 

src/applications/badges/storage/PhabricatorBadgesBadge.php

@@ -20,8 +20,6 @@ final class PhabricatorBadgesBadge extends PhabricatorBadgesDAO
   protected $status;
   protected $creatorPHID;
 
-  private $awards = self::ATTACHABLE;
-
   const STATUS_ACTIVE = 'open';
   const STATUS_ARCHIVED = 'closed';
 
@@ -84,15 +82,6 @@ final class PhabricatorBadgesBadge extends PhabricatorBadgesDAO
     return ($this->getStatus() == self::STATUS_ARCHIVED);
   }
 
-  public function attachAwards(array $awards) {
-    $this->awards = $awards;
-    return $this;
-  }
-
-  public function getAwards() {
-    return $this->assertAttached($this->awards);
-  }
-
   public function getViewURI() {
     return '/badges/view/'.$this->getID().'/';
   }

src/applications/badges/view/PhabricatorBadgesRecipientsListView.php

@@ -3,6 +3,7 @@
 final class PhabricatorBadgesRecipientsListView extends AphrontView {
 
   private $badge;
+  private $awards;
   private $handles;
 
   public function setBadge(PhabricatorBadgesBadge $badge) {
@@ -10,6 +11,11 @@ final class PhabricatorBadgesRecipientsListView extends AphrontView {
     return $this;
   }
 
+  public function setAwards(array $awards) {
+    $this->awards = $awards;
+    return $this;
+  }
+
   public function setHandles(array $handles) {
     $this->handles = $handles;
     return $this;
@@ -20,7 +26,7 @@ final class PhabricatorBadgesRecipientsListView extends AphrontView {
 
     $badge = $this->badge;
     $handles = $this->handles;
-    $awards = mpull($badge->getAwards(), null, 'getRecipientPHID');
+    $awards = mpull($this->awards, null, 'getRecipientPHID');
 
     $can_edit = PhabricatorPolicyFilter::hasCapability(
       $viewer,

src/applications/badges/xaction/PhabricatorBadgesBadgeAwardTransaction.php

@@ -6,25 +6,17 @@ final class PhabricatorBadgesBadgeAwardTransaction
   const TRANSACTIONTYPE = 'badge.award';
 
   public function generateOldValue($object) {
-    return mpull($object->getAwards(), 'getRecipientPHID');
+    return null;
   }
 
   public function applyExternalEffects($object, $value) {
-    $awards = $object->getAwards();
-    $awards = mpull($awards, null, 'getRecipientPHID');
-
     foreach ($value as $phid) {
-      $award = idx($awards, $phid);
-      if (!$award) {
-        $award = PhabricatorBadgesAward::initializeNewBadgesAward(
-          $this->getActor(),
-          $object,
-          $phid);
-        $award->save();
-        $awards[] = $award;
-      }
+      $award = PhabricatorBadgesAward::initializeNewBadgesAward(
+        $this->getActor(),
+        $object,
+        $phid);
+      $award->save();
     }
-    $object->attachAwards($awards);
     return;
   }
 
@@ -71,6 +63,7 @@ final class PhabricatorBadgesBadgeAwardTransaction
       }
 
       foreach ($user_phids as $user_phid) {
+        // Check if a valid user
         $user = id(new PhabricatorPeopleQuery())
           ->setViewer($this->getActor())
           ->withPHIDs(array($user_phid))
@@ -80,6 +73,20 @@ final class PhabricatorBadgesBadgeAwardTransaction
             pht(
               'Recipient PHID "%s" is not a valid user PHID.',
               $user_phid));
+          continue;
+        }
+
+        // Check if already awarded
+        $award = id(new PhabricatorBadgesAwardQuery())
+          ->setViewer($this->getActor())
+          ->withRecipientPHIDs(array($user_phid))
+          ->withBadgePHIDs(array($object->getPHID()))
+          ->executeOne();
+        if ($award) {
+          $errors[] = $this->newInvalidError(
+            pht(
+              '%s has already been awarded this badge.',
+              $user->getUsername()));
         }
       }
     }

src/applications/badges/xaction/PhabricatorBadgesBadgeRevokeTransaction.php

@@ -10,13 +10,16 @@ final class PhabricatorBadgesBadgeRevokeTransaction
   }
 
   public function applyExternalEffects($object, $value) {
-    $awards = $object->getAwards();
+    $awards = id(new PhabricatorBadgesAwardQuery())
+      ->setViewer($this->getActor())
+      ->withRecipientPHIDs($value)
+      ->withBadgePHIDs(array($object->getPHID()))
+      ->execute();
     $awards = mpull($awards, null, 'getRecipientPHID');
 
     foreach ($value as $phid) {
       $awards[$phid]->delete();
     }
-    $object->attachAwards($awards);
     return;
   }