Merge lp:~elachuni/rnr-server/filter-reviews into lp:rnr-server

[Anthony Lenton]

Overview
========
This branch implements the agreed solution from bug #688112 providing an optional 'version' argument when fetching reviews, and making all arguments to the reviews getter optional.

Details
=======
For fetching reviews you now have an additional 'version' component in the url that allows you to filter by version. Additionally, if the language, origin, distroseries or version are specified as "any", reviews for any value of that parameter will be retrieved.

See the comments in the bug for a full discussion.

This branch needs lp:~elachuni/rnr-server/rnrclient-filter-reviews to work correctly.

=== modified file 'src/reviewsapp/api/handlers.py'
--- src/reviewsapp/api/handlers.py	2011-01-10 19:15:32 +0000
+++ src/reviewsapp/api/handlers.py	2011-01-19 19:59:08 +0000
@@ -87,8 +87,8 @@
     )
     allowed_methods = ('GET',)
 
-    def read(self, request, language, origin, distroseries, package_name,
-             page='1'):
+    def read(self, request, language, origin, distroseries, version,
+             package_name, page='1'):
         # The %2F used for slashes is itself being quoted before reaching
         # the handler.
         software_item = urllib.unquote_plus(urllib.unquote_plus(package_name))
@@ -96,10 +96,18 @@
         batch_size = settings.API_REVIEWS_PAGINATION
         batch_start = batch_size * (int(page) - 1)
         batch_end = batch_start + batch_size
+        kwargs = {
+            'language': language,
+            'repository__distroseries': distroseries,
+            'repository__origin': origin,
+            'version': version,
+        }
+        kwargs = dict(item for item in kwargs.items() if item[1] != 'any')
         return Review.objects.filter(
-            repository__origin=origin, repository__distroseries=distroseries,
-            softwareitem__package_name=package_name, softwareitem__app_name=appname,
-            hide=False, language=language).order_by(
+            softwareitem__package_name=package_name,
+            softwareitem__app_name=appname,
+            hide=False,
+            **kwargs).order_by(
                 '-usefulness_percentage')[batch_start:batch_end]
 
 

=== modified file 'src/reviewsapp/api/urls.py'
--- src/reviewsapp/api/urls.py	2011-01-04 16:45:55 +0000
+++ src/reviewsapp/api/urls.py	2011-01-19 19:59:08 +0000
@@ -83,13 +83,14 @@
         submit_usefulness_resource, name='rnr-api-recommendations'),
 
     # gets the reviews (and ratings) for a given pkg/app
-    #  GET /1.0/en/ubuntu/+binary/lucid/gnome-games;sane/page/1/
-    #  GET /1.0/en/ubuntu/+binary/lucid/2vcard/
-    url(r'^1.0/(?P<language>\w+)/(?P<origin>\w+)/(?P<distroseries>\w+)/'
-        r'(?P<package_name>[\w%+.;-]+)/$', show_reviews_resource,
-        name='rnr-api-reviews-for-package'),
-    url(r'^1.0/(?P<language>\w+)/(?P<origin>\w+)/(?P<distroseries>\w+)/'
-        r'(?P<package_name>[\w%+.;-]+)/page/(?P<page>\d+)/$',
+    #  GET /1.0/en/ubuntu/+binary/lucid/1:2.32.0-0ubuntu1/gnome-games;sane/page/1/
+    #  GET /1.0/en/ubuntu/+binary/lucid/1:2.32.0-0ubuntu1/2vcard/
+    url(r'^1.0/(?P<language>\w+)/(?P<origin>\w+)/(?P<distroseries>\w+)/'
+        r'(?P<version>[-\w+.:~]+)/(?P<package_name>[\w%+.;-]+)/$',
+        show_reviews_resource, name='rnr-api-reviews-for-package'),
+    url(r'^1.0/(?P<language>\w+)/(?P<origin>\w+)/(?P<distroseries>\w+)/'
+        r'(?P<version>[-\w+.:~]+)/(?P<package_name>[\w%+.;-]+)/'
+        r'page/(?P<page>\d+)/$',
         show_reviews_resource, name='rnr-api-reviews-for-package'),
 
     # submits a new review for a given pkg/app

=== modified file 'src/reviewsapp/tests/test_handlers.py'
--- src/reviewsapp/tests/test_handlers.py	2011-01-04 16:42:36 +0000
+++ src/reviewsapp/tests/test_handlers.py	2011-01-19 19:59:08 +0000
@@ -148,12 +148,11 @@
 class ShowReviewsHandlerTestCase(TestCaseWithFactory):
     def test_review_for_package(self):
         review = self.factory.makeReview(
-            software_item=self.factory.makeSoftwareItem('package_foo'),
-            repository=self.factory.makeRepository('ubuntu', 'hardy'),
-            language='de')
+            software_item=self.factory.makeSoftwareItem('package_foo'))
         handler = ShowReviewsHandler()
 
