From ec4e432d5513b225492d10fcb635593fdf3c2117 Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Wed, 4 Mar 2015 11:38:04 +0100 Subject: [PATCH] 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). --- actions/apifriendshipscreate.php | 4 ++-- actions/atompubsubscriptionfeed.php | 15 ++++----------- actions/invite.php | 2 +- actions/subscribe.php | 2 +- classes/Subscription.php | 12 ++++++++++++ classes/Subscription_queue.php | 3 +++ classes/User.php | 2 +- lib/activityimporter.php | 4 ++-- lib/activitymover.php | 2 +- 9 files changed, 27 insertions(+), 19 deletions(-) diff --git a/actions/apifriendshipscreate.php b/actions/apifriendshipscreate.php index e7caf96869..3997a8b51c 100644 --- 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 413277a5d3..6a483b95c0 100644 --- 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 f99dd4d783..89b7e83bf6 100644 --- 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 4002c9fbb0..320409afa0 100644 --- 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 5c5101ad1d..ec9ae51841 100644 --- 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 405eca93fd..02cc72f1f2 100644 --- 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 2f4670a2df..f543a75528 100644 --- 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 5bef4cfb07..c4dd797e6d 100644 --- 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 fe33e9081e..ac828d9491 100644 --- 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,