Merge lp:~aaronp/rnr-server/get-usefulness-by-user into lp:rnr-server

[Aaron Peachey]

New handler/url to enable retrieval of all usefulness records that have been voted by a particular user and associated tests. Can be used on client to determine which reviews a user has submitted usefulness for without the need to send all usefulness vote details back with every single review.

[Michael Nelson (michael.nelson)]

Hey Aaron! Thanks for the branch.. it's looking great!

There's a small typo in your makeUsefulness() factory method ('review = self.makeUser()'), but other than that, it looks ready to land :)

If you've got time, here are a few optional things that you may (or may not) want to think about:
 * I wonder if it'd be worthwhile cutting the username out of the url:
    r'^1.0/usefulness/$
   and instead adding optional filtering via GET (/1.0/usefulness/?username=aaronp)? To make it easier, we could just require the username GET param at the moment, but it means we can extend the API later without adding more urls? See what you think.
 * A few long lines there... if you get a chance, just snip them to 78 chars (no problem if you don't...).
 * Great tests! I assume we'll also want to add this call to the rnr client? If so, just patch your local rnr client (parts/required-isd-branches/rnrclient/rnrclient.py) and add a basic test to test_rnrclient() that exercises the new method. Update the description of this MP with a link to the patch for rnrclient and we can make sure it lands at the same time.

[Aaron Peachey (aaronp)]

Hi Michael,

Thanks for the feedback. I'll take a look and resubmit.

Thanks
Aaron

[lp:~aaronp/rnr-server/get-usefulness-by-user]

152. By Aaron Peachey on 2011-03-15

* wrap long lines
* src/reviewsapp/test/factory.py: fix silly typo

[Aaron Peachey (aaronp)]

Changes made as per feedback. Also added ability to filter by review_id as it was trivial to add.

Diff for rnrclient below:

34c34
<
---
>
149a150,166
> @validate('review_id', int, required=False)
> @validate_pattern('username', r'[^\n]+', required=False)
> @returns_json
> def get_usefulness(self, review_id=None, username=None):
> """Get a list of usefulness filtered by username/review_id"""
> if not username and not review_id:
> return None
> get_string = "?"
> if username:
> get_string = '%susername=%s' % (get_string, username)
> if review_id:
> get_string = "%s&review_id=%s" % (get_string, str(review_id))
> else:
> get_string = '%sreview_id=%s' % (get_string, review_id)
>
> return self._get('/usefulness/%s' % get_string,
> scheme=PUBLIC_API_SCHEME)

[Aaron Peachey (aaronp)]

I'm re-thinking the patch now. Is this a better way?

> @validate('review_id', int, required=False)
> @validate_pattern('username', r'[^\n]+', required=False)
> @returns_json
> def get_usefulness(self, review_id=None, username=None):
> """Get a list of usefulness filtered by username/review_id"""
> if not username and not review_id:
> return None
>
> data = {}
> if username:
> data ['username'] = username
> if review_id:
> data ['review_id'] = str(review_id)
>
> return self._get('/usefulness/%s' % get_string, data=data,
> scheme=PUBLIC_API_SCHEME)

[Michael Nelson (michael.nelson)]

Hey again Aaron!

On Tue, Mar 15, 2011 at 1:47 PM, Aaron Peachey <email address hidden> wrote:

> I'm re-thinking the patch now. Is this a better way?
>

Yeah, this one is nicer... thanks, although your test should show (I think?)
that you've left get_string in there - but I'll remove that when running it
before landing.

With the changes to the actual branch, can you pls revert the changes to
config/main.cfg . A great way for your next branch (/me hopes :) ), is to
add a django_project/local.cfg with just the customisations you need - this
will overwrite anything in main.cfg, and won't get committed.

The rest looks great! Thanks for being the first community contributor :)
Once you revert the main.cfg, I'll land the patch to rnrclient and then this
branch.

-Michael

[lp:~aaronp/rnr-server/get-usefulness-by-user]

153. By Aaron Peachey on 2011-03-16

- update urls.py and handlers.py to allow GET parameters and support
filtering by username and/or review_id
- update tests for new handlers
- add tests for rnrclient changes