-        reviews = handler.read(None, 'de', 'ubuntu', 'hardy', 'package_foo')
+        reviews = handler.read(None, 'any', 'any', 'any', 'any',
+            'package_foo')
 
         self.assertEqual(1, len(reviews))
         self.assertEqual(review, reviews[0])
@@ -161,12 +160,10 @@
     def test_review_for_app(self):
         review = self.factory.makeReview(
             software_item=self.factory.makeSoftwareItem('package_foo',
-                app_name='app_foo'),
-            repository=self.factory.makeRepository('ubuntu', 'hardy'),
-            language='en')
+                app_name='app_foo'))
         handler = ShowReviewsHandler()
 
-        reviews = handler.read(None, 'en', 'ubuntu', 'hardy',
+        reviews = handler.read(None, 'any', 'any', 'any', 'any',
                                'package_foo;app_foo')
 
         self.assertEqual(1, len(reviews))
@@ -174,60 +171,134 @@
 
     def test_review_for_language(self):
         package = self.factory.makeSoftwareItem('package_foo')
-        repo = self.factory.makeRepository('ubuntu', 'hardy')
         review1 = self.factory.makeReview(software_item=package,
-            repository=repo, language='en')
+            language='en')
         review2 = self.factory.makeReview(software_item=package,
-            repository=repo, language='de')
+            language='de')
         handler = ShowReviewsHandler()
 
-        reviews = handler.read(None, 'en', 'ubuntu', 'hardy', 'package_foo')
+        reviews = handler.read(None, 'en', 'any', 'any', 'any',
+            'package_foo')
 
         self.assertEqual(1, len(reviews))
         self.assertIn(review1, reviews)
         self.assertNotIn(review2, reviews)
 
+    def test_review_for_any_language(self):
+        package = self.factory.makeSoftwareItem('package_foo')
+        repository = self.factory.makeRepository('ubuntu', 'hardy')
+        review1 = self.factory.makeReview(software_item=package,
+            language='en', repository=repository, version='1.0')
+        review2 = self.factory.makeReview(software_item=package,
+            language='de', repository=repository, version='1.0')
+        handler = ShowReviewsHandler()
+
+        reviews = handler.read(None, 'any', 'ubuntu', 'hardy', '1.0',
+            'package_foo')
+
+        self.assertEqual(2, len(reviews))
+        self.assertIn(review1, reviews)
+        self.assertIn(review2, reviews)
+
     def test_review_from_distroseries(self):
         package = self.factory.makeSoftwareItem('package_foo')
         review1 = self.factory.makeReview(software_item=package,
+            repository=self.factory.makeRepository('ubuntu', 'hardy'))
+        review2 = self.factory.makeReview(software_item=package,
+            repository=self.factory.makeRepository('ubuntu', 'lucid'))
+        handler = ShowReviewsHandler()
+
+        reviews = handler.read(None, 'any', 'any', 'hardy', 'any',
+            'package_foo')
+
+        self.assertEqual(1, len(reviews))
+        self.assertIn(review1, reviews)
+        self.assertNotIn(review2, reviews)
+
+    def test_review_from_any_distroseries(self):
+        package = self.factory.makeSoftwareItem('package_foo')
+        review1 = self.factory.makeReview(software_item=package,
             repository=self.factory.makeRepository('ubuntu', 'hardy'),
-            language='en')
+            language='en', version='+-1')
         review2 = self.factory.makeReview(software_item=package,
             repository=self.factory.makeRepository('ubuntu', 'lucid'),
-            language='en')
+            language='en', version='+-1')
         handler = ShowReviewsHandler()
 
