Merge lp:~fgallina/rnr-server/click-reviews-return-useroid-as-username into lp:rnr-server
- click-reviews-return-useroid-as-username
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Fabián Ezequiel Gallina | ||||
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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Natalia Bidart (community) | Approve | ||
Ricardo Kirkner (community) | Approve | ||
Review via email: mp+257106@code.launchpad.net |
Commit message
Return the User openid as reviewer_username in clickreviews
Description of the change
Ricardo Kirkner (ricardokirkner) : | # |
Fabián Ezequiel Gallina (fgallina) : | # |
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?
Natalia Bidart (nataliabidart) : | # |
Ubuntu One Auto Pilot (otto-pilot) wrote : | # |
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/
/usr/lib/
Not deleting ./virtualenv/bin
New python executable in ./virtualenv/
Installing distribute.
Installing pip....
./virtualenv/
Requirement already satisfied (use --upgrade to upgrade): coverage in /usr/lib/
Downloading/
Downloading django_
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/
Requirement already satisfied (use --upgrade to upgrade): mock in /usr/lib/
Requirement already satisfied (use --upgrade to upgrade): paste==1.7.5.1 in /usr/lib/
Requirement already satisfied (use --upgrade to upgrade): pep8 in /usr/lib/
Requirement already satisfied (use --upgrade to upgrade): piston-mini-client in /usr/lib/
Requirement already satisfied (use --upgrade to upgrade): testtools in /usr/lib/
Downloading/
Running setup.py egg_info for package wsgi-intercept
Requirement already satisfied (use --upgrade to upgrade): django>=1.1 in /usr/lib/
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/
Creating test database for alias 'default' (':memory:')...
Syncing...
Creating tables ...
Creating table django_admin_log
Creating table auth_permission
Creating table auth_group_
Creating table auth_group
Creating table auth_user_groups
Creating table auth_user_
Creating table auth_user
Creating table django_content_type
Creating table django_session
Creating table django_site
Creating table south_migration
Creating table piston_nonce
Creating t...
- 304. By Fabián Ezequiel Gallina
-
Fix tests
Preview Diff
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') |
LGTM