Hey, Thanks for the review. I updated my patch On Thu, 3 Sep 2015 21:00:14 +0200 Raphael Hertzog <hert...@debian.org> wrote: > > If you want to test this, you have to add many news to a single package. > You can do that manually I guess (either in an interactive Python shell > or with the help of "./manage.py tracker_receive_news"), or better, you > script it. > > 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. To run it i did ./manage.py shell and inside the interactive shell i run execfile() to add news. I can't seem to find out why is not working when simply doing ./manage.py shell < distro_tracker/mail/create_news.py Even though __name__ is __builtin__ it doesn't work. For the tests its much easier since i just need to call a method. > > 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. 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. Cheers, Orestis
From 04006d9c10502af8a629cbf6dac9c0777d528023 Mon Sep 17 00:00:00 2001 From: Orestis Ioannou <ores...@oioannou.com> Date: Wed, 2 Sep 2015 10:38:10 +0200 Subject: [PATCH 1/3] Add pagination for the package news --- distro_tracker/core/panels.py | 8 ++--- distro_tracker/core/retrieve_data.py | 22 ++++++++++++++ .../core/templates/core/_news_pagination.html | 34 ++++++++++++++++++++++ .../core/templates/core/package_news.html | 8 +++++ .../core/templates/core/panels/news.html | 28 +++--------------- distro_tracker/core/views.py | 20 +++++++++++++ distro_tracker/project/urls.py | 4 +++ 7 files changed, 95 insertions(+), 29 deletions(-) create mode 100644 distro_tracker/core/templates/core/_news_pagination.html create mode 100644 distro_tracker/core/templates/core/package_news.html diff --git a/distro_tracker/core/panels.py b/distro_tracker/core/panels.py index 8c2cd82..8c7c1d5 100644 --- a/distro_tracker/core/panels.py +++ b/distro_tracker/core/panels.py @@ -23,8 +23,8 @@ from distro_tracker.core.models import PseudoPackageName from distro_tracker.core.models import ActionItem from distro_tracker.core.models import PackageExtractedInfo from distro_tracker.core.models import MailingList -from distro_tracker.core.models import News from distro_tracker.core.models import BinaryPackageBugStats +from distro_tracker.core.retrieve_data import paginate_package_news from debian.debian_support import AptPkgVersion from collections import defaultdict @@ -803,11 +803,9 @@ class NewsPanel(BasePanel): @cached_property def context(self): - news = News.objects.prefetch_related('signed_by') - news = news.filter(package=self.package).order_by('-datetime_created') - news = news[:self.NEWS_LIMIT] + page = self.request.GET.get('page') or 1 return { - 'news': news + 'news': paginate_package_news(self.package, self.NEWS_LIMIT, page) } @property diff --git a/distro_tracker/core/retrieve_data.py b/distro_tracker/core/retrieve_data.py index 45b4c9b..3f1b257 100644 --- a/distro_tracker/core/retrieve_data.py +++ b/distro_tracker/core/retrieve_data.py @@ -21,6 +21,7 @@ from distro_tracker.core.models import PackageExtractedInfo from distro_tracker.core.models import BinaryPackageName from distro_tracker.core.models import BinaryPackage from distro_tracker.core.models import SourcePackageDeps +from distro_tracker.core.models import News from distro_tracker.core.utils.packages import ( extract_information_from_sources_entry, extract_information_from_packages_entry, @@ -32,6 +33,7 @@ from distro_tracker.accounts.models import UserEmail from django.utils.six import reraise from django.db import transaction from django.db import models +from django.core.paginator import Paginator, EmptyPage, PageNotAnInteger from debian import deb822 import re @@ -155,6 +157,26 @@ def retrieve_repository_info(sources_list_entry): return repository_information +def paginate_package_news(package, limit, page): + """ + Retrieves the news of a `package` and creates a pagination object at a + specific `page` + """ + news = News.objects.prefetch_related('signed_by') + package_news = (news.filter(package=package) + .order_by('-datetime_created')) + paginator = Paginator(package_news, limit) + try: + news = paginator.page(page) + except PageNotAnInteger: + # If page is not an integer, deliver first page. + news = paginator.page(1) + except EmptyPage: + # If page is out of range (e.g. 9999), deliver last page of results. + news = paginator.page(paginator.num_pages) + return news + + class PackageUpdateTask(BaseTask): """ A subclass of the :class:`BaseTask <distro_tracker.core.tasks.BaseTask>` diff --git a/distro_tracker/core/templates/core/_news_pagination.html b/distro_tracker/core/templates/core/_news_pagination.html new file mode 100644 index 0000000..9c30499 --- /dev/null +++ b/distro_tracker/core/templates/core/_news_pagination.html @@ -0,0 +1,34 @@ +<ul class="list-group list-group-flush"> + {% for news_item in news %} + <li class="list-group-item"> + [<span class="news-date">{{ news_item.datetime_created|date:"Y-m-d" }}</span>] + <a href="{% url 'dtracker-news-page' news_item.pk %}"> + <span class="news-title">{{ news_item.title }}</span> + </a> + {% if news_item.created_by %}(<span class="news-creator">{{ news_item.created_by }}</span>){% endif %} + {% with signers=news_item.signed_by.all %} + {% if signers and signers.0.name != news_item.created_by %} + {% spaceless %} + <span>(signed by: </span> + {% for signer in signers %} + <span class="news-signer">{{ signer.name }}</span> + {% if not forloop.last %}<span>, </span>{% endif %} + {% endfor %} + <span>)</span> + {% endspaceless %} + {% endif %} + {% endwith %} + </li> + {% endfor %} +</ul> +<div class="pagination"> + <span class="step-links"> + {% if news.has_previous %} + <a href="?page={{ news.previous_page_number }}">previous</a> + {% endif %} + <span class="current">Page {{ news.number }} of {{ news.paginator.num_pages }}.</span> + {% if news.has_next %} + <a href="?page={{ news.next_page_number }}">next</a> + {% endif %} + </span> +</div> diff --git a/distro_tracker/core/templates/core/package_news.html b/distro_tracker/core/templates/core/package_news.html new file mode 100644 index 0000000..6537800 --- /dev/null +++ b/distro_tracker/core/templates/core/package_news.html @@ -0,0 +1,8 @@ +{% extends 'core/base.html' %} + +{% block content %} +<h1 class="text-center">News for package <a href="{% url 'dtracker-package-page' package %}">{{ package }}</a></h1> +<div class="row-fluid"> +{% include "core/_news_pagination.html" %} +</div> +{% endblock %} diff --git a/distro_tracker/core/templates/core/panels/news.html b/distro_tracker/core/templates/core/panels/news.html index 1cc38fd..478ca14 100644 --- a/distro_tracker/core/templates/core/panels/news.html +++ b/distro_tracker/core/templates/core/panels/news.html @@ -3,7 +3,7 @@ {% block panel-header %} <div class="row-fluid"> <div class="pull-left"> - <span>{{ panel.title }}</span> + <span>{{ panel.title }} <a href="{% url 'dtracker-package-news' package.name %}"><i class='icon-share'></i></a></span> </div> <div class="pull-right"> <a title="rss feed" href="{% url 'dtracker-package-rss-news-feed' package.name %}"> @@ -14,27 +14,7 @@ {% endblock %} {% block panel-body %} -<ul class="list-group list-group-flush"> - {% for news_item in panel.context.news %} - <li class="list-group-item"> - [<span class="news-date">{{ news_item.datetime_created|date:"Y-m-d" }}</span>] - <a href="{% url 'dtracker-news-page' news_item.pk %}"> - <span class="news-title">{{ news_item.title }}</span> - </a> - {% if news_item.created_by %}(<span class="news-creator">{{ news_item.created_by }}</span>){% endif %} - {% with signers=news_item.signed_by.all %} - {% if signers and signers.0.name != news_item.created_by %} - {% spaceless %} - <span>(signed by: </span> - {% for signer in signers %} - <span class="news-signer">{{ signer.name }}</span> - {% if not forloop.last %}<span>, </span>{% endif %} - {% endfor %} - <span>)</span> - {% endspaceless %} - {% endif %} - {% endwith %} - </li> - {% endfor %} -</ul> +{% with news=panel.context.news %} + {% include "core/_news_pagination.html" %} +{% endwith %} {% endblock %} diff --git a/distro_tracker/core/views.py b/distro_tracker/core/views.py index 16b958b..a5c11ca 100644 --- a/distro_tracker/core/views.py +++ b/distro_tracker/core/views.py @@ -26,6 +26,7 @@ from django.views.decorators.cache import cache_control from django.core.mail import send_mail from django.core.exceptions import PermissionDenied from django.core.urlresolvers import reverse, reverse_lazy +from distro_tracker.core.retrieve_data import paginate_package_news from django.utils.http import urlquote from distro_tracker.core.models import get_web_package from distro_tracker.core.forms import CreateTeamForm @@ -176,6 +177,25 @@ def news_page(request, news_id): }) +def package_news(request, package_name): + """ + A view that renders all the news of a package. Pagination included + """ + package = get_web_package(package_name) + if not package: + raise Http404 + _DEFAULT_NEWS_LIMIT = 30 + NEWS_LIMIT = getattr( + settings, + 'DISTRO_TRACKER_NEWS_PANEL_LIMIT', + _DEFAULT_NEWS_LIMIT) + page = request.GET.get('page') or 1 + return render(request, 'core/package_news.html', { + 'package': package, + 'news': paginate_package_news(package, NEWS_LIMIT, page), + }) + + class ActionItemJsonView(View): """ View renders a :class:`distro_tracker.core.models.ActionItem` in a JSON diff --git a/distro_tracker/project/urls.py b/distro_tracker/project/urls.py index cbe1fe8..f841858 100644 --- a/distro_tracker/project/urls.py +++ b/distro_tracker/project/urls.py @@ -213,6 +213,10 @@ urlpatterns = patterns( url(r'^teams/(?P<slug>.+?)/$', TeamDetailsView.as_view(), name='dtracker-team-page'), + # Package news page + url(r'^pkg/(?P<package_name>.+)/news/', + 'distro_tracker.core.views.package_news', + name='dtracker-package-news'), # Dedicated package page url(r'^pkg/(?P<package_name>[^/]+)/?$', -- 2.1.4 From e067b2e651d0791913f3d99242020f6b4ee19f02 Mon Sep 17 00:00:00 2001 From: Orestis Ioannou <ores...@oioannou.com> Date: Sun, 6 Sep 2015 16:25:14 +0200 Subject: [PATCH 2/3] Add script to automatically create many news for a package --- distro_tracker/mail/create_news.py | 45 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 distro_tracker/mail/create_news.py diff --git a/distro_tracker/mail/create_news.py b/distro_tracker/mail/create_news.py new file mode 100644 index 0000000..1bd4db8 --- /dev/null +++ b/distro_tracker/mail/create_news.py @@ -0,0 +1,45 @@ +# -*- coding: utf-8 -*- + +# Copyright 2013 The Distro Tracker Developers +# See the COPYRIGHT file at the top-level directory of this distribution and +# at http://deb.li/DTAuthors +# +# This file is part of Distro Tracker. It is subject to the license terms +# in the LICENSE file found in the top-level directory of this +# distribution and at http://deb.li/DTLicense. No part of Distro Tracker, +# including this file, may be copied, modified, propagated, or distributed +# except according to the terms contained in the LICENSE file. +""" +Adds many news in a package +""" +from __future__ import unicode_literals +from django.utils.encoding import force_bytes +from distro_tracker.core.models import SourcePackageName, SourcePackage +from distro_tracker.mail.mail_news import process + +from email.message import Message +from django.db import transaction + + +def add_news(name, version=None): + package_name = SourcePackageName.objects.get(name=name) + if version: + package = SourcePackage.objects.get(source_package_name=package_name, + version=version) + else: + package = SourcePackage.objects.filter( + source_package_name=package_name)[0] + # create dummy news + for i in range(1, 70): + message = Message() + subject = 'Some message ' + str(i) + content = 'Some message content ' + str(i) + message['Subject'] = subject + message['X-Distro-Tracker-Package'] = package.name + message.set_payload(content) + process(force_bytes(message.as_string(), 'utf-8')) + +# execute from django shell +if __name__ == '__builtin__': + add_news('libreoffice') + transaction.commit() -- 2.1.4 From b0aa2aa9118f0a9119039bf90a1ad1d9a6d7ab5e Mon Sep 17 00:00:00 2001 From: Orestis Ioannou <ores...@oioannou.com> Date: Mon, 7 Sep 2015 14:26:43 +0200 Subject: [PATCH 3/3] Add tests for news pagination --- distro_tracker/core/tests/tests_panels.py | 94 +++++++++++++++++++++++++++++++ distro_tracker/core/tests/tests_views.py | 87 ++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+) diff --git a/distro_tracker/core/tests/tests_panels.py b/distro_tracker/core/tests/tests_panels.py index bf5c91b..a35621a 100644 --- a/distro_tracker/core/tests/tests_panels.py +++ b/distro_tracker/core/tests/tests_panels.py @@ -24,6 +24,7 @@ from distro_tracker.core.models import PackageName from distro_tracker.core.models import SourcePackage from distro_tracker.core.models import Repository, SourcePackageRepositoryEntry from distro_tracker.core.panels import VersionedLinks, DeadPackageWarningPanel +from distro_tracker.mail.create_news import add_news class VersionedLinksPanelTests(TestCase): @@ -219,3 +220,96 @@ class DeadPackageWarningPanelTests(TestCase): SourcePackageRepositoryEntry.objects.create( source_package=self.srcpkg, repository=self.repo1) self.assertFalse(self.panel.context['disappeared']) + + +class NewsPanelTest(TestCase): + """ + Tests for the package view. + """ + def setUp(self): + self.package = SourcePackageName.objects.create(name='dummy-package') + self.src_pkg = SourcePackage.objects.create( + source_package_name=self.package, version='1.0.0') + self.src_pkg.save() + add_news('dummy-package', '1.0.0') + + def get_package_page_response(self, kwargs, page=None): + url = reverse('dtracker-package-page', kwargs=kwargs) + if not page: + return self.client.get(url) + else: + return self.client.get('%s?page=%s' % (url, page)) + + def find_link_in_content(self, content, link): + """ + Helper method to parse response and get link for package news + """ + html = soup(content) + for a_tag in html.findAll('a', {'href': True}): + if a_tag['href'] == link: + return True + return False + + def is_next_in_pagination(self, content): + """ + Helper method to find pagination in content + """ + html = soup(content) + pagination = html.find("div", {'class': 'pagination'}) + if 'next' not in str(pagination): + return False + return True + + def get_current_status(self, content, page): + """ + Helper method to find if current status is correct + """ + html = soup(content) + current = html.find("span", {'class': 'current'}).text + if ('Page ' + page + ' of') not in current: + return False + return True + + def test_news_pagination(self): + """ + Tests pagination in the news panel + """ + response = self.get_package_page_response({ + 'package_name': self.package.name, + }) + news_link = reverse('dtracker-package-news', kwargs={ + 'package_name': self.package.name + }) + self.assertTrue(self.find_link_in_content(response.content, news_link)) + self.assertTrue(self.is_next_in_pagination(response.content)) + self.assertTrue(self.get_current_status(response.content, '1')) + # find next link + self.assertTrue(self.find_link_in_content(response.content, '?page=2')) + # verify no previous link + self.assertFalse(self.find_link_in_content(response.content, '?page=1')) + + def test_other_news_pages(self): + """ + Tests that page numbers give correct results + """ + response = self.get_package_page_response({ + 'package_name': self.package.name, + }, 2) + news_link = reverse('dtracker-package-news', kwargs={ + 'package_name': self.package.name + }) + self.assertTrue(self.find_link_in_content(response.content, news_link)) + # find next link + self.assertTrue(self.find_link_in_content(response.content, '?page=3')) + # verify previous link + self.assertTrue(self.find_link_in_content(response.content, '?page=1')) + + # test non existent page + response = self.get_package_page_response({ + 'package_name': self.package.name, + }, 100) + self.assertFalse(self.is_next_in_pagination(response.content)) + # verify redirection to last page + self.assertTrue(self.get_current_status(response.content, '3')) + # verify no previous link + self.assertTrue(self.find_link_in_content(response.content, '?page=2')) diff --git a/distro_tracker/core/tests/tests_views.py b/distro_tracker/core/tests/tests_views.py index e45c116..316a601 100644 --- a/distro_tracker/core/tests/tests_views.py +++ b/distro_tracker/core/tests/tests_views.py @@ -14,6 +14,7 @@ Tests for the Distro Tracker core views. """ from __future__ import unicode_literals +from bs4 import BeautifulSoup as soup from distro_tracker.test import TestCase from django.test.utils import override_settings from distro_tracker.core.models import BinaryPackage, BinaryPackageName @@ -21,6 +22,7 @@ from distro_tracker.core.models import SourcePackageName, SourcePackage from distro_tracker.core.models import PackageName, PseudoPackageName from distro_tracker.core.models import News from distro_tracker.core.models import ActionItem, ActionItemType +from distro_tracker.mail.create_news import add_news import json from django.core.urlresolvers import reverse @@ -477,3 +479,88 @@ class ActionItemJsonViewTest(TestCase): })) self.assertEqual(response.status_code, 404) + + +class NewsPanelTest(TestCase): + """ + Tests for the package view. + """ + def setUp(self): + self.package = SourcePackageName.objects.create(name='dummy-package') + self.src_pkg = SourcePackage.objects.create( + source_package_name=self.package, version='1.0.0') + self.src_pkg.save() + add_news('dummy-package', '1.0.0') + + def get_news_page(self, kwargs, page=None): + url = reverse('dtracker-package-news', kwargs=kwargs) + if not page: + return self.client.get(url) + else: + return self.client.get('%s?page=%s' % (url, page)) + + def find_link_in_content(self, content, link): + """ + Helper method to parse response and get link for package news + """ + html = soup(content) + for a_tag in html.findAll('a', {'href': True}): + if a_tag['href'] == link: + return True + return False + + def is_next_in_pagination(self, content): + """ + Helper method to find pagination in content + """ + html = soup(content) + pagination = html.find("div", {'class': 'pagination'}) + if 'next' not in str(pagination): + return False + return True + + def get_current_status(self, content, page): + """ + Helper method to find if current status is correct + """ + html = soup(content) + current = html.find("span", {'class': 'current'}).text + if ('Page ' + page + ' of') not in current: + return False + return True + + def test_news_pagination(self): + """ + Tests pagination in the news panel + """ + response = self.get_news_page({ + 'package_name': self.package.name + }) + self.assertTrue(self.is_next_in_pagination(response.content)) + self.assertTrue(self.get_current_status(response.content, '1')) + # find next link + self.assertTrue(self.find_link_in_content(response.content, '?page=2')) + # verify no previous link + self.assertFalse(self.find_link_in_content(response.content, '?page=1')) + + def test_other_news_pages(self): + """ + Tests that page numbers give correct results + """ + response = self.get_news_page({ + 'package_name': self.package.name + }, 2) + # find next link + self.assertTrue(self.find_link_in_content(response.content, '?page=3')) + # verify previous link + self.assertTrue(self.find_link_in_content(response.content, '?page=1')) + + # test non existent page + response = self.get_news_page({ + 'package_name': self.package.name, + }, 100) + self.assertFalse(self.is_next_in_pagination(response.content)) + # verify redirection to last page + self.assertTrue(self.get_current_status(response.content, '3')) + # verify no previous link + self.assertTrue(self.find_link_in_content(response.content, '?page=2')) -- 2.1.4
signature.asc
Description: OpenPGP digital signature