-        reviews = handler.read(None, 'en', 'ubuntu', 'hardy', 'package_foo')
+        reviews = handler.read(None, 'en', 'ubuntu', 'any', '+-1',
+            'package_foo')
 
-        self.assertEqual(1, len(reviews))
+        self.assertEqual(2, len(reviews))
         self.assertIn(review1, reviews)
-        self.assertNotIn(review2, reviews)
+        self.assertIn(review2, reviews)
 
     def test_review_from_origin(self):
         package = self.factory.makeSoftwareItem('package_foo')
         review1 = self.factory.makeReview(software_item=package,
+            repository=self.factory.makeRepository('ubuntu', 'hardy'))
+        review2 = self.factory.makeReview(software_item=package,
+            repository=self.factory.makeRepository('debian', 'lenny'))
+        handler = ShowReviewsHandler()
+
+        reviews = handler.read(None, 'any', 'ubuntu', 'any', 'any',
+            'package_foo')
+
+        self.assertEqual(1, len(reviews))
+        self.assertIn(review1, reviews)
+        self.assertNotIn(review2, reviews)
+
+    def test_review_from_any_origin(self):
+        package = self.factory.makeSoftwareItem('package_foo')
+        review1 = self.factory.makeReview(software_item=package,
             repository=self.factory.makeRepository('ubuntu', 'hardy'),
-            language='en')
-        review2 = self.factory.makeReview(software_item=package,
-            repository=self.factory.makeRepository('debian', 'lenny'),
-            language='en')
-        handler = ShowReviewsHandler()
-
-        reviews = handler.read(None, 'en', 'ubuntu', 'hardy', 'package_foo')
+            language='en', version='vanilla')
+        review2 = self.factory.makeReview(software_item=package,
+            repository=self.factory.makeRepository('debian', 'hardy'),
+            language='en', version='vanilla')
+        handler = ShowReviewsHandler()
+
+        reviews = handler.read(None, 'en', 'any', 'hardy', 'vanilla',
+            'package_foo')
+
+        self.assertEqual(2, len(reviews))
+        self.assertIn(review1, reviews)
+        self.assertIn(review2, reviews)
+
+    def test_review_for_version(self):
+        package = self.factory.makeSoftwareItem('package_foo')
+        review1 = self.factory.makeReview(software_item=package,
+            version='0.2')
+        review2 = self.factory.makeReview(software_item=package,
+            version='0.3')
+        handler = ShowReviewsHandler()
+
+        reviews = handler.read(None, 'any', 'any', 'any', '0.2',
+            'package_foo')
 
         self.assertEqual(1, len(reviews))
         self.assertIn(review1, reviews)
         self.assertNotIn(review2, reviews)
 
+    def test_review_for_any_version(self):
+        package = self.factory.makeSoftwareItem('package_foo')
+        repository = self.factory.makeRepository('ubuntu', 'hardy')
+        review1 = self.factory.makeReview(software_item=package,
+            language='en', repository=repository, version='1.0')
+        review2 = self.factory.makeReview(software_item=package,
+            language='en', repository=repository, version='2.0')
+        handler = ShowReviewsHandler()
+
+        reviews = handler.read(None, 'en', 'ubuntu', 'hardy', 'any',
+            'package_foo')
+
+        self.assertEqual(2, len(reviews))
+        self.assertIn(review1, reviews)
+        self.assertIn(review2, reviews)
+
     def test_review_sorting(self):
         package = self.factory.makeSoftwareItem('pkg_foo')
