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):
>          """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-24 12:31:07 +0000
> @@ -840,6 +842,80 @@
>          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)
> +        self.assertEqual(review.id, response.id)
> +        self.assertNotEqual(review.rating, response.rating)
> +        self.assertNotEqual(review.summary, response.summary)
> +        self.assertNotEqual(review.review_text, response.review_text)

I think you should be using assertEqual above to ensure the new values match the posted values. Actually, rather than checking the response, we should check the database model (as for all we know, looking at this test, the response may included the updated values without having saved it to the db).

Something like:
           review_reloaded = Review.objects.get(review.id)
	   self.assertEqual('new summary', review_reloaded.summary)

> +
> +    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)

Nice!

> +
> +    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)

date_created=old_time (no spaces)

> +        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)

indent

> +        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)
> +

We try to keep two blank lines between classes (PEP8)

>  class DeleteReviewHandlerTestCase(TestCaseWithFactory):
>      def _delete_review_id(self, review_id, user=None):
>          if user is None:
>
> === 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-24 12:31:07 +0000
> @@ -29,6 +29,7 @@
>      'RnRReviewStatsTestCase',
>      'RnRServerStatusTestCase',
>      'RnRSubmitReviewTestCase',
> +    'RnRModifyReviewTestCase',
>      'RnRDeleteReviewTestCase',
>      'UsefulnessAPITestCase',
>      ]
> @@ -272,6 +273,39 @@
>          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)

I'm guessing they were meant to line up?

> +
> +        self.assertEqual(review.id, response['id'])
> +        self.assertEqual(rating, response['rating'])
> +        self.assertEqual(summary, response['summary'])
> +        self.assertEqual(review_text, response['review_text'])

Excellent work Aaron!

> +
>
>  class RnRSubmitReviewTestCase(RnRTxBaseTestCase):
>
>