Merge lp:~michael.nelson/rnr-server/1564209-return-openid-for-username into lp:rnr-server

Proposed by Michael Nelson on 2016-04-04
Status: Merged
Approved by: Michael Nelson on 2016-04-04
Approved revision: 317
Merged at revision: 316
Proposed branch: lp:~michael.nelson/rnr-server/1564209-return-openid-for-username
Merge into: lp:rnr-server
Diff against target: 87 lines (+35/-3)
4 files modified
src/reviewsapp/api/handlers.py (+5/-3)
src/reviewsapp/models.py (+5/-0)
src/reviewsapp/tests/test_handlers.py (+12/-0)
src/reviewsapp/tests/test_models.py (+13/-0)
To merge this branch: bzr merge lp:~michael.nelson/rnr-server/1564209-return-openid-for-username
Reviewer Review Type Date Requested Status
Thomi Richards (community) 2016-04-04 Approve on 2016-04-04
Review via email: mp+290850@code.launchpad.net

Commit message

Return openid for username.

To post a comment you must log in.
Thomi Richards (thomir) wrote :

This looks OK to me, but I have very little idea what I'm looking at. If this is important / security-vulnerable code we really ought to have someone familiar with the system check this out.

review: Approve
Michael Nelson (michael.nelson) wrote :

On Mon, Apr 4, 2016 at 2:31 PM Thomi Richards <email address hidden>
wrote:

> Review: Approve
>
> This looks OK to me, but I have very little idea what I'm looking at. If
> this is important / security-vulnerable code we really ought to have
> someone familiar with the system check this out.
>
>
Thanks Thomi. I'll land it now but have Fabian verify if it's OK or needs
more changes.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/reviewsapp/api/handlers.py'
2--- src/reviewsapp/api/handlers.py 2016-02-19 17:11:09 +0000
3+++ src/reviewsapp/api/handlers.py 2016-04-04 04:11:07 +0000
4@@ -310,9 +310,11 @@
5 'version': version,
6 }
7 kwargs = dict(item for item in kwargs.items() if item[1] != 'any')
8- return Review.objects.filter(
9- softwareitem__package_name=package_name, hide=False,
10- **kwargs).order_by("-version", sort_key)[batch_start:batch_end]
11+ return (Review.objects.prefetch_related('reviewer__useropenid_set')
12+ .select_related('softwareitem', 'reviewer', 'repository')
13+ .filter(softwareitem__package_name=package_name, hide=False,
14+ **kwargs)
15+ .order_by("-version", sort_key)[batch_start:batch_end])
16
17
18 class ShowReviewsSinceHandler(ShowReviewsHandler):
19
20=== modified file 'src/reviewsapp/models.py'
21--- src/reviewsapp/models.py 2014-09-24 22:39:43 +0000
22+++ src/reviewsapp/models.py 2016-04-04 04:11:07 +0000
23@@ -272,6 +272,11 @@
24 return self.package_name()
25
26 def reviewer_username(self):
27+ # Use .all() so that `prefetch_related` have effect.
28+ openid_objs = list(self.reviewer.useropenid_set.all())
29+ if openid_objs:
30+ openid_obj = openid_objs[-1]
31+ return openid_obj.claimed_id.split('/')[-1]
32 return self.reviewer.username
33
34 def reviewer_displayname(self):
35
36=== modified file 'src/reviewsapp/tests/test_handlers.py'
37--- src/reviewsapp/tests/test_handlers.py 2016-02-19 17:11:09 +0000
38+++ src/reviewsapp/tests/test_handlers.py 2016-04-04 04:11:07 +0000
39@@ -56,6 +56,7 @@
40 from piston.emitters import Emitter
41 from piston.handler import typemapper
42
43+from core.tests.helpers import assert_no_extra_queries_after
44 from reviewsapp.api.handlers import (
45 Django13JSONEncoder,
46 Django13JSONEmitter,
47@@ -841,6 +842,17 @@
48 response_decoded = simplejson.loads(response.content)
49 self.assertEqual(2, len(response_decoded))
50
51+ def test_no_extra_queries_for_openid(self):
52+ # The username returns an openid identifier from a related
53+ # model.
54+ package = self.factory.makeSoftwareItem('foo')
55+ self.factory.makeReview(software_item=package)
56+
57+ with patch_settings(CACHE_MIDDLEWARE_SECONDS=0):
58+ assert_no_extra_queries_after(
59+ lambda: self.factory.makeReview(software_item=package),
60+ self.client.get, self._reviews_url_for_package('foo'))
61+
62 def test_request_cached(self):
63 # Requests for reviews are cached.
64 url = self._reviews_url_for_package('package_foo')
65
66=== modified file 'src/reviewsapp/tests/test_models.py'
67--- src/reviewsapp/tests/test_models.py 2014-08-12 14:38:31 +0000
68+++ src/reviewsapp/tests/test_models.py 2016-04-04 04:11:07 +0000
69@@ -138,6 +138,19 @@
70 self.assertEqual(1, review.usefulness_favorable)
71 self.assertEqual(50, review.usefulness_percentage)
72
73+ def test_reviewer_username_returns_openid(self):
74+ expected = 'theopenid'
75+ user = self.factory.makeUser(
76+ username='bogusfromlogin', open_id=expected)
77+ review = self.factory.makeReview(reviewer=user)
78+ self.assertEqual(review.reviewer_username(), expected)
79+
80+ def test_reviewer_username_fallbacks_to_username(self):
81+ expected = 'bogusfromlogin'
82+ user = self.factory.makeUser(username=expected, open_id=False)
83+ review = self.factory.makeReview(reviewer=user)
84+ self.assertEqual(review.reviewer_username(), expected)
85+
86
87 class ReviewFlagTestCase(TestCaseWithFactory):
88

Subscribers

People subscribed via source and target branches