Merge lp:~vasilev/psiphon/sprint3-579678 into lp:psiphon

[Robert Vasilev]

Paging, sorting and filtering have been implemented in users.php.

Upd:
Rod's comments on 2010-09-09 have been implemented.

[Rod (rod-psiphon)]

This was a team review. Here are our comments:

- After trying the new functionality, we have some minor changes that weren't in the original spec.

1. Display the total number of users in all cases, even when no filter.
2. Add the paging controls to the top of the page (but also leave at the bottom).
3. Originally when add/edit user and return to list, the new/modified user was shown in bold. Now it returns to the default with no filter and page 1. Can you make it return to the page with the new user, shown in bold? This isn't required: please make a bug to backlog it if it's too much work.

- For code readability, don't mix access control check and input validation ("if (!user_has_permission($config, $record_user, "accounts") || (isset($_GET["ord"]) && (abs($_GET["ord"]) > count($column_number_to_field_name))))"

Also, this check won't catch all cases. the logic is mixed in with the later if condition: "if (!$_GET["ord"] || !is_numeric($_GET["rod"]))". It would be easier to review and maintain if the input validation and normalization is in one place.

Should validate same value used, even if we can reason that it's safe and/or equivalent: e.g., abs((int)$_GET["ord"]) is used vs. abs($_GET["ord"]) is validated.

- Always escape user input in output, even if we don't think it's a risk: "$filterby_string = "&filterby=" . $_GET['filterby'];" and "$ord_string = "&ord=". $_GET["ord"];"

Also when making the page number links below:

if (isset($_GET["ord"])) {
    $ord_string = "&ord=". $_GET["ord"];
}

See other files for examples of escaping. We use the helper function escape_html.

- "SELECT COUNT(*) AS cnt FROM user WHERE {$query_where_sql}" is repeated twice in a row; could keep result in variable. Even though MySQL caches results. If you always display the user total (filter or no) then: 1. always get total; 2. get filtered count when filter.

- Please use a more descriptive variable name: "echo ($pn);"

review: Needs Fixing

[Robert Vasilev (vasilev)]

Most of your recommendations have been implemented.

In detail:
> 1. Display the total number of users in all cases, even when no filter.
Done.

> 2. Add the paging controls to the top of the page (but also leave at the bottom).
Done.

> 3. Originally when add/edit user and return to list, the new/modified user was
> shown in bold. Now it returns to the default with no filter and page 1. Can
> you make it return to the page with the new user, shown in bold? This isn't
> required: please make a bug to backlog it if it's too much work.
Implemented for editing/disabling case.
For adding case it's quite unobvious to predict on which page new user will appear for different sorting modes.
It will be better to implement highlinghting feature for new users as anover ticket.

> - For code readability, don't mix access control check and input validation
> ...
> Should validate same value used, even if we can reason that it's safe and/or equivalent
Done.

> - Always escape user input in output, even if we don't think it's a risk:
...
> See other files for examples of escaping. We use the helper function escape_html.
Have not found any example of escaping integers with escape_html() in Psiphon code.
Escaping integers with escape_html() not implemented because of unnecessarity.

> - "SELECT COUNT(*) AS cnt FROM user WHERE {$query_where_sql}" is repeated
> twice in a row; could keep result in variable. Even though MySQL caches
> results. If you always display the user total (filter or no) then: 1. always
> get total; 2. get filtered count when filter.
Done. But could not 'keep result in variable' because of '$query_where_sql' may be modified by adding filtering conditions, and results will not be equivalent.

> - Please use a more descriptive variable name: "echo ($pn);"
Done.

=== modified file 'trunk/www/config.php'
--- trunk/www/config.php	2010-06-28 20:44:20 +0000
+++ trunk/www/config.php	2010-09-14 09:13:48 +0000
@@ -58,6 +58,7 @@
 $config[max_login_delay] = 30; // 30 seconds
 
 $config['tickets_ppage'] = 20;
+$config['users_ppage'] = 100;
 
 $config["sender"] = "[e-mail]";
 $config["invitation_sender"] = "[e-mail]";

=== modified file 'trunk/www/includes/lang.php'
--- trunk/www/includes/lang.php	2010-09-07 16:38:40 +0000
+++ trunk/www/includes/lang.php	2010-09-14 09:13:48 +0000
@@ -4630,6 +4630,71 @@
 Please begin using this URL to log into Psiphon.",
     ),
 
