473242 - Admin panel sorting doesn't sort for all uploads, just current page

Status: RESOLVED FIXED

(Websites :: communitystore.mozilla.org, defect)

Description

[Ryan Doherty (:rdoherty)]

We should have the sorting options for the admin panel sort for all the designs, not just the currently viewed page.

Comment 1

[Ryan Doherty (:rdoherty)]

Attachment: patch #1

First stab at sorting. Definitely could use a review.

Comment 2

[Alex Buchanan [:abuchanan]]

Comment on attachment 372171 [details] [diff] [review]
patch #1

>Index: models/upload.class.php
>===================================================================
>--- models/upload.class.php	(revision 24344)
>+++ models/upload.class.php	(working copy)

All the model stuff looks fine to me.


>Index: controllers/admin.class.php
>===================================================================
>--- controllers/admin.class.php	(revision 24344)
>+++ controllers/admin.class.php	(working copy)
>+        $this->view->set('body_id', 'admin');
>+        
>+        $this->sortOrder = array_key_exists('order', $_GET) ? $_GET['order'] : 'desc';
>+        $this->view->set('sortOrder', $this->sortOrder);
>+        
>+        $this->sortColumn = array_key_exists('sortby', $_GET) ? $_GET['sortby'] : false;
>+        $this->view->set('sortColumn', $this->sortColumn);

These should be sanitized before being set to the template.

        
>+        $urlParts = parse_url($_SERVER['REQUEST_URI']);
>+        $this->view->set('urlPath', $urlParts['path']);

