Maintain 'page' parameter for block from subscribers list, block & make-admin from group members list.

Refactored some of the returnto handling code. It looks like we have several different ways of handling this in the software, icky!
Marked the session-based functions with fixmes (they'll stomp on other forms when multiple tabs/windows are used) and combined some commonish bits of code between ProfileFormAction and the group block & makeadmin actions where they're using hidden form parameters. Extended that to allow passing dynamic parameters (eg 'page') as well as static ones (action, target user/group).

7 files changed, 178 insertions, 66 deletions

diff --git a/actions/groupblock.php b/actions/groupblock.php
index 88d7634a2..fc95c0e66 100644
--- a/actions/groupblock.php
+++ b/actions/groupblock.php
@@ -41,7 +41,7 @@ if (!defined('STATUSNET') && !defined('LACONICA')) {
  * @link     http://status.net/
  */
 
-class GroupblockAction extends Action
+class GroupblockAction extends RedirectingAction
 {
     var $profile = null;
     var $group = null;
@@ -117,9 +117,7 @@ class GroupblockAction extends Action
         parent::handle($args);
         if ($_SERVER['REQUEST_METHOD'] == 'POST') {
             if ($this->arg('no')) {
-                common_redirect(common_local_url('groupmembers',
-                                                 array('nickname' => $this->group->nickname)),
-                                303);
+                $this->returnToArgs();
             } elseif ($this->arg('yes')) {
                 $this->blockProfile();
             } elseif ($this->arg('blockto')) {
@@ -196,23 +194,20 @@ class GroupblockAction extends Action
             $this->serverError(_("Database error blocking user from group."));
             return false;
         }
+        
+        $this->returnToArgs();
+    }
 
-        // Now, gotta figure where we go back to
-        foreach ($this->args as $k => $v) {
-            if ($k == 'returnto-action') {
-                $action = $v;
-            } elseif (substr($k, 0, 9) == 'returnto-') {
-                $args[substr($k, 9)] = $v;
-            }
-        }
-
-        if ($action) {
-            common_redirect(common_local_url($action, $args), 303);
-        } else {
-            common_redirect(common_local_url('groupmembers',
-                                             array('nickname' => $this->group->nickname)),
-                            303);
-        }
+    /**
+     * If we reached this form without returnto arguments, default to
+     * the top of the group's member list.
+     * 
+     * @return string URL
+     */
+    function defaultReturnTo()
+    {
+        return common_local_url('groupmembers',
+                                array('nickname' => $this->group->nickname));
     }
 
     function showScripts()
diff --git a/actions/groupmembers.php b/actions/groupmembers.php
index fb4e46dbc..6d0701239 100644
--- a/actions/groupmembers.php
+++ b/actions/groupmembers.php
@@ -205,8 +205,7 @@ class GroupMemberListItem extends ProfileListItem
             !$this->profile->isAdmin($this->group)) {
             $this->out->elementStart('li', 'entity_make_admin');
             $maf = new MakeAdminForm($this->out, $this->profile, $this->group,
-                                     array('action' => 'groupmembers',
-                                           'nickname' => $this->group->nickname));
+                                     $this->returnToArgs());
             $maf->show();
             $this->out->elementEnd('li');
         }
@@ -220,8 +219,7 @@ class GroupMemberListItem extends ProfileListItem
         if (!empty($user) && $user->id != $this->profile->id && $user->isAdmin($this->group)) {
             $this->out->elementStart('li', 'entity_block');
             $bf = new GroupBlockForm($this->out, $this->profile, $this->group,
-                                array('action' => 'groupmembers',
-                                      'nickname' => $this->group->nickname));
+                                     $this->returnToArgs());
             $bf->show();
             $this->out->elementEnd('li');
         }
@@ -244,6 +242,23 @@ class GroupMemberListItem extends ProfileListItem
             $aAttrs['rel'] = 'nofollow';
         }
     }
