Merge lp:~fgallina/rnr-server/edit-click-review-handler into lp:rnr-server

[Fabián Ezequiel Gallina]

clickreviews: Added review edit endpoint

PUT click/api/1.0/reviews/<review_id>/ with summary, review_text and
rating in request.data will modify a review provided it has no
moderations pending and it was created within the
`MODIFY_WINDOW_MINUTES` window.

[lp:~fgallina/rnr-server/edit-click-review-handler]

294. By Fabián Ezequiel Gallina on 2015-03-09

Merged trunk into edit-click-review-handler.

[lp:~fgallina/rnr-server/edit-click-review-handler]

295. By Fabián Ezequiel Gallina on 2015-03-10

Fixes per review

296. By Fabián Ezequiel Gallina on 2015-03-10

Small cleanup

[Natalia Bidart (nataliabidart)]

Looks good!

review: Approve

=== modified file 'src/clickreviews/api/handlers.py'
--- src/clickreviews/api/handlers.py	2015-02-04 19:54:12 +0000
+++ src/clickreviews/api/handlers.py	2015-03-10 16:48:49 +0000
@@ -18,6 +18,7 @@
     ClickPackageReviewFlagForm,
     ClickPackageReviewForm,
     ClickPackageReviewModerationStatusForm,
+    ClickPackageReviewUpdateForm,
 )
 from core.api.pagination import make_paginated_response
 from core.models import BaseModeration
@@ -65,7 +66,7 @@
 
 
 class ClickReviewHandler(BaseHandler):
-    allowed_methods = ('POST',)
+    allowed_methods = ('POST', 'PUT')
     anonymous = ClickReviewAnonymousHandler
     fields = REVIEW_FIELDS
 
@@ -81,6 +82,21 @@
 
         return review
 
+    def update(self, request, review_id):
+        """Use PUT verb to modify an existing review."""
+        review = get_object_or_404(
+            ClickPackageReview, id=review_id, reviewer=request.user)
+
+        form = ClickPackageReviewUpdateForm(request.data, instance=review)
+        if form.is_valid():
+            review = form.save()
+            # XXX: Refactor this to be triggered on review save/delete.
+            review.click_package.update_stats()
+            response = review
+        else:
+            response = HttpResponseBadRequest(form.errors_json())
+        return response
+
 
 class ClickReviewsStatsHandler(BaseHandler):
     allowed_methods = ('GET',)

=== modified file 'src/clickreviews/api/urls.py'
--- src/clickreviews/api/urls.py	2014-10-07 19:04:20 +0000
+++ src/clickreviews/api/urls.py	2015-03-10 16:48:49 +0000
@@ -27,6 +27,7 @@
 
 review_resource = CSRFExemptResource(handler=ClickReviewHandler,
                                      authentication=auth)
+review_resource.callmap['PATCH'] = 'patch'
 review_resource = never_cache(review_resource)
 
 # Although the review stats resource will be cached by the cache
