Hello Orestis,
sorry that I did not find the time to get back to you earlier.
On Mon, 19 Oct 2015, Orestis Ioannou wrote:
> I reworked most of the patch, using the ListView and factorising most of
> the code.
The code looks clean and slick. Good work!
> Let me know if this needs more work :)
But I don't see any test. I would suggest to test:
1/ that the package page contains a link to the package news page (the
link you added in the header of the panel)
2/ that the package page contains a link to the "?page=2" version of the
package news page when you have more news than the configured limit (and
possibly that you don't have any such link when you're below)
3/ the package news page does return the expected content for "?page=2"
just to ensure that pagination works and that we display something that
looks OK
Just a few comments still:
> diff --git a/distro_tracker/core/panels.py b/distro_tracker/core/panels.py
> index 8c2cd82..668cb64 100644
> --- a/distro_tracker/core/panels.py
> +++ b/distro_tracker/core/panels.py
> @@ -805,9 +805,11 @@ class NewsPanel(BasePanel):
> def context(self):
> news = News.objects.prefetch_related('signed_by')
> news =
> news.filter(package=self.package).order_by('-datetime_created')
> + more_pages = True if len(news) > self.NEWS_LIMIT else False
This will force the evaluation of the full query. We don't want to
retrieve everything here... one way is to use news.count() but we can
possibly avoid that query altogether if we evaluate the length of the
result after the truncation to NEWS_LIMIT.
If we are on the limit, chances are high that we have more entries...
So that would give:
news = list(news[:self.NEWS_LIMIT])
more_pages = len(news) == self.NEWS_LIMIT
The first line forces the evaluation of the queryset with the appropriate
LIMIT SQL clause. And then we check how many lines we got back.
> return {
> - 'news': news
> + 'news': news,
> + 'has_more': more_pages
> }
> --- a/distro_tracker/core/views.py
> +++ b/distro_tracker/core/views.py
> @@ -176,6 +176,35 @@ def news_page(request, news_id):
> })
>
>
> +class PackageNews(ListView):
[...]
> + def get_queryset(self):
> + news = News.objects.prefetch_related('signed_by')
> + return(news.filter(package=self.package)
> + .order_by('-datetime_created'))
Or more cleanly:
news = self.package.news_set.prefetch_related('signed_by')
return news.order_by('-datetime_created')
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/
