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/ 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/_{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: 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 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/_{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