Opened 12 years ago
Closed 7 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 )
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
Subtickets
Attachments (1)
Change History (23)
comment:2 Changed 12 years ago by
[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
comment:3 Changed 12 years ago by
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.
comment:4 Changed 12 years ago by
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 Changed 12 years ago by
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 Changed 12 years ago by
:: <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 }}&{{ 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 Changed 12 years ago by
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.
comment:8 Changed 12 years ago by
[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
comment:9 Changed 12 years ago by
Status: | New → Closed |
---|
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 Changed 12 years ago by
Owner: | changed from Bernhard Keller to Christopher Webber |
---|---|
Status: | Closed → In Progress |
Sorry, moving from closed -> in progress because it still needs test. But assigning to myself for that :)
comment:11 Changed 12 years ago by
Code is there. We need to use it in more places, probably. And Chris wants some "testing".
comment:12 Changed 12 years ago by
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 Changed 11 years ago by
The original url for this bug was http://bugs.foocorp.net/issues/329 .
Relations:
#142: related, #241: related, #31: blocked
comment:14 Changed 11 years ago by
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 Changed 10 years ago by
Keywords: | bitesized added |
---|
comment:16 Changed 10 years ago by
Owner: | Caleb Davis deleted |
---|---|
Status: | accepted → assigned |
Removing ownership of this ticket. If someone wants to work on this it's a good simple ticket.
comment:17 Changed 10 years ago by
Status: | assigned → accepted |
---|
comment:18 Changed 9 years ago by
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 Changed 9 years ago by
Keywords: | test added |
---|
comment:20 Changed 9 years ago by
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!
Changed 7 years ago by
Attachment: | paginator_tests.py added |
---|
Copy of sidthekid's prototype tests, just so we don't lose the Pastebin content.
comment:21 Changed 7 years ago by
Owner: | set to Ben Sturmfels |
---|---|
Status: | accepted → in_progress |
comment:22 Changed 7 years ago by
Owner: | Ben Sturmfels deleted |
---|---|
Resolution: | → fixed |
Status: | in_progress → closed |
Thank you! Fixed in e17566b, 58b3a65