You could do this with _GET['page'].  Could you put this in the View class, so it could be reused?  Like, View::currentPath()?  This should also be sanitized before being set to the template.

       
>     function all() {
>@@ -23,7 +36,7 @@
> 
>         $model = new Upload();
>         
>-        $uploads = $model->selectRange($perPage, $offset, 'reviewed=1');
>+        $uploads = $model->selectRange($perPage, $offset, 'reviewed=1', $this->sortColumn, $this->sortOrder);
>         $totalUploads = $model->getCount();
>         
>         $pagination = new Pagination();
>@@ -31,7 +44,9 @@
>         $pagination->limit($perPage);
>         $pagination->currentPage($page);
>         $pagination->adjacents(4);
>+        $pagination->target('?order='.$this->sortOrder."&sortby=".$this->sortColumn);
> 
>+        
>         $this->view->set('pagination', $pagination);
>         $this->view->set('perpage', $perPage);
>         $this->view->set('page', $page);
>@@ -53,7 +68,7 @@
> 
>         $model = new Upload();
>        
>-        $uploads = $model->selectUnreviewed($perPage, $offset);
>+        $uploads = $model->selectUnreviewed($perPage, $offset, $this->sortColumn, $this->sortOrder);
>         $totalUploads = $model->getCount('reviewed = 0');
> 
>         $pagination = new Pagination();
>@@ -61,7 +76,8 @@
>         $pagination->limit($perPage);
>         $pagination->currentPage($page);
>         $pagination->adjacents(4);
>-
>+        $pagination->target('?order='.$this->sortOrder."&sortby=".$this->sortColumn);
>+        
>         $this->view->set('pagination', $pagination);
>         $this->view->set('perpage', $perPage);
>         $this->view->set('page', $page);

There is some duplication here.  Not a huge deal, just wondering if you have any ideas for cleaning it up?


>Index: controllers/gallery.class.php
>===================================================================
>--- controllers/gallery.class.php	(revision 24344)
>+++ controllers/gallery.class.php	(working copy)

>@@ -223,6 +223,7 @@
>+         $originalQuery = $query;
>@@ -233,7 +234,7 @@
>-         $this->view->set('query', $query);
>+         $this->view->set('query', $originalQuery);

could you comment these lines, so it's clear why you're backing up $query


>Index: docroot/media/js/designs-table.js
>===================================================================
>--- docroot/media/js/designs-table.js	(revision 24344)
>+++ docroot/media/js/designs-table.js	(working copy)
>@@ -7,24 +7,8 @@
>     }
>     $(document).ready(function() {
> 
>-        $('table').tablesorter({sortList: [[1, 0]],
>-                                headers: {
>-                                    0: { sorter: false }
>-                                },
>-                                textExtraction: function(el) {
>-                                    var cbox = $(el).children("input[@type=checkbox]");
>-                                    if ($(cbox).length > 0) {
>-                                        return $(cbox).attr('checked').toString();
>-                                    }
>-                                    return el.innerHTML;
>-                                }
>-                               });

Bye, bye tablesorter :) It's probably for the better

>-        $('table').columnManager({listTargetID: 'change-cols-form',
>-                                   onClass: 'colOn',
>-                                   offClass: 'colOff',
>-                                   hideInList: [1],
>-                                   saveState: true
>-                                 });

Did you mean to nuke the columnManager?

>Index: views/admin/designs-table.tpl.php
>===================================================================
>--- views/admin/designs-table.tpl.php	(revision 24344)
>+++ views/admin/designs-table.tpl.php	(working copy)
>@@ -14,32 +14,61 @@
> <?= $this->fetch('admin/edit-design-form') ?>
> 
> <?= $this->fetch('admin/edit-design-error') ?>
>-<a href="#TB_inline?height=450&width=300&inlineId=hidden-change-cols-form" id="change-cols-button" title="Change Columns" class="thickbox button">Change Columns</a>
>+<a href="#TB_inline?height=450&amp;width=300&amp;inlineId=hidden-change-cols-form" id="change-cols-button" title="Change Columns" class="thickbox button">Change Columns</a>

If, by chance, you nuked the columnManager on purpose, you can take this out too.

> 
> <table id="table">
>     <thead>
>         <tr>
>         <th><input type="checkbox" id="selectall" /></th>
>-        <th>#</th>
>-        <th>Date</th>
>-        <th>Preview</th>
>-        <th>Author</th>
>-        <th>Email</th>
>-        <th>Website</th>
>-        <th>Name</th>
>-        <th>Type</th>
>-        <th>Size</th>
>-        <th>IP</th>
>-        <th>Tags</th>
>-        <th>Theme</th>
>-        <th>BG Color</th>
>-        <th title="Anonymous">Anon</th>
>-        <th title="Featured">Feat</th>
>-        <th title="Popular">Pop</th>
>-        <th title="Approved">App</th>
>-        <th title="Reviewed">Rev</th>
>-        <th title="Design Community">Des. Comm.</th>
>-        <th>Delete</th>
>+        
>+        <?php
>+            $columns = array('id'       => array('name' => '#'),
>+                             'created'     => array('name' => 'Date'),
>+                             'preview'  => array('name' => 'Preview', 'sortable' => false),
>+                             'firstname'   => array('name' => 'Author'),
>+                             'email'    => array('name' => 'Email'),
>+                             'website'    => array('name' => 'Website'),
>+                             'designname'    => array('name' => 'Name'),
>+                             'type'    => array('name' => 'Type'),
>+                             'size'    => array('name' => 'Size'),
>+                             'ip'    => array('name' => 'IP'),
>+                             'tags'    => array('name' => 'Tags', 'sortable' => false),
>+                             'theme'    => array('name' => 'Theme', 'sortable' => false),
>+                             'bg'    => array('name' => 'BG Color'),
>+                             'anonymous'    => array('name' => 'Anon', 'title' => 'Anonymous'),
>+                             'featured'    => array('name' => 'Feat', 'title' => 'Featured'),
>+                             'popular'    => array('name' => 'Pop', 'title' => 'Popular'),
>+                             'approved'    => array('name' => 'App', 'title' => 'Approved'),
>+                             'reviewed'    => array('name' => 'Rev', 'title'=> 'Reviewed'),
>+                             'designcommunity'    => array('name' => 'Des. Comm.', 'title' => 'Design Community'),
>+                             'delete'       => array('name' => 'Delete', 'sortable'=>false)
>+            );
>+        ?>
>+        
>+        <?php
>+            foreach($columns as $column => $properties) {
>+                $title = array_key_exists('title', $properties) ? $properties['title'] : '';
>+                $sortable = array_key_exists('sortable', $properties) ? $properties['sortable'] : true;
>+                $name = $properties['name'];      
>+        ?>
>+            <th title="<?php echo $title ?>">
>+                <?php if($sortable) { ?>
>+                    <?php
>+                        if($column == $sortColumn) {
>+                            $sort = $sortOrder == 'asc' ? 'desc' : 'asc';
>+                            $currentColumn = true;
>+                        } else {
>+                            $sort = 'asc';
>+                            $currentColumn = false;
>+                        }
>+                    ?>
>+                    <a href="<?php echo $urlPath ?>?order=<?php echo $sort ?>&amp;sortby=<?php echo $column ?>"><?php echo $name; ?></a> <?php if($currentColumn) { if($sortOrder== 'desc') { ?>&uarr;<? } else { ?>&darr;<?php }} ?>
>+                <?php } else { ?>
>+                    <?php echo $name; ?>
>+                <?php } ?>
>+            </th>
>+        
>+        <?php } ?>
>         </tr>
>     </thead>
>     <tbody>

This is a lot of PHP logic for a template file.  Can we abstract all this out into a SortableTable class?  It could do all this preparation outside of the template, and then you could set an array of column objects (or arrays) to the template.

>@@ -109,7 +138,6 @@
>                 
> <div id="pagination">
>     <?php $pagination->show(); ?>
>-
>     <div class="per-page-options">
>         <ul class="per-page">
>             <?php
>@@ -119,7 +147,7 @@
>                 <?php if($number == $perpage) { ?>
>                     <li><strong><?php echo $number; ?></strong></li>
>                 <?php } else { ?>
>-                    <li><a href="?pageno=<?php echo $page; ?>&amp;perpage=<?php echo $number; ?>"><?php echo $number; ?></a></li>
>+                    <li><a href="?pageno=<?php echo $page; ?>&amp;perpage=<?php echo $number; ?>&amp;sortby=<?php echo $sortColumn?>&amp;order=<?php echo $sortOrder ?>"><?php echo $number; ?></a></li>
>                 <?php } ?>
>             <?php } ?>
>         </ul>

