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

Proposed by Fabián Ezequiel Gallina on 2015-04-22
Status: Merged
Approved by: Fabián Ezequiel Gallina on 2015-04-22
Approved revision: 304
Merged at revision: 299
Proposed branch: lp:~fgallina/rnr-server/click-reviews-return-useroid-as-username
Merge into: lp:rnr-server
Diff against target: 348 lines (+126/-42)
7 files modified
src/clickreviews/api/handlers.py (+8/-0)
src/clickreviews/models.py (+6/-0)
src/clickreviews/tests/test_handlers.py (+12/-19)
src/clickreviews/tests/test_models.py (+19/-0)
src/core/tests/factory.py (+4/-2)
src/core/tests/helpers.py (+56/-0)
src/core/tests/test_utilities.py (+21/-21)
To merge this branch: bzr merge lp:~fgallina/rnr-server/click-reviews-return-useroid-as-username
Reviewer Review Type Date Requested Status
Natalia Bidart Approve on 2015-04-22
Ricardo Kirkner (community) 2015-04-22 Approve on 2015-04-22
Review via email: mp+257106@code.launchpad.net

Commit message

Return the User openid as reviewer_username in clickreviews

To post a comment you must log in.
Ricardo Kirkner (ricardokirkner) wrote :

LGTM

review: Approve
Natalia Bidart (nataliabidart) wrote :

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?

review: Approve
Ubuntu One Auto Pilot (otto-pilot) wrote :
Download full text (9.1 KiB)

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

Read more...

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