+
+    /**
+     * Fetch necessary return-to arguments for the profile forms
+     * to return to this list when they're done.
+     * 
+     * @return array
+     */
+    protected function returnToArgs()
+    {
+        $args = array('action' => 'groupmembers',
+                      'nickname' => $this->group->nickname);
+        $page = $this->out->arg('page');
+        if ($page) {
+            $args['param-page'] = $page;
+        }
+        return $args;
+    }
 }
 
 /**
diff --git a/actions/makeadmin.php b/actions/makeadmin.php
index f19348648..9ccb44230 100644
--- a/actions/makeadmin.php
+++ b/actions/makeadmin.php
@@ -41,7 +41,7 @@ if (!defined('STATUSNET') && !defined('LACONICA')) {
  * @link     http://status.net/
  */
 
-class MakeadminAction extends Action
+class MakeadminAction extends RedirectingAction
 {
     var $profile = null;
     var $group = null;
@@ -148,20 +148,19 @@ class MakeadminAction extends Action
                                $this->group->getBestName());
         }
 
-        foreach ($this->args as $k => $v) {
-            if ($k == 'returnto-action') {
-                $action = $v;
-            } else if (substr($k, 0, 9) == 'returnto-') {
-                $args[substr($k, 9)] = $v;
-            }
-        }
+        $this->returnToArgs();
+    }
 
-        if ($action) {
-            common_redirect(common_local_url($action, $args), 303);
-        } else {
-            common_redirect(common_local_url('groupmembers',
-                                             array('nickname' => $this->group->nickname)),
-                            303);
-        }
+    /**
+     * If we reached this form without returnto arguments, default to
+     * the top of the group's member list.
+     * 
+     * @return string URL
+     */
+    function defaultReturnTo()
+    {
+        return common_local_url('groupmembers',
+                                array('nickname' => $this->group->nickname));
     }
+
 }
diff --git a/actions/subscribers.php b/actions/subscribers.php
index 6dda7312d..6fdf43e2c 100644
--- a/actions/subscribers.php
+++ b/actions/subscribers.php
@@ -157,9 +157,13 @@ class SubscribersListItem extends SubscriptionListItem
         $user = common_current_user();
 
         if (!empty($user) && $this->owner->id == $user->id) {
-            $bf = new BlockForm($this->out, $this->profile,
-                                array('action' => 'subscribers',
-                                      'nickname' => $this->owner->nickname));
+            $returnto = array('action' => 'subscribers',
+                              'nickname' => $this->owner->nickname);
+            $page = $this->out->arg('page');
+            if ($page) {
+                $returnto['param-page'] = $page;
+            }
+            $bf = new BlockForm($this->out, $this->profile, $returnto);
             $bf->show();
         }
     }
