Need a paginator with tests

[Christopher Allan Webber]

We need a nice paginator. We may also need a wrapper paginator task
(or just a specialized paginator) for mongodb queries.

`http://flask.pocoo.org/snippets/44/ <http://flask.pocoo.org/snippets/44/>`_
would be a good starting point


-  Add paginator(s)
-  Make sure said paginator works efficiently with mongodb/mongokit
   queries
-  add tests

[Christopher Allan Webber]

Bernard Keller (hunabku) expressed interest in this, assigning to
him.

Thanks Bernard, feel free to ask questions if you get stuck!

[Bernhard Keller]

[https://gitorious.org/\ :sub:`hunabku/mediagoblin/hunabkus-mediagoblin/commits/hunabku/master](https://gitorious.org/`\ hunabku/mediagoblin/hunabkus-mediagoblin/commits/hunabku/master)

first version for pagination.

basic components are "class Pagination" in utils.py [should be
moved to a different file]
object\_gallery.html
pagination.html

TODO
change db queries to range based pagination
add optional sorting order and sorting attribute, currently using
'created' in descending order

[Christopher Allan Webber]

So this is a great start.

Some comments:


-  I really like the use of your pagination macro, well done!
-  Pagination macro has a next link, could use a previous
-  When you do the {% import %} of the macro, could you do it at
   the top of the template, as one would in a python module? I think
   that looks nicer.
-  It would be nice to have more indentation inside the templates,
   eg:

   {% macro foo() %} {# bar #} {% endmacro %}


When there are blocks "containing" things, always indent. You've
mostly got this down, just apply it broadly.


-  instead of relative imports, please import abolutely. I prefer:

   from mediagoblin.util import Pagination


over

::

    from ..util import Pagination

relative imports are a cool feature, but easier to forget what's
going on.


-  A few indentation related nitpicks here:

   ::

       media_entries = pagination(
           { 'per_page': 2,
             'request': request,
             'collection':'MediaEntry',
             'query': { 'uploader':user, 'state':'processed'} } )
       
       #if no data is available, return NotFound


should be:

::

        media_entries = pagination(
            {'per_page': 2,
             'request': request,
             'collection': 'MediaEntry',
             'query': {'uploader': user, 'state': 'processed'} } )
    
        # if no data is available, return NotFound

(notice space after start of comment, no space between dict and
first key, space between collection: and MediaEntry)


-  Instead of asking for the request and query information and the
   collection, you could just do the query outside of the paginator
   and pass in the cursor instead. Eg:

   def user\_home(request): [...] pagination = Pagination() cursor =
   request.db.MediaEntry.find( {'uploader':user, 'state':'processed'})
   media\_entries = pagination( page=int(request.GET.get('page', 1)),
   per\_page=2, cursor=cursor)


This way you don't need to pass in the request and the Paginator
code is much less tied to the request object.


-  Any reason you're doing:

   ::

       def __call__(self, args):


Why not accept all as proper arguments? This would probably be
nicer:

::

        def __call__(self, cursor, page=1, per_page=30):


-  Maybe it makes more sense to do what you're doing in *call* in
   *init*? That would make a lot more sense for things like def
   pages() because then you'd already have that data by the time the
   paginator was instantiated. Or rather, maybe *init* can store the
   query, the cursor, page number, per\_page, and *call* could return
   the appropriate "slice" of that query... I guess it would look
   like:

   def user\_home(request): [...] cursor = request.db.MediaEntry.find(
   {'uploader':user, 'state':'processed'}) pagination = Pagination(
   cursor=cursor, page=int(request.GET.get('page', 1)), per\_page=2)
   media\_entries = pagination()


does that make any sense?


-  Last nitpick... could you wrap the comments that are going over
   80 characters?

Overall this looks really good. We still don't have the unit
testing stuff nicely in place for stuff like this, so if things
look good after the next pass, reassign to me and we'll make sure
we can get the appropriate stuff in place so that the tests can be
completed on this.

I know that was a lot of feedback but I'm happy about the direction
you're taking this and you joining as a contributor! Let me know if
you have any questions about the things I've said here.

[Bernhard Keller]

I really appreciate the detailed review, thanks a lot for taking
the time.

All the style tips I surely change, and I have to say, that the
interface with Pagination(page, per\_page ..) and than just the
media\_entries = pagination() is a lot nicer.

The reasons for the coupling with the Request object where(are)


-  to generate the urls for the other pages, we need the current
   url to preserve the current page and all the possible other GET
   objects(except ?page=X)
   i agree that the coupling is bad, maybe there is some way to avoid
   this, much appreciate tips into other directions here. Maybe some
   global url modifier ?
-  from the request object i can grab the GET['page'] value
   directly and do some error checking(negative values, not a number
   ..)
   Its probably better to do that at some place higher up.

[Christopher Allan Webber]

Re `#1 </issues/1>`_: In the template instead of:

::

            <a href={{pagination.url_generator(page)}}>{{ page }}</a>

you could do:

::

            <a href={{ self.request.path_info }}?page={{ page }}>{{ page }}</a>

and I think that would work just as well doing it in the template
that way? Then you could ditch url\_generator().

One more thing: if you could add docstrings to each method that
would be great... maybe also a docstring on the class briefly
explaining what it does if that's appropriate.

[Bernhard Keller]

::

     <a href={{ self.request.path_info }}?page={{ page }}>{{ page }}</a>

works fine, but doesn't allow me to keep other possible GET
information,
for example the url
`http://mediagoblin.com/u/guest/?page=2&sort=asc <http://mediagoblin.com/u/guest/?page=2&sort=asc>`_
would lose the sort=asc information

::

       <a href={{ self.request.path}}?page={{ page }}&amp;{{ request.query_string }}>{{ page }}</a>

the above keeps the url complete, but for it to work, i have to
previously delete the old page information, otherwise it would add
a duplicate page to the GET part

so directly above the rendering i'm deleting the old information

::

    del request.GET['page']

not a very pretty hack :)

[Christopher Allan Webber]

haven't tested it but something like this probably works:

::

    import copy
    import urllib
    
    def get_page_url(path_info, page_no, get_params=None):
        """ 
        Get a new page based off of the path_info, the new page number,
        and existing get parameters.
        """ 
        new_get_params = copy.copy(get_params or {})
        new_get_params['page'] = page_no
        return "%s?%s" % (
            path_info, urllib.urlencode(new_get_params))

You could even make that a class method of Pagination so that
you'll always have it on hand already assuming you're in the
paginator macro? Or you could just pass that method into the
context.

[Bernhard Keller]

[https://gitorious.org/\ :sub:`hunabku/mediagoblin/hunabkus-mediagoblin/commits/pagination](https://gitorious.org/`\ hunabku/mediagoblin/hunabkus-mediagoblin/commits/pagination)

should be working fine now, tests still have to be added

[Christopher Allan Webber]

Merged and pushed. I made a few changes, largely stylistic. One not
very stylistic one though is that I made it so that the decorator
actually passes the current page number into the view, which I
think reduces redundancy.

Anyway, thanks Bernhard, this was a great first submission!

[Christopher Allan Webber]

Sorry, moving from closed -> in progress because it still needs
test. But assigning to myself for that :)

[Elrond]

Code is there. We need to use it in more places, probably. And
Chris wants some "testing".

[Christopher Allan Webber]

Never got tests added to this... shouldn't be too hard though,
assigning to Caleb!

[Will Kahn-Greene]

The original url for this bug was ​http://bugs.foocorp.net/issues/329 .
Relations:
#142: related, #241: related, #31: blocked

[Will Kahn-Greene]

The outstanding part of this bug is to add tests for the paginator. That's it.

Bumping it for someone to look at and work on.

[Christopher Allan Webber]

Removing ownership of this ticket. If someone wants to work on this it's a good simple ticket.

[Siddhartha Sahai]

This is a pretty dumb way of unit testing pagination, and totally not the way to get a database handle but I don't know any other way right now, can someone point out an easier method to get a db cursor?

​http://pastebin.com/yhAkZNwz

[Christopher Allan Webber]

Hey sidthekid, I just checked it out. A much better way to do this I think would be to make use of the fixture tooling we have.

If you do something like this:

from mediagoblin.tests.tools import fixture_add_user, fixture_add_comment

def test_foo(test_app):
    fixture_add_user()
    # now add comments....

Take a look at the tests in test_collections for examples.

By putting test_app as a parameter, you get a pre-setup mediagoblin app, with database and everything init'ed.

Hope that helps!

[Ben Sturmfels]

Copy of sidthekid's prototype tests, just so we don't lose the Pastebin content.

[Ben Sturmfels]

I've now incorporated and extended sidthekid's test_properties in related ticket #5465. Closing #5465 should then allow us to also close this ticket. Yay!

[Boris Bobrov]

Thank you! Fixed in e17566b, 58b3a65