On Mon, 07 Sep 2015, Orestis Ioannou wrote:
> Thanks for the review.
> I updated my patch
Thanks. Here are my comments.
> > And then you reuse that code for functional testing with selenium (cf
> > functional_tests/tests.py).
>
> Added a script. I am not sure though if the directory i put it is the
> correct.
Well, when I mentioned "./manage.py tracker_receive_news" it was a
suggestion for a way to inject many news for some manual testing. I don't
think it's an appropriate interface to just create News objects in
the context of automated testing.
A simple loop to create News objects should be enough, no?
for i in range(100):
pkg.news_set.create(title="News {}".format(i),
created_by="Author {}".format(i))
> > There's possibly also room for simplication by using generic views like
> > django.views.generic.list.ListView, they support pagination natively...
>
> I am not sure how to add the ListView for the panel so i didn't do as
> you suggested but i managed to factorize as much code as possible both
> in the templates and in the view-panel.
You don't use the ListView in the panel but only on the dedicated news
page. In the package page, in the news panel, you just reuse the same
bit of template and you feed the list of news that matches what is
actually displayed on page 1 of the "news page".
The ListView[1] has MultipleObjectMixin[2] as ancestor so you can
set a class attribute "paginate_by = 30" and it will do the right thing
for you (you just have to use variables provided in the context aka
object_list/is_paginated/paginator/page_obj, you can set
"context_object_name = 'news'" in the ListView if you want to the context
to provide the list of News under that name instead of object_list so that
you can reuse the same name in the news panel and in the news page).
[1]
https://docs.djangoproject.com/en/1.8/ref/class-based-views/generic-display/#listview
[2]
https://docs.djangoproject.com/en/1.8/ref/class-based-views/mixins-multiple-object/#django.views.generic.list.MultipleObjectMixin
I don't necessarily want the package page to paginate. If the user wants to
see more news, the link he's offered redirect him to the page dedicated to
news. You can either only put a link pointing to page 2, or you can offer
a list of pages ready to access... it's up to you.
> I also added some tests. The tests for the view and the panel are quite
> similar. I am not sure if there's a better way to do this.
Yes there is: don't duplicate features and handle pagination only on the
news page... :-)
I hope it's enough guidance for you. Let me know if you have questions.
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/
