Merge lp:~aaronp/rnr-server/modify-only into lp:rnr-server
- modify-only
- Merge into trunk
Status: | Superseded | ||||
---|---|---|---|---|---|
Proposed branch: | lp:~aaronp/rnr-server/modify-only | ||||
Merge into: | lp:rnr-server | ||||
Diff against target: |
411 lines (+199/-15) 9 files modified
django_project/config_dev/config/main.cfg (+1/-0) django_project/config_dev/schema.py (+1/-0) src/reviewsapp/api/handlers.py (+32/-7) src/reviewsapp/api/urls.py (+9/-1) src/reviewsapp/forms.py (+16/-1) src/reviewsapp/models/reviews.py (+8/-0) src/reviewsapp/tests/test_handlers.py (+83/-1) src/reviewsapp/tests/test_models.py (+10/-0) src/reviewsapp/tests/test_rnrclient.py (+39/-5) |
||||
To merge this branch: | bzr merge lp:~aaronp/rnr-server/modify-only | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Aaron Peachey (community) | Needs Resubmitting | ||
Ratings and Reviews Developers | Pending | ||
Review via email: mp+62115@code.launchpad.net |
This proposal has been superseded by a proposal from 2011-05-28.
Commit message
Description of the change
adds functionality for user to modify their existing review within 120 minutes (config option) of submission.
Diff of associated rnrclient changes:
=== modified file 'rnrclient.py'
--- rnrclient.py 2011-05-06 06:35:36 +0000
+++ rnrclient.py 2011-05-24 12:07:16 +0000
@@ -174,3 +174,14 @@
"""Delete a review"""
return self._post(
+
+ @validate(
+ @validate('rating', int)
+ @validate_
+ @validate_
+ @returns_json
+ def modify_review(self, review_id, rating, summary, review_text):
+ """Modify an existing review"""
+ data = {'rating':rating, 'summary':summary, 'review_
+ return self._put(
+ scheme=
Michael Nelson (michael.nelson) wrote : | # |
- 188. By Aaron Peachey
-
* changes as per merge proposal feedback
* add JSONErrorMixin class to centralise returning
of error json to multiple handlers that use it
Aaron Peachey (aaronp) wrote : | # |
Thanks Michael. All changes made as per feedback.
- 189. By Aaron Peachey
-
update rnrclient test to cater for change to
rnrclient to return ReviewDetails insteadof json
Aaron Peachey (aaronp) wrote : | # |
New commit (r189) with slight change to test case for rnrclient because I realised when testing my changes in the client that we should return a ReviewDetails object instead of json, for consistency.
Therefore, diff for the rnrclient branch is now:
=== modified file 'rnrclient.py'
--- rnrclient.py 2011-05-06 06:35:36 +0000
+++ rnrclient.py 2011-05-28 12:06:31 +0000
@@ -174,3 +174,14 @@
"""Delete a review"""
return self._post(
+
+ @validate(
+ @validate('rating', int)
+ @validate_
+ @validate_
+ @returns(
+ def modify_review(self, review_id, rating, summary, review_text):
+ """Modify an existing review"""
+ data = {'rating':rating, 'summary':summary, 'review_
+ return self._put(
+ scheme=
Unmerged revisions
Preview Diff
1 | === modified file 'django_project/config_dev/config/main.cfg' | |||
2 | --- django_project/config_dev/config/main.cfg 2011-04-21 14:55:23 +0000 | |||
3 | +++ django_project/config_dev/config/main.cfg 2011-05-28 12:13:24 +0000 | |||
4 | @@ -100,6 +100,7 @@ | |||
5 | 100 | token_cache_expiry_hours = 4 | 100 | token_cache_expiry_hours = 4 |
6 | 101 | reviewsapp_media_root = src/reviewsapp/media | 101 | reviewsapp_media_root = src/reviewsapp/media |
7 | 102 | enable_artificial_oops = False | 102 | enable_artificial_oops = False |
8 | 103 | modify_window_minutes = 120 | ||
9 | 103 | 104 | ||
10 | 104 | [sso_api] | 105 | [sso_api] |
11 | 105 | sso_api_service_root = https://login.staging.ubuntu.com/api/1.0 | 106 | sso_api_service_root = https://login.staging.ubuntu.com/api/1.0 |
12 | 106 | 107 | ||
13 | === modified file 'django_project/config_dev/schema.py' | |||
14 | --- django_project/config_dev/schema.py 2011-04-20 16:47:48 +0000 | |||
15 | +++ django_project/config_dev/schema.py 2011-05-28 12:13:24 +0000 | |||
16 | @@ -57,6 +57,7 @@ | |||
17 | 57 | rnr.force_https_in_environment = BoolConfigOption() | 57 | rnr.force_https_in_environment = BoolConfigOption() |
18 | 58 | rnr.allow_multiple_reviews_for_testing = BoolConfigOption(default=False) | 58 | rnr.allow_multiple_reviews_for_testing = BoolConfigOption(default=False) |
19 | 59 | rnr.enable_artificial_oops = BoolConfigOption() | 59 | rnr.enable_artificial_oops = BoolConfigOption() |
20 | 60 | rnr.modify_window_minutes = IntConfigOption() | ||
21 | 60 | 61 | ||
22 | 61 | # Launchpad | 62 | # Launchpad |
23 | 62 | launchpad = ConfigSection() | 63 | launchpad = ConfigSection() |
24 | 63 | 64 | ||
25 | === modified file 'src/reviewsapp/api/handlers.py' | |||
26 | --- src/reviewsapp/api/handlers.py 2011-05-05 14:32:04 +0000 | |||
27 | +++ src/reviewsapp/api/handlers.py 2011-05-28 12:13:24 +0000 | |||
28 | @@ -26,6 +26,7 @@ | |||
29 | 26 | 'ServerStatusHandler', | 26 | 'ServerStatusHandler', |
30 | 27 | 'ShowReviewsHandler', | 27 | 'ShowReviewsHandler', |
31 | 28 | 'SubmitReviewHandler', | 28 | 'SubmitReviewHandler', |
32 | 29 | 'ModifyReviewHandler', | ||
33 | 29 | 'ShowUsefulnessHandler', | 30 | 'ShowUsefulnessHandler', |
34 | 30 | 'DeleteReviewHandler', | 31 | 'DeleteReviewHandler', |
35 | 31 | ] | 32 | ] |
36 | @@ -44,7 +45,7 @@ | |||
37 | 44 | from django.utils import simplejson | 45 | from django.utils import simplejson |
38 | 45 | from piston.handler import BaseHandler | 46 | from piston.handler import BaseHandler |
39 | 46 | 47 | ||
41 | 47 | from reviewsapp.forms import ReviewForm | 48 | from reviewsapp.forms import ReviewForm, ReviewEditForm |
42 | 48 | from reviewsapp.models import ( | 49 | from reviewsapp.models import ( |
43 | 49 | Repository, | 50 | Repository, |
44 | 50 | Review, | 51 | Review, |
45 | @@ -219,11 +220,7 @@ | |||
46 | 219 | review = Review(reviewer=request.user) | 220 | review = Review(reviewer=request.user) |
47 | 220 | form = ReviewForm(request.data, instance=review) | 221 | form = ReviewForm(request.data, instance=review) |
48 | 221 | if not form.is_valid(): | 222 | if not form.is_valid(): |
54 | 222 | errors = dict((key, [unicode(v) for v in values]) | 223 | return HttpResponseBadRequest(form.errors_json()) |
50 | 223 | for (key, values) in form.errors.items()) | ||
51 | 224 | response = simplejson.dumps({'errors': errors}) | ||
52 | 225 | return HttpResponseBadRequest(response) | ||
53 | 226 | |||
55 | 227 | review = form.save() | 224 | review = form.save() |
56 | 228 | 225 | ||
57 | 229 | # Automatically flag when appropriate. | 226 | # Automatically flag when appropriate. |
58 | @@ -239,11 +236,39 @@ | |||
59 | 239 | 236 | ||
60 | 240 | return review | 237 | return review |
61 | 241 | 238 | ||
62 | 239 | class ModifyReviewHandler(BaseHandler): | ||
63 | 240 | allowed_methods = ('PUT',) | ||
64 | 241 | |||
65 | 242 | def update(self, request, review_id): | ||
66 | 243 | """Uses PUT verb to modify an existing review""" | ||
67 | 244 | review = get_object_or_404(Review, id=review_id, reviewer=request.user) | ||
68 | 245 | |||
69 | 246 | |||
70 | 247 | # check review modify window has not closed before allowing update | ||
71 | 248 | time_diff = datetime.utcnow() - review.date_created | ||
72 | 249 | time_limit = timedelta(minutes=settings.MODIFY_WINDOW_MINUTES) | ||
73 | 250 | if time_diff > time_limit: | ||
74 | 251 | return HttpResponseForbidden( | ||
75 | 252 | "This review cannot be edited (edit window expired).") | ||
76 | 253 | elif review.is_awaiting_moderation(): | ||
77 | 254 | return HttpResponseForbidden( | ||
78 | 255 | "This review cannot be edited (pending moderation).") | ||
79 | 256 | else: | ||
80 | 257 | form = ReviewEditForm(request.POST, instance=review) | ||
81 | 258 | if not form.is_valid(): | ||
82 | 259 | return HttpResponseBadRequest(form.errors_json()) | ||
83 | 260 | else: | ||
84 | 261 | review = form.save() | ||
85 | 262 | review.softwareitem_in_repository.update_stats() | ||
86 | 263 | return review | ||
87 | 264 | |||
88 | 265 | |||
89 | 242 | class DeleteReviewHandler(BaseHandler): | 266 | class DeleteReviewHandler(BaseHandler): |
90 | 243 | allowed_methods = ('POST',) | 267 | allowed_methods = ('POST',) |
91 | 244 | 268 | ||
92 | 245 | def create(self, request, review_id): | 269 | def create(self, request, review_id): |
94 | 246 | review = get_object_or_404(Review, id=review_id, reviewer=request.user, date_deleted=None) | 270 | review = get_object_or_404(Review, id=review_id, reviewer=request.user, |
95 | 271 | date_deleted=None) | ||
96 | 247 | return review.delete() | 272 | return review.delete() |
97 | 248 | 273 | ||
98 | 249 | class FlagReviewHandler(BaseHandler): | 274 | class FlagReviewHandler(BaseHandler): |
99 | 250 | 275 | ||
100 | === modified file 'src/reviewsapp/api/urls.py' | |||
101 | --- src/reviewsapp/api/urls.py 2011-05-05 12:35:58 +0000 | |||
102 | +++ src/reviewsapp/api/urls.py 2011-05-28 12:13:24 +0000 | |||
103 | @@ -31,7 +31,8 @@ | |||
104 | 31 | SubmitUsefulnessHandler, | 31 | SubmitUsefulnessHandler, |
105 | 32 | RetrieveReviewHandler, | 32 | RetrieveReviewHandler, |
106 | 33 | ShowUsefulnessHandler, | 33 | ShowUsefulnessHandler, |
108 | 34 | DeleteReviewHandler | 34 | DeleteReviewHandler, |
109 | 35 | ModifyReviewHandler, | ||
110 | 35 | ) | 36 | ) |
111 | 36 | from reviewsapp.auth import SSOOAuthAuthentication | 37 | from reviewsapp.auth import SSOOAuthAuthentication |
112 | 37 | from reviewsapp.api.decorators import dont_vary | 38 | from reviewsapp.api.decorators import dont_vary |
113 | @@ -69,6 +70,8 @@ | |||
114 | 69 | # The following POST handlers have POST data and will not be cached. | 70 | # The following POST handlers have POST data and will not be cached. |
115 | 70 | submit_review_resource = Resource(handler=SubmitReviewHandler, | 71 | submit_review_resource = Resource(handler=SubmitReviewHandler, |
116 | 71 | authentication=auth) | 72 | authentication=auth) |
117 | 73 | modify_review_resource = Resource(handler=ModifyReviewHandler, | ||
118 | 74 | authentication=auth) | ||
119 | 72 | flag_review_resource = Resource(handler=FlagReviewHandler, | 75 | flag_review_resource = Resource(handler=FlagReviewHandler, |
120 | 73 | authentication=auth) | 76 | authentication=auth) |
121 | 74 | submit_usefulness_resource = Resource(handler=SubmitUsefulnessHandler, | 77 | submit_usefulness_resource = Resource(handler=SubmitUsefulnessHandler, |
122 | @@ -133,6 +136,11 @@ | |||
123 | 133 | # submits a new review for a given pkg/app | 136 | # submits a new review for a given pkg/app |
124 | 134 | # POST /1.0/reviews/ | 137 | # POST /1.0/reviews/ |
125 | 135 | url(r'^1.0/reviews/$', submit_review_resource, name='rnr-api-reviews'), | 138 | url(r'^1.0/reviews/$', submit_review_resource, name='rnr-api-reviews'), |
126 | 139 | |||
127 | 140 | # modifies an existing review | ||
128 | 141 | # PUT /1.0/reviews/modify/100 | ||
129 | 142 | url(r'^1.0/reviews/modify/(?P<review_id>\d+)/$', | ||
130 | 143 | modify_review_resource, name='rnr-api-modify-review'), | ||
131 | 136 | 144 | ||
132 | 137 | # flag inappropriate reviews | 145 | # flag inappropriate reviews |
133 | 138 | # POST /1.0/reviews/1/+report-review/ | 146 | # POST /1.0/reviews/1/+report-review/ |
134 | 139 | 147 | ||
135 | === modified file 'src/reviewsapp/forms.py' | |||
136 | --- src/reviewsapp/forms.py 2011-05-05 12:35:58 +0000 | |||
137 | +++ src/reviewsapp/forms.py 2011-05-28 12:13:24 +0000 | |||
138 | @@ -24,6 +24,7 @@ | |||
139 | 24 | __all__ = [ | 24 | __all__ = [ |
140 | 25 | 'ReviewModerationFilterForm', | 25 | 'ReviewModerationFilterForm', |
141 | 26 | 'ReviewModerationModerateForm', | 26 | 'ReviewModerationModerateForm', |
142 | 27 | 'ReviewEditForm', | ||
143 | 27 | 'ReviewForm', | 28 | 'ReviewForm', |
144 | 28 | ] | 29 | ] |
145 | 29 | 30 | ||
146 | @@ -31,6 +32,7 @@ | |||
147 | 31 | from django.conf import settings | 32 | from django.conf import settings |
148 | 32 | from django.db.models import Count | 33 | from django.db.models import Count |
149 | 33 | from django.utils.translation import ugettext_lazy as _ | 34 | from django.utils.translation import ugettext_lazy as _ |
150 | 35 | from django.utils import simplejson | ||
151 | 34 | 36 | ||
152 | 35 | from reviewsapp.models import ( | 37 | from reviewsapp.models import ( |
153 | 36 | Architecture, | 38 | Architecture, |
154 | @@ -43,6 +45,13 @@ | |||
155 | 43 | from reviewsapp.widgets import MultipleSubmitButton | 45 | from reviewsapp.widgets import MultipleSubmitButton |
156 | 44 | 46 | ||
157 | 45 | 47 | ||
158 | 48 | class JSONErrorMixin(object): | ||
159 | 49 | def errors_json(self): | ||
160 | 50 | errs = dict((key, [unicode(v) for v in values]) | ||
161 | 51 | for (key, values) in self.errors.items()) | ||
162 | 52 | return simplejson.dumps({'errors': errs}) | ||
163 | 53 | |||
164 | 54 | |||
165 | 46 | class ReviewModerationModerateForm(forms.Form): | 55 | class ReviewModerationModerateForm(forms.Form): |
166 | 47 | """A simple form that can display itself.""" | 56 | """A simple form that can display itself.""" |
167 | 48 | moderation_text = forms.CharField(required=False) | 57 | moderation_text = forms.CharField(required=False) |
168 | @@ -53,7 +62,13 @@ | |||
169 | 53 | coerce=int) | 62 | coerce=int) |
170 | 54 | 63 | ||
171 | 55 | 64 | ||
173 | 56 | class ReviewForm(forms.ModelForm): | 65 | class ReviewEditForm(forms.ModelForm, JSONErrorMixin): |
174 | 66 | class Meta: | ||
175 | 67 | model = Review | ||
176 | 68 | fields = ('summary', 'review_text', 'rating') | ||
177 | 69 | |||
178 | 70 | |||
179 | 71 | class ReviewForm(forms.ModelForm, JSONErrorMixin): | ||
180 | 57 | 72 | ||
181 | 58 | # The following extra fields are used to calculate their model | 73 | # The following extra fields are used to calculate their model |
182 | 59 | # equivalents. | 74 | # equivalents. |
183 | 60 | 75 | ||
184 | === modified file 'src/reviewsapp/models/reviews.py' | |||
185 | --- src/reviewsapp/models/reviews.py 2011-05-06 01:17:56 +0000 | |||
186 | +++ src/reviewsapp/models/reviews.py 2011-05-28 12:13:24 +0000 | |||
187 | @@ -342,6 +342,14 @@ | |||
188 | 342 | def reviewer_displayname(self): | 342 | def reviewer_displayname(self): |
189 | 343 | return self.reviewer.get_full_name() | 343 | return self.reviewer.get_full_name() |
190 | 344 | 344 | ||
191 | 345 | def is_awaiting_moderation(self): | ||
192 | 346 | """Return true if the review has any pending moderations""" | ||
193 | 347 | moderations = self.reviewmoderation_set.filter( | ||
194 | 348 | status=ReviewModeration.PENDING_STATUS).count() | ||
195 | 349 | if moderations == 0: | ||
196 | 350 | return False | ||
197 | 351 | return True | ||
198 | 352 | |||
199 | 345 | def delete(self): | 353 | def delete(self): |
200 | 346 | """Delete a review submitted by the user""" | 354 | """Delete a review submitted by the user""" |
201 | 347 | self.hide = True | 355 | self.hide = True |
202 | 348 | 356 | ||
203 | === modified file 'src/reviewsapp/tests/test_handlers.py' | |||
204 | --- src/reviewsapp/tests/test_handlers.py 2011-05-06 01:17:56 +0000 | |||
205 | +++ src/reviewsapp/tests/test_handlers.py 2011-05-28 12:13:24 +0000 | |||
206 | @@ -26,6 +26,7 @@ | |||
207 | 26 | 'ServerStatusHandlerTestCase', | 26 | 'ServerStatusHandlerTestCase', |
208 | 27 | 'ShowReviewsHandlerTestCase', | 27 | 'ShowReviewsHandlerTestCase', |
209 | 28 | 'SubmitReviewHandlerTestCase', | 28 | 'SubmitReviewHandlerTestCase', |
210 | 29 | 'ModifyReviewHandlerTestCase', | ||
211 | 29 | 'UsefulnessHandlerTestCase', | 30 | 'UsefulnessHandlerTestCase', |
212 | 30 | 'ShowUsefulnessHandlerTestCase', | 31 | 'ShowUsefulnessHandlerTestCase', |
213 | 31 | 'DeleteReviewHandlerTestCase', | 32 | 'DeleteReviewHandlerTestCase', |
214 | @@ -53,7 +54,8 @@ | |||
215 | 53 | ShowReviewsHandler, | 54 | ShowReviewsHandler, |
216 | 54 | SubmitUsefulnessHandler, | 55 | SubmitUsefulnessHandler, |
217 | 55 | ShowUsefulnessHandler, | 56 | ShowUsefulnessHandler, |
219 | 56 | DeleteReviewHandler | 57 | DeleteReviewHandler, |
220 | 58 | ModifyReviewHandler, | ||
221 | 57 | ) | 59 | ) |
222 | 58 | from reviewsapp.models import ( | 60 | from reviewsapp.models import ( |
223 | 59 | Repository, | 61 | Repository, |
224 | @@ -840,6 +842,86 @@ | |||
225 | 840 | self.assertEqual(httplib.OK, first.status_code) | 842 | self.assertEqual(httplib.OK, first.status_code) |
226 | 841 | self.assertEqual(httplib.OK, second.status_code) | 843 | self.assertEqual(httplib.OK, second.status_code) |
227 | 842 | 844 | ||
228 | 845 | class ModifyReviewHandlerTestCase(TestCaseWithFactory): | ||
229 | 846 | |||
230 | 847 | post_data = {'rating':4, 'summary':'summary','review_text':'text'} | ||
231 | 848 | bad_post_data = {'summary':'summary','review_text':'text'} | ||
232 | 849 | oversize_post_data = {'rating': 4,'summary':'a'*82,'review_text':'text'} | ||
233 | 850 | |||
234 | 851 | def _modify_review_id(self, review_id, post_data, user=None): | ||
235 | 852 | if user is None: | ||
236 | 853 | user = self.factory.makeUser() | ||
237 | 854 | request = Mock() | ||
238 | 855 | request.POST = post_data | ||
239 | 856 | request.user = user | ||
240 | 857 | handler = ModifyReviewHandler() | ||
241 | 858 | return handler.update(request, review_id) | ||
242 | 859 | |||
243 | 860 | def test_modify_review(self): | ||
244 | 861 | review = self.factory.makeReview() | ||
245 | 862 | response = self._modify_review_id(review.id, self.post_data, | ||
246 | 863 | review.reviewer) | ||
247 | 864 | review_reloaded = Review.objects.get(id=review.id) | ||
248 | 865 | self.assertEqual(review.id, response.id) | ||
249 | 866 | self.assertEqual(self.post_data['rating'], response.rating) | ||
250 | 867 | self.assertEqual(self.post_data['summary'], response.summary) | ||
251 | 868 | self.assertEqual(self.post_data['review_text'], response.review_text) | ||
252 | 869 | |||
253 | 870 | def test_non_existent_review(self): | ||
254 | 871 | review = self.factory.makeReview() | ||
255 | 872 | self.assertRaises(Http404, self._modify_review_id, review.id+1, | ||
256 | 873 | self.post_data, review.reviewer) | ||
257 | 874 | |||
258 | 875 | def test_wrong_user(self): | ||
259 | 876 | review = self.factory.makeReview() | ||
260 | 877 | wrong_user = self.factory.makeUser(username='modifier') | ||
261 | 878 | self.assertRaises(Http404, self._modify_review_id, review.id, | ||
262 | 879 | self.post_data, wrong_user) | ||
263 | 880 | |||
264 | 881 | def test_invalid_data_modify_fails(self): | ||
265 | 882 | review = self.factory.makeReview() | ||
266 | 883 | #missing parameter | ||
267 | 884 | response = self._modify_review_id(review.id, self.bad_post_data, review.reviewer) | ||
268 | 885 | self.assertEqual(httplib.BAD_REQUEST, response.status_code) | ||
269 | 886 | #overlength field | ||
270 | 887 | response = self._modify_review_id(review.id, self.oversize_post_data, | ||
271 | 888 | review.reviewer) | ||
272 | 889 | self.assertEqual(httplib.BAD_REQUEST, response.status_code) | ||
273 | 890 | |||
274 | 891 | def test_review_awaiting_moderation_fails(self): | ||
275 | 892 | review_moderation = self.factory.makeReviewModeration() | ||
276 | 893 | review = review_moderation.review | ||
277 | 894 | response = self._modify_review_id(review.id, self.post_data, | ||
278 | 895 | review.reviewer) | ||
279 | 896 | self.assertEqual(httplib.FORBIDDEN, response.status_code) | ||
280 | 897 | |||
281 | 898 | def test_modify_time_limits(self): | ||
282 | 899 | old_time = datetime.utcnow() - timedelta(settings.MODIFY_WINDOW_MINUTES + 60) | ||
283 | 900 | review = self.factory.makeReview(date_created=old_time) | ||
284 | 901 | response = self._modify_review_id(review.id, self.post_data, | ||
285 | 902 | review.reviewer) | ||
286 | 903 | self.assertEqual(httplib.FORBIDDEN, response.status_code) | ||
287 | 904 | |||
288 | 905 | def test_stats_update_on_modify(self): | ||
289 | 906 | software_item = self.factory.makeSoftwareItem(package_name='compiz-core', | ||
290 | 907 | ratings_total=0, | ||
291 | 908 | ratings_average=0) | ||
292 | 909 | review = self.factory.makeReview(software_item=software_item, rating=2) | ||
293 | 910 | review2 = self.factory.makeReview(software_item=software_item, rating=4) | ||
294 | 911 | software_item = SoftwareItem.objects.get(id=software_item.id) | ||
295 | 912 | |||
296 | 913 | # 2 reviews with ratings of 2 + 4 = 6 (average of 3) | ||
297 | 914 | self.assertEqual(2, software_item.ratings_total) | ||
298 | 915 | self.assertEqual(3, software_item.ratings_average) | ||
299 | 916 | |||
300 | 917 | self._modify_review_id(review.id, self.post_data, review.reviewer) | ||
301 | 918 | software_item = SoftwareItem.objects.get(id=software_item.id) | ||
302 | 919 | |||
303 | 920 | # 2 reviews with ratings of 4 + 4 = 8 (average of 4) | ||
304 | 921 | self.assertEqual(2, software_item.ratings_total) | ||
305 | 922 | self.assertEqual(4, software_item.ratings_average) | ||
306 | 923 | |||
307 | 924 | |||
308 | 843 | class DeleteReviewHandlerTestCase(TestCaseWithFactory): | 925 | class DeleteReviewHandlerTestCase(TestCaseWithFactory): |
309 | 844 | def _delete_review_id(self, review_id, user=None): | 926 | def _delete_review_id(self, review_id, user=None): |
310 | 845 | if user is None: | 927 | if user is None: |
311 | 846 | 928 | ||
312 | === modified file 'src/reviewsapp/tests/test_models.py' | |||
313 | --- src/reviewsapp/tests/test_models.py 2011-03-17 14:29:54 +0000 | |||
314 | +++ src/reviewsapp/tests/test_models.py 2011-05-28 12:13:24 +0000 | |||
315 | @@ -106,6 +106,16 @@ | |||
316 | 106 | self.assertEqual( | 106 | self.assertEqual( |
317 | 107 | u'lang: en, rating: 4, summary: \ufffdA summary\n' | 107 | u'lang: en, rating: 4, summary: \ufffdA summary\n' |
318 | 108 | u'\ufffdA review', unicode(review)) | 108 | u'\ufffdA review', unicode(review)) |
319 | 109 | |||
320 | 110 | def test_pending_moderation(self): | ||
321 | 111 | review = self.factory.makeReview() | ||
322 | 112 | result = review.is_awaiting_moderation() | ||
323 | 113 | self.assertEqual(False, result) | ||
324 | 114 | |||
325 | 115 | review_moderation = self.factory.makeReviewModeration() | ||
326 | 116 | review = review_moderation.review | ||
327 | 117 | result = review.is_awaiting_moderation() | ||
328 | 118 | self.assertEqual(True, result) | ||
329 | 109 | 119 | ||
330 | 110 | 120 | ||
331 | 111 | class ReviewFlagTestCase(TestCaseWithFactory): | 121 | class ReviewFlagTestCase(TestCaseWithFactory): |
332 | 112 | 122 | ||
333 | === modified file 'src/reviewsapp/tests/test_rnrclient.py' | |||
334 | --- src/reviewsapp/tests/test_rnrclient.py 2011-05-05 14:32:04 +0000 | |||
335 | +++ src/reviewsapp/tests/test_rnrclient.py 2011-05-28 12:13:24 +0000 | |||
336 | @@ -29,6 +29,7 @@ | |||
337 | 29 | 'RnRReviewStatsTestCase', | 29 | 'RnRReviewStatsTestCase', |
338 | 30 | 'RnRServerStatusTestCase', | 30 | 'RnRServerStatusTestCase', |
339 | 31 | 'RnRSubmitReviewTestCase', | 31 | 'RnRSubmitReviewTestCase', |
340 | 32 | 'RnRModifyReviewTestCase', | ||
341 | 32 | 'RnRDeleteReviewTestCase', | 33 | 'RnRDeleteReviewTestCase', |
342 | 33 | 'UsefulnessAPITestCase', | 34 | 'UsefulnessAPITestCase', |
343 | 34 | ] | 35 | ] |
344 | @@ -248,30 +249,63 @@ | |||
345 | 248 | invalidate_paginated_reviews_for(review) | 249 | invalidate_paginated_reviews_for(review) |
346 | 249 | 250 | ||
347 | 250 | class RnRDeleteReviewTestCase(RnRTxBaseTestCase): | 251 | class RnRDeleteReviewTestCase(RnRTxBaseTestCase): |
349 | 251 | 252 | ||
350 | 252 | def test_delete_review(self): | 253 | def test_delete_review(self): |
351 | 253 | user = self.factory.makeUser() | 254 | user = self.factory.makeUser() |
352 | 254 | review = self.factory.makeReview(reviewer=user) | 255 | review = self.factory.makeReview(reviewer=user) |
353 | 255 | token, consumer = self.factory.makeOAuthTokenAndConsumer(user=user) | 256 | token, consumer = self.factory.makeOAuthTokenAndConsumer(user=user) |
355 | 256 | 257 | ||
356 | 257 | auth = OAuthAuthorizer(token.token, token.token_secret, consumer.key, | 258 | auth = OAuthAuthorizer(token.token, token.token_secret, consumer.key, |
358 | 258 | consumer.secret) | 259 | consumer.secret) |
359 | 259 | 260 | ||
360 | 260 | lp_verify_packagename_method = ( | 261 | lp_verify_packagename_method = ( |
361 | 261 | 'reviewsapp.utilities.WebServices.' | 262 | 'reviewsapp.utilities.WebServices.' |
362 | 262 | 'lp_verify_packagename_in_repository') | 263 | 'lp_verify_packagename_in_repository') |
364 | 263 | 264 | ||
365 | 264 | mock_verify_package = Mock() | 265 | mock_verify_package = Mock() |
366 | 265 | mock_verify_package.return_value = True | 266 | mock_verify_package.return_value = True |
367 | 266 | 267 | ||
368 | 267 | api = RatingsAndReviewsAPI(auth=auth) | 268 | api = RatingsAndReviewsAPI(auth=auth) |
370 | 268 | 269 | ||
371 | 269 | with patch(lp_verify_packagename_method, mock_verify_package): | 270 | with patch(lp_verify_packagename_method, mock_verify_package): |
372 | 270 | response = api.delete_review(review_id=review.id) | 271 | response = api.delete_review(review_id=review.id) |
373 | 271 | 272 | ||
374 | 272 | self.assertEqual(review.id, response['id']) | 273 | self.assertEqual(review.id, response['id']) |
375 | 273 | self.assertNotEqual(None, response['date_deleted']) | 274 | self.assertNotEqual(None, response['date_deleted']) |
376 | 274 | 275 | ||
377 | 276 | class RnRModifyReviewTestCase(RnRTxBaseTestCase): | ||
378 | 277 | def test_modify_review(self): | ||
379 | 278 | user = self.factory.makeUser() | ||
380 | 279 | review = self.factory.makeReview(reviewer=user) | ||
381 | 280 | token, consumer = self.factory.makeOAuthTokenAndConsumer(user=user) | ||
382 | 281 | |||
383 | 282 | auth = OAuthAuthorizer(token.token, token.token_secret, consumer.key, | ||
384 | 283 | consumer.secret) | ||
385 | 284 | |||
386 | 285 | lp_verify_packagename_method = ( | ||
387 | 286 | 'reviewsapp.utilities.WebServices.' | ||
388 | 287 | 'lp_verify_packagename_in_repository') | ||
389 | 288 | |||
390 | 289 | mock_verify_package = Mock() | ||
391 | 290 | mock_verify_package.return_value = True | ||
392 | 291 | |||
393 | 292 | api = RatingsAndReviewsAPI(auth=auth) | ||
394 | 293 | |||
395 | 294 | rating = 4 | ||
396 | 295 | summary = 'summary' | ||
397 | 296 | review_text = 'review text' | ||
398 | 297 | |||
399 | 298 | with patch(lp_verify_packagename_method, mock_verify_package): | ||
400 | 299 | response = api.modify_review(review_id=review.id, | ||
401 | 300 | rating=rating, | ||
402 | 301 | summary=summary, | ||
403 | 302 | review_text=review_text) | ||
404 | 303 | |||
405 | 304 | self.assertEqual(review.id, response.id) | ||
406 | 305 | self.assertEqual(rating, response.rating) | ||
407 | 306 | self.assertEqual(summary, response.summary) | ||
408 | 307 | self.assertEqual(review_text, response.review_text) | ||
409 | 308 | |||
410 | 275 | 309 | ||
411 | 276 | class RnRSubmitReviewTestCase(RnRTxBaseTestCase): | 310 | class RnRSubmitReviewTestCase(RnRTxBaseTestCase): |
412 | 277 | 311 |
Excellent work Aaron - as far as I can see, you've tested everything very well. I've only got a few small simplifications, formatting points or wording suggestions below. See what you think.
> === modified file 'src/reviewsapp /api/handlers. py' api/handlers. py 2011-05-05 14:32:04 +0000 api/handlers. py 2011-05-24 12:31:07 +0000 dler(BaseHandle r): or_404( Review, id=review_id, reviewer= request. user) minutes= settings. MODIFY_ WINDOW_ MINUTES) bidden( "Time to modify review has expired")
> --- src/reviewsapp/
> +++ src/reviewsapp/
> @@ -239,6 +240,32 @@
>
> return review
>
> +class ModifyReviewHan
> + allowed_methods = ('PUT',)
> +
> + def update(self, request, review_id):
> + """Uses PUT verb to modify an existing review"""
> + review = get_object_
> +
> + # check review modify window has not closed before allowing update
> + time_diff = datetime.utcnow() - review.date_created
> + time_limit = timedelta(
> + if time_diff > time_limit:
> + return HttpResponseFor
Hrm - I wonder if we can get a balance of simple language with the actual reason
in parenthesis? Maybe:
"This review can not be edited (edit window expired)."
> + elif review. is_awaiting_ moderation( ): bidden( "Can't modify review with pending moderation")
> + return HttpResponseFor
"This review can not be edited (pending moderation)."
> + else: request. POST, instance=review) items() ) dumps({ 'errors' : errors}) Request( response)
> + form = ReviewEditForm(
> + if not form.is_valid():
> + errors = dict((key, [unicode(v) for v in values])
> + for (key, values) in form.errors.
> + response = simplejson.
> + return HttpResponseBad
I noticed that we're already doing this for the SubmitReviewHandler - it'd be
nice to DRY it up. Feel free to leave it, but if you're keen, we could add an
'errors_json' to our forms. We'd only need to define it once if we use a mixin
like:
class JSONErrorMixin( object) :
def errors_json():
...
and then the forms that use this would simply do:
class ReviewEditForm( forms.ModelForm , JSONErrorMixin):
...
Then the above handler could be simplified to:
if not form.is_valid(): form.errors_ json())
return HttpResponse(
Again, feel free to leave this - and tackle it only if it interests you.
> === modified file 'src/reviewsapp /models/ reviews. py' models/ reviews. py 2011-05-06 01:17:56 +0000 models/ reviews. py 2011-05-24 12:31:07 +0000 displayname( self): get_full_ name() moderation( self): n.objects. filter( ReviewModeratio n.PENDING_ STATUS) .count( )
> --- src/reviewsapp/
> +++ src/reviewsapp/
> @@ -342,6 +342,15 @@
> def reviewer_
> return self.reviewer.
>
> + def is_awaiting_
> + """Return true if the review has any pending moderations"""
> + moderations = ReviewModeratio
> + review=self,
> + status=
Just a simplification, you should be able to do:
moderations = self.reviewmode ration_ set.filter( status= ...).count( )
> + if moderations == 0:
> + return False
> + return True
> +
> def delete(self):
> """...