Merge lp:~fgallina/rnr-server/click-reviews-return-useroid-as-username into lp:rnr-server

[Fabián Ezequiel Gallina]

Return the User openid as reviewer_username in clickreviews

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

[Natalia Bidart (nataliabidart)]

Overall looks good, added a few comments for this implementation.

What I wonder it will not be a better approach to migrate usernames as we go, that means: we update the username value with the openid value, that we could have pre-stored in the instances.

What do you think?

[Natalia Bidart (nataliabidart)]

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~fgallina/rnr-server/click-reviews-return-useroid-as-username into lp:rnr-server failed. Below is the output from the failed tests.

rm -rf ./virtualenv
sed -e '1,/# Dependencies/d;s/^rnr-server/./g;s/bazaar.isd/bazaar.launchpad.net/g' config-manager.txt > config-manager.txt.tmp
/usr/lib/config-manager/cm.py update config-manager.txt.tmp
Not deleting ./virtualenv/bin
New python executable in ./virtualenv/bin/python
Installing distribute.............................................................................................................................................................................................done.
Installing pip...............done.
./virtualenv/bin/pip install -r requirements.txt
Requirement already satisfied (use --upgrade to upgrade): coverage in /usr/lib/python2.7/dist-packages (from -r requirements.txt (line 4))
Downloading/unpacking django-factory==0.11 (from -r requirements.txt (line 5))
  Downloading django_factory-0.11.tar.gz
  Running setup.py egg_info for package django-factory

    warning: no previously-included files matching '*.pyc' found anywhere in distribution
Requirement already satisfied (use --upgrade to upgrade): mechanize in /usr/lib/python2.7/dist-packages (from -r requirements.txt (line 6))
Requirement already satisfied (use --upgrade to upgrade): mock in /usr/lib/python2.7/dist-packages (from -r requirements.txt (line 7))
Requirement already satisfied (use --upgrade to upgrade): paste==1.7.5.1 in /usr/lib/python2.7/dist-packages (from -r requirements.txt (line 8))
Requirement already satisfied (use --upgrade to upgrade): pep8 in /usr/lib/python2.7/dist-packages (from -r requirements.txt (line 9))
Requirement already satisfied (use --upgrade to upgrade): piston-mini-client in /usr/lib/python2.7/dist-packages (from -r requirements.txt (line 10))
Requirement already satisfied (use --upgrade to upgrade): testtools in /usr/lib/python2.7/dist-packages (from -r requirements.txt (line 11))
Downloading/unpacking wsgi-intercept==0.5.1 (from -r requirements.txt (line 12))
  Running setup.py egg_info for package wsgi-intercept

Requirement already satisfied (use --upgrade to upgrade): django>=1.1 in /usr/lib/python2.7/dist-packages (from django-factory==0.11->-r requirements.txt (line 5))
Installing collected packages: django-factory, wsgi-intercept
  Running setup.py install for django-factory

    warning: no previously-included files matching '*.pyc' found anywhere in distribution
  Running setup.py install for wsgi-intercept

Successfully installed django-factory wsgi-intercept
Cleaning up...
[localhost] local: virtualenv/bin/python django_project/manage.py test -v 2 --failfast src
Creating test database for alias 'default' (':memory:')...
Syncing...
Creating tables ...
Creating table django_admin_log
Creating table auth_permission
Creating table auth_group_permissions
Creating table auth_group
Creating table auth_user_groups
Creating table auth_user_user_permissions
Creating table auth_user
Creating table django_content_type
Creating table django_session
Creating table django_site
Creating table south_migrationhistory
Creating table piston_nonce
Creating t...

[lp:~fgallina/rnr-server/click-reviews-return-useroid-as-username]

304. By Fabián Ezequiel Gallina on 2015-04-22

Fix tests

