Opened 13 years ago

Closed 8 years ago

#55 closed defect (fixed)

Need a paginator with tests

Reported by: Christopher Allan Webber Owned by:
Priority: minor Milestone:
Component: programming Keywords: bitesized, test
Cc: Parent Tickets:

Description (last modified by Will Kahn-Greene)

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

` <>`_
would be a good starting point

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

Attachments (1) (1.5 KB ) - added by Ben Sturmfels 8 years ago.
Copy of sidthekid's prototype tests, just so we don't lose the Pastebin content.

Download all attachments as: .zip

Change History (23)

comment:1 by Christopher Allan Webber, 13 years ago

Owner: set to Bernhard Keller
Bernard Keller (hunabku) expressed interest in this, assigning to

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

comment:2 by Bernhard Keller, 13 years ago

[\ :sub:`hunabku/mediagoblin/hunabkus-mediagoblin/commits/hunabku/master](`\ hunabku/mediagoblin/hunabkus-mediagoblin/commits/hunabku/master)

first version for pagination.

basic components are "class Pagination" in [should be
moved to a different file]

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

comment:3 by Christopher Allan Webber, 13 years ago

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,

   {% 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



    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,
             '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


        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

   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.

comment:4 by Bernhard Keller, 13 years ago

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.

comment:5 by Christopher Allan Webber, 13 years ago

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.

comment:6 by Bernhard Keller, 13 years ago


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

works fine, but doesn't allow me to keep other possible GET
for example the url
` <>`_
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 :)

comment:7 by Christopher Allan Webber, 13 years ago

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

comment:8 by Bernhard Keller, 13 years ago

[\ :sub:`hunabku/mediagoblin/hunabkus-mediagoblin/commits/pagination](`\ hunabku/mediagoblin/hunabkus-mediagoblin/commits/pagination)

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

comment:9 by Christopher Allan Webber, 13 years ago

Status: NewClosed
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!

comment:10 by Christopher Allan Webber, 13 years ago

Owner: changed from Bernhard Keller to Christopher Webber
Status: ClosedIn Progress
Sorry, moving from closed -> in progress because it still needs
test. But assigning to myself for that :)

comment:11 by Elrond, 13 years ago

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

comment:12 by Christopher Allan Webber, 13 years ago

Owner: changed from Christopher Webber to Caleb Davis
Never got tests added to this... shouldn't be too hard though,
assigning to Caleb!

comment:13 by Will Kahn-Greene, 12 years ago

The original url for this bug was .
#142: related, #241: related, #31: blocked

comment:14 by Will Kahn-Greene, 12 years ago

Component: programming
Description: modified (diff)

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.

comment:15 by Christopher Allan Webber, 12 years ago

Keywords: bitesized added

comment:16 by Christopher Allan Webber, 11 years ago

Owner: Caleb Davis removed
Status: acceptedassigned

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

comment:17 by Christopher Allan Webber, 11 years ago

Status: assignedaccepted

comment:18 by Siddhartha Sahai, 10 years ago

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?

comment:19 by ShawnRisk, 10 years ago

Keywords: test added

comment:20 by Christopher Allan Webber, 10 years ago

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 import fixture_add_user, fixture_add_comment

def test_foo(test_app):
    # 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!

by Ben Sturmfels, 8 years ago

Attachment: added

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

comment:21 by Ben Sturmfels, 8 years ago

Owner: set to Ben Sturmfels
Status: acceptedin_progress

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!

comment:22 by Boris Bobrov, 8 years ago

Owner: Ben Sturmfels removed
Resolution: fixed
Status: in_progressclosed

Thank you! Fixed in e17566b, 58b3a65

Note: See TracTickets for help on using tickets.