Subscription::ensureStart skips AlreadyFulfilledException

Sometimes we just want to accept the user's wrong, but when it comes
to remote APIs etc. we probably want to let the client know it has
done something already (in this case multiple identical subscription
requests - which might indicate to it that it should refresh the sub
lists or something).

diff --git a/actions/apifriendshipscreate.php b/actions/apifriendshipscreate.php
index e7caf9686914b7720eeec02dca120ba5856b5119..3997a8b51c4161d48621e0112bb143fe7683903f 100644 (file)
--- a/actions/apifriendshipscreate.php
+++ b/actions/apifriendshipscreate.php
@@ -100,8 +100,8 @@ class ApiFriendshipsCreateAction extends ApiAuthAction
 
         try {
             Subscription::start($this->scoped, $this->other);
-        } catch (Exception $e) {
-            $this->clientError($e->getMessage(), 403);
+        } catch (AlreadyFulfilledException $e) {
+            $this->clientError($e->getMessage(), 409);
         }
 
         $this->initDocument($this->format);

diff --git a/actions/atompubsubscriptionfeed.php b/actions/atompubsubscriptionfeed.php
index 413277a5d355e31150189dd8ecd0385a59064ec5..6a483b95c09f7c1cfea1d7ea0749add33a300758 100644 (file)
--- a/actions/atompubsubscriptionfeed.php
+++ b/actions/atompubsubscriptionfeed.php
@@ -230,18 +230,11 @@ class AtompubsubscriptionfeedAction extends AtompubAction
                 $this->clientError(sprintf(_('Unknown profile %s.'), $person->id));
             }
 