diff --git a/lib/profileformaction.php b/lib/profileformaction.php
index 8a934666e..0ffafe5fb 100644
--- a/lib/profileformaction.php
+++ b/lib/profileformaction.php
@@ -41,7 +41,7 @@ if (!defined('STATUSNET') && !defined('LACONICA')) {
  * @link     http://status.net/
  */
 
-class ProfileFormAction extends Action
+class ProfileFormAction extends RedirectingAction
 {
     var $profile = null;
 
@@ -102,29 +102,6 @@ class ProfileFormAction extends Action
     }
 
     /**
-     * Return to the calling page based on hidden arguments
-     *
-     * @return void
-     */
-
-    function returnToArgs()
-    {
-        foreach ($this->args as $k => $v) {
-            if ($k == 'returnto-action') {
-                $action = $v;
-            } else if (substr($k, 0, 9) == 'returnto-') {
-                $args[substr($k, 9)] = $v;
-            }
-        }
-
-        if ($action) {
-            common_redirect(common_local_url($action, $args), 303);
-        } else {
-            $this->clientError(_("No return-to arguments."));
-        }
-    }
-
-    /**
      * handle a POST request
      *
      * sub-classes should overload this request
diff --git a/lib/redirectingaction.php b/lib/redirectingaction.php
new file mode 100644
index 000000000..f11585274
--- /dev/null
+++ b/lib/redirectingaction.php
@@ -0,0 +1,96 @@
+<?php
+/**
+ * Superclass for actions that redirect to a given return-to page on completion.
+ *
+ * PHP version 5
+ *
+ * StatusNet - the distributed open-source microblogging tool
+ * Copyright (C) 2009-2010, StatusNet, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * @category Action
+ * @package  StatusNet
+ * @author   Evan Prodromou <evan@status.net>
+ * @author   Brion Vibber <brion@status.net>
+ * @license  http://www.fsf.org/licensing/licenses/agpl.html AGPLv3
+ * @link     http://status.net/
+ */
+
+if (!defined('STATUSNET') && !defined('LACONICA')) {
+    exit(1);
+}
+
+/**
+ * Superclass for actions that redirect to a given return-to page on completion.
+ *
+ * @category Action
+ * @package  StatusNet
+ * @author   Evan Prodromou <evan@status.net>
+ * @author   Brion Vibber <brion@status.net>
+ * @license  http://www.fsf.org/licensing/licenses/agpl.html AGPLv3
+ * @link     http://status.net/
+ */
+
+
+class RedirectingAction extends Action
+{
+
+    /**
+     * Redirect browser to the page our hidden parameters requested,
+     * or if none given, to the url given by $this->defaultReturnTo().
+     * 
+     * To be called only after successful processing.
+     * 
+     * @fixme rename this -- it obscures Action::returnToArgs() which
+     * returns a list of arguments, and is a bit confusing.
+     * 
+     * @return void
+     */
+    function returnToArgs()
+    {
+        // Now, gotta figure where we go back to
+        $action = false;
+        $args = array();
+        $params = array();
+        foreach ($this->args as $k => $v) {
+            if ($k == 'returnto-action') {
+                $action = $v;
+            } else if (substr($k, 0, 15) == 'returnto-param-') {
+                $params[substr($k, 15)] = $v;
+            } elseif (substr($k, 0, 9) == 'returnto-') {
+                $args[substr($k, 9)] = $v;
+            }
+        }
+
+        if ($action) {
+            common_redirect(common_local_url($action, $args, $params), 303);
+        } else {
+            $url = $this->defaultReturnToUrl();
+        }
+        common_redirect($url, 303);
+    }
+
+    /**
+     * If we reached this form without returnto arguments, where should
+     * we go? May be overridden by subclasses to a reasonable destination
+     * for that action; default implementation throws an exception.
+     * 
+     * @return string URL
+     */
+    function defaultReturnTo()
+    {
+        $this->clientError(_("No return-to arguments."));
+    }
+}
diff --git a/lib/util.php b/lib/util.php
index bbc334176..6905df839 100644
--- a/lib/util.php
+++ b/lib/util.php
@@ -1279,12 +1279,38 @@ function common_mtrand($bytes)
     return $enc;
 }
 
+/**
+ * Record the given URL as the return destination for a future
+ * form submission, to be read by common_get_returnto().
+ * 
+ * @param string $url
+ * 
+ * @fixme as a session-global setting, this can allow multiple forms
+ * to conflict and overwrite each others' returnto destinations if
+ * the user has multiple tabs or windows open.
+ * 
+ * Should refactor to index with a token or otherwise only pass the
+ * data along its intended path.
+ */
 function common_set_returnto($url)
 {
     common_ensure_session();
     $_SESSION['returnto'] = $url;
 }
 
+/**
+ * Fetch a return-destination URL previously recorded by
+ * common_set_returnto().
+ * 
+ * @return mixed URL string or null
+ * 
+ * @fixme as a session-global setting, this can allow multiple forms
+ * to conflict and overwrite each others' returnto destinations if
+ * the user has multiple tabs or windows open.
+ * 
+ * Should refactor to index with a token or otherwise only pass the
+ * data along its intended path.
+ */
 function common_get_returnto()
 {
     common_ensure_session();