+     'user_class_filter' => array(
+        'en' => "User Class Filter",
+        'fa' => "User Class Filter",
+        'ru' => "User Class Filter",
+        'uz' => "User Class Filter",
+        'tk' => "User Class Filter",
+        'cn' => "User Class Filter",
+        'ar' => "User Class Filter",
+        'vt' => "User Class Filter",
+        'fr' => "User Class Filter",
+        'es' => "User Class Filter",
+    ),
+
+     'no_filter' => array(
+        'en' => "No filter",
+        'fa' => "No filter",
+        'ru' => "No filter",
+        'uz' => "No filter",
+        'tk' => "No filter",
+        'cn' => "No filter",
+        'ar' => "No filter",
+        'vt' => "No filter",
+        'fr' => "No filter",
+        'es' => "No filter",
+    ),
+
+     'apply_filter' => array(
+        'en' => "Apply filter",
+        'fa' => "Apply filter",
+        'ru' => "Apply filter",
+        'uz' => "Apply filter",
+        'tk' => "Apply filter",
+        'cn' => "Apply filter",
+        'ar' => "Apply filter",
+        'vt' => "Apply filter",
+        'fr' => "Apply filter",
+        'es' => "Apply filter",
+    ),
+
+     'filtered_items_count' => array(
+        'en' => "Filtered items count",
+        'fa' => "Filtered items count",
+        'ru' => "Filtered items count",
+        'uz' => "Filtered items count",
+        'tk' => "Filtered items count",
+        'cn' => "Filtered items count",
+        'ar' => "Filtered items count",
+        'vt' => "Filtered items count",
+        'fr' => "Filtered items count",
+        'es' => "Filtered items count",
+    ),
+
+     'total_users_count' => array(
+        'en' => "Total users",
+        'fa' => "Total users",
+        'ru' => "Total users",
+        'uz' => "Total users",
+        'tk' => "Total users",
+        'cn' => "Total users",
+        'ar' => "Total users",
+        'vt' => "Total users",
+        'fr' => "Total users",
+        'es' => "Total users",
+    ),
+
 // DO NOT MODIFY THIS LINE -- END OF TRANSLATIONS
 );
 

=== modified file 'trunk/www/user-edit.php'
--- trunk/www/user-edit.php	2010-07-16 19:17:57 +0000
+++ trunk/www/user-edit.php	2010-09-14 09:13:48 +0000
@@ -39,6 +39,25 @@
     // 403.php exits
 }
 
+// Chaining paging, filtering, and sorting info back to user list page
+$_GET['filterby'] = cast_str_to_int($_GET['filterby']);
+$_GET['ofs'] = cast_str_to_int($_GET['ofs']);
+$_GET['ord'] = cast_str_to_int($_GET['ord']);
+$filterby_string = '';
+if (isset($_GET['filterby'])) {
+    $filterby_string = "&filterby=" . $_GET['filterby'];
+}
+$ord_string='';
+if (isset($_GET["ord"])) {
+    $ord_string = "&ord=". $_GET["ord"];
+}
+$ofs_string = '';
+if (isset($_GET["ofs"])) {
+    $ofs_string = "&ofs=". $_GET["ofs"];
+}
+$paging_sorting_filtering_params = $ofs_string.$ord_string.$filterby_string;
+
+
 // This page has two modes: add and edit. If there are no GET params, then it's
 // in add-mode. If there's an 'user_id' GET param, this it's in edit-mode. (The
 // 'user_id' is the ID of the user to edit.)
@@ -386,6 +405,7 @@
 
 $action = getenv("SCRIPT_NAME");
 if ($user_id !== Null) $action .= "?user_id=$user_id";
+$action .= $paging_sorting_filtering_params;
 
 include($_SERVER[DOCUMENT_ROOT]."/header.php");
 ?>
@@ -522,9 +542,9 @@
     <input type="submit" value="<?=msg("submit")?>"/>
     <br/>
     <?if ($submit_success) {?>
-        <a href="users.php?user_id=<?=$user_id?>"><b><?=msg("back_to_users")?></b></a>
+        <a href="users.php?user_id=<?=$user_id.$paging_sorting_filtering_params?>"><b><?=msg("back_to_users")?></b></a>
     <?} else {?>
-        <a href="users.php"><?=msg("cancel")?></a>
+        <a href="users.php?<?=$paging_sorting_filtering_params?>"><?=msg("cancel")?></a>
     <?}?>
 
     <?if ($page_mode == "ADD") {?>

=== modified file 'trunk/www/users.php'
--- trunk/www/users.php	2010-07-05 20:29:49 +0000
+++ trunk/www/users.php	2010-09-14 09:13:48 +0000
@@ -29,6 +29,8 @@
 
 $user_edit_page = 'user-edit.php';
 
+$column_number_to_field_name = array(1=>'uname', 2=>'email', 3=>'grp', 4=>'current_proxy', 5=>'proxies', 6=>'inv_actv', 7=>'inv_sent');
+$default_sort_field = 1;
 //
 // Business logic
 //
@@ -40,13 +42,25 @@
     include($_SERVER[DOCUMENT_ROOT]."/http-errors/403.php");
     // 403.php exits
 }
