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

Proposed by Aaron Peachey
Status: Merged
Approved by: Michael Nelson
Approved revision: 153
Merged at revision: 154
Proposed branch: lp:~aaronp/rnr-server/get-usefulness-by-user
Merge into: lp:rnr-server
Diff against target: 289 lines (+166/-0)
6 files modified
src/reviewsapp/api/handlers.py (+31/-0)
src/reviewsapp/api/urls.py (+13/-0)
src/reviewsapp/models/reviews.py (+7/-0)
src/reviewsapp/tests/factory.py (+21/-0)
src/reviewsapp/tests/test_handlers.py (+79/-0)
src/reviewsapp/tests/test_rnrclient.py (+15/-0)
To merge this branch: bzr merge lp:~aaronp/rnr-server/get-usefulness-by-user
Reviewer Review Type Date Requested Status
Danny Tamez (community) Needs Fixing
Michael Nelson (community) Approve
Review via email: mp+53580@code.launchpad.net

This proposal supersedes a proposal from 2011-03-15.

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Michael Nelson (michael.nelson) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Aaron Peachey (aaronp) wrote : Posted in a previous version of this proposal

Hi Michael,

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

Thanks
Aaron

Revision history for this message
Aaron Peachey (aaronp) wrote : Posted in a previous version of this proposal

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)

Revision history for this message
Aaron Peachey (aaronp) wrote : Posted in a previous version of this proposal

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)

Revision history for this message
Michael Nelson (michael.nelson) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Aaron Peachey (aaronp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Aaron Peachey (aaronp) wrote :

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)

Revision history for this message
Michael Nelson (michael.nelson) wrote :

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