Fix tests

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/clickreviews/api/handlers.py'
2--- src/clickreviews/api/handlers.py 2015-03-10 14:56:42 +0000
3+++ src/clickreviews/api/handlers.py 2015-04-22 19:38:49 +0000
4@@ -58,6 +58,7 @@
5 package_name = form.cleaned_data['package_name']
6
7 return (ClickPackageReview.objects.published()
8+ .prefetch_related('reviewer__useropenid_set')
9 .select_related('click_package', 'reviewer')
10 .filter(click_package__package_name=package_name)
11 # XXX: Order by date until usefulness is implemented.
12@@ -172,6 +173,9 @@
13 _model = None
14 _form = None
15
16+ def _prepare_queryset(self, queryset):
17+ return queryset
18+
19 def read(self, request, status=None, pk=None):
20 if status is None:
21 queryset = self._model.objects.all()
22@@ -188,6 +192,7 @@
23 .order_by('-num_flags', 'date_created')
24 .select_related()
25 .prefetch_related('flag_set__user'))
26+ queryset = self._prepare_queryset(queryset)
27
28 if pk is not None:
29 return get_object_or_404(queryset, pk=pk)
30@@ -288,3 +293,6 @@
31 'get_full_name',
32 'last_name')))),
33 'status')
34+
35+ def _prepare_queryset(self, queryset):
36+ return queryset.prefetch_related('review__reviewer__useropenid_set')
37
38=== modified file 'src/clickreviews/models.py'
39--- src/clickreviews/models.py 2015-03-10 19:16:55 +0000
40+++ src/clickreviews/models.py 2015-04-22 19:38:49 +0000
41@@ -74,6 +74,12 @@
42 return self.click_package.package_name
43
44 def reviewer_username(self):
45+ # Use .all() so that `prefetch_related` have effect.
46+ openid_objs = list(self.reviewer.useropenid_set.all())
47+ if openid_objs:
48+ openid_obj = openid_objs[-1]
49+ return openid_obj.claimed_id.split('/')[-1]
50+ # Production queries show this won't happen, use as safeguard.
51 return self.reviewer.username
52
53 def reviewer_displayname(self):
54
55=== modified file 'src/clickreviews/tests/test_handlers.py'
56--- src/clickreviews/tests/test_handlers.py 2015-03-10 19:16:55 +0000
57+++ src/clickreviews/tests/test_handlers.py 2015-04-22 19:38:49 +0000
58@@ -23,7 +23,7 @@
59 )
60 from clickreviews.tests.factory import TestCaseWithFactory
61 from core.models import BaseModeration
62-from core.tests.helpers import SSOTestCaseMixin
63+from core.tests.helpers import SSOTestCaseMixin, assert_no_extra_queries_after
64
65
66 class ClickReviewsHandlerTestCase(TestCaseWithFactory):
67@@ -91,7 +91,7 @@
68 'date_created': DateTimeAwareJSONEncoder().encode(
69 review.date_created).strip('"'),
70 'rating': 5,
71- 'reviewer_username': review.reviewer.username,
72+ 'reviewer_username': review.reviewer_username(),
73 'reviewer_displayname': review.reviewer.get_full_name(),
74 'summary': 'Excellent',
75 'review_text': 'Plain brilliant piece of software',
76@@ -253,10 +253,10 @@
77 package = self.factory.makeClickPackage()
78 for _ in range(5):
79 self.factory.makeClickPackageReview(click_package=package)
80-
81- self.assertNumQueries(1, self._get_reviews, data={
82- 'package_name': package.package_name,
83- })
84+ assert_no_extra_queries_after(
85+ lambda: self.factory.makeClickPackageReview(click_package=package),
86+ self._get_reviews, data={'package_name': package.package_name}
87+ )
88
89 def _update_review(self, review, data, user=None):
90 # Post a review as an authenticated user.
91@@ -953,19 +953,12 @@
92 data['errors']['status'])
93
94 def test_read_query_count(self):
95- # 1 query for counting the moderations
96- # 1 query for moderations themselves
97- # 1 query for prefetching flags
98- expected_queries = 3
99- url = reverse(self.url_name)
100-
101- for _ in range(10):
102- self.make_moderation_object()
103- self.assertNumQueries(expected_queries, lambda: self.request_get(url))
104-
105- for _ in range(10):
106- self.make_moderation_object()
107- self.assertNumQueries(expected_queries, lambda: self.request_get(url))
108+ for _ in range(5):
109+ self.make_moderation_object()
110+
111+ assert_no_extra_queries_after(
112+ lambda: self.make_moderation_object(),
113+ self.request_get, reverse(self.url_name))
114
115 def test_ordering(self):
116 # the following are sorted in reverse expected order.
117
118=== modified file 'src/clickreviews/tests/test_models.py'
119--- src/clickreviews/tests/test_models.py 2015-03-09 20:47:12 +0000
120+++ src/clickreviews/tests/test_models.py 2015-04-22 19:38:49 +0000
121@@ -157,6 +157,25 @@
122 # Stats should be updated.
123 self.assertEqual(review.click_package.histogram, '[0, 0, 0, 0, 0]')
124
125+ def test_reviewer_username_same_username_and_openid(self):
126+ expected = 'theuoid'
127+ user = self.factory.makeUser(username=expected, open_id=expected)
128+ review = self.factory.makeClickPackageReview(reviewer=user)
129+ self.assertEqual(review.reviewer_username(), expected)
130+
131+ def test_reviewer_username_returns_openid(self):
132+ expected = 'theopenid'
133+ user = self.factory.makeUser(
134+ username='bogusfromlogin', open_id=expected)
135+ review = self.factory.makeClickPackageReview(reviewer=user)
136+ self.assertEqual(review.reviewer_username(), expected)
137+
138+ def test_reviewer_username_fallbacks_to_username(self):
139+ expected = 'bogusfromlogin'
140+ user = self.factory.makeUser(username=expected, open_id=False)
141+ review = self.factory.makeClickPackageReview(reviewer=user)
142+ self.assertEqual(review.reviewer_username(), expected)
143+
144
145 class BaseClickPackageModerationTestCase(TestCaseWithFactory):
146
147
148=== modified file 'src/core/tests/factory.py'
149--- src/core/tests/factory.py 2014-09-22 19:42:51 +0000
150+++ src/core/tests/factory.py 2015-04-22 19:38:49 +0000
151@@ -50,12 +50,14 @@
152
153 # Create an openid record too.
154 if open_id is None:
155- open_id = full_claimed_id(self.getUniqueString(prefix='ident-'))
156+ open_id = self.getUniqueString(prefix='ident-')
157 elif open_id is False:
158 return user
159
160+ claimed_id = full_claimed_id(open_id)
161+
162 UserOpenID.objects.create(
163- user=user, claimed_id=open_id, display_id=open_id)
164+ user=user, claimed_id=claimed_id, display_id=claimed_id)
165
166 return user
167
168
169=== modified file 'src/core/tests/helpers.py'
170--- src/core/tests/helpers.py 2014-10-07 19:04:20 +0000
171+++ src/core/tests/helpers.py 2015-04-22 19:38:49 +0000
172@@ -1,3 +1,7 @@
173+from operator import itemgetter
174+
175+from django.db import connection
176+from django.test.utils import CaptureQueriesContext
177 from mock import patch
178
179 from core.utilities import web_services
180@@ -27,3 +31,55 @@
181 }
182 data.update(**kwargs)
183 return data
184+
185+
186+def assert_no_extra_queries_after(modify_data, f, *args, **kwargs):
187+ """Assert function performs equal query numbers after some change.
188+
189+ Many views perform terribly because they perform extra queries
190+ for every item in some list. For example when looping over a
191+ list of users to present them, if a value is accessed through
192+ a foreign key then it may perform an extra query. If care
193+ is not taken then the view may perform terribly when
194+ operating on a large list. This is often worked around via
195+ .prefetch_related() or .select_related().
196+
197+ This function helps in testing that this doesn't happen. The arguments
198+ to this function are a callable to modify data, and a function
199+ to call before and after (*args and **kwargs are passed
200+ through to the latter function). Both calls to the function
201+ should issue the same number of queries.
202+
203+ For instance
204+
205+ self.factory.makeUser()
206+ assert_no_extra_queries_after(
207+ self.factory.makeUser,
208+ self.client.get, '/users'):
209+
210+ Will first request the view and record how many queries were
211+ done. It will then run the modify function, creating another user
212+ in this case, and then request the view again. If the second
213+ request doesn't issue the same number of queries then an
214+ AssertionError will be raised.
215+ """
216+ first = CaptureQueriesContext(connection)
217+ with first:
218+ f(*args, **kwargs)
219+ modify_data()
220+ second = CaptureQueriesContext(connection)
221+ with second:
222+ f(*args, **kwargs)
223+ if len(first) != len(second):
224+ message = ("Different number of queries for the "
225+ "second execution. {} != {}".format(
226+ len(first), len(second)))
227+
228+ def format_queries(qs):
229+ return "\n\n".join(map(str, map(itemgetter('sql'), qs)))
230+
231+ message += "\nFirst queries:\n"
232+ message += format_queries(first.captured_queries)
233+ message += "\n\n\nSecond queries:\n"
234+ message += format_queries(second.captured_queries)
235+ raise AssertionError(message)
236
237=== modified file 'src/core/tests/test_utilities.py'
238--- src/core/tests/test_utilities.py 2014-10-03 17:54:35 +0000
239+++ src/core/tests/test_utilities.py 2015-04-22 19:38:49 +0000
240@@ -376,22 +376,22 @@
241 sso_user_data = self.mock_data_for_account(
242 displayname='John Does', email='user@email.com')
243 self.mock_get_data.return_value = sso_user_data
244- openid = full_claimed_id('existent')
245+ open_id = 'existent'
246 self.factory.makeUser(
247 first_name='John', last_name='Does', email='user@email.com',
248- open_id=openid)
249- user, created = update_or_create_user_from_oauth('existent')
250+ open_id=open_id)
251+ user, created = update_or_create_user_from_oauth(open_id)
252 self.assertIsNotNone(user)
253 self.assertFalse(created)
254 self.assert_user_data(user, sso_user_data)
255
256 def test_missing_account_data_and_existing_user(self):
257 self.mock_get_data.return_value = None
258- openid = full_claimed_id('existent')
259+ open_id = 'existent'
260 self.factory.makeUser(
261 first_name='John', last_name='Does', email='user@email.com',
262- open_id=openid)
263- user, created = update_or_create_user_from_oauth('existent')
264+ open_id=open_id)
265+ user, created = update_or_create_user_from_oauth(open_id)
266 self.assertIsNotNone(user)
267 self.assertFalse(created)
268
269@@ -399,12 +399,12 @@
270 sso_user_data = self.mock_data_for_account(
271 displayname='Juan Hace', email='juan@email.com')
272 self.mock_get_data.return_value = sso_user_data
273- openid = full_claimed_id('existent')
274+ open_id = 'existent'
275 self.factory.makeUser(
276 first_name='John', last_name='Does', email='user@email.com',
277- open_id=openid)
278+ open_id=open_id)
279 user, created = update_or_create_user_from_oauth(
280- 'existent', update_window=None)
281+ open_id, update_window=None)
282 self.assertIsNotNone(user)
283 self.assertFalse(created)
284 self.assert_user_data(user, sso_user_data)
285@@ -413,12 +413,12 @@
286 sso_user_data = self.mock_data_for_account(
287 displayname='Juan Hace', email='juan@email.com')
288 self.mock_get_data.return_value = sso_user_data
289- openid = full_claimed_id('existent')
290+ open_id = 'existent'
291 user = self.factory.makeUser(
292 first_name='John', last_name='Does', email='user@email.com',
293- open_id=openid)
294+ open_id=open_id)
295 logged_user, created = update_or_create_user_from_oauth(
296- 'existent', update_window=60 * 60 * 24)
297+ open_id, update_window=60 * 60 * 24)
298 self.assertIsNotNone(logged_user)
299 self.assertFalse(created)
300 current_sso_user_data = self.mock_data_for_account(
301@@ -429,12 +429,12 @@
302 sso_user_data = self.mock_data_for_account(
303 verified=False, displayname='Juan Hace', email='juan@email.com')
304 self.mock_get_data.return_value = sso_user_data
305- openid = full_claimed_id('existent')
306+ open_id = 'existent'
307 self.factory.makeUser(
308 first_name='John', last_name='Does', email='user@email.com',
309- open_id=openid)
310+ open_id=open_id)
311 user, created = update_or_create_user_from_oauth(
312- 'existent', update_window=None)
313+ open_id, update_window=None)
314 self.assertIsNotNone(user)
315 self.assertFalse(created)
316 self.assertEqual(user.get_full_name(), 'John Does')
317@@ -444,12 +444,12 @@
318 sso_user_data = self.mock_data_for_account(
319 displayname='Juan Hace', email='')
320 self.mock_get_data.return_value = sso_user_data
321- openid = full_claimed_id('existent')
322+ open_id = 'existent'
323 self.factory.makeUser(
324 first_name='John', last_name='Does', email='user@email.com',
325- open_id=openid)
326+ open_id=open_id)
327 user, created = update_or_create_user_from_oauth(
328- 'existent', update_window=None)
329+ open_id, update_window=None)
330 self.assertIsNotNone(user)
331 self.assertFalse(created)
332 self.assertEqual(user.get_full_name(), 'John Does')
333@@ -459,12 +459,12 @@
334 sso_user_data = self.mock_data_for_account(
335 displayname=None, email='new@email.com')
336 self.mock_get_data.return_value = sso_user_data
337- openid = full_claimed_id('existent')
338+ open_id = 'existent'
339 self.factory.makeUser(
340 first_name='John', last_name='Does', email='user@email.com',
341- open_id=openid)
342+ open_id=open_id)
343 user, created = update_or_create_user_from_oauth(
344- 'existent', update_window=None)
345+ open_id, update_window=None)
346 self.assertIsNotNone(user)
347 self.assertFalse(created)
348 self.assertEqual(user.get_full_name(), 'John Does')

Subscribers

People subscribed via source and target branches