-    
+
 // Screen title. Must come before the header.php include.
 
 $title = msg("users_management");
 
 $_GET['disable'] = cast_str_to_int($_GET['disable']);
 $last_modified_user_id = cast_str_to_int($_GET['user_id']);
+$_GET['filterby'] = cast_str_to_int($_GET['filterby']);
+$_GET['ord'] = cast_str_to_int($_GET['ord']);
+// If 'ord' param is not set or invalid, sort by default column
+if (!$_GET["ord"] || (abs($_GET["ord"]) > count($column_number_to_field_name)))
+{
+    $_GET["ord"] = (int)$default_sort_field;
+}
+$orderby_sql = " ORDER BY ".$column_number_to_field_name[abs($_GET["ord"])];
+if ($_GET["ord"]<0)
+{
+    $orderby_sql.=" DESC";
+}
 
 // We need to build up the where clause that will be used in the main query.
 $query_where_sql = get_user_managed_classes_sql_condition($config, $record_user);
@@ -105,19 +119,118 @@
     break;
 }
 
+// count total number of users  (unfiltered)
+$query = "SELECT COUNT(id) AS cnt FROM user WHERE {$query_where_sql}";
+$result = db_query_execute($config, $query, convert_null_array($query_where_sql_params));
+$record = db_fetch_result($config, $result);
+$total_users_count = $record["cnt"];
+db_free_result($result);
+
+$filterby_string = '';
+if (isset($_GET['filterby'])) {
+    $filterby_string = "&amp;filterby=" . $_GET['filterby'];
+
+    // apply user class filter
+    $query_where_sql .= " AND user.grp = :filter_user_class";
+    $query_where_sql_params[':filter_user_class'] = $_GET['filterby'];
+    $selected_filter = $_GET['filterby'];
+
+    // count filtered users without LIMITs
+    $query = "SELECT COUNT(id) AS cnt FROM user WHERE {$query_where_sql}";
+    $result = db_query_execute($config, $query, convert_null_array($query_where_sql_params));
+    $record = db_fetch_result($config, $result);
+    $filtered_users_count = $record["cnt"];
+    db_free_result($result);
+}
+
+// pager (from tickets.php)/////////////////////////////////////////////////////
+$query = "SELECT COUNT(*) AS cnt FROM user WHERE {$query_where_sql}";
+$result = db_query_execute($config, $query, convert_null_array($query_where_sql_params));
+$record = db_fetch_result($config, $result);
+$count = $record["cnt"];
+db_free_result($result);
+
+$per_page = $config['users_ppage'];
+if (($_GET["ofs"]=(int)$_GET["ofs"])<0) $_GET["ofs"]=0;
+$last_page=floor(abs($count-0.1)/$per_page);
+if ($_GET["ofs"]>$last_page) include($_SERVER[DOCUMENT_ROOT]."/http-errors/404.php");
+$ord_string='';
+if (isset($_GET["ord"])) {
+    $ord_string = "&amp;ord=". $_GET["ord"];
+}
+$ofs_string = '';
+if (isset($_GET["ofs"])) {
+    $ofs_string = "&amp;ofs=". $_GET["ofs"];
+}
+
+$page_numbers_control='';
+for ($page=0; $page<=$last_page; $page++)
+{
+    if ($page!=$_GET["ofs"])
+    {
+       $page_numbers_control.="<a href=\"".getenv("SCRIPT_NAME")."?ofs={$page}{$ord_string}{$filterby_string}\">".($page+1)."</a> &nbsp;\n";
+    }
+    else
+    {
+       $page_numbers_control.="[ <b>".($page+1)."</b> ] &nbsp;\n";
+   }
+}
+
+$limit_offset = abs((int)($_GET[ofs] * $per_page));
+$limit_count = abs((int)$per_page);
+
+// end of pager ////////////////////////////////////////////////////////////////
+$paging_sorting_filtering_params = $ofs_string.$ord_string.$filterby_string;
+
+function print_sort_header($col_number, $label, $filterby_string)
+{
+    echo "<a href=\"" . getenv("SCRIPT_NAME") . "?ord=";
+    if ($_GET["ord"] == $col_number)
+        echo "-{$col_number}{$filterby_string}\"><b>" . $label . " /\\</b>";
+    elseif ($_GET["ord"] == -$col_number)
+        echo "{$col_number}{$filterby_string}\"><b>" . $label . " \\/</b>";
+    else
+        echo "-{$col_number}{$filterby_string}\">" . $label;
+    echo "</a>\n";
+}
+
 include($_SERVER[DOCUMENT_ROOT]."/header.php");
 ?>
