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).
This commit is contained in:
Mikael Nordfeldth 2015-03-04 11:38:04 +01:00
parent 8c933a6c06
commit ec4e432d55
9 changed files with 27 additions and 19 deletions

View File

@ -100,8 +100,8 @@ class ApiFriendshipsCreateAction extends ApiAuthAction
try { try {
Subscription::start($this->scoped, $this->other); Subscription::start($this->scoped, $this->other);
} catch (Exception $e) { } catch (AlreadyFulfilledException $e) {
$this->clientError($e->getMessage(), 403); $this->clientError($e->getMessage(), 409);
} }
$this->initDocument($this->format); $this->initDocument($this->format);

View File

@ -230,18 +230,11 @@ class AtompubsubscriptionfeedAction extends AtompubAction
$this->clientError(sprintf(_('Unknown profile %s.'), $person->id)); $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 // 409 Conflict
// TRANS: Client error displayed trying to subscribe to an already subscribed profile. $this->clientError($e->getMessage(), 409);
// 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));
} }
Event::handle('EndAtomPubNewActivity', array($activity, $sub)); Event::handle('EndAtomPubNewActivity', array($activity, $sub));

View File

@ -118,7 +118,7 @@ class InviteAction extends Action
$this->already[] = $other; $this->already[] = $other;
} else { } else {
try { try {
Subscription::start($profile, $other); Subscription::ensureStart($profile, $other);
$this->subbed[] = $other; $this->subbed[] = $other;
} catch (Exception $e) { } catch (Exception $e) {
// subscription failed, but keep working // subscription failed, but keep working

View File

@ -122,7 +122,7 @@ class SubscribeAction extends Action
{ {
// Throws exception on error // Throws exception on error
$sub = Subscription::start($this->user->getProfile(), $sub = Subscription::ensureStart($this->user->getProfile(),
$this->other); $this->other);
if ($this->boolean('ajax')) { if ($this->boolean('ajax')) {

View File

@ -94,6 +94,7 @@ class Subscription extends Managed_DataObject
if (Event::handle('StartSubscribe', array($subscriber, $other))) { if (Event::handle('StartSubscribe', array($subscriber, $other))) {
$otherUser = User::getKV('id', $other->id); $otherUser = User::getKV('id', $other->id);
if ($otherUser instanceof User && $otherUser->subscribe_policy == User::SUBSCRIBE_POLICY_MODERATE && !$force) { 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 = Subscription_queue::saveNew($subscriber, $other);
$sub->notify(); $sub->notify();
} else { } else {
@ -132,6 +133,17 @@ class Subscription extends Managed_DataObject
return $sub; 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. * Low-level subscription save.
* Outside callers should use Subscription::start() * Outside callers should use Subscription::start()

View File

@ -36,6 +36,9 @@ class Subscription_queue extends Managed_DataObject
public static function saveNew(Profile $subscriber, Profile $subscribed) 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 = new Subscription_queue();
$rq->subscriber = $subscriber->id; $rq->subscriber = $subscriber->id;
$rq->subscribed = $subscribed->id; $rq->subscribed = $subscribed->id;

View File

@ -354,7 +354,7 @@ class User extends Managed_DataObject
common_log(LOG_WARNING, sprintf("Default user %s does not exist.", $defnick), common_log(LOG_WARNING, sprintf("Default user %s does not exist.", $defnick),
__FILE__); __FILE__);
} else { } else {
Subscription::start($profile, $defuser->getProfile()); Subscription::ensureStart($profile, $defuser->getProfile());
} }
} }

View File

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

View File

@ -146,7 +146,7 @@ class ActivityMover extends QueueHandler
"Changing sub to {$act->objects[0]->id}". "Changing sub to {$act->objects[0]->id}".
"by {$act->actor->id} to {$remote->nickname}."); "by {$act->actor->id} to {$remote->nickname}.");
$otherProfile = $otherUser->getProfile(); $otherProfile = $otherUser->getProfile();
Subscription::start($otherProfile, $remote); Subscription::ensureStart($otherProfile, $remote);
Subscription::cancel($otherProfile, $user->getProfile()); Subscription::cancel($otherProfile, $user->getProfile());
} else { } else {
$this->log(LOG_NOTICE, $this->log(LOG_NOTICE,