-        repository = self.factory.makeRepository('ubuntu', 'hardy')
-        review1 = self.factory.makeReview(software_item=package,
-            repository=repository,
-            language='en')
-        review2 = self.factory.makeReview(software_item=package,
-            repository=repository,
-            language='en')
+        review1 = self.factory.makeReview(software_item=package)
+        review2 = self.factory.makeReview(software_item=package)
         showHandler = ShowReviewsHandler()
         usefulnessHandler = SubmitUsefulnessHandler()
 
@@ -235,7 +306,8 @@
         request = Mock()
         request.user = self.factory.makeUser()
         usefulnessHandler.create(request, review2.id)
-        reviews = showHandler.read(None, 'en', 'ubuntu', 'hardy', 'pkg_foo')
+        reviews = showHandler.read(None, 'any', 'any', 'any', 'any',
+            'pkg_foo')
 
         self.assertEqual(2, len(reviews))
         # review2 is sorted first
@@ -246,31 +318,28 @@
         request.user = self.factory.makeUser()
         usefulnessHandler.create(request, review1.id)
 
-        reviews = showHandler.read(None, 'en', 'ubuntu', 'hardy', 'pkg_foo')
+        reviews = showHandler.read(None, 'any', 'any', 'any', 'any',
+            'pkg_foo')
 
         self.assertEqual(2, len(reviews))
         # review1 is sorted first
         self.assertEquals(review1, reviews[0])
 
-    def _make_5_reviews_for_package(self, package_name, origin='ubuntu',
-                                    distro_series='lucid', language='en'):
+    def _make_5_reviews_for_package(self, package_name):
         package = self.factory.makeSoftwareItem(package_name)
-        repository = self.factory.makeRepository(
-            origin=origin, distro_series=distro_series)
-        foo_reviews = []
         for count in range(5):
-            foo_reviews.append(self.factory.makeReview(
-                software_item=package, language=language,
-                repository=repository))
+            self.factory.makeReview(software_item=package)
 
-    def _reviews_url_for_package(self, package_name, origin='ubuntu',
-                                 distro_series='lucid', language='en',
+    def _reviews_url_for_package(self, package_name, origin='any',
+                                 distro_series='any', language='any',
+                                 version='any',
                                  page=None):
         kwargs = {
             'language': language,
             'origin': origin,
             'distroseries': distro_series,
             'package_name': package_name,
+            'version': version,
             }
         if page is not None:
             kwargs['page'] = page
@@ -423,7 +492,8 @@
         software_item = self.factory.makeSoftwareItem(
             package_name=self.required_data['package_name'],
             ratings_total=0, ratings_average=0)
-        url_keys = ('language', 'origin', 'distroseries', 'package_name')
+        url_keys = ('language', 'origin', 'distroseries', 'version',
+            'package_name')
         url_kwargs = dict((key, self.required_data[key]) for key in url_keys)
         url = reverse('rnr-api-reviews-for-package', kwargs=url_kwargs)
         self.client.get(url)
@@ -484,7 +554,8 @@
         url_kwargs = dict(language=review.language,
             origin=review.repository.origin,
             distroseries=review.repository.distroseries,
-            package_name=review.softwareitem.package_name)
+            package_name=review.softwareitem.package_name,
+            version=review.version)
         url = reverse('rnr-api-reviews-for-package', kwargs=url_kwargs)
         self.client.get(url)
         key = cache_key_for_url(url)

=== modified file 'src/reviewsapp/tests/test_rnrclient.py'
--- src/reviewsapp/tests/test_rnrclient.py	2011-01-10 19:15:32 +0000
+++ src/reviewsapp/tests/test_rnrclient.py	2011-01-19 19:59:08 +0000
@@ -148,12 +148,14 @@
             distroseries=review.repository.distroseries,
             packagename=review.package_name(),
             appname=review.app_name(),
+            version = review.version
         )
 
         self.assertEqual(1, len(reviews))
         self.assertEqual(review.package_name(), reviews[0].package_name)
         self.assertEqual(review.app_name(), reviews[0].app_name)
         self.assertEqual(review.summary, reviews[0].summary)
