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

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):
> """...

Thanks Michael. All changes made as per feedback.

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)