Merge lp:~aaronp/rnr-server/modify-only into lp:rnr-server

[Aaron Peachey]

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('/reviews/delete/%s/' % review_id, data={},
             scheme=AUTHENTICATED_API_SCHEME)
+
+ @validate('review_id', int)
+ @validate('rating', int)
+ @validate_pattern('summary', r'[^\n]+')
+ @validate_pattern('review_text', r'[^\n]+')
+ @returns_json
+ def modify_review(self, review_id, rating, summary, review_text):
+ """Modify an existing review"""
+ data = {'rating':rating, 'summary':summary, 'review_text':review_text}
+ return self._put('/reviews/modify/%s/' % review_id, data=data,
+ scheme=AUTHENTICATED_API_SCHEME)

[Michael Nelson (michael.nelson)]

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'
> --- src/reviewsapp/api/handlers.py 2011-05-05 14:32:04 +0000
> +++ src/reviewsapp/api/handlers.py 2011-05-24 12:31:07 +0000
> @@ -239,6 +240,32 @@
>
> return review
>
> +class ModifyReviewHandler(BaseHandler):
> + allowed_methods = ('PUT',)
> +
> + def update(self, request, review_id):
> + """Uses PUT verb to modify an existing review"""
> + review = get_object_or_404(Review, id=review_id, reviewer=request.user)
> +
> + # check review modify window has not closed before allowing update
> + time_diff = datetime.utcnow() - review.date_created
> + time_limit = timedelta(minutes=settings.MODIFY_WINDOW_MINUTES)
> + if time_diff > time_limit:
> + return HttpResponseForbidden("Time to modify review has expired")

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():
> + return HttpResponseForbidden("Can't modify review with pending moderation")

"This review can not be edited (pending moderation)."

> + else:
> + form = ReviewEditForm(request.POST, instance=review)
> + if not form.is_valid():
> + errors = dict((key, [unicode(v) for v in values])
> + for (key, values) in form.errors.items())
> + response = simplejson.dumps({'errors': errors})
> + return HttpResponseBadRequest(response)

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():
    return HttpResponse(form.errors_json())

Again, feel free to leave this - and tackle it only if it interests you.

> === modified file 'src/reviewsapp/models/reviews.py'
> --- src/reviewsapp/models/reviews.py 2011-05-06 01:17:56 +0000
> +++ src/reviewsapp/models/reviews.py 2011-05-24 12:31:07 +0000
> @@ -342,6 +342,15 @@
> def reviewer_displayname(self):
> return self.reviewer.get_full_name()
>
> + def is_awaiting_moderation(self):
> + """Return true if the review has any pending moderations"""
> + moderations = ReviewModeration.objects.filter(
> + review=self,
> + status=ReviewModeration.PENDING_STATUS).count()

Just a simplification, you should be able to do:
           moderations = self.reviewmoderation_set.filter(status=...).count()

