Consolidate some precondition checks for repeats, fix a couple bits in the repeat command.

Notice::saveNew() now does these checks directly when making a repeat:
* make sure the original is valid and existing
* stop you from repeating your own message
* stop you from repeating something you've previously repeated
* prevent repeats of any non-public messages
* explicit inScope() check to make sure you can read the original too (just in case there's a funky extension at play that changes scoping rules)

These error conditions throw exceptions, which the caller either uses as an error message or passes on up the stack, without having to duplicate the checks in each i/o channel.

actions/apistatusesretweet.php

@@ -78,42 +78,6 @@ class ApiStatusesRetweetAction extends ApiAuthAction
 
         $this->user = $this->auth_user;
 
-        if ($this->user->id == $this->original->profile_id) {
-            // TRANS: Client error displayed trying to repeat an own notice through the API.
-            $this->clientError(_('Cannot repeat your own notice.'),
-                               400, $this->format);
-            return false;
-        }
-
-        // Is it OK to repeat that notice (general enough scope)?
-
-        if ($this->original->scope != Notice::SITE_SCOPE &&
-            $this->original->scope != Notice::PUBLIC_SCOPE) {
-            $this->clientError(_('You may not repeat a private notice.'),
-                               403,
-                               $this->format);
-            return false;
-        }
-
-        $profile = $this->user->getProfile();
-
-        // Can the profile actually see that notice?
-
-        if (!$this->original->inScope($profile)) {
-            $this->clientError(_('No access to that notice.'),
-                               403,
-                               $this->format);
-            return false;
-        }
-
-        if ($profile->hasRepeated($id)) {
-            // TRANS: Client error displayed trying to re-repeat a notice through the API.
-            $this->clientError(_('Already repeated that notice.'),
-                               400, $this->format);
-            return false;
-        }
-
-
         return true;
     }
 

actions/repeat.php

@@ -73,20 +73,6 @@ class RepeatAction extends Action
             return false;
         }
 
-        // Is it OK to repeat that notice (general enough scope)?
-
-        if ($this->notice->scope != Notice::SITE_SCOPE &&
-            $this->notice->scope != Notice::PUBLIC_SCOPE) {
-            $this->clientError(_('You may not repeat a private notice.'),
-                               403);
-        }
-
-        if ($this->user->id == $this->notice->profile_id) {
-            // TRANS: Client error displayed when trying to repeat an own notice.
-            $this->clientError(_('You cannot repeat your own notice.'));
-            return false;
-        }
-
         $token  = $this->trimmed('token-'.$id);
 
         if (empty($token) || $token != common_session_token()) {
@@ -94,21 +80,6 @@ class RepeatAction extends Action
             return false;
         }
 
-        $profile = $this->user->getProfile();
-
-        // Can the profile actually see that notice?
-
-        if (!$this->notice->inScope($profile)) {
-            $this->clientError(_('No access to that notice.'), 403);
-        }
-
-
-        if ($profile->hasRepeated($id)) {
-            // TRANS: Client error displayed when trying to repeat an already repeated notice.
-            $this->clientError(_('You already repeated that notice.'));
-            return false;
-        }
-
         return true;
     }
 

classes/Notice.php

@@ -350,13 +350,31 @@ class Notice extends Memcached_DataObject
 
             $repeat = Notice::staticGet('id', $repeat_of);
 
-            if (!empty($repeat) &&
+            if (empty($repeat)) {
-                $repeat->scope != Notice::SITE_SCOPE &&
+                throw new ClientException(_('Cannot repeat; original notice is missing or deleted.'));
+            }
+
+            if ($profile->id == $repeat->profile_id) {
+                // TRANS: Client error displayed when trying to repeat an own notice.
+                throw new ClientException(_('You cannot repeat your own notice.'));
+            }
+
+            if ($repeat->scope != Notice::SITE_SCOPE &&
                 $repeat->scope != Notice::PUBLIC_SCOPE) {
+                // TRANS: Client error displayed when trying to repeat a non-public notice.
                 throw new ClientException(_('Cannot repeat a private notice.'), 403);
             }
 
-            // XXX: Check for access...?
+            if (!$repeat->inScope($profile)) {
+                // The generic checks above should cover this, but let's be sure!
+                // TRANS: Client error displayed when trying to repeat a notice you cannot access.
+                throw new ClientException(_('Cannot repeat a notice you cannot read.'), 403);
+            }
+
+            if ($profile->hasRepeated($repeat->id)) {
+                // TRANS: Client error displayed when trying to repeat an already repeated notice.
+                throw new ClientException(_('You already repeated that notice.'));
+            }
 
             $notice->repeat_of = $repeat_of;
         } else {
@@ -1567,6 +1585,15 @@ class Notice extends Memcached_DataObject
         return $location;
     }
 
+    /**
+     * Convenience function for posting a repeat of an existing message.
+     *
+     * @param int $repeater_id: profile ID of user doing the repeat
+     * @param string $source: posting source key, eg 'web', 'api', etc
+     * @return Notice
+     *
+     * @throws Exception on failure or permission problems
+     */
     function repeat($repeater_id, $source)
     {
         $author = Profile::staticGet('id', $this->profile_id);

lib/command.php

@@ -537,44 +537,15 @@ class RepeatCommand extends Command
     {
         $notice = $this->getNotice($this->other);
 
-        if($this->user->id == $notice->profile_id)
+        try {
-        {
+            $repeat = $notice->repeat($this->user->id, $channel->source());
-            // TRANS: Error text shown when trying to repeat an own notice.
+            $recipient = $notice->getProfile();
-            $channel->error($this->user, _('Cannot repeat your own notice.'));
-            return;
-        }
-
-        // Is it OK to repeat that notice (general enough scope)?
-
-        if ($notice->scope != Notice::SITE_SCOPE &&
-            $notice->scope != Notice::PUBLIC_SCOPE) {
-            $channel->error($this->user, _('You may not repeat a private notice.'));
-        }
-
-        $profile = $this->user->getProfile();
-
-        // Can the profile actually see that notice?
-
-        if (!$notice->inScope($profile)) {
-            $channel->error($this->user, _('You have no access to that notice.'));
-        }
-
-        if ($profile->hasRepeated($notice->id)) {
-            // TRANS: Error text shown when trying to repeat an notice that was already repeated by the user.
-            $channel->error($this->user, _('Already repeated that notice.'));
-            return;
-        }
-
-        $repeat = $notice->repeat($this->user->id, $channel->source);
-
-        if ($repeat) {
 
             // TRANS: Message given having repeated a notice from another user.
             // TRANS: %s is the name of the user for which the notice was repeated.
             $channel->output($this->user, sprintf(_('Notice from %s repeated.'), $recipient->nickname));
-        } else {
+        } catch (Exception $e) {
-            // TRANS: Error text shown when repeating a notice fails with an unknown reason.
+            $channel->error($this->user, $e->getMessage());
-            $channel->error($this->user, _('Error repeating notice.'));
         }
     }
 }