=== modified file 'src/clickreviews/api/handlers.py'
--- src/clickreviews/api/handlers.py	2015-03-10 14:56:42 +0000
+++ src/clickreviews/api/handlers.py	2015-04-22 19:38:49 +0000
@@ -58,6 +58,7 @@
         package_name = form.cleaned_data['package_name']
 
         return (ClickPackageReview.objects.published()
+                .prefetch_related('reviewer__useropenid_set')
                 .select_related('click_package', 'reviewer')
                 .filter(click_package__package_name=package_name)
                 # XXX: Order by date until usefulness is implemented.
@@ -172,6 +173,9 @@
     _model = None
     _form = None
 
+    def _prepare_queryset(self, queryset):
+        return queryset
+
     def read(self, request, status=None, pk=None):
         if status is None:
             queryset = self._model.objects.all()
@@ -188,6 +192,7 @@
                     .order_by('-num_flags', 'date_created')
                     .select_related()
                     .prefetch_related('flag_set__user'))
+        queryset = self._prepare_queryset(queryset)
 
         if pk is not None:
             return get_object_or_404(queryset, pk=pk)
@@ -288,3 +293,6 @@
                       'get_full_name',
                       'last_name')))),
         'status')
+
+    def _prepare_queryset(self, queryset):
+        return queryset.prefetch_related('review__reviewer__useropenid_set')
 
=== modified file 'src/clickreviews/models.py'
--- src/clickreviews/models.py	2015-03-10 19:16:55 +0000
+++ src/clickreviews/models.py	2015-04-22 19:38:49 +0000
@@ -74,6 +74,12 @@
         return self.click_package.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]
+        # Production queries show this won't happen, use as safeguard.
         return self.reviewer.username
 
     def reviewer_displayname(self):
 
=== modified file 'src/clickreviews/tests/test_handlers.py'
--- src/clickreviews/tests/test_handlers.py	2015-03-10 19:16:55 +0000
+++ src/clickreviews/tests/test_handlers.py	2015-04-22 19:38:49 +0000
@@ -23,7 +23,7 @@
 )
 from clickreviews.tests.factory import TestCaseWithFactory
 from core.models import BaseModeration
-from core.tests.helpers import SSOTestCaseMixin
+from core.tests.helpers import SSOTestCaseMixin, assert_no_extra_queries_after
 
 
 class ClickReviewsHandlerTestCase(TestCaseWithFactory):
@@ -91,7 +91,7 @@
             'date_created': DateTimeAwareJSONEncoder().encode(
                 review.date_created).strip('"'),
             'rating': 5,
-            'reviewer_username': review.reviewer.username,
+            'reviewer_username': review.reviewer_username(),
             'reviewer_displayname': review.reviewer.get_full_name(),
             'summary': 'Excellent',
             'review_text': 'Plain brilliant piece of software',
@@ -253,10 +253,10 @@
         package = self.factory.makeClickPackage()
         for _ in range(5):
             self.factory.makeClickPackageReview(click_package=package)
-
-        self.assertNumQueries(1, self._get_reviews, data={
-            'package_name': package.package_name,
-        })
+        assert_no_extra_queries_after(
+            lambda: self.factory.makeClickPackageReview(click_package=package),
+            self._get_reviews, data={'package_name': package.package_name}
+        )
 
     def _update_review(self, review, data, user=None):
         # Post a review as an authenticated user.
@@ -953,19 +953,12 @@
                       data['errors']['status'])
 
     def test_read_query_count(self):
-        # 1 query for counting the moderations
-        # 1 query for moderations themselves
-        # 1 query for prefetching flags
-        expected_queries = 3
-        url = reverse(self.url_name)
-
-        for _ in range(10):
-            self.make_moderation_object()
-        self.assertNumQueries(expected_queries, lambda: self.request_get(url))
-
-        for _ in range(10):
-            self.make_moderation_object()
-        self.assertNumQueries(expected_queries, lambda: self.request_get(url))
+        for _ in range(5):
+            self.make_moderation_object()
+
+        assert_no_extra_queries_after(
+            lambda: self.make_moderation_object(),
+            self.request_get, reverse(self.url_name))
 
     def test_ordering(self):
         # the following are sorted in reverse expected order.
 
=== modified file 'src/clickreviews/tests/test_models.py'
--- src/clickreviews/tests/test_models.py	2015-03-09 20:47:12 +0000
+++ src/clickreviews/tests/test_models.py	2015-04-22 19:38:49 +0000
@@ -157,6 +157,25 @@
         # Stats should be updated.
         self.assertEqual(review.click_package.histogram, '[0, 0, 0, 0, 0]')
 
