On Mon, May 2, 2011 at 3:21 PM, Aaron Peachey <email address hidden> wrote:
Aaron Peachey has proposed merging lp:~aaronp/rnr-server/delete-review into lp:rnr-server.

Requested reviews:
 Ratings and Reviews Developers (rnr-developers)

Hey Aaron! Thanks for the branch!


For more details, see:
https://code.launchpad.net/~aaronp/rnr-server/delete-review/+merge/59658

add's functionality for a user to self-delete a review they have submitted.

Great!
 
Adds a new field in the Review model to hold a date/time of deletion (where null = not deleted) and uses existing hide flag.

A thought - can the review be unhidden by some other process? ie., something like:

1) users creates review, 
2) it gets flagged (and is hidden)
3) user deletes review (deletion tiime set, already hidden), but still in moderation queue
4) moderator marks the review as OK (and it is unhidden, even though deletion timestamp is set).

I guess we just need to ensure that these reviews don't appear for moderation? What do you think?

Also, whenever we change the database models, we need to create a new migration so that the production databases can be updated. Normally this is as simple as `fab manage:'migrate reviewsapp add_deletion_timestamp --auto'`. But if it's a hassle, I can add that on to your branch afterwards.

Some other thoughts below.
 
--

https://code.launchpad.net/~aaronp/rnr-server/delete-review/+merge/59658
Your team Ratings and Reviews Developers is requested to review the proposed merge of lp:~aaronp/rnr-server/delete-review into lp:rnr-server.

=== modified file 'src/reviewsapp/api/handlers.py'
--- src/reviewsapp/api/handlers.py      2011-04-12 10:43:30 +0000
+++ src/reviewsapp/api/handlers.py      2011-05-02 13:21:35 +0000
@@ -27,6 +27,7 @@
    'ShowReviewsHandler',
    'SubmitReviewHandler',
    'ShowUsefulnessHandler',
+    'DeleteReviewHandler',
 ]

 import urllib
@@ -37,6 +38,7 @@
    HttpResponse,
    HttpResponseBadRequest,
    HttpResponseNotFound,
+    HttpResponseForbidden,
    )
 from django.shortcuts import get_object_or_404
 from django.utils import simplejson
@@ -236,6 +238,23 @@

        return review

+class DeleteReviewHandler(BaseHandler):
+    allowed_methods = ('POST',)

Unrelated to your branch (as we already seem to have these multiple handlers), but we should have been able to have a single handler resource that takes different methods... ie, rather than creating a new handler here and using a POST, we could use I've not used the functionality yet, but we should really be able to have a siingle ReviewHandler that uses create (POST), read (GET), update (PUT), etc.

Depending how you feel about it, I might even have a go at renaming the SubmitReviewHandler to do this when you've finished, or let me know if it's something that interests you too.
 
+
+    def create(self, request, review_id):
+        try:
+            review = Review.objects.get(id=review_id)
+        except Review.DoesNotExist:
+            return HttpResponseNotFound('No matching review found')

We should be able to simplify the above with a single:

get_object_or_404(Review, id=review_id)


+
+        try:
+            deleted = review.delete(request.user)
+            return deleted
+        except Review.ReviewNotUsersOwn:
+            return HttpResponseForbidden('Can only delete own reviews')

I notice you've put the security in the model's delete method itself (raising ReviewNotUsersOwn etc.). For consistency, we should really be doing that here in the handler - the model code shouldn't need to care about view level authentication details. So we could update our above get_object_or_404 to:

get_object_or_404(Review, id=review_id, reviewer=request.user)

which will ensure that only the reviewer can delete their own reviews, otherwise they'll see a 404, removing the need for the try/except here?

+        except Review.ReviewAlreadyDeleted:
+            return HttpResponseForbidden('Review already deleted')
+


Similarly, we can handle the ReviewAlreadyDeleted situation (and no longer need the exception), by updating our get_object_or_404 to:

get_object_or_404(Review, id=review_id, reviewer=request.user, date_deleted=None)

here in the handler.

So the complete handler can simply be something like:
{{{
    review = get_object_or_404(Review, id=review_id, reviewer=request.user, date_deleted=None)
    return review.delete()
}}}
 
 class FlagReviewHandler(BaseHandler):
    allowed_methods = ('POST',)

=== modified file 'src/reviewsapp/api/urls.py'
--- src/reviewsapp/api/urls.py  2011-03-23 16:02:43 +0000
+++ src/reviewsapp/api/urls.py  2011-05-02 13:21:35 +0000
@@ -30,7 +30,8 @@
    SubmitReviewHandler,
    SubmitUsefulnessHandler,
    RetrieveReviewHandler,
-    ShowUsefulnessHandler
+    ShowUsefulnessHandler,
+    DeleteReviewHandler
 )
 from reviewsapp.auth import SSOOAuthAuthentication
 from reviewsapp.api.decorators import dont_vary
@@ -72,6 +73,8 @@
    authentication=auth)
 submit_usefulness_resource = Resource(handler=SubmitUsefulnessHandler,
    authentication=auth)
+delete_review_resource = Resource(handler=DeleteReviewHandler,
+    authentication=auth)

 urlpatterns = patterns('',
    # get status of the service (usually just "ok", might be "read-only")
@@ -135,4 +138,9 @@
    #  POST /1.0/reviews/1/+report-review/
    url(r'^1.0/reviews/(?P<review_id>\d+)/flags/$',
        flag_review_resource),
+
+    # delete own reviews
+    #  POST /1.0/reviews/100/delete
+    url(r'^1.0/reviews/(?P<review_id>\d+)/delete/$',
+        delete_review_resource),
 )

