Hey, Thanks for the review. I updated my patch On Thu, 3 Sep 2015 21:00:14 +0200 Raphael Hertzog <hertzog@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 <orestis@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 <orestis@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 <orestis@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
Attachment:
signature.asc
Description: OpenPGP digital signature