[Aaron Peachey (aaronp)]

Thanks for the feedback again Michael. I've made the changes to revert main.cfg and resubmitted.
Let me know if anything else.
cheers
Aaron

Unmerged revisions

=== modified file 'django_project/config_dev/config/main.cfg'
--- django_project/config_dev/config/main.cfg	2011-03-15 10:21:30 +0000
+++ django_project/config_dev/config/main.cfg	2011-03-15 10:41:35 +0000
@@ -101,13 +101,13 @@
 
 [sso_api]
 sso_api_service_root = https://login.staging.ubuntu.com/api/1.0
-sso_api_auth_username = replace_with_your_sso_apiuser
-sso_api_auth_password = replace_with_your_sso_apipassword
+sso_api_auth_username = aaronp
+sso_api_auth_password = bonny123
 # this option will disable talking to the sso_api and instead extract
 # token_secret and consumer_secret from the  PLAINTEXT signature.
 # This avoids the need for a special ubuntu-sso API account
 # and is very useful for local testing
-sso_auth_mode_no_ubuntu_sso_plaintext_only = False
+sso_auth_mode_no_ubuntu_sso_plaintext_only = True
 
 [launchpad]
 lp_service = production
 
=== modified file 'src/reviewsapp/api/handlers.py'
--- src/reviewsapp/api/handlers.py	2011-03-11 15:02:54 +0000
+++ src/reviewsapp/api/handlers.py	2011-03-15 10:41:35 +0000
@@ -26,6 +26,7 @@
     'ServerStatusHandler',
     'ShowReviewsHandler',
     'SubmitReviewHandler',
+    'ShowUsefulnessHandler',
 ]
 
 import urllib
@@ -88,6 +89,33 @@
         except Review.DoesNotExist:
             return HttpResponseNotFound('No matching review found')
 
+class ShowUsefulnessHandler(BaseHandler):
+    model = Usefulness
+    fields = (
+        'review_id',
+        'username',
+        'useful',
+    )
+    allowed_methods = ('GET',)
+
+    def read(self, request):
+        username = request.GET.get('username', None)
+        review_id = request.GET.get('review_id', None)
+
+        if not username and not review_id:
+            return HttpResponseBadRequest('No valid parameters passed.'
+               + ' Please pass at least username or review_id')
+
+        if username:
+            response = Usefulness.objects.filter(user__username=username)
+            if review_id:
+                response = response.filter(
+                    review__id=review_id)
+        else:
+            response = Usefulness.objects.filter(
+                review__id=review_id)
+
+        return response
 
 class ShowReviewsHandler(BaseHandler):
     model = Review
@@ -228,3 +256,6 @@
             return HttpResponse('"Updated"', status=200)
         else:
             return HttpResponse('"Not modified"', status=200)
+
+
+
 
=== modified file 'src/reviewsapp/api/urls.py'
--- src/reviewsapp/api/urls.py	2011-03-08 16:04:05 +0000
+++ src/reviewsapp/api/urls.py	2011-03-15 10:41:35 +0000
@@ -30,6 +30,7 @@
     SubmitReviewHandler,
     SubmitUsefulnessHandler,
     RetrieveReviewHandler,
+    ShowUsefulnessHandler
 )
 from reviewsapp.auth import SSOOAuthAuthentication
 from reviewsapp.api.decorators import dont_vary
@@ -58,6 +59,11 @@
 retrieve_review_resource.__name__ = "Resource"
 retrieve_review_resource = dont_vary(retrieve_review_resource)
 