We could also abstract this part, similar to Pagination and SortableTable

Comment 3

[Ryan Doherty (:rdoherty)]

Reassigning back to myself as I have an almost ready patch.

Comment 4

[Ryan Doherty (:rdoherty)]

Attachment: v2!

completely rewrote previous patch, modularized everything. much better now :)

Comment 5

[Alex Buchanan [:abuchanan]]

Comment on attachment 387076 [details] [diff] [review]
v2!

This works, except...

When sorting the IP or BG Color columns I get,

Warning: mysqli_fetch_assoc() expects parameter 1 to be mysqli_result, boolean given in /Users/alexbuchanan/community_store/trunk/models/upload.class.php on line 144

Sorting doesn't maintain "per page" count.  Try showing 100 items per page, then sorting by Author.  You'll be back at the default 20

Comment 6

[Ryan Doherty (:rdoherty)]

Attachment: v3 suckas!

Fixed issues, same day turnaround!

Comment 7

[Ryan Doherty (:rdoherty)]

r29816

For QA:
I rewrote all of the table sorting behavior, pagination and per-page code. Will need testing to verify all components behave appropriately when used.

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

Oops, I think I meant to reopen this bug when I pointed out that https://community-store.authstage.mozilla.com/admin/all?order=desc&sortby=&pageno=5&perpage=100 is showing extra (i.e. blank) pages, not the mass-delete bug (sorry!)