@@ -76,6 +77,8 @@
 urlpatterns = patterns(
     '',
     url(r'^1.0/reviews/$', review_resource, name='click-api-reviews'),
+    url(r'^1.0/reviews/(?P<review_id>\d+)/$',
+        review_resource, name='click-api-reviews'),
     url(r'^1.0/review-stats/$', reviews_stats_resource,
         name='click-api-reviews-stats'),
     url(r'^1.0/review-stats/(?P<package_name>[^/]+)/$',

=== modified file 'src/clickreviews/forms.py'
--- src/clickreviews/forms.py	2014-10-07 19:04:20 +0000
+++ src/clickreviews/forms.py	2015-03-10 16:48:49 +0000
@@ -1,6 +1,7 @@
 from django import forms
 from django.conf import settings
 from django.core import validators
+from django.utils.timezone import now
 from django.utils.translation import ugettext as _
 
 from clickreviews.models import (
@@ -86,6 +87,27 @@
                 package_name))
 
 
+class ClickPackageReviewUpdateForm(forms.ModelForm, JSONErrorMixin):
+
+    can_modify = forms.BooleanField(required=False, widget=forms.HiddenInput)
+
+    def clean_can_modify(self):
+        review = self.instance
+        time_diff = (now() - review.date_created).seconds
+        time_limit = settings.MODIFY_WINDOW_MINUTES * 60
+        if time_diff > time_limit:
+            raise forms.ValidationError(
+                "This review cannot be edited (edit window expired).")
+        elif review.is_awaiting_moderation():
+            raise forms.ValidationError(
+                "This review cannot be edited (pending moderation).")
+        return True
+
+    class Meta:
+        model = ClickPackageReview
+        fields = ('summary', 'review_text', 'rating')
+
+
 class ClickPackageReviewFilterForm(forms.Form, JSONErrorMixin):
     package_name = forms.CharField(max_length=255)
 

=== modified file 'src/clickreviews/models.py'
--- src/clickreviews/models.py	2014-10-09 13:27:06 +0000
+++ src/clickreviews/models.py	2015-03-10 16:48:49 +0000
@@ -70,6 +70,10 @@
     def reviewer_displayname(self):
         return self.reviewer.get_full_name()
 
+    def is_awaiting_moderation(self):
+        """Return true if the review has any pending moderations."""
+        return self.moderation_set.pending().exists()
+
 
 # Q) So, what's this tonguetwister all about?  A) In click packages,
 # not only reviews can be flagged for moderation but also the click
@@ -79,7 +83,8 @@
 class ClickPackageModeration(BaseModeration):
     """Track moderation of Click Packages."""
 
-    package = models.ForeignKey(ClickPackage)
+    package = models.ForeignKey(
+        ClickPackage, related_name='moderation_set')
 
     objects = ModerationManager()
 
@@ -122,7 +127,8 @@
 class ClickPackageReviewModeration(BaseModeration):
     """Track moderation of Click Package reviews."""
 
-    review = models.ForeignKey(ClickPackageReview)
+    review = models.ForeignKey(
+        ClickPackageReview, related_name='moderation_set')
 
     objects = ModerationManager()
 

=== modified file 'src/clickreviews/tests/test_forms.py'
--- src/clickreviews/tests/test_forms.py	2014-10-07 19:04:20 +0000
+++ src/clickreviews/tests/test_forms.py	2015-03-10 16:48:49 +0000
@@ -1,11 +1,16 @@
+import datetime
 import mock
 
+from django.conf import settings
+from django.utils.timezone import now
+
 from clickreviews.forms import (
     ClickPackageFlagForm,
     ClickPackageModerationStatusForm,
     ClickPackageReviewFlagForm,
     ClickPackageReviewForm,
     ClickPackageReviewModerationStatusForm,
+    ClickPackageReviewUpdateForm,
 )
 from clickreviews.models import (
     ClickPackage,
@@ -94,6 +99,61 @@
         self.assertEqual(True, form.is_valid())
 
 
+class ClickPackageReviewUpdateFormTestCase(TestCaseWithFactory):
+
+    def setUp(self):
+        super(ClickPackageReviewUpdateFormTestCase, self).setUp()
+        self.valid_data = {
+            'summary': 'New summary',
+            'review_text': 'New review body',
+            'rating': 5}
+
+    def test_valid(self):
+        review = self.factory.makeClickPackageReview(rating=3)
+        form = ClickPackageReviewUpdateForm(self.valid_data, instance=review)
+        self.assertTrue(form.is_valid())
+        review = form.save()
+        for key, value in self.valid_data.items():
+            self.assertEqual(getattr(review, key), self.valid_data[key])
+
+    def assert_field_required(self, field_name):
+        del self.valid_data[field_name]
+        review = self.factory.makeClickPackageReview()
+        form = ClickPackageReviewUpdateForm(self.valid_data, instance=review)
+        self.assertFalse(form.is_valid())
+        self.assertIn(field_name, form.errors)
+        self.assertIn('This field is required.', form.errors[field_name])
+
+    def test_summary_required(self):
+        self.assert_field_required('summary')
+
+    def test_review_text_required(self):
+        self.assert_field_required('review_text')
+
+    def test_rating_required(self):
+        self.assert_field_required('rating')
+
+    def test_modify_window_expired(self):
+        created = now() - datetime.timedelta(
+            minutes=settings.MODIFY_WINDOW_MINUTES, seconds=1)
+        review = self.factory.makeClickPackageReview(date_created=created)
+        form = ClickPackageReviewUpdateForm(self.valid_data, instance=review)
+        self.assertFalse(form.is_valid())
+        self.assertIn('can_modify', form.errors)
+        self.assertIn("This review cannot be edited (edit window expired).",
+                      form.errors['can_modify'])
+
+    def test_is_awaiting_moderation(self):
+        review = self.factory.makeClickPackageReview()
+        self.factory.makeClickPackageReviewModeration(review=review)
+        self.assertTrue(review.is_awaiting_moderation())
+        form = ClickPackageReviewUpdateForm(self.valid_data, instance=review)
+        self.assertFalse(form.is_valid())
+        self.assertIn('can_modify', form.errors)
+        self.assertIn("This review cannot be edited (pending moderation).",
+                      form.errors['can_modify'])
+
+
 class BaseClickPackageFlagFormTestCase(TestCaseWithFactory):
 
     form = None

=== modified file 'src/clickreviews/tests/test_handlers.py'
--- src/clickreviews/tests/test_handlers.py	2015-02-04 19:54:12 +0000
+++ src/clickreviews/tests/test_handlers.py	2015-03-10 16:48:49 +0000
@@ -26,17 +26,18 @@
 from core.tests.helpers import SSOTestCaseMixin
 
 
-class ClickReviewsHandlerPostTestCase(TestCaseWithFactory):
+class ClickReviewsHandlerTestCase(TestCaseWithFactory):
 
     def setUp(self):
-        super(ClickReviewsHandlerPostTestCase, self).setUp()
+        super(ClickReviewsHandlerTestCase, self).setUp()
 
         # Ensure we don't hit the network for our tests.
         is_authed_fn = ('core.api.auth.SSOOAuthAuthentication.'
                         'is_authenticated')
-        patcher = mock.patch(is_authed_fn, new_callable=mock.Mock)
+        patcher = mock.patch(
+            is_authed_fn, new_callable=mock.Mock, return_value=True)
         self.addCleanup(patcher.stop)
-        patcher.start().return_value = True
+        self.mock_is_authenticated = patcher.start()
 
         click_verify_fn = ('clickreviews.forms.'
                            'verify_click_package_published')
@@ -132,19 +133,13 @@
         self.assertEqual(1, click_package.ratings_total)
         self.assertEqual(5, click_package.ratings_average)
 
-
-class ClickReviewsHandlerGetTestCase(TestCaseWithFactory):
-
     def _get_reviews(self, data=None):
         url = reverse('click-api-reviews')
         kwargs = {}
         if data is not None:
             kwargs['data'] = data
-        is_authenticated_method = (
-            'core.api.auth.SSOOAuthAuthentication.is_authenticated')
-        with mock.patch(is_authenticated_method) as mock_is_authenticated:
-            mock_is_authenticated.return_value = False
-            return self.client.get(url, **kwargs)
+        self.mock_is_authenticated.return_value = False
+        return self.client.get(url, **kwargs)
 
     def test_get_multiple_reviews_for_package(self):
         package = self.factory.makeClickPackage()
@@ -252,6 +247,44 @@
             'package_name': package.package_name,
         })
 