+        self.assertEqual(review.version, reviews[0].version)
 
     def test_get_reviews_page_2(self):
         # The rnr_client can get multiple pages of results.

=== modified file 'src/reviewsapp/tests/test_utilities.py'
--- src/reviewsapp/tests/test_utilities.py	2011-01-10 20:26:56 +0000
+++ src/reviewsapp/tests/test_utilities.py	2011-01-19 19:59:08 +0000
@@ -167,11 +167,12 @@
 
 class InvalidatePaginatedReviewsTestCase(TestCaseWithFactory):
     def _url_for_package(self, package_name, origin='ubuntu',
-        distro_series='lucid', language='en', page=None):
+        distro_series='lucid', language='en', version='1.0', page=None):
         kwargs = {
             'language': language,
             'origin': origin,
             'distroseries': distro_series,
+            'version': version,
             'package_name': urllib.quote_plus(package_name),
             }
         if page is not None:
@@ -180,10 +181,10 @@
         return reverse('rnr-api-reviews-for-package', kwargs=kwargs)
 
     def _request_reviews_for_package(self, package_name, origin='ubuntu',
-        distro_series='lucid', language='en', page=None):
+        distro_series='lucid', language='en', version='1.0', page=None):
 
         url = self._url_for_package(package_name, origin, distro_series,
-            language, page)
+            language, version, page)
 
         with patch_settings(API_REVIEWS_PAGINATION=2):
             response = self.client.get(url)
@@ -191,7 +192,7 @@
         return response
 
     def _populate_reviews_and_cache(self, package_name, origin='ubuntu',
-        distro_series='lucid', language='en', pages=1):
+        distro_series='lucid', language='en', version='1.0', pages=1):
         package = self.factory.makeSoftwareItem(package_name)
         repository = self.factory.makeRepository(
             origin=origin, distro_series=distro_series)
@@ -201,8 +202,9 @@
         reviews = []
         for review_num in range(pages * 2):
             reviews.append(
-                self.factory.makeReview(software_item=package, language=language,
-                    repository=repository))
+                self.factory.makeReview(software_item=package,
+                    language=language, repository=repository,
+                    version=version))
 
         # Now ensure the cache is populated by requesting each url.
         self._request_reviews_for_package(package_name)

=== modified file 'src/reviewsapp/utilities.py'
--- src/reviewsapp/utilities.py	2011-01-12 18:50:58 +0000
+++ src/reviewsapp/utilities.py	2011-01-19 19:59:08 +0000
@@ -166,11 +166,18 @@
 
 
 def invalidate_paginated_reviews_for(review):
+    # XXX achuni 2011-01-19: Here we're invalidating pages only for the exact
+    # version, language, distroseries and origin of the review.  We'd also
+    # need to invalidate partial matches, up to including all reviews
+    # for the package (/reviews/any/any/any/any/packagename/).  This is
+    # a lot of work.  Is it worth it?
     language = review.language
     origin = review.repository.origin
     distroseries = review.repository.distroseries
     package_name = review.softwareitem.package_name
     app_name = review.softwareitem.app_name
+    version = review.version
+
     if app_name:
         package_app_name = "{0};{1}".format(package_name, app_name)
     else:
@@ -180,7 +187,8 @@
         repository__origin=origin, repository__distroseries=distroseries,
         softwareitem__package_name=package_name,
         softwareitem__app_name=app_name, hide=False,
-        language=language).count()
+        language=language,
+        version=version).count()
 
     # Ensure we've got a list of all the possible page numbers,
     # including None (ie. first page is displayed if there is no page
@@ -190,7 +198,7 @@
     for page in pages:
         url_kwargs = dict(
             language=language, origin=origin, distroseries=distroseries,
-            package_name=urllib.quote_plus(package_app_name))
+            version=version, package_name=urllib.quote_plus(package_app_name))
         if page:
             url_kwargs['page'] = page