review: Approve
Revision history for this message
Danny Tamez (zematynnad) wrote :

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
Revision history for this message
Michael Nelson (michael.nelson) wrote :

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.
>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
26 'ServerStatusHandler',26 'ServerStatusHandler',
27 'ShowReviewsHandler',27 'ShowReviewsHandler',
28 'SubmitReviewHandler',28 'SubmitReviewHandler',
29 'ShowUsefulnessHandler',
29]30]
3031
31import urllib32import urllib
@@ -88,6 +89,33 @@
88 except Review.DoesNotExist:89 except Review.DoesNotExist:
89 return HttpResponseNotFound('No matching review found')90 return HttpResponseNotFound('No matching review found')
9091
92class ShowUsefulnessHandler(BaseHandler):
93 model = Usefulness
94 fields = (
95 'review_id',
96 'username',
97 'useful',
98 )
99 allowed_methods = ('GET',)
100
101 def read(self, request):
102 username = request.GET.get('username', None)
103 review_id = request.GET.get('review_id', None)
104
105 if not username and not review_id:
106 return HttpResponseBadRequest('No valid parameters passed.'
107 + ' Please pass at least username or review_id')
108
109 if username:
110 response = Usefulness.objects.filter(user__username=username)
111 if review_id:
112 response = response.filter(
113 review__id=review_id)
114 else:
115 response = Usefulness.objects.filter(
116 review__id=review_id)
117
118 return response
91119
92class ShowReviewsHandler(BaseHandler):120class ShowReviewsHandler(BaseHandler):
93 model = Review121 model = Review
@@ -228,3 +256,6 @@
228 return HttpResponse('"Updated"', status=200)256 return HttpResponse('"Updated"', status=200)
229 else:257 else:
230 return HttpResponse('"Not modified"', status=200)258 return HttpResponse('"Not modified"', status=200)
259
260
261
231262
=== 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 @@
30 SubmitReviewHandler,30 SubmitReviewHandler,
31 SubmitUsefulnessHandler,31 SubmitUsefulnessHandler,
32 RetrieveReviewHandler,32 RetrieveReviewHandler,
33 ShowUsefulnessHandler
33)34)
34from reviewsapp.auth import SSOOAuthAuthentication35from reviewsapp.auth import SSOOAuthAuthentication
35from reviewsapp.api.decorators import dont_vary36from reviewsapp.api.decorators import dont_vary
@@ -58,6 +59,11 @@
58retrieve_review_resource.__name__ = "Resource"59retrieve_review_resource.__name__ = "Resource"
59retrieve_review_resource = dont_vary(retrieve_review_resource)60retrieve_review_resource = dont_vary(retrieve_review_resource)
6061
62# The show usefulness resource is cached.
63show_usefulness_resource = Resource(handler=ShowUsefulnessHandler)
64show_usefulness_resource.__name__ = "Resource"
65show_usefulness_resource = dont_vary(show_usefulness_resource)
66
6167
62# The following POST handlers have POST data and will not be cached.68# The following POST handlers have POST data and will not be cached.
63submit_review_resource = Resource(handler=SubmitReviewHandler,69submit_review_resource = Resource(handler=SubmitReviewHandler,
@@ -99,6 +105,13 @@
99 url(r'^1.0/reviews/(?P<review_id>\d+)/recommendations/$',105 url(r'^1.0/reviews/(?P<review_id>\d+)/recommendations/$',
100 submit_usefulness_resource, name='rnr-api-recommendations'),106 submit_usefulness_resource, name='rnr-api-recommendations'),
101107
108 # get the filtered usefulness records using parameters for filtering
109 # GET /1.0/reviews/usefulness/?username=aaronp
110 # GET /1.0/reviews/usefulness/?review_id=100
111 # GET /1.0/reviews/usefulness/?username=aaronp&review_id=100
112 url(r'^1.0/usefulness/$',
113 show_usefulness_resource, name='rnr-api-usefulness-filter'),
114
102 # gets a single review for a given review id115 # gets a single review for a given review id
103 # GET /1.0/reviews/100/116 # GET /1.0/reviews/100/
104 url(r'^1.0/reviews/(?P<review_id>\d+)/$',117 url(r'^1.0/reviews/(?P<review_id>\d+)/$',
105118
=== 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 @@
367 user = models.ForeignKey(User)367 user = models.ForeignKey(User)
368 useful = models.BooleanField(db_index=True)368 useful = models.BooleanField(db_index=True)
369369
370 def username(self):
371 return self.user.username
372
373 def review_id(self):
374 return self.review.id
375
370 class Meta:376 class Meta:
371 app_label = 'reviewsapp'377 app_label = 'reviewsapp'
372378
373379
380
374class RNRSettings(models.Model):381class RNRSettings(models.Model):
375 ACTIVE_MODE_FLAG_SUMMARY = 'Automatically Flagged'382 ACTIVE_MODE_FLAG_SUMMARY = 'Automatically Flagged'
376 ACTIVE_MODE_FLAG_DESCRIPTION = ('This review was automatically flagged '383 ACTIVE_MODE_FLAG_DESCRIPTION = ('This review was automatically flagged '
377384
=== 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 @@
44 SoftwareItemInRepository,44 SoftwareItemInRepository,
45 Token,45 Token,
46 Consumer,46 Consumer,
47 Usefulness
47 )48 )
48from reviewsapp.utilities import full_claimed_id49from reviewsapp.utilities import full_claimed_id
4950
5051
52class RequestTestObject(object):
53 """Test object for emulating GET requests"""
54 def __init__(self,fakeGetDict=None):
55 if fakeGetDict:
56 self.GET = fakeGetDict
57 else:
58 self.GET = {}
59
60
51class ReviewsObjectFactory(object):61class ReviewsObjectFactory(object):
52 """A factory for creating model objects for testing."""62 """A factory for creating model objects for testing."""
5363
@@ -136,6 +146,17 @@
136 return Repository.objects.create(146 return Repository.objects.create(
137 origin=origin, distroseries=distro_series)147 origin=origin, distroseries=distro_series)
138148
149 def makeUsefulness(self, review=None, user=None, useful=True):
150 if review is None:
151 review = self.makeReview()
152 if user is None:
153 user = self.makeUser()
154
155 usefulness = Usefulness.objects.create(review=review, user=user,
156 useful=useful)
157
158 return usefulness
159
139 def makeReview(self, reviewer=None, software_item=None, repository=None,160 def makeReview(self, reviewer=None, software_item=None, repository=None,
140 version=None, rating="5", summary=None, review_text=None,161 version=None, rating="5", summary=None, review_text=None,
141 date_created=None, language=None, hide=False, num_flags=0,162 date_created=None, language=None, hide=False, num_flags=0,
142163
=== 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 @@
27 'ShowReviewsHandlerTestCase',27 'ShowReviewsHandlerTestCase',
28 'SubmitReviewHandlerTestCase',28 'SubmitReviewHandlerTestCase',
29 'UsefulnessHandlerTestCase',29 'UsefulnessHandlerTestCase',
30 'ShowUsefulnessHandlerTestCase',
30 ]31 ]
3132
3233
@@ -49,6 +50,7 @@
49 ServerStatusHandler,50 ServerStatusHandler,
50 ShowReviewsHandler,51 ShowReviewsHandler,
51 SubmitUsefulnessHandler,52 SubmitUsefulnessHandler,
53 ShowUsefulnessHandler,
52)54)
53from reviewsapp.models import (55from reviewsapp.models import (
54 Repository,56 Repository,
@@ -57,6 +59,7 @@
57 SoftwareItem,59 SoftwareItem,
58 )60 )
59from reviewsapp.tests.factory import TestCaseWithFactory61from reviewsapp.tests.factory import TestCaseWithFactory
62from reviewsapp.tests.factory import RequestTestObject
60from reviewsapp.tests.helpers import patch_settings63from reviewsapp.tests.helpers import patch_settings
61from reviewsapp.tests.matchers import (64from reviewsapp.tests.matchers import (
62 ContainsStatsForPackage,65 ContainsStatsForPackage,
@@ -164,6 +167,82 @@
164 NoReverseMatch, reverse, 'rnr-api-reviews-stats',167 NoReverseMatch, reverse, 'rnr-api-reviews-stats',
165 kwargs={'days': days})168 kwargs={'days': days})
166169
170class ShowUsefulnessHandlerTestCase(TestCaseWithFactory):
171 def test_read_for_single_parameter(self):
172 '''test that a single usefulness vote by a user is returned when
173 searching for that user'''
174 review = self.factory.makeReview(
175 software_item=self.factory.makeSoftwareItem('package_bar'))
176 user = self.factory.makeUser()
177 usefulness = self.factory.makeUsefulness(review, user)
178
179 handler = ShowUsefulnessHandler()
180
181 request1 = RequestTestObject({'username': user.username})
182 request2 = RequestTestObject({'review_id' : review.id})
183
184 results1 = handler.read(request1)
185 results2 = handler.read(request2)
186
187 self.assertEqual(1, len(results1))
188 self.assertEqual(usefulness, results1[0])
189 self.assertEqual(usefulness, results2[0])
190
191 def test_read_filtering(self):
192 '''test that a passed parameters results in all relevant items
193 being returned, but no unwanted items'''
194 review1 = self.factory.makeReview(
195 software_item=self.factory.makeSoftwareItem('package_bar'))
196 review2 = self.factory.makeReview(
197 software_item=self.factory.makeSoftwareItem('foo_bar'))
198 user1 = self.factory.makeUser()
199 user2 = self.factory.makeUser()
200 usefulness1 = self.factory.makeUsefulness(review1, user1)
201 usefulness2 = self.factory.makeUsefulness(review1, user2)
202 usefulness3 = self.factory.makeUsefulness(review2, user1)
203
204 handler = ShowUsefulnessHandler()
205
206 request1 = RequestTestObject({'username':user1.username})
207 request2 = RequestTestObject({'review_id':review1.id})
208 request3 = RequestTestObject(
209 {'username':user1.username, 'review_id':review2.id}
210 )
211
212 results1 = handler.read(request1)
213 results2 = handler.read(request2)
214 results3 = handler.read(request3)
215
216 #test username filter results
217 self.assertEqual(2, len(results1))
218 self.assertIn(usefulness1, results1)
219 self.assertIn(usefulness3, results1)
220 self.assertNotIn(usefulness2, results1)
221
222 #test review_id filter results
223 self.assertEqual(2, len(results2))
224 self.assertIn(usefulness1, results2)
225 self.assertIn(usefulness2, results2)
226 self.assertNotIn(usefulness3, results2)
227
228 #test usefulness AND review_id filter results
229 self.assertEqual(1, len(results3))
230 self.assertNotIn(usefulness1, results3)
231 self.assertNotIn(usefulness2, results3)
232 self.assertIn(usefulness3, results3)
233
234
235 def test_read_for_no_parameters_gives_400_error(self):
236 '''test that a http 400 error is returned if no valid parameter
237 is passed'''
238 handler = ShowUsefulnessHandler()
239 request1 = RequestTestObject()
240 request2 = RequestTestObject({'something':'nothing'})
241 results1 = handler.read(request1)
242 results2 = handler.read(request2)
243 self.assertEqual(httplib.BAD_REQUEST, results1.status_code)
244 self.assertEqual(httplib.BAD_REQUEST, results2.status_code)
245
167246
168class ShowReviewsHandlerTestCase(TestCaseWithFactory):247class ShowReviewsHandlerTestCase(TestCaseWithFactory):
169248
170249
=== 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 @@
451 count = Usefulness.objects.filter(review=review).count()451 count = Usefulness.objects.filter(review=review).count()
452 self.assertEqual(1, count)452 self.assertEqual(1, count)
453453
454 def test_get_usefulness(self):
455 api = RatingsAndReviewsAPI()
456 software_item = self.factory.makeSoftwareItem(app_name='something2')
457 review = self.factory.makeReview(software_item=software_item)
458 user = self.factory.makeUser()
459 usefulness = self.factory.makeUsefulness(review=review, user=user)
460
461 response = api.get_usefulness(review_id=review.id, username=user.username)
462
463 self.assertEqual(1, len(response))
464 self.assertEqual(user.username, response[0].get('username'))
465 self.assertEqual(review.id, response[0].get('review_id'))
466 self.assertEqual(usefulness.useful, response[0].get('useful'))
467
468

Subscribers

People subscribed via source and target branches