+    def _update_review(self, review, data, user=None):
+        # Post a review as an authenticated user.
+        url = reverse('click-api-reviews', args=(review.pk,))
+        if user is None:
+            user = review.reviewer
+        self.client.login(username=user.username, password='test')
+        response = self.client.put(
+            url, data=json.dumps(data), content_type='application/json')
+        return response
+
+    def test_update_review(self):
+        review = self.factory.makeClickPackageReview()
+        data = {'summary': 'New summary',
+                'review_text': 'Review text',
+                'rating': 5}
+        response = self._update_review(review, data)
+        self.assertEqual(response.status_code, httplib.OK)
+        review = json.loads(response.content)
+        for key, value in data.items():
+            self.assertEqual(review[key], data[key])
+
+    def test_update_review_missing_data(self):
+        review = self.factory.makeClickPackageReview()
+        response = self._update_review(review, {})
+        self.assertEqual(response.status_code, httplib.BAD_REQUEST)
+        errors = json.loads(response.content)
+        expected = {'errors':
+                    {'rating': ['This field is required.'],
+                     'review_text': ['This field is required.'],
+                     'summary': ['This field is required.']}}
+        self.assertEqual(errors, expected)
+
+    def test_update_review_wrong_user(self):
+        review = self.factory.makeClickPackageReview()
+        user = self.factory.makeUser()
+        response = self._update_review(review, {}, user)
+        self.assertEqual(response.status_code, httplib.NOT_FOUND)
+
 
 class ClickReviewsStatsHandlerTestCase(TestCaseWithFactory):
 

=== modified file 'src/clickreviews/tests/test_models.py'
--- src/clickreviews/tests/test_models.py	2014-10-07 19:04:20 +0000
+++ src/clickreviews/tests/test_models.py	2015-03-10 16:48:49 +0000
@@ -101,6 +101,24 @@
         self.assertFalse(form.is_valid(), 'form should fail validation')
         self.assertTrue(field in form.errors)
 
+    def test_is_awaiting_moderation_no_moderations(self):
+        review = self.factory.makeClickPackageReview()
+        review.moderation_set.all().delete()
+        self.assertFalse(review.is_awaiting_moderation())
+
+    def test_is_awaiting_moderation_with_pending_moderations(self):
+        review = self.factory.makeClickPackageReview()
+        self.factory.makeClickPackageReviewModeration(review=review)
+        self.assertTrue(review.is_awaiting_moderation())
+
+    def test_is_awaiting_moderation_with_no_pending_moderations(self):
+        review = self.factory.makeClickPackageReview()
+        self.factory.makeClickPackageReviewModeration(
+            review=review, status=ClickPackageReviewModeration.APPROVED_STATUS)
+        self.factory.makeClickPackageReviewModeration(
+            review=review, status=ClickPackageReviewModeration.REJECTED_STATUS)
+        self.assertFalse(review.is_awaiting_moderation())
+
 
 class BaseClickPackageModerationTestCase(TestCaseWithFactory):