-
+<div>
+    <form method="GET" action="<?=getenv("SCRIPT_NAME")?>">
+    <label><?=msg('user_class_filter') ?>: </label>
+<?
+
+$user_classes = get_user_class_names($config, $record_user, 1, 1);
+
+echo_combo_from_array(
+        'filterby',
+        $user_classes,
+        Null, // no transform function
+        $selected_filter,
+        1, msg('no_filter'));
+
+?>
+<input type="submit" value="<?=msg("apply_filter") ?>">
+<? if (isset($filtered_users_count)) {
+    echo " &nbsp;&nbsp; ".msg('filtered_items_count').": ".$filtered_users_count." &nbsp;&nbsp; ";
+}
+echo msg('total_users_count').": ".$total_users_count;
+?>
+</form>
+
+</div>
+<? echo $page_numbers_control; ?>
 <table class="brd">
     <tr>
-        <th><?=msg("name")?></th>
-        <th><?=msg("email")?></th>
-        <th><?=msg("grp")?></th>
-        <th><?=msg("current_proxy")?></th>
-        <th><?=msg("proxies")?></th>
-        <th><?=msg("invites_accepted")?></th>
-        <th><?=msg("invites_sent")?></th>
-        <th><a href="<?=$user_edit_page?>"><?=msg("add_new_user")?></a></th>
+        <th><? print_sort_header(1, msg("name"), $filterby_string) ?></th>
+        <th><? print_sort_header(2, msg("email"), $filterby_string)?></th>
+        <th><? print_sort_header(3, msg("grp"), $filterby_string)?></th>
+        <th><? print_sort_header(4, msg("current_proxy"), $filterby_string)?></th>
+        <th><? print_sort_header(5, msg("proxies"), $filterby_string)?></th>
+        <th><? print_sort_header(6, msg("invites_accepted"), $filterby_string)?></th>
+        <th><? print_sort_header(7, msg("invites_sent"), $filterby_string)?></th>
+        <th><a href="<?=$user_edit_page."?$paging_sorting_filtering_params" ?>"><?=msg("add_new_user")?></a></th>
     </tr>
 
 <?
@@ -125,7 +238,7 @@
 $query = "SELECT user.*, ".
                 "GROUP_CONCAT(userproxy.proxyid SEPARATOR '".$proxy_separator."') AS proxies ".
          "FROM user LEFT OUTER JOIN userproxy ON user.id = userproxy.userid ".
-         "WHERE {$query_where_sql} GROUP BY uname ORDER BY uname";
+         "WHERE {$query_where_sql} GROUP BY uname {$orderby_sql} LIMIT {$limit_offset}, {$limit_count}";
 
 $result = db_query_execute($config, $query, convert_null_array($query_where_sql_params));
 
@@ -168,12 +281,12 @@
     echo "<td>{$_p}{$record['inv_sent']}{$_s}</td>\n";
 
     echo "<td>{$_p}&nbsp;";
-    echo "<a href=\"".$user_edit_page."?user_id={$record[id]}\">".msg("edit")."</a> &nbsp; ";
+    echo "<a href=\"".$user_edit_page."?user_id={$record[id]}{$paging_sorting_filtering_params}\">".msg("edit")."</a> &nbsp; ";
     $t=msg($record[locked]?"enable_account":"disable_account");
     if ($record[id]!=$record_user[id])
     {
         // The "fresh" GET parameter ensures that the disable link is always coloured as not-visited.
-        echo "<a href=\"".getenv("SCRIPT_NAME")."?disable={$record[id]}&amp;fresh=".bin2hex(microtime())."\" title=\"{$t}\">X</a>";
+        echo "<a href=\"".getenv("SCRIPT_NAME")."?disable={$record[id]}{$paging_sorting_filtering_params}&amp;fresh=".bin2hex(microtime())."\" title=\"{$t}\">X</a>";
     }
     else
     {
@@ -188,7 +301,7 @@
 ?>
 </table>
 <?
-
+echo "<br>\n".$page_numbers_control;
 db_free_result($result);
 
 include($_SERVER[DOCUMENT_ROOT]."/footer.php");