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

[Aaron Peachey]

[r=noodles] Adds handler for retrieval of usefulness records per review or user (aaronp)

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

[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

[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

[Aaron Peachey (aaronp)]

btw - correct, re the get_String left in. I had issues with the computer that my server was set up on so was feeling my way around for the second patch. Running the test just now confirmed that it was incorrect, and also that data=data was incorrect.

Here's the correct diff, 3rd time lucky:

34c34
<
---
>
150,166d149
< @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', args=data,
< scheme=PUBLIC_API_SCHEME)

[Michael Nelson (michael.nelson)]

Great, thanks again Aaron. I've landed the patch for rnrclient and will set this branch to land now too.

review: Approve

[Danny Tamez (zematynnad)]

Aaron - thanks for this. Everything looks good except for some minor issues.
Could you make the following changes?

Please change line 112 of src/reviewsapp/api/urls.py so that it uses parameterized urls for each variation rather than one url with a query string. We like to avoid query strings to be able to use caching.

Also (as long as you're resubmitting) - not required but it would be nice to fix some PEP8 issues:
please remove the extra blank line at 339 of src/reviewsapp/models/reviews.py
please remove the blank line at the end of src/reviewsapp/api/handlers.py
please add another blank line before the definitions of classes ShowUsefulnessHandler and ShowReviewsHandler.
src/reviewsapp/tests/test_handlers.py
please add another blank line before def of _get_url_kwargs_dict_for_review, before def of class ServerStatusHandlerTestCase, before def of class ShowUsefulnessHandlerTesetCase
please add spaces around '=' when they aren't keyword assignments (lines 70, 72,
remove space before ":" line 181 and after "{" at 534
add space after ":" 205, 206, 208
remove blank line at line 233
please remove blank line at end of file in src/reviewsapp/tests/test_rnrclient.py
and trim line 456 to 80 chars or less

review: Needs Fixing

[Michael Nelson (michael.nelson)]

On Wed, Mar 16, 2011 at 4:53 PM, Danny Tamez <email address hidden>wrote:

> Review: Needs Fixing
> Aaron - thanks for this. Everything looks good except for some minor
> issues.
> Could you make the following changes?
>

Hey Danny - this branch has already been approved and landed, but just on
your points below:

>
> Please change line 112 of src/reviewsapp/api/urls.py so that it uses
> parameterized urls for each variation rather than one url with a query
> string. We like to avoid query strings to be able to use caching.
>

The original MP that Aaron submitted was using a url like:

r'^1.0/usefulness/(?P<username>\w+)/$'

and I asked him to change this to use GET parms so that this API call could
be more general (filter not just by username, but also by reviewid etc.) in
the future (and aaron added a filter by reviewid just because it was easy),
based on my (possibly wrong?) assumption that caching was not so relevant
for this api call (I don't see 1000 requests for the usefulness objects for
the one user).

If you disagree, then we'll need to fix it as a separate branch since this
one has landed.

> Also (as long as you're resubmitting) - not required but it would be nice
> to fix some PEP8 issues:
>

Same - one of us can submit a branch fixing these. Similar to you, I didn't
want to require those kind of changes from community contributors. I think
when we have a make target (or auto-lint check) for this kind of stuff it
will be much easier for community contributors to check, but until then, I'm
happy to merge branches locally and then do the lint/PEP8 fixes as a
separate commit before landing it (I didn't do this this morning). Let me
know what you think.

-Michael

> please remove the extra blank line at 339 of
> src/reviewsapp/models/reviews.py
> please remove the blank line at the end of src/reviewsapp/api/handlers.py
> please add another blank line before the definitions of classes
> ShowUsefulnessHandler and ShowReviewsHandler.
> src/reviewsapp/tests/test_handlers.py
> please add another blank line before def of
> _get_url_kwargs_dict_for_review, before def of class
> ServerStatusHandlerTestCase, before def of class
> ShowUsefulnessHandlerTesetCase
> please add spaces around '=' when they aren't keyword assignments (lines
> 70, 72,
> remove space before ":" line 181 and after "{" at 534
> add space after ":" 205, 206, 208
> remove blank line at line 233
> please remove blank line at end of file in
> src/reviewsapp/tests/test_rnrclient.py
> and trim line 456 to 80 chars or less
>
> --
>
> https://code.launchpad.net/~aaronp/rnr-server/get-usefulness-by-user/+merge/53580
> You are reviewing the proposed merge of
> lp:~aaronp/rnr-server/get-usefulness-by-user into lp:rnr-server.
>

=== 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-16 08:45:50 +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-16 08:45:50 +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-15 14:19:57 +0000
+++ src/reviewsapp/models/reviews.py	2011-03-16 08:45:50 +0000
@@ -367,10 +367,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-15 14:19:57 +0000
+++ src/reviewsapp/tests/factory.py	2011-03-16 08:45:50 +0000
@@ -44,10 +44,20 @@
     SoftwareItemInRepository,
     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."""
 
@@ -136,6 +146,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 14:19:57 +0000
+++ src/reviewsapp/tests/test_handlers.py	2011-03-16 08:45:50 +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-16 08:45:50 +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'))
+
+