=== modified file 'src/reviewsapp/models/reviews.py'
--- src/reviewsapp/models/reviews.py    2011-03-22 15:43:40 +0000
+++ src/reviewsapp/models/reviews.py    2011-05-02 13:21:35 +0000
@@ -298,6 +298,7 @@
    language = models.SlugField(max_length=10)
    hide = models.BooleanField(
        'Hide review from the UI', db_index=True, default=False)
+    date_deleted=models.DateTimeField(blank=True, null=True)

Great - we just need that migration too.
 
    # The following get updated when a usefulness vote is added
    usefulness_total = models.IntegerField(default=0)
    usefulness_favorable = models.IntegerField(default=0)
@@ -340,6 +341,23 @@

    def reviewer_displayname(self):
        return self.reviewer.get_full_name()
+
+    def delete(self, user):
+        """Delete a review submitted by the user
+
+        @param user: The user deleting the review
+
+        """
+        if self.reviewer != user:
+            raise self.ReviewNotUsersOwn()
+        elif self.date_deleted is not None:
+            raise self.ReviewAlreadyDeleted()
+        else:

As above, I think if the handler is handling the above situations itself, we can reemove the above (and the related exceptions).
 
+            self.hide = True
+            self.date_deleted = datetime.utcnow()
+            self.save()
+            self.softwareitem_in_repository.update_stats()
+            return self.id


oh - interesting. I expected the method to be returning self, rather than self id (ie. so the api client gets back the details of the updated review, with the date_deleted set)? I think that'd be the norm for (non-DELETE) rest-api requests.
 
    def flag(self, user, summary, description):
        """Flag an object as possibly requiring moderation.
@@ -377,6 +395,12 @@

    class Meta:
        app_label = 'reviewsapp'
+
+    class ReviewAlreadyDeleted(Exception):
+        """Raised when user tries to delete a review already deleted"""
+
+    class ReviewNotUsersOwn(Exception):
+        """Raised when user tries to delete another user's review"""


 class Usefulness(models.Model):

=== modified file 'src/reviewsapp/tests/test_handlers.py'
--- src/reviewsapp/tests/test_handlers.py       2011-04-12 07:44:43 +0000
+++ src/reviewsapp/tests/test_handlers.py       2011-05-02 13:21:35 +0000
@@ -28,6 +28,7 @@
    'SubmitReviewHandlerTestCase',
    'UsefulnessHandlerTestCase',
    'ShowUsefulnessHandlerTestCase',
+    'DeleteReviewHandlerTestCase',
    ]


@@ -51,6 +52,7 @@
    ShowReviewsHandler,
    SubmitUsefulnessHandler,
    ShowUsefulnessHandler,
+    DeleteReviewHandler
 )
 from reviewsapp.models import (
    Repository,
@@ -838,6 +840,66 @@
        self.assertEqual(httplib.OK, second.status_code)


+
+class DeleteReviewHandlerTestCase(TestCaseWithFactory):
+    def _delete_review_id(self, review_id, user=None):
+        if user is None:
+            user = self.factory.makeUser()
+        request = Mock()
+        request.POST = None
+        request.user = user
+        handler = DeleteReviewHandler()
+        return handler.create(request, review_id)
+
+    def test_delete_review(self):
+        review = self.factory.makeReview(
+        software_item=self.factory.makeSoftwareItem('package_foo'),
+        app_name='app_foo',
+        repository=self.factory.makeRepository('ubuntu', 'hardy'),
+        language='en')

AFAICS, there's no reason why you couldn't just use:

             review = self.factory.makeReview()

here and for all tests below right? (the software item, repository, language etc., aren't relevant for these tests are they?)

+
+        response = self._delete_review_id(review.id, review.reviewer)
+
+        self.assertEqual(review.id, response)
+
+    def test_non_existent_review(self):
+        review = self.factory.makeReview(
+        software_item=self.factory.makeSoftwareItem('package_foo'),
+        app_name='app_foo',
+        repository=self.factory.makeRepository('ubuntu', 'hardy'),
+        language='en')
+
+        response = self._delete_review_id(review.id+1, review.reviewer)
+
+        self.assertEqual(httplib.NOT_FOUND, response.status_code)
+
+    def test_already_deleted(self):
+        review = self.factory.makeReview(
+        software_item=self.factory.makeSoftwareItem('package_foo'),
+        app_name='app_foo',
+        repository=self.factory.makeRepository('ubuntu', 'hardy'),
+        language='en')
+
+        response_good = self._delete_review_id(review.id, review.reviewer)
+        response_bad = self._delete_review_id(review.id, review.reviewer)
+
+        self.assertEqual(review.id, response_good)
+        self.assertEqual(httplib.FORBIDDEN, response_bad.status_code)
+
+    def test_wrong_user(self):
+        review = self.factory.makeReview(
+        software_item=self.factory.makeSoftwareItem('package_foo'),
+        app_name='app_foo',
+        repository=self.factory.makeRepository('ubuntu', 'hardy'),
+        language='en')
+
+        wrong_user = self.factory.makeUser()
+
+        response = self._delete_review_id(review.id, wrong_user)
+
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+
 class FlagReviewHandlerTestCase(TestCaseWithFactory):
    def _flag_review_id(self, review_id, user=None, data=None):
        if user is None:



Great - thanks again! Let me know whether you've time to update with any of the changes (otherwise I can add to the branch too - whatever suits).

-Michael