+    def test_reviewer_username_same_username_and_openid(self):
+        expected = 'theuoid'
+        user = self.factory.makeUser(username=expected, open_id=expected)
+        review = self.factory.makeClickPackageReview(reviewer=user)
+        self.assertEqual(review.reviewer_username(), expected)
+
+    def test_reviewer_username_returns_openid(self):
+        expected = 'theopenid'
+        user = self.factory.makeUser(
+            username='bogusfromlogin', open_id=expected)
+        review = self.factory.makeClickPackageReview(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.makeClickPackageReview(reviewer=user)
+        self.assertEqual(review.reviewer_username(), expected)
+
 
 class BaseClickPackageModerationTestCase(TestCaseWithFactory):
 
 
=== modified file 'src/core/tests/factory.py'
--- src/core/tests/factory.py	2014-09-22 19:42:51 +0000
+++ src/core/tests/factory.py	2015-04-22 19:38:49 +0000
@@ -50,12 +50,14 @@
 
         # Create an openid record too.
         if open_id is None:
-            open_id = full_claimed_id(self.getUniqueString(prefix='ident-'))
+            open_id = self.getUniqueString(prefix='ident-')
         elif open_id is False:
             return user
 
+        claimed_id = full_claimed_id(open_id)
+
         UserOpenID.objects.create(
-            user=user, claimed_id=open_id, display_id=open_id)
+            user=user, claimed_id=claimed_id, display_id=claimed_id)
 
         return user
 
 
=== modified file 'src/core/tests/helpers.py'
--- src/core/tests/helpers.py	2014-10-07 19:04:20 +0000
+++ src/core/tests/helpers.py	2015-04-22 19:38:49 +0000
@@ -1,3 +1,7 @@
+from operator import itemgetter
+
+from django.db import connection
+from django.test.utils import CaptureQueriesContext
 from mock import patch
 
 from core.utilities import web_services
@@ -27,3 +31,55 @@
         }
         data.update(**kwargs)
         return data
+
+
+def assert_no_extra_queries_after(modify_data, f, *args, **kwargs):
+    """Assert function performs equal query numbers after some change.
+
+    Many views perform terribly because they perform extra queries
+    for every item in some list. For example when looping over a
+    list of users to present them, if a value is accessed through
+    a foreign key then it may perform an extra query. If care
+    is not taken then the view may perform terribly when
+    operating on a large list. This is often worked around via
+    .prefetch_related() or .select_related().
+
+    This function helps in testing that this doesn't happen. The arguments
+    to this function are a callable to modify data, and a function
+    to call before and after (*args and **kwargs are passed
+    through to the latter function). Both calls to the function
+    should issue the same number of queries.
+
+    For instance
+
+    self.factory.makeUser()
+    assert_no_extra_queries_after(
+        self.factory.makeUser,
+        self.client.get, '/users'):
+
+    Will first request the view and record how many queries were
+    done. It will then run the modify function, creating another user
+    in this case, and then request the view again. If the second
+    request doesn't issue the same number of queries then an
+    AssertionError will be raised.
+    """
+    first = CaptureQueriesContext(connection)
+    with first:
+        f(*args, **kwargs)
+    modify_data()
+    second = CaptureQueriesContext(connection)
+    with second:
+        f(*args, **kwargs)
+    if len(first) != len(second):
+        message = ("Different number of queries for the "
+                   "second execution. {} != {}".format(
+                       len(first), len(second)))
+
+        def format_queries(qs):
+            return "\n\n".join(map(str, map(itemgetter('sql'), qs)))
+
+        message += "\nFirst queries:\n"
+        message += format_queries(first.captured_queries)
+        message += "\n\n\nSecond queries:\n"
+        message += format_queries(second.captured_queries)
+        raise AssertionError(message)
 
=== modified file 'src/core/tests/test_utilities.py'
--- src/core/tests/test_utilities.py	2014-10-03 17:54:35 +0000
+++ src/core/tests/test_utilities.py	2015-04-22 19:38:49 +0000
@@ -376,22 +376,22 @@
         sso_user_data = self.mock_data_for_account(
             displayname='John Does', email='[e-mail]')
         self.mock_get_data.return_value = sso_user_data
