Merge lp:~aaronp/rnr-server/get-usefulness-by-user into lp:rnr-server
- get-usefulness-by-user
- Merge into trunk
Status: | Superseded |
---|---|
Proposed branch: | lp:~aaronp/rnr-server/get-usefulness-by-user |
Merge into: | lp:rnr-server |
Diff against target: |
310 lines (+169/-3) 7 files modified
django_project/config_dev/config/main.cfg (+3/-3) 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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ratings and Reviews Developers | Pending | ||
Review via email: mp+53214@code.launchpad.net |
This proposal has been superseded by a proposal from 2011-03-15.
Commit message
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.
Michael Nelson (michael.nelson) wrote : | # |
Aaron Peachey (aaronp) wrote : | # |
Hi Michael,
Thanks for the feedback. I'll take a look and resubmit.
Thanks
Aaron
- 152. By Aaron Peachey
-
* wrap long lines
* src/reviewsapp/test/factory. py: fix silly typo
Aaron Peachey (aaronp) wrote : | # |
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(
> @validate_
> @returns_json
> def get_usefulness(
> """Get a list of usefulness filtered by username/
> 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(
> scheme=
Aaron Peachey (aaronp) wrote : | # |
I'm re-thinking the patch now. Is this a better way?
> @validate(
> @validate_
> @returns_json
> def get_usefulness(
> """Get a list of usefulness filtered by username/
> 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(
> scheme=
Michael Nelson (michael.nelson) wrote : | # |
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_
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
- 153. By Aaron Peachey
-
- 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) wrote : | # |
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
Preview Diff
1 | === modified file 'django_project/config_dev/config/main.cfg' |
2 | --- django_project/config_dev/config/main.cfg 2011-03-15 10:21:30 +0000 |
3 | +++ django_project/config_dev/config/main.cfg 2011-03-15 10:41:35 +0000 |
4 | @@ -101,13 +101,13 @@ |
5 | |
6 | [sso_api] |
7 | sso_api_service_root = https://login.staging.ubuntu.com/api/1.0 |
8 | -sso_api_auth_username = replace_with_your_sso_apiuser |
9 | -sso_api_auth_password = replace_with_your_sso_apipassword |
10 | +sso_api_auth_username = aaronp |
11 | +sso_api_auth_password = bonny123 |
12 | # this option will disable talking to the sso_api and instead extract |
13 | # token_secret and consumer_secret from the PLAINTEXT signature. |
14 | # This avoids the need for a special ubuntu-sso API account |
15 | # and is very useful for local testing |
16 | -sso_auth_mode_no_ubuntu_sso_plaintext_only = False |
17 | +sso_auth_mode_no_ubuntu_sso_plaintext_only = True |
18 | |
19 | [launchpad] |
20 | lp_service = production |
21 | |
22 | === modified file 'src/reviewsapp/api/handlers.py' |
23 | --- src/reviewsapp/api/handlers.py 2011-03-11 15:02:54 +0000 |
24 | +++ src/reviewsapp/api/handlers.py 2011-03-15 10:41:35 +0000 |
25 | @@ -26,6 +26,7 @@ |
26 | 'ServerStatusHandler', |
27 | 'ShowReviewsHandler', |
28 | 'SubmitReviewHandler', |
29 | + 'ShowUsefulnessHandler', |
30 | ] |
31 | |
32 | import urllib |
33 | @@ -88,6 +89,33 @@ |
34 | except Review.DoesNotExist: |
35 | return HttpResponseNotFound('No matching review found') |
36 | |
37 | +class ShowUsefulnessHandler(BaseHandler): |
38 | + model = Usefulness |
39 | + fields = ( |
40 | + 'review_id', |
41 | + 'username', |
42 | + 'useful', |
43 | + ) |
44 | + allowed_methods = ('GET',) |
45 | + |
46 | + def read(self, request): |
47 | + username = request.GET.get('username', None) |
48 | + review_id = request.GET.get('review_id', None) |
49 | + |
50 | + if not username and not review_id: |
51 | + return HttpResponseBadRequest('No valid parameters passed.' |
52 | + + ' Please pass at least username or review_id') |
53 | + |
54 | + if username: |
55 | + response = Usefulness.objects.filter(user__username=username) |
56 | + if review_id: |
57 | + response = response.filter( |
58 | + review__id=review_id) |
59 | + else: |
60 | + response = Usefulness.objects.filter( |
61 | + review__id=review_id) |
62 | + |
63 | + return response |
64 | |
65 | class ShowReviewsHandler(BaseHandler): |
66 | model = Review |
67 | @@ -228,3 +256,6 @@ |
68 | return HttpResponse('"Updated"', status=200) |
69 | else: |
70 | return HttpResponse('"Not modified"', status=200) |
71 | + |
72 | + |
73 | + |
74 | |
75 | === modified file 'src/reviewsapp/api/urls.py' |
76 | --- src/reviewsapp/api/urls.py 2011-03-08 16:04:05 +0000 |
77 | +++ src/reviewsapp/api/urls.py 2011-03-15 10:41:35 +0000 |
78 | @@ -30,6 +30,7 @@ |
79 | SubmitReviewHandler, |
80 | SubmitUsefulnessHandler, |
81 | RetrieveReviewHandler, |
82 | + ShowUsefulnessHandler |
83 | ) |
84 | from reviewsapp.auth import SSOOAuthAuthentication |
85 | from reviewsapp.api.decorators import dont_vary |
86 | @@ -58,6 +59,11 @@ |
87 | retrieve_review_resource.__name__ = "Resource" |
88 | retrieve_review_resource = dont_vary(retrieve_review_resource) |
89 | |
90 | +# The show usefulness resource is cached. |
91 | +show_usefulness_resource = Resource(handler=ShowUsefulnessHandler) |
92 | +show_usefulness_resource.__name__ = "Resource" |
93 | +show_usefulness_resource = dont_vary(show_usefulness_resource) |
94 | + |
95 | |
96 | # The following POST handlers have POST data and will not be cached. |
97 | submit_review_resource = Resource(handler=SubmitReviewHandler, |
98 | @@ -99,6 +105,13 @@ |
99 | url(r'^1.0/reviews/(?P<review_id>\d+)/recommendations/$', |
100 | submit_usefulness_resource, name='rnr-api-recommendations'), |
101 | |
102 | + # get the filtered usefulness records using parameters for filtering |
103 | + # GET /1.0/reviews/usefulness/?username=aaronp |
104 | + # GET /1.0/reviews/usefulness/?review_id=100 |
105 | + # GET /1.0/reviews/usefulness/?username=aaronp&review_id=100 |
106 | + url(r'^1.0/usefulness/$', |
107 | + show_usefulness_resource, name='rnr-api-usefulness-filter'), |
108 | + |
109 | # gets a single review for a given review id |
110 | # GET /1.0/reviews/100/ |
111 | url(r'^1.0/reviews/(?P<review_id>\d+)/$', |
112 | |
113 | === modified file 'src/reviewsapp/models/reviews.py' |
114 | --- src/reviewsapp/models/reviews.py 2011-03-11 15:13:18 +0000 |
115 | +++ src/reviewsapp/models/reviews.py 2011-03-15 10:41:35 +0000 |
116 | @@ -332,10 +332,17 @@ |
117 | user = models.ForeignKey(User) |
118 | useful = models.BooleanField(db_index=True) |
119 | |
120 | + def username(self): |
121 | + return self.user.username |
122 | + |
123 | + def review_id(self): |
124 | + return self.review.id |
125 | + |
126 | class Meta: |
127 | app_label = 'reviewsapp' |
128 | |
129 | |
130 | + |
131 | class RNRSettings(models.Model): |
132 | ACTIVE_MODE_FLAG_SUMMARY = 'Automatically Flagged' |
133 | ACTIVE_MODE_FLAG_DESCRIPTION = ('This review was automatically flagged ' |
134 | |
135 | === modified file 'src/reviewsapp/tests/factory.py' |
136 | --- src/reviewsapp/tests/factory.py 2011-03-14 11:22:40 +0000 |
137 | +++ src/reviewsapp/tests/factory.py 2011-03-15 10:41:35 +0000 |
138 | @@ -42,10 +42,20 @@ |
139 | SoftwareItem, |
140 | Token, |
141 | Consumer, |
142 | + Usefulness |
143 | ) |
144 | from reviewsapp.utilities import full_claimed_id |
145 | |
146 | |
147 | +class RequestTestObject(object): |
148 | + """Test object for emulating GET requests""" |
149 | + def __init__(self,fakeGetDict=None): |
150 | + if fakeGetDict: |
151 | + self.GET = fakeGetDict |
152 | + else: |
153 | + self.GET = {} |
154 | + |
155 | + |
156 | class ReviewsObjectFactory(object): |
157 | """A factory for creating model objects for testing.""" |
158 | |
159 | @@ -114,6 +124,17 @@ |
160 | return Repository.objects.create( |
161 | origin=origin, distroseries=distro_series) |
162 | |
163 | + def makeUsefulness(self, review=None, user=None, useful=True): |
164 | + if review is None: |
165 | + review = self.makeReview() |
166 | + if user is None: |
167 | + user = self.makeUser() |
168 | + |
169 | + usefulness = Usefulness.objects.create(review=review, user=user, |
170 | + useful=useful) |
171 | + |
172 | + return usefulness |
173 | + |
174 | def makeReview(self, reviewer=None, software_item=None, repository=None, |
175 | version=None, rating="5", summary=None, review_text=None, |
176 | date_created=None, language=None, hide=False, num_flags=0, |
177 | |
178 | === modified file 'src/reviewsapp/tests/test_handlers.py' |
179 | --- src/reviewsapp/tests/test_handlers.py 2011-03-15 09:45:43 +0000 |
180 | +++ src/reviewsapp/tests/test_handlers.py 2011-03-15 10:41:35 +0000 |
181 | @@ -27,6 +27,7 @@ |
182 | 'ShowReviewsHandlerTestCase', |
183 | 'SubmitReviewHandlerTestCase', |
184 | 'UsefulnessHandlerTestCase', |
185 | + 'ShowUsefulnessHandlerTestCase', |
186 | ] |
187 | |
188 | |
189 | @@ -49,6 +50,7 @@ |
190 | ServerStatusHandler, |
191 | ShowReviewsHandler, |
192 | SubmitUsefulnessHandler, |
193 | + ShowUsefulnessHandler, |
194 | ) |
195 | from reviewsapp.models import ( |
196 | Repository, |
197 | @@ -57,6 +59,7 @@ |
198 | SoftwareItem, |
199 | ) |
200 | from reviewsapp.tests.factory import TestCaseWithFactory |
201 | +from reviewsapp.tests.factory import RequestTestObject |
202 | from reviewsapp.tests.helpers import patch_settings |
203 | from reviewsapp.tests.matchers import ( |
204 | ContainsStatsForPackage, |
205 | @@ -164,6 +167,82 @@ |
206 | NoReverseMatch, reverse, 'rnr-api-reviews-stats', |
207 | kwargs={'days': days}) |
208 | |
209 | +class ShowUsefulnessHandlerTestCase(TestCaseWithFactory): |
210 | + def test_read_for_single_parameter(self): |
211 | + '''test that a single usefulness vote by a user is returned when |
212 | + searching for that user''' |
213 | + review = self.factory.makeReview( |
214 | + software_item=self.factory.makeSoftwareItem('package_bar')) |
215 | + user = self.factory.makeUser() |
216 | + usefulness = self.factory.makeUsefulness(review, user) |
217 | + |
218 | + handler = ShowUsefulnessHandler() |
219 | + |
220 | + request1 = RequestTestObject({'username': user.username}) |
221 | + request2 = RequestTestObject({'review_id' : review.id}) |
222 | + |
223 | + results1 = handler.read(request1) |
224 | + results2 = handler.read(request2) |
225 | + |
226 | + self.assertEqual(1, len(results1)) |
227 | + self.assertEqual(usefulness, results1[0]) |
228 | + self.assertEqual(usefulness, results2[0]) |
229 | + |
230 | + def test_read_filtering(self): |
231 | + '''test that a passed parameters results in all relevant items |
232 | + being returned, but no unwanted items''' |
233 | + review1 = self.factory.makeReview( |
234 | + software_item=self.factory.makeSoftwareItem('package_bar')) |
235 | + review2 = self.factory.makeReview( |
236 | + software_item=self.factory.makeSoftwareItem('foo_bar')) |
237 | + user1 = self.factory.makeUser() |
238 | + user2 = self.factory.makeUser() |
239 | + usefulness1 = self.factory.makeUsefulness(review1, user1) |
240 | + usefulness2 = self.factory.makeUsefulness(review1, user2) |
241 | + usefulness3 = self.factory.makeUsefulness(review2, user1) |
242 | + |
243 | + handler = ShowUsefulnessHandler() |
244 | + |
245 | + request1 = RequestTestObject({'username':user1.username}) |
246 | + request2 = RequestTestObject({'review_id':review1.id}) |
247 | + request3 = RequestTestObject( |
248 | + {'username':user1.username, 'review_id':review2.id} |
249 | + ) |
250 | + |
251 | + results1 = handler.read(request1) |
252 | + results2 = handler.read(request2) |
253 | + results3 = handler.read(request3) |
254 | + |
255 | + #test username filter results |
256 | + self.assertEqual(2, len(results1)) |
257 | + self.assertIn(usefulness1, results1) |
258 | + self.assertIn(usefulness3, results1) |
259 | + self.assertNotIn(usefulness2, results1) |
260 | + |
261 | + #test review_id filter results |
262 | + self.assertEqual(2, len(results2)) |
263 | + self.assertIn(usefulness1, results2) |
264 | + self.assertIn(usefulness2, results2) |
265 | + self.assertNotIn(usefulness3, results2) |
266 | + |
267 | + #test usefulness AND review_id filter results |
268 | + self.assertEqual(1, len(results3)) |
269 | + self.assertNotIn(usefulness1, results3) |
270 | + self.assertNotIn(usefulness2, results3) |
271 | + self.assertIn(usefulness3, results3) |
272 | + |
273 | + |
274 | + def test_read_for_no_parameters_gives_400_error(self): |
275 | + '''test that a http 400 error is returned if no valid parameter |
276 | + is passed''' |
277 | + handler = ShowUsefulnessHandler() |
278 | + request1 = RequestTestObject() |
279 | + request2 = RequestTestObject({'something':'nothing'}) |
280 | + results1 = handler.read(request1) |
281 | + results2 = handler.read(request2) |
282 | + self.assertEqual(httplib.BAD_REQUEST, results1.status_code) |
283 | + self.assertEqual(httplib.BAD_REQUEST, results2.status_code) |
284 | + |
285 | |
286 | class ShowReviewsHandlerTestCase(TestCaseWithFactory): |
287 | |
288 | |
289 | === modified file 'src/reviewsapp/tests/test_rnrclient.py' |
290 | --- src/reviewsapp/tests/test_rnrclient.py 2011-03-11 15:13:18 +0000 |
291 | +++ src/reviewsapp/tests/test_rnrclient.py 2011-03-15 10:41:35 +0000 |
292 | @@ -451,3 +451,18 @@ |
293 | count = Usefulness.objects.filter(review=review).count() |
294 | self.assertEqual(1, count) |
295 | |
296 | + def test_get_usefulness(self): |
297 | + api = RatingsAndReviewsAPI() |
298 | + software_item = self.factory.makeSoftwareItem(app_name='something2') |
299 | + review = self.factory.makeReview(software_item=software_item) |
300 | + user = self.factory.makeUser() |
301 | + usefulness = self.factory.makeUsefulness(review=review, user=user) |
302 | + |
303 | + response = api.get_usefulness(review_id=review.id, username=user.username) |
304 | + |
305 | + self.assertEqual(1, len(response)) |
306 | + self.assertEqual(user.username, response[0].get('username')) |
307 | + self.assertEqual(review.id, response[0].get('review_id')) |
308 | + self.assertEqual(usefulness.useful, response[0].get('useful')) |
309 | + |
310 | + |
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: 1.0/usefulness/ $ s/?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. 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.
* I wonder if it'd be worthwhile cutting the username out of the url:
r'^
and instead adding optional filtering via GET (/1.0/usefulnes
* 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/