+# The show usefulness resource is cached.
+show_usefulness_resource = Resource(handler=ShowUsefulnessHandler)
+show_usefulness_resource.__name__ = "Resource"
+show_usefulness_resource = dont_vary(show_usefulness_resource)
+
 
 # The following POST handlers have POST data and will not be cached.
 submit_review_resource = Resource(handler=SubmitReviewHandler,
@@ -99,6 +105,13 @@
     url(r'^1.0/reviews/(?P<review_id>\d+)/recommendations/$',
         submit_usefulness_resource, name='rnr-api-recommendations'),
 
+    # get the filtered usefulness records using parameters for filtering
+    #  GET /1.0/reviews/usefulness/?username=aaronp
+    #  GET /1.0/reviews/usefulness/?review_id=100
+    #  GET /1.0/reviews/usefulness/?username=aaronp&review_id=100
+    url(r'^1.0/usefulness/$',
+        show_usefulness_resource, name='rnr-api-usefulness-filter'),
+
     # gets a single review for a given review id
     # GET /1.0/reviews/100/
     url(r'^1.0/reviews/(?P<review_id>\d+)/$',
 
=== modified file 'src/reviewsapp/models/reviews.py'
--- src/reviewsapp/models/reviews.py	2011-03-11 15:13:18 +0000
+++ src/reviewsapp/models/reviews.py	2011-03-15 10:41:35 +0000
@@ -332,10 +332,17 @@
     user = models.ForeignKey(User)
     useful = models.BooleanField(db_index=True)
 
+    def username(self):
+        return self.user.username
+
+    def review_id(self):
+        return self.review.id
+
     class Meta:
         app_label = 'reviewsapp'
 
 
+
 class RNRSettings(models.Model):
     ACTIVE_MODE_FLAG_SUMMARY = 'Automatically Flagged'
     ACTIVE_MODE_FLAG_DESCRIPTION = ('This review was automatically flagged '
 
=== modified file 'src/reviewsapp/tests/factory.py'
--- src/reviewsapp/tests/factory.py	2011-03-14 11:22:40 +0000
+++ src/reviewsapp/tests/factory.py	2011-03-15 10:41:35 +0000
@@ -42,10 +42,20 @@
     SoftwareItem,
     Token,
     Consumer,
+    Usefulness
     )
 from reviewsapp.utilities import full_claimed_id
 
 
+class RequestTestObject(object):
+    """Test object for emulating GET requests"""
+    def __init__(self,fakeGetDict=None):
+        if fakeGetDict:
+            self.GET = fakeGetDict
+        else:
+            self.GET = {}
+
+
 class ReviewsObjectFactory(object):
     """A factory for creating model objects for testing."""
 
@@ -114,6 +124,17 @@
         return Repository.objects.create(
             origin=origin, distroseries=distro_series)
 
+    def makeUsefulness(self, review=None, user=None, useful=True):
+        if review is None:
+            review = self.makeReview()
+        if user is None:
+            user = self.makeUser()
+
+        usefulness = Usefulness.objects.create(review=review, user=user,
+                        useful=useful)
+
+        return usefulness
+
     def makeReview(self, reviewer=None, software_item=None, repository=None,
                    version=None, rating="5", summary=None, review_text=None,
                    date_created=None, language=None, hide=False, num_flags=0,
 
=== modified file 'src/reviewsapp/tests/test_handlers.py'
--- src/reviewsapp/tests/test_handlers.py	2011-03-15 09:45:43 +0000
+++ src/reviewsapp/tests/test_handlers.py	2011-03-15 10:41:35 +0000
@@ -27,6 +27,7 @@
     'ShowReviewsHandlerTestCase',
     'SubmitReviewHandlerTestCase',
     'UsefulnessHandlerTestCase',
+    'ShowUsefulnessHandlerTestCase',
     ]
 
 
@@ -49,6 +50,7 @@
     ServerStatusHandler,
     ShowReviewsHandler,
     SubmitUsefulnessHandler,
+    ShowUsefulnessHandler,
 )
 from reviewsapp.models import (
     Repository,
@@ -57,6 +59,7 @@
     SoftwareItem,
     )
 from reviewsapp.tests.factory import TestCaseWithFactory
+from reviewsapp.tests.factory import RequestTestObject
 from reviewsapp.tests.helpers import patch_settings
 from reviewsapp.tests.matchers import (
     ContainsStatsForPackage,
@@ -164,6 +167,82 @@
                 NoReverseMatch, reverse, 'rnr-api-reviews-stats',
                 kwargs={'days': days})
 
+class ShowUsefulnessHandlerTestCase(TestCaseWithFactory):
+    def test_read_for_single_parameter(self):
+        '''test that a single usefulness vote by a user is returned when
+         searching for that user'''
+        review = self.factory.makeReview(
+            software_item=self.factory.makeSoftwareItem('package_bar'))
+        user = self.factory.makeUser()
+        usefulness = self.factory.makeUsefulness(review, user)
+
+        handler = ShowUsefulnessHandler()
+
+        request1 = RequestTestObject({'username': user.username})
+        request2 = RequestTestObject({'review_id' : review.id})
+
+        results1 = handler.read(request1)
+        results2 = handler.read(request2)
+
+        self.assertEqual(1, len(results1))
+        self.assertEqual(usefulness, results1[0])
+        self.assertEqual(usefulness, results2[0])
+
+    def test_read_filtering(self):
+        '''test that a passed parameters results in all relevant items
+        being returned, but no unwanted items'''
+        review1 = self.factory.makeReview(
+            software_item=self.factory.makeSoftwareItem('package_bar'))
+        review2 = self.factory.makeReview(
+            software_item=self.factory.makeSoftwareItem('foo_bar'))
+        user1 = self.factory.makeUser()
+        user2 = self.factory.makeUser()
+        usefulness1 = self.factory.makeUsefulness(review1, user1)
+        usefulness2 = self.factory.makeUsefulness(review1, user2)
+        usefulness3 = self.factory.makeUsefulness(review2, user1)
+
+        handler = ShowUsefulnessHandler()
+
+        request1 = RequestTestObject({'username':user1.username})
+        request2 = RequestTestObject({'review_id':review1.id})
+        request3 = RequestTestObject(
+            {'username':user1.username, 'review_id':review2.id}
+            )
+
+        results1 = handler.read(request1)
+        results2 = handler.read(request2)
+        results3 = handler.read(request3)
+
+        #test username filter results
+        self.assertEqual(2, len(results1))
+        self.assertIn(usefulness1, results1)
+        self.assertIn(usefulness3, results1)
+        self.assertNotIn(usefulness2, results1)
+
+        #test review_id filter results
+        self.assertEqual(2, len(results2))
+        self.assertIn(usefulness1, results2)
+        self.assertIn(usefulness2, results2)
+        self.assertNotIn(usefulness3, results2)
+
+        #test usefulness AND review_id filter results
+        self.assertEqual(1, len(results3))
+        self.assertNotIn(usefulness1, results3)
+        self.assertNotIn(usefulness2, results3)
+        self.assertIn(usefulness3, results3)
+
+
+    def test_read_for_no_parameters_gives_400_error(self):
+        '''test that a http 400 error is returned if no valid parameter
+        is passed'''
+        handler = ShowUsefulnessHandler()
+        request1 = RequestTestObject()
+        request2 = RequestTestObject({'something':'nothing'})
+        results1 = handler.read(request1)
+        results2 = handler.read(request2)
+        self.assertEqual(httplib.BAD_REQUEST, results1.status_code)
+        self.assertEqual(httplib.BAD_REQUEST, results2.status_code)
+
 
 class ShowReviewsHandlerTestCase(TestCaseWithFactory):
 
 
=== modified file 'src/reviewsapp/tests/test_rnrclient.py'
--- src/reviewsapp/tests/test_rnrclient.py	2011-03-11 15:13:18 +0000
+++ src/reviewsapp/tests/test_rnrclient.py	2011-03-15 10:41:35 +0000
@@ -451,3 +451,18 @@
         count = Usefulness.objects.filter(review=review).count()
         self.assertEqual(1, count)
 
+    def test_get_usefulness(self):
+        api = RatingsAndReviewsAPI()
+        software_item = self.factory.makeSoftwareItem(app_name='something2')
+        review = self.factory.makeReview(software_item=software_item)
+        user = self.factory.makeUser()
+        usefulness = self.factory.makeUsefulness(review=review, user=user)
+
+        response = api.get_usefulness(review_id=review.id, username=user.username)
+
+        self.assertEqual(1, len(response))
+        self.assertEqual(user.username, response[0].get('username'))
+        self.assertEqual(review.id, response[0].get('review_id'))
+        self.assertEqual(usefulness.useful, response[0].get('useful'))
+
+