-        openid = full_claimed_id('existent')
+        open_id = 'existent'
         self.factory.makeUser(
             first_name='John', last_name='Does', email='[e-mail]',
-            open_id=openid)
-        user, created = update_or_create_user_from_oauth('existent')
+            open_id=open_id)
+        user, created = update_or_create_user_from_oauth(open_id)
         self.assertIsNotNone(user)
         self.assertFalse(created)
         self.assert_user_data(user, sso_user_data)
 
     def test_missing_account_data_and_existing_user(self):
         self.mock_get_data.return_value = None
-        openid = full_claimed_id('existent')
+        open_id = 'existent'
         self.factory.makeUser(
             first_name='John', last_name='Does', email='[e-mail]',
-            open_id=openid)
-        user, created = update_or_create_user_from_oauth('existent')
+            open_id=open_id)
+        user, created = update_or_create_user_from_oauth(open_id)
         self.assertIsNotNone(user)
         self.assertFalse(created)
 
@@ -399,12 +399,12 @@
         sso_user_data = self.mock_data_for_account(
             displayname='Juan Hace', email='[e-mail]')
         self.mock_get_data.return_value = sso_user_data
-        openid = full_claimed_id('existent')
+        open_id = 'existent'
         self.factory.makeUser(
             first_name='John', last_name='Does', email='[e-mail]',
-            open_id=openid)
+            open_id=open_id)
         user, created = update_or_create_user_from_oauth(
-            'existent', update_window=None)
+            open_id, update_window=None)
         self.assertIsNotNone(user)
         self.assertFalse(created)
         self.assert_user_data(user, sso_user_data)
@@ -413,12 +413,12 @@
         sso_user_data = self.mock_data_for_account(
             displayname='Juan Hace', email='[e-mail]')
         self.mock_get_data.return_value = sso_user_data
-        openid = full_claimed_id('existent')
+        open_id = 'existent'
         user = self.factory.makeUser(
             first_name='John', last_name='Does', email='[e-mail]',
-            open_id=openid)
+            open_id=open_id)
         logged_user, created = update_or_create_user_from_oauth(
-            'existent', update_window=60 * 60 * 24)
+            open_id, update_window=60 * 60 * 24)
         self.assertIsNotNone(logged_user)
         self.assertFalse(created)
         current_sso_user_data = self.mock_data_for_account(
@@ -429,12 +429,12 @@
         sso_user_data = self.mock_data_for_account(
             verified=False, displayname='Juan Hace', email='[e-mail]')
         self.mock_get_data.return_value = sso_user_data
-        openid = full_claimed_id('existent')
+        open_id = 'existent'
         self.factory.makeUser(
             first_name='John', last_name='Does', email='[e-mail]',
-            open_id=openid)
+            open_id=open_id)
         user, created = update_or_create_user_from_oauth(
-            'existent', update_window=None)
+            open_id, update_window=None)
         self.assertIsNotNone(user)
         self.assertFalse(created)
         self.assertEqual(user.get_full_name(), 'John Does')
@@ -444,12 +444,12 @@
         sso_user_data = self.mock_data_for_account(
             displayname='Juan Hace', email='')
         self.mock_get_data.return_value = sso_user_data
-        openid = full_claimed_id('existent')
+        open_id = 'existent'
         self.factory.makeUser(
             first_name='John', last_name='Does', email='[e-mail]',
-            open_id=openid)
+            open_id=open_id)
         user, created = update_or_create_user_from_oauth(
-            'existent', update_window=None)
+            open_id, update_window=None)
         self.assertIsNotNone(user)
         self.assertFalse(created)
         self.assertEqual(user.get_full_name(), 'John Does')
@@ -459,12 +459,12 @@
         sso_user_data = self.mock_data_for_account(
             displayname=None, email='[e-mail]')
         self.mock_get_data.return_value = sso_user_data
-        openid = full_claimed_id('existent')
+        open_id = 'existent'
         self.factory.makeUser(
             first_name='John', last_name='Does', email='[e-mail]',
-            open_id=openid)
+            open_id=open_id)
         user, created = update_or_create_user_from_oauth(
-            'existent', update_window=None)
+            open_id, update_window=None)
         self.assertIsNotNone(user)
         self.assertFalse(created)
         self.assertEqual(user.get_full_name(), 'John Does')