-            if (Subscription::exists($this->_profile, $profile)) {
+            try {
+                $sub = Subscription::start($this->_profile, $profile);
+            } catch (AlreadyFulfilledException $e) {
                 // 409 Conflict
-                // TRANS: Client error displayed trying to subscribe to an already subscribed profile.
-                // TRANS: %s is the profile the user already has a subscription on.
-                $this->clientError(sprintf(_('Already subscribed to %s.'),
-                                           $person->id),
-                                   409);
-            }
-
-            if (Subscription::start($this->_profile, $profile)) {
-                $sub = Subscription::pkeyGet(array('subscriber' => $this->_profile->id,
-                                                   'subscribed' => $profile->id));
+                $this->clientError($e->getMessage(), 409);
             }
 
             Event::handle('EndAtomPubNewActivity', array($activity, $sub));

diff --git a/actions/invite.php b/actions/invite.php
index f99dd4d783497a0cd76f8f170de7254362cd4e32..89b7e83bf6d19a32899e05db5fe2b32ac1439cb3 100644 (file)
--- a/actions/invite.php
+++ b/actions/invite.php
@@ -118,7 +118,7 @@ class InviteAction extends Action
                         $this->already[] = $other;
                     } else {
                         try {
-                            Subscription::start($profile, $other);
+                            Subscription::ensureStart($profile, $other);
                             $this->subbed[] = $other;
                         } catch (Exception $e) {
                             // subscription failed, but keep working

diff --git a/actions/subscribe.php b/actions/subscribe.php
index 4002c9fbb0164267250333f87255f782489616b1..320409afa07460d36c94d427c4ee8bc014667375 100644 (file)
--- a/actions/subscribe.php
+++ b/actions/subscribe.php
@@ -122,7 +122,7 @@ class SubscribeAction extends Action
     {
         // Throws exception on error
 
-        $sub = Subscription::start($this->user->getProfile(),
+        $sub = Subscription::ensureStart($this->user->getProfile(),
                                    $this->other);
 
         if ($this->boolean('ajax')) {

diff --git a/classes/Subscription.php b/classes/Subscription.php
index 5c5101ad1d1bb430ed0493037782b0ed4c0c87b9..ec9ae518415dc9f347e194742f5356821b51e7e1 100644 (file)
--- a/classes/Subscription.php
+++ b/classes/Subscription.php
@@ -94,6 +94,7 @@ class Subscription extends Managed_DataObject
         if (Event::handle('StartSubscribe', array($subscriber, $other))) {
             $otherUser = User::getKV('id', $other->id);
             if ($otherUser instanceof User && $otherUser->subscribe_policy == User::SUBSCRIBE_POLICY_MODERATE && !$force) {
+                // Will throw an AlreadyFulfilledException if this queue item already exists.
                 $sub = Subscription_queue::saveNew($subscriber, $other);
                 $sub->notify();
             } else {
@@ -132,6 +133,17 @@ class Subscription extends Managed_DataObject
         return $sub;
     }
 
+    static function ensureStart(Profile $subscriber, Profile $other, $force=false)
+    {
+        try {
+            $sub = self::start($subscriber, $other, $force);
+        } catch (AlreadyFulfilledException $e) {
+            // Nothing to see here, move along...
+            return self::getSubscription($subscriber, $other);
+        }
+        return $sub;
+    }
+
     /**
      * Low-level subscription save.
      * Outside callers should use Subscription::start()

diff --git a/classes/Subscription_queue.php b/classes/Subscription_queue.php
index 405eca93fd0f7309ffdd6c5e78b1375c2918120e..02cc72f1f24c82b32e6f3085b55c4a7c81cebe60 100644 (file)
--- a/classes/Subscription_queue.php
+++ b/classes/Subscription_queue.php
@@ -36,6 +36,9 @@ class Subscription_queue extends Managed_DataObject
 
     public static function saveNew(Profile $subscriber, Profile $subscribed)
     {
+        if (self::exists($subscriber, $subscribed)) {
+            throw new AlreadyFulfilledException(_('This subscription request is already in progress.'));
+        }
         $rq = new Subscription_queue();
         $rq->subscriber = $subscriber->id;
         $rq->subscribed = $subscribed->id;

diff --git a/classes/User.php b/classes/User.php
index 2f4670a2dfbe8092e400d1f5dc7ed1724674ea71..f543a7552855fb74af4ac729bf8cfb579c3e2c47 100644 (file)
--- a/classes/User.php
+++ b/classes/User.php
@@ -354,7 +354,7 @@ class User extends Managed_DataObject
                     common_log(LOG_WARNING, sprintf("Default user %s does not exist.", $defnick),
                                __FILE__);
                 } else {
-                    Subscription::start($profile, $defuser->getProfile());
+                    Subscription::ensureStart($profile, $defuser->getProfile());
                 }
             }
 

diff --git a/lib/activityimporter.php b/lib/activityimporter.php
index 5bef4cfb072475e66cdaf73d1b4b4267107f78ed..c4dd797e6d8076e11767e1fcb9a950b05aca30fa 100644 (file)
--- a/lib/activityimporter.php
+++ b/lib/activityimporter.php
@@ -109,7 +109,7 @@ class ActivityImporter extends QueueHandler
 
             // XXX: don't do this for untrusted input!
 
-            Subscription::start($otherProfile, $profile);
+            Subscription::ensureStart($otherProfile, $profile);
         } else if (empty($activity->actor)
                    || $activity->actor->id == $author->id) {
 
@@ -123,7 +123,7 @@ class ActivityImporter extends QueueHandler
                 throw new ClientException(_('Unknown profile.'));
             }
 
-            Subscription::start($profile, $otherProfile);
+            Subscription::ensureStart($profile, $otherProfile);
         } else {
             // TRANS: Client exception thrown when trying to import an event not related to the importing user.
             throw new Exception(_('This activity seems unrelated to our user.'));

diff --git a/lib/activitymover.php b/lib/activitymover.php
index fe33e9081ec1236c3670a6226ffb8466a6c79183..ac828d94912f940a56fa1d9587a104d2bf02cf34 100644 (file)
--- a/lib/activitymover.php
+++ b/lib/activitymover.php
@@ -146,7 +146,7 @@ class ActivityMover extends QueueHandler
                                "Changing sub to {$act->objects[0]->id}".
                                "by {$act->actor->id} to {$remote->nickname}.");
                     $otherProfile = $otherUser->getProfile();
-                    Subscription::start($otherProfile, $remote);
+                    Subscription::ensureStart($otherProfile, $remote);
                     Subscription::cancel($otherProfile, $user->getProfile());
                 } else {
                     $this->log(LOG_NOTICE,