> + if moderations == 0:
> + return False
> + return True
> +
> def delete(self):
> """...

[lp:~aaronp/rnr-server/modify-only]

188. By Aaron Peachey on 2011-05-26

* changes as per merge proposal feedback
* add JSONErrorMixin class to centralise returning
     of error json to multiple handlers that use it

[Aaron Peachey (aaronp)]

Thanks Michael. All changes made as per feedback.

review: Needs Resubmitting

[lp:~aaronp/rnr-server/modify-only]

189. By Aaron Peachey on 2011-05-28

update rnrclient test to cater for change to
rnrclient to return ReviewDetails insteadof json

[Aaron Peachey (aaronp)]

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('/reviews/delete/%s/' % review_id, data={},
             scheme=AUTHENTICATED_API_SCHEME)
+
+ @validate('review_id', int)
+ @validate('rating', int)
+ @validate_pattern('summary', r'[^\n]+')
+ @validate_pattern('review_text', r'[^\n]+')
+ @returns(ReviewDetails)
+ def modify_review(self, review_id, rating, summary, review_text):
+ """Modify an existing review"""
+ data = {'rating':rating, 'summary':summary, 'review_text':review_text}
+ return self._put('/reviews/modify/%s/' % review_id, data=data,
+ scheme=AUTHENTICATED_API_SCHEME)

Unmerged revisions

=== modified file 'django_project/config_dev/config/main.cfg'
--- django_project/config_dev/config/main.cfg	2011-04-21 14:55:23 +0000
+++ django_project/config_dev/config/main.cfg	2011-05-28 12:13:24 +0000
@@ -100,6 +100,7 @@
 token_cache_expiry_hours = 4
 reviewsapp_media_root = src/reviewsapp/media
 enable_artificial_oops = False
+modify_window_minutes = 120
 
 [sso_api]
 sso_api_service_root = https://login.staging.ubuntu.com/api/1.0
 
=== modified file 'django_project/config_dev/schema.py'
--- django_project/config_dev/schema.py	2011-04-20 16:47:48 +0000
+++ django_project/config_dev/schema.py	2011-05-28 12:13:24 +0000
@@ -57,6 +57,7 @@
     rnr.force_https_in_environment = BoolConfigOption()
     rnr.allow_multiple_reviews_for_testing = BoolConfigOption(default=False)
     rnr.enable_artificial_oops = BoolConfigOption()
+    rnr.modify_window_minutes = IntConfigOption()
 
     # Launchpad
     launchpad = ConfigSection()
 
=== modified file 'src/reviewsapp/api/handlers.py'
--- src/reviewsapp/api/handlers.py	2011-05-05 14:32:04 +0000
+++ src/reviewsapp/api/handlers.py	2011-05-28 12:13:24 +0000
@@ -26,6 +26,7 @@
     'ServerStatusHandler',
     'ShowReviewsHandler',
     'SubmitReviewHandler',
+    'ModifyReviewHandler',
     'ShowUsefulnessHandler',
     'DeleteReviewHandler',
 ]
@@ -44,7 +45,7 @@
 from django.utils import simplejson
 from piston.handler import BaseHandler
 
-from reviewsapp.forms import ReviewForm
+from reviewsapp.forms import ReviewForm, ReviewEditForm
 from reviewsapp.models import (
     Repository,
     Review,
@@ -219,11 +220,7 @@
         review = Review(reviewer=request.user)
         form = ReviewForm(request.data, instance=review)
         if not form.is_valid():
-            errors = dict((key, [unicode(v) for v in values])
-                for (key, values) in form.errors.items())
-            response = simplejson.dumps({'errors': errors})
-            return HttpResponseBadRequest(response)
-
+            return HttpResponseBadRequest(form.errors_json())
         review = form.save()
 
         # Automatically flag when appropriate.
@@ -239,11 +236,39 @@
 
         return review
 
+class ModifyReviewHandler(BaseHandler):
+    allowed_methods = ('PUT',)
+    
+    def update(self, request, review_id):
+        """Uses PUT verb to modify an existing review"""
+        review = get_object_or_404(Review, id=review_id, reviewer=request.user)
+        
+        
+        # check review modify window has not closed before allowing update
+        time_diff = datetime.utcnow() - review.date_created
+        time_limit = timedelta(minutes=settings.MODIFY_WINDOW_MINUTES)
+        if time_diff > time_limit:
+            return HttpResponseForbidden(
+                "This review cannot be edited (edit window expired).")
+        elif review.is_awaiting_moderation():
+            return HttpResponseForbidden(
+                "This review cannot be edited (pending moderation).")
+        else:
+            form = ReviewEditForm(request.POST, instance=review)
+            if not form.is_valid():
+                return HttpResponseBadRequest(form.errors_json())
+            else:
+                review = form.save()
+                review.softwareitem_in_repository.update_stats()
+                return review
+
+
 class DeleteReviewHandler(BaseHandler):
     allowed_methods = ('POST',)
     
     def create(self, request, review_id):
-        review = get_object_or_404(Review, id=review_id, reviewer=request.user, date_deleted=None)
+        review = get_object_or_404(Review, id=review_id, reviewer=request.user, 
+                                   date_deleted=None)
         return review.delete()
 
 class FlagReviewHandler(BaseHandler):
 
=== modified file 'src/reviewsapp/api/urls.py'
--- src/reviewsapp/api/urls.py	2011-05-05 12:35:58 +0000
+++ src/reviewsapp/api/urls.py	2011-05-28 12:13:24 +0000
@@ -31,7 +31,8 @@
     SubmitUsefulnessHandler,
     RetrieveReviewHandler,
     ShowUsefulnessHandler,
-    DeleteReviewHandler
+    DeleteReviewHandler,
+    ModifyReviewHandler,
 )
 from reviewsapp.auth import SSOOAuthAuthentication
 from reviewsapp.api.decorators import dont_vary
@@ -69,6 +70,8 @@
 # The following POST handlers have POST data and will not be cached.
 submit_review_resource = Resource(handler=SubmitReviewHandler,
     authentication=auth)
+modify_review_resource = Resource(handler=ModifyReviewHandler,
+    authentication=auth)
 flag_review_resource = Resource(handler=FlagReviewHandler,
     authentication=auth)
 submit_usefulness_resource = Resource(handler=SubmitUsefulnessHandler,
@@ -133,6 +136,11 @@
     # submits a new review for a given pkg/app
     #  POST /1.0/reviews/
     url(r'^1.0/reviews/$', submit_review_resource, name='rnr-api-reviews'),
+    
+    # modifies an existing review
+    # PUT /1.0/reviews/modify/100
+    url(r'^1.0/reviews/modify/(?P<review_id>\d+)/$',
+        modify_review_resource, name='rnr-api-modify-review'),
 
     # flag inappropriate reviews
     #  POST /1.0/reviews/1/+report-review/
 
=== modified file 'src/reviewsapp/forms.py'
--- src/reviewsapp/forms.py	2011-05-05 12:35:58 +0000
+++ src/reviewsapp/forms.py	2011-05-28 12:13:24 +0000
@@ -24,6 +24,7 @@
 __all__ = [
     'ReviewModerationFilterForm',
     'ReviewModerationModerateForm',
+    'ReviewEditForm',
     'ReviewForm',
     ]
 
@@ -31,6 +32,7 @@
 from django.conf import settings
 from django.db.models import Count
 from django.utils.translation import ugettext_lazy as _
+from django.utils import simplejson
 
 from reviewsapp.models import (
     Architecture,
@@ -43,6 +45,13 @@
 from reviewsapp.widgets import MultipleSubmitButton
 
 
+class JSONErrorMixin(object):
+    def errors_json(self):
+        errs = dict((key, [unicode(v) for v in values])
+                    for (key, values) in self.errors.items())
+        return simplejson.dumps({'errors': errs})
+
+
 class ReviewModerationModerateForm(forms.Form):
     """A simple form that can display itself."""
     moderation_text = forms.CharField(required=False)
@@ -53,7 +62,13 @@
         coerce=int)
 
 
-class ReviewForm(forms.ModelForm):
+class ReviewEditForm(forms.ModelForm, JSONErrorMixin):
+    class Meta:
+        model = Review
+        fields = ('summary', 'review_text', 'rating') 
+
+
+class ReviewForm(forms.ModelForm, JSONErrorMixin):
 
     # The following extra fields are used to calculate their model
     # equivalents.
 
=== modified file 'src/reviewsapp/models/reviews.py'
--- src/reviewsapp/models/reviews.py	2011-05-06 01:17:56 +0000
+++ src/reviewsapp/models/reviews.py	2011-05-28 12:13:24 +0000
@@ -342,6 +342,14 @@
     def reviewer_displayname(self):
         return self.reviewer.get_full_name()
     
+    def is_awaiting_moderation(self):
+        """Return true if the review has any pending moderations"""
+        moderations = self.reviewmoderation_set.filter(
+            status=ReviewModeration.PENDING_STATUS).count()
+        if moderations == 0:
+            return False
+        return True
+    
     def delete(self):
         """Delete a review submitted by the user"""
         self.hide = True
 
=== modified file 'src/reviewsapp/tests/test_handlers.py'
--- src/reviewsapp/tests/test_handlers.py	2011-05-06 01:17:56 +0000
+++ src/reviewsapp/tests/test_handlers.py	2011-05-28 12:13:24 +0000
@@ -26,6 +26,7 @@
     'ServerStatusHandlerTestCase',
     'ShowReviewsHandlerTestCase',
     'SubmitReviewHandlerTestCase',
+    'ModifyReviewHandlerTestCase',
     'UsefulnessHandlerTestCase',
     'ShowUsefulnessHandlerTestCase',
     'DeleteReviewHandlerTestCase',
@@ -53,7 +54,8 @@
     ShowReviewsHandler,
     SubmitUsefulnessHandler,
     ShowUsefulnessHandler,
-    DeleteReviewHandler
+    DeleteReviewHandler,
+    ModifyReviewHandler,
 )
 from reviewsapp.models import (
     Repository,
@@ -840,6 +842,86 @@
         self.assertEqual(httplib.OK, first.status_code)
         self.assertEqual(httplib.OK, second.status_code)
 
+class ModifyReviewHandlerTestCase(TestCaseWithFactory):
+    
+    post_data = {'rating':4, 'summary':'summary','review_text':'text'}
+    bad_post_data = {'summary':'summary','review_text':'text'}
+    oversize_post_data = {'rating': 4,'summary':'a'*82,'review_text':'text'}
+    
+    def _modify_review_id(self, review_id, post_data, user=None):
+        if user is None:
+            user = self.factory.makeUser()
+        request = Mock()
+        request.POST = post_data
+        request.user = user
+        handler = ModifyReviewHandler()
+        return handler.update(request, review_id)
+    
+    def test_modify_review(self):
+        review = self.factory.makeReview()
+        response = self._modify_review_id(review.id, self.post_data, 
+                                          review.reviewer)
+        review_reloaded = Review.objects.get(id=review.id)
+        self.assertEqual(review.id, response.id)
+        self.assertEqual(self.post_data['rating'], response.rating)
+        self.assertEqual(self.post_data['summary'], response.summary)
+        self.assertEqual(self.post_data['review_text'], response.review_text)
+        
+    def test_non_existent_review(self):
+        review = self.factory.makeReview()
+        self.assertRaises(Http404, self._modify_review_id, review.id+1, 
+            self.post_data, review.reviewer)
+    
+    def test_wrong_user(self):
+        review = self.factory.makeReview()
+        wrong_user = self.factory.makeUser(username='modifier')
+        self.assertRaises(Http404, self._modify_review_id, review.id, 
+            self.post_data, wrong_user)
+    
+    def test_invalid_data_modify_fails(self):
+        review = self.factory.makeReview()
+        #missing parameter
+        response = self._modify_review_id(review.id, self.bad_post_data, review.reviewer)
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+        #overlength field
+        response = self._modify_review_id(review.id, self.oversize_post_data, 
+                                          review.reviewer)
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+    
+    def test_review_awaiting_moderation_fails(self):
+        review_moderation = self.factory.makeReviewModeration()
+        review = review_moderation.review
+        response = self._modify_review_id(review.id, self.post_data, 
+                                          review.reviewer)
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+        
+    def test_modify_time_limits(self):
+        old_time = datetime.utcnow() - timedelta(settings.MODIFY_WINDOW_MINUTES + 60)
+        review = self.factory.makeReview(date_created=old_time)
+        response = self._modify_review_id(review.id, self.post_data,
+            review.reviewer)
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+    
+    def test_stats_update_on_modify(self):
+        software_item = self.factory.makeSoftwareItem(package_name='compiz-core',
+                                                      ratings_total=0, 
+                                                      ratings_average=0)
+        review = self.factory.makeReview(software_item=software_item, rating=2)
+        review2 = self.factory.makeReview(software_item=software_item, rating=4)
+        software_item = SoftwareItem.objects.get(id=software_item.id)
+        
+        # 2 reviews with ratings of 2 + 4 = 6 (average of 3)
+        self.assertEqual(2, software_item.ratings_total)
+        self.assertEqual(3, software_item.ratings_average)
+        
+        self._modify_review_id(review.id, self.post_data, review.reviewer)
+        software_item = SoftwareItem.objects.get(id=software_item.id)
+        
+        # 2 reviews with ratings of 4 + 4 = 8 (average of 4)
+        self.assertEqual(2, software_item.ratings_total)
+        self.assertEqual(4, software_item.ratings_average)
+
+
 class DeleteReviewHandlerTestCase(TestCaseWithFactory):
     def _delete_review_id(self, review_id, user=None):
         if user is None:
 
=== modified file 'src/reviewsapp/tests/test_models.py'
--- src/reviewsapp/tests/test_models.py	2011-03-17 14:29:54 +0000
+++ src/reviewsapp/tests/test_models.py	2011-05-28 12:13:24 +0000
@@ -106,6 +106,16 @@
         self.assertEqual(
                 u'lang: en, rating: 4, summary: \ufffdA summary\n'
                 u'\ufffdA review', unicode(review))
+                
+    def test_pending_moderation(self):
+        review = self.factory.makeReview()
+        result = review.is_awaiting_moderation()
+        self.assertEqual(False, result)
+        
+        review_moderation = self.factory.makeReviewModeration()
+        review = review_moderation.review
+        result = review.is_awaiting_moderation()
+        self.assertEqual(True, result)
 
 
 class ReviewFlagTestCase(TestCaseWithFactory):
 
=== modified file 'src/reviewsapp/tests/test_rnrclient.py'
--- src/reviewsapp/tests/test_rnrclient.py	2011-05-05 14:32:04 +0000
+++ src/reviewsapp/tests/test_rnrclient.py	2011-05-28 12:13:24 +0000
@@ -29,6 +29,7 @@
     'RnRReviewStatsTestCase',
     'RnRServerStatusTestCase',
     'RnRSubmitReviewTestCase',
+    'RnRModifyReviewTestCase',
     'RnRDeleteReviewTestCase',
     'UsefulnessAPITestCase',
     ]
@@ -248,30 +249,63 @@
         invalidate_paginated_reviews_for(review)
 
 class RnRDeleteReviewTestCase(RnRTxBaseTestCase):
-    
+
     def test_delete_review(self):
         user = self.factory.makeUser()
         review = self.factory.makeReview(reviewer=user)
         token, consumer = self.factory.makeOAuthTokenAndConsumer(user=user)
- 
+
         auth = OAuthAuthorizer(token.token, token.token_secret, consumer.key,
-            consumer.secret)       
+            consumer.secret)
 
         lp_verify_packagename_method = (
             'reviewsapp.utilities.WebServices.'
             'lp_verify_packagename_in_repository')
-        
+
         mock_verify_package = Mock()
         mock_verify_package.return_value = True
 
         api = RatingsAndReviewsAPI(auth=auth)
-        
+
         with patch(lp_verify_packagename_method, mock_verify_package):
             response = api.delete_review(review_id=review.id)
 
         self.assertEqual(review.id, response['id'])
         self.assertNotEqual(None, response['date_deleted'])
 
+class RnRModifyReviewTestCase(RnRTxBaseTestCase):
+   def test_modify_review(self):
+        user = self.factory.makeUser()
+        review = self.factory.makeReview(reviewer=user)
+        token, consumer = self.factory.makeOAuthTokenAndConsumer(user=user)
+
+        auth = OAuthAuthorizer(token.token, token.token_secret, consumer.key,
+            consumer.secret)
+
+        lp_verify_packagename_method = (
+            'reviewsapp.utilities.WebServices.'
+            'lp_verify_packagename_in_repository')
+
+        mock_verify_package = Mock()
+        mock_verify_package.return_value = True
+
+        api = RatingsAndReviewsAPI(auth=auth)
+
+        rating = 4
+        summary = 'summary'
+        review_text = 'review text'
+
+        with patch(lp_verify_packagename_method, mock_verify_package):
+            response = api.modify_review(review_id=review.id,
+                                         rating=rating,
+                                         summary=summary,
+                                         review_text=review_text)
+
+        self.assertEqual(review.id, response.id)
+        self.assertEqual(rating, response.rating)
+        self.assertEqual(summary, response.summary)
+        self.assertEqual(review_text, response.review_text)
+
 
 class RnRSubmitReviewTestCase(RnRTxBaseTestCase):