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

[Michael Nelson]

Return openid for username.

See https://bugs.launchpad.net/rnr-server/+bug/1564209 for details.

Change mostly based on:
https://code.launchpad.net/~fgallina/rnr-server/click-reviews-return-useroid-as-username/+merge/257106

[Thomi Richards (thomir-deactivatedaccount)]

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)]

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.

=== modified file 'src/reviewsapp/api/handlers.py'
--- src/reviewsapp/api/handlers.py	2016-02-19 17:11:09 +0000
+++ src/reviewsapp/api/handlers.py	2016-04-04 04:11:07 +0000
@@ -310,9 +310,11 @@
             'version': version,
         }
         kwargs = dict(item for item in kwargs.items() if item[1] != 'any')
-        return Review.objects.filter(
-            softwareitem__package_name=package_name, hide=False,
-            **kwargs).order_by("-version", sort_key)[batch_start:batch_end]
+        return (Review.objects.prefetch_related('reviewer__useropenid_set')
+                .select_related('softwareitem', 'reviewer', 'repository')
+                .filter(softwareitem__package_name=package_name, hide=False,
+                        **kwargs)
+                .order_by("-version", sort_key)[batch_start:batch_end])
 
 
 class ShowReviewsSinceHandler(ShowReviewsHandler):

=== modified file 'src/reviewsapp/models.py'
--- src/reviewsapp/models.py	2014-09-24 22:39:43 +0000
+++ src/reviewsapp/models.py	2016-04-04 04:11:07 +0000
@@ -272,6 +272,11 @@
             return self.package_name()
 
     def reviewer_username(self):
+        # Use .all() so that `prefetch_related` have effect.
+        openid_objs = list(self.reviewer.useropenid_set.all())
+        if openid_objs:
+            openid_obj = openid_objs[-1]
+            return openid_obj.claimed_id.split('/')[-1]
         return self.reviewer.username
 
     def reviewer_displayname(self):

=== modified file 'src/reviewsapp/tests/test_handlers.py'
--- src/reviewsapp/tests/test_handlers.py	2016-02-19 17:11:09 +0000
+++ src/reviewsapp/tests/test_handlers.py	2016-04-04 04:11:07 +0000
@@ -56,6 +56,7 @@
 from piston.emitters import Emitter
 from piston.handler import typemapper
 
+from core.tests.helpers import assert_no_extra_queries_after
 from reviewsapp.api.handlers import (
     Django13JSONEncoder,
     Django13JSONEmitter,
@@ -841,6 +842,17 @@
         response_decoded = simplejson.loads(response.content)
         self.assertEqual(2, len(response_decoded))
 
+    def test_no_extra_queries_for_openid(self):
+        # The username returns an openid identifier from a related
+        # model.
+        package = self.factory.makeSoftwareItem('foo')
+        self.factory.makeReview(software_item=package)
+
+        with patch_settings(CACHE_MIDDLEWARE_SECONDS=0):
+            assert_no_extra_queries_after(
+                lambda: self.factory.makeReview(software_item=package),
+                self.client.get, self._reviews_url_for_package('foo'))
+
     def test_request_cached(self):
         # Requests for reviews are cached.
         url = self._reviews_url_for_package('package_foo')

=== modified file 'src/reviewsapp/tests/test_models.py'
--- src/reviewsapp/tests/test_models.py	2014-08-12 14:38:31 +0000
+++ src/reviewsapp/tests/test_models.py	2016-04-04 04:11:07 +0000
@@ -138,6 +138,19 @@
         self.assertEqual(1, review.usefulness_favorable)
         self.assertEqual(50, review.usefulness_percentage)
 
+    def test_reviewer_username_returns_openid(self):
+        expected = 'theopenid'
+        user = self.factory.makeUser(
+            username='bogusfromlogin', open_id=expected)
+        review = self.factory.makeReview(reviewer=user)
+        self.assertEqual(review.reviewer_username(), expected)
+
+    def test_reviewer_username_fallbacks_to_username(self):
+        expected = 'bogusfromlogin'
+        user = self.factory.makeUser(username=expected, open_id=False)
+        review = self.factory.makeReview(reviewer=user)
+        self.assertEqual(review.reviewer_username(), expected)
+
 
 class ReviewFlagTestCase(TestCaseWithFactory):