Merge into trunk : delete-review : Code : Ratings and Reviews server

Reviewer	Date Requested	Status
Michael Nelson (community)	2011-05-02	Approve on 2011-05-06
Aaron Peachey (community)		Needs Resubmitting on 2011-05-06
Review via email: mp+59658@code.launchpad.net

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2011-05-03:

#

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

On Mon, May 2, 2011 at 3:21 PM, Aaron Peachey <alpeachey@gmail.com> 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

review: Needs Information

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2011-05-03:

#

Actually, I was just chatting with achuni about this:

{{{
14:25 <achuni> we need to add a bit of doc or a comment to urls.py
14:26 <achuni> so that people consider the http/https issue when adding a new url
14:26 <achuni> while you're there...
14:27 <achuni> could you ask him to change it to /reviews/delete/<id>/?
14:27 <achuni> I know it's nicer to be able to say /reviews/42 -> brings the review, and /reviews/42/delete -> deletes it
14:28 <achuni> but /reviews/42 is a public (http) request, and /reviews/42/delete needs to be authenticated
14:28 <noodles> oh,
14:29 <noodles> that rules out my suggestion, which was GET /reviews/42 to get, and PUT /reviews/42 to update it (for eg., setting date_deleted)
14:29 <achuni> as a rule of thumb, it's best to leave all the variable part of the url pattern at the end, and make sure it has a unique non-variable prefix
14:29 <achuni> ah that would be extremely hard to do also :-/
14:29 <noodles> yep.
14:29 <achuni> well, unless we don't care about doing https for everybody
14:29 <achuni> but it screws with caching
14:30 <achuni> (I understand retrieved reviews aren't cached if they're fetched over https)
14:30 <noodles> Is it not possible to do GET as http, and PUT/POST/DELETE etc. as https? or is it url only?
14:30 <achuni> (piston-mini-client should fix that really... sucky pos)
14:31 <achuni> atm piston-mini-client distinguishes them by url
14:31 <achuni> ...and Apache's redirect rules do so too I think?
14:31 <achuni> or can Apache say "this url, if it's a GET request, redirect to http"?
14:31 <achuni> I'd bet it can do that, yeah
14:32 <achuni> but we'd still need to realize (on the server) that one url would sometimes need to be authenticated, others not
14:32 <noodles> http://stackoverflow.com/questions/2838728/rewrite-url-for-put-request
}}}

So for the moment, it might be best just to modify the url and not worry about the handler changes I mentioned as possibilities. If we find we can do per method apache redirects as above, then I'll do a separate branch updating the handlers.

Actually, I was just chatting with achuni about this:

{{{
14:25 <achuni> we need to add a bit of doc or a comment to urls.py
14:26 <achuni> so that people consider the http/https issue when adding a new url
14:26 <achuni> while you're there...
14:27 <achuni> could you ask him to change it to /reviews/delete/<id>/?
14:27 <achuni> I know it's nicer to be able to say /reviews/42 -> brings the review, and /reviews/42/delete -> deletes it
14:28 <achuni> but /reviews/42 is a public (http) request, and /reviews/42/delete needs to be authenticated
14:28 <noodles> oh,
14:29 <noodles> that rules out my suggestion, which was GET /reviews/42 to get, and PUT /reviews/42 to update it (for eg., setting date_deleted)
14:29 <achuni> as a rule of thumb, it's best to leave all the variable part of the url pattern at the end, and make sure it has a unique non-variable prefix
14:29 <achuni> ah that would be extremely hard to do also :-/
14:29 <noodles> yep.
14:29 <achuni> well, unless we don't care about doing https for everybody
14:29 <achuni> but it screws with caching
14:30 <achuni> (I understand retrieved reviews aren't cached if they're fetched over https)
14:30 <noodles> Is it not possible to do GET as http, and PUT/POST/DELETE etc. as https? or is it url only?
14:30 <achuni> (piston-mini-client should fix that really... sucky pos)
14:31 <achuni> atm piston-mini-client distinguishes them by url
14:31 <achuni> ...and Apache's redirect rules do so too I think?
14:31 <achuni> or can Apache say "this url, if it's a GET request, redirect to http"?
14:31 <achuni> I'd bet it can do that, yeah
14:32 <achuni> but we'd still need to realize (on the server) that one url would sometimes need to be authenticated, others not
14:32 <noodles> http://stackoverflow.com/questions/2838728/rewrite-url-for-put-request
}}}

So for the moment, it might be best just to modify the url and not worry about the handler changes I mentioned as possibilities. If we find we can do per method apache redirects as above, then I'll do a separate branch updating the handlers.

Revision history for this message

Aaron Peachey (aaronp) wrote on 2011-05-03:

#

Hi Michael,
Thanks alot for the speedy review. You can probably tell I'm just feeling my way around django for now so I appreciate the comprehensive feedback.

I'm happy to make the suggested changes from you and achuni and resubmit shortly.

Just a couple of things:
1. Re the risk of a deleted review being unhidden. Good point! Should I just modify the moderation stuff to prevent a review being queued for moderation (or deleted from the queue if it's there already) if it has a deleted date or add a new boolean field ' deleted' (would still need to modify the moderation chide). I'm thinking the latter is cleaner, especially if we want to treat hidden reviews differently to deleted reviews later e.g. Allowing a user to resubmit for that app.

2. Re get_object_or_404 - the reason I differentiated between the reasons is to return more useful feedback to the client, as i've written some code in usc that uses the body of the response to give the user a clue as to why it failed. Would I still be able to do this using thie get_object_or_404 method? I.e. Saying the review does not exist is technically only correct if it can't be found using its id - what do you think?

Cheers
Aaron

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2011-05-04:

#

> Hi Michael,
> Thanks alot for the speedy review. You can probably tell I'm just feeling my
> way around django for now so I appreciate the comprehensive feedback.

No problem! It's great that you're getting in there from your desktop background :) I hope you're enjoying django!

>
> I'm happy to make the suggested changes from you and achuni and resubmit
> shortly.
>
> Just a couple of things:
> 1. Re the risk of a deleted review being unhidden. Good point! Should I just
> modify the moderation stuff to prevent a review being queued for moderation
> (or deleted from the queue if it's there already) if it has a deleted date or
> add a new boolean field ' deleted' (would still need to modify the moderation
> chide). I'm thinking the latter is cleaner, especially if we want to treat
> hidden reviews differently to deleted reviews later e.g. Allowing a user to
> resubmit for that app.

It should be as simple as updating the query in views.reviewmoderation_list from:
review_moderations = ReviewModeration.objects.all()
to:
review_moderations = ReviewModeration.objects.all(review__date_deleted=None)

(and adding a test to ensure they're not included of course). I don't think we need another deleted field given that you've already got date_deleted?

Actually, something else... currently people can't create more than one view for an app (in a given series). See reviewsapp.forms.ReviewForm.clean(), we'll just need a similar update to the filter there used to determine whether the user already has a review, so that they can submit a new one if they delete the old one (?). Does that make sense?

>
> 2. Re get_object_or_404 - the reason I differentiated between the reasons is
> to return more useful feedback to the client, as i've written some code in usc
> that uses the body of the response to give the user a clue as to why it
> failed. Would I still be able to do this using thie get_object_or_404 method?

No - that makes sense if you want to do that, but two points:
1) I'd be very careful depending on the body of the response (it could be modified, translated etc. in sca without us knowing that it'll affect the client. If you are depending on the body text, I'd check for it in the test and add a comment so it's not removed from the test.
2) Is it really necessary? Are they not options that should never be presented in the client UI anyway? (A button to delete someone else's review, or a review of your own that you've already deleted?)

> I.e. Saying the review does not exist is technically only correct if it can't
> be found using its id - what do you think?

Right - but I'm not sure how likely the other two are in the client UI, but you're in a better position to know that.

Thanks!
>
> Cheers
> Aaron

> Hi Michael,
> Thanks alot for the speedy review. You can probably tell I'm just feeling my
> way around django for now so I appreciate the comprehensive feedback.

No problem! It's great that you're getting in there from your desktop background :) I hope you're enjoying django!

> 
> I'm happy to make the suggested changes from you and achuni and resubmit
> shortly.
> 
> Just a couple of things:
> 1. Re the risk of a deleted review being unhidden. Good point! Should I just
> modify the moderation stuff to prevent a review being queued for moderation
> (or deleted from the queue if it's there already) if it has a deleted date or
> add a new boolean field ' deleted' (would still need to modify the moderation
> chide). I'm thinking the latter is cleaner, especially if we want to treat
> hidden reviews differently to deleted reviews later e.g. Allowing a user to
> resubmit for that app.

It should be as simple as updating the query in views.reviewmoderation_list from:
    review_moderations = ReviewModeration.objects.all()
to:
    review_moderations = ReviewModeration.objects.all(review__date_deleted=None)

(and adding a test to ensure they're not included of course). I don't think we need another deleted field given that you've already got date_deleted?

Actually, something else... currently people can't create more than one view for an app (in a given series). See reviewsapp.forms.ReviewForm.clean(), we'll just need a similar update to the filter there used to determine whether the user already has a review, so that they can submit a new one if they delete the old one (?). Does that make sense?

> 
> 2. Re get_object_or_404 - the reason I differentiated between the reasons is
> to return more useful feedback to the client, as i've written some code in usc
> that uses the body of the response to give the user a clue as to why it
> failed. Would I still be able to do this using thie get_object_or_404 method?

No - that makes sense if you want to do that, but two points:
 1) I'd be very careful depending on the body of the response (it could be modified, translated etc. in sca without us knowing that it'll affect the client. If you are depending on the body text, I'd check for it in the test and add a comment so it's not removed from the test.
 2) Is it really necessary? Are they not options that should never be presented in the client UI anyway? (A button to delete someone else's review, or a review of your own that you've already deleted?)

> I.e. Saying the review does not exist is technically only correct if it can't
> be found using its id - what do you think?

Right - but I'm not sure how likely the other two are in the client UI, but you're in a better position to know that.

Thanks!
> 
> Cheers
> Aaron

Revision history for this message

Aaron Peachey (aaronp) wrote on 2011-05-05:

#

Yeah, that makes sense, technically in the client the user should not be allowed to delete the review if it's already deleted or if it's not theirs, so it may be overkill to cater for it here. I'll implement using the get_object_or_404 and we'll see how it goes. If it's coded well enough in client (which it might not be - remember I wrote the code!!) then it shouldn't be an issue.

Revision history for this message

Aaron Peachey (aaronp) wrote on 2011-05-05:

#

OK all the changes are made.
I wasn't able to do the migration, as it's complaining that the --auto option doesn't exist (??). Anything I can do there?
I felt my way around the tests a bit and had to make some changes to the factory to make it work. Assuming with default values it won't impact any other tests but not 100% sure.

Thanks again!
Aaron

review: Needs Resubmitting

lp:~aaronp/rnr-server/delete-review updated on 2011-05-05

184. By Aaron Peachey on 2011-05-05: * delete review functionality:
- modify to simplify handler in case of error
- ensure deleted reviews don't get moderated
- ensure user can submit review if one has been deleted
- update order of url parts
- add/modify test cases for above changes

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2011-05-05:

#

On Thu, May 5, 2011 at 2:54 PM, Aaron Peachey <email address hidden> wrote:

> Review: Resubmit
> OK all the changes are made.
> I wasn't able to do the migration, as it's complaining that the --auto
> option doesn't exist (??). Anything I can do there?
>

Yeah, sorry, I should have checked the command I gave you. It's:

fab manage:'startmigration reviewsapp add_review_date_deleted --auto'

It'll tell you that a bunch of other fields have changed too,, but most are
just because some model fields have utcnow as the default value, and so can
be deleted from the migration. It should just contain the forward and
backward to add/remove your new column, as well as all the auto-generated
orm data. Either check one of the other simple migrations, or it's fine just
to leave it and I'll add to your branch before landing.

I'll try to get to your other changes asap. Thanks!

> I felt my way around the tests a bit and had to make some changes to the
> factory to make it work. Assuming with default values it won't impact any
> other tests but not 100% sure.
>
> Thanks again!
> Aaron
> --
> https://code.launchpad.net/~aaronp/rnr-server/delete-review/+merge/59658
> You are reviewing the proposed merge of lp:~aaronp/rnr-server/delete-review
> into lp:rnr-server.
>

--

Michael Nelson

Canonical Ltd.

+49 176 491 53481 (mob)

IRC nick: noodles (noodles775 on Freenode)

Ubuntu - Linux for human beings | www.ubuntu.com | www.canonical.com

lp:~aaronp/rnr-server/delete-review updated on 2011-05-05

185. By Aaron Peachey on 2011-05-05: add migration to bring in new date_deleted field to Review

Revision history for this message

Aaron Peachey (aaronp) wrote on 2011-05-05:

#

Easy, thanks Michael. I've pushed the migration now too.

lp:~aaronp/rnr-server/delete-review updated on 2011-05-05

186. By Aaron Peachey on 2011-05-05: - test_rnrclient.py: add test for delete review in rnrclient
- api/handlers.py: return date_deleted in review response

Revision history for this message

Aaron Peachey (aaronp) wrote on 2011-05-05:

#

One more thing for now. Updated and tested rnrclient (diff below).
Added a test for the rnrclient change and added date_deleted to the fields in the show review handler so it gets returned when a review is successfully deleted (I don't know why this worked, but I took a guess and it did!)

=== modified file 'rnrclient.py'
--- rnrclient.py 2011-03-17 09:17:26 +0000
+++ rnrclient.py 2011-05-05 14:10:04 +0000
@@ -167,3 +167,10 @@

return self._get('usefulness/', args=data,
scheme=PUBLIC_API_SCHEME)
+
+ @validate('review_id', int)
+ @returns_json
+ def delete_review(self, review_id):
+ """Delete a review"""
+ return self._post('/reviews/delete/%s/' % review_id, data = {},
+ scheme=AUTHENTICATED_API_SCHEME)

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2011-05-05:

#

Wow, great work Aaron. AFAICS, there are only a few small tiny issues (again, I'm happy to do them if you're getting sick of me, but if you're keen, go for it :) ).

1) Test ensuring reviews stats updated after delete (you've added the code already). You can either mock the review.softwareitem_in_repository to ensure update_stats is called, or setup the test to check the actual stats afterwards.
2) AFAICS, the two exceptions you created are no longer necessary? (ReviewAlreadyDeleted and ReviewNotUsersOwn)
3) test_delete_review currently just checks that the same review is returned... it should also be checking that hide is false and date_deleted is not none right, like you've done on the api test? (it's tested in part implicitly with test_already_deleted)
4) With test_wrong_user, the first var (reviewer) is never used and unnecessary.

Also, is there a reason to catch the exceptions in your _request_delete_id helper? It hides the fact that an exception is occurring in the test itself, whereas using assertRaises() would make it obvious and explicit.

Revision history for this message

Aaron Peachey (aaronp) wrote on 2011-05-05:

#

> Wow, great work Aaron. AFAICS, there are only a few small tiny issues (again,
> I'm happy to do them if you're getting sick of me, but if you're keen, go for
> it :) ).

Thanks. Happy to.

> 1) Test ensuring reviews stats updated after delete (you've added the code
> already). You can either mock the review.softwareitem_in_repository to ensure
> update_stats is called, or setup the test to check the actual stats
> afterwards.
OK, thanks.

> 2) AFAICS, the two exceptions you created are no longer necessary?
> (ReviewAlreadyDeleted and ReviewNotUsersOwn)
Yep, forgot to delete them!

> 3) test_delete_review currently just checks that the same review is
> returned... it should also be checking that hide is false and date_deleted is
> not none right, like you've done on the api test? (it's tested in part
> implicitly with test_already_deleted)
Yep, will change that.

> 4) With test_wrong_user, the first var (reviewer) is never used and
> unnecessary.
Thanks.

>
> Also, is there a reason to catch the exceptions in your _request_delete_id
> helper? It hides the fact that an exception is occurring in the test itself,
> whereas using assertRaises() would make it obvious and explicit.
Thanks for that. It took me a while to figure out how to resolve that (turns out I've used a hack!!) and I didn't know assertRaises existed. Will fix that too.

Cheers. Will resubmit soon.

thanks
Aaron

lp:~aaronp/rnr-server/delete-review updated on 2011-05-06

187. By Aaron Peachey on 2011-05-06: - models/reviews.py: remove unused exceptions
- tests/test_handlers.py: tweak tests and add test
to check stats are updated on delete

Revision history for this message

Aaron Peachey (aaronp) wrote on 2011-05-06:

#

All changes made. (Re-)Re-submitted.
Thanks

review: Needs Resubmitting

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2011-05-06:

#

Excellent, thanks Aaron! I'll land the rnrclient change first then this one.

review: Approve

Ratings and Reviews server

Merge lp:~aaronp/rnr-server/delete-review into lp:rnr-server

Commit message

Description of the change

Preview Diff

Subscribers

 === 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-06 01:20:55 +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
@@ -174,6 +176,7 @@
          'summary',
          'review_text',
          'hide',
++        'date_deleted',
          'version',
          'usefulness_total',
          'usefulness_favorable',
@@ -236,6 +239,12 @@
          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)
++        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-06 01:20:55 +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/delete/100
++    url(r'^1.0/reviews/delete/(?P<review_id>\d+)/$',
++        delete_review_resource),
+ )
 === modified file 'src/reviewsapp/forms.py'
 --- src/reviewsapp/forms.py	2011-03-18 17:07:30 +0000
 +++ src/reviewsapp/forms.py	2011-05-06 01:20:55 +0000
@@ -92,10 +92,12 @@
          if not settings.ALLOW_MULTIPLE_REVIEWS_FOR_TESTING:
              # Ensure a user cannot submit a second review for the same app
++            # unless their first one has been deleted by them
              previous_reviews = Review.objects.filter(
                  reviewer=self.instance.reviewer,
                  softwareitem=self.cleaned_data['software_item'],
--                repository=self.cleaned_data['repository'])
++                repository=self.cleaned_data['repository'],
++                date_deleted=None)
              if previous_reviews:
                  raise forms.ValidationError(
                      "A user cannot create multiple reviews for an app.")
 === added file 'src/reviewsapp/migrations/0010_add_review_date_deleted.py'
 --- src/reviewsapp/migrations/0010_add_review_date_deleted.py	1970-01-01 00:00:00 +0000
 +++ src/reviewsapp/migrations/0010_add_review_date_deleted.py	2011-05-06 01:20:55 +0000
@@ -0,0 +1,157 @@
++
++from south.db import db
++from django.db import models
++from reviewsapp.models import *
++
++class Migration:
++
++    def forwards(self, orm):
++        # Adding field 'Review.date_deleted'
++        db.add_column('reviewsapp_review', 'date_deleted', orm['reviewsapp.review:date_deleted'])
++
++    def backwards(self, orm):
++        # Deleting field 'Review.date_deleted'
++        db.delete_column('reviewsapp_review', 'date_deleted')
++
++    models = {
++        'auth.group': {
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}),
++            'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'})
++        },
++        'auth.permission': {
++            'Meta': {'unique_together': "(('content_type', 'codename'),)"},
++            'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
++            'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'name': ('django.db.models.fields.CharField', [], {'max_length': '50'})
++        },
++        'auth.user': {
++            'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
++            'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}),
++            'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
++            'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True', 'blank': 'True'}),
++            'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False', 'blank': 'True'}),
++            'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False', 'blank': 'True'}),
++            'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
++            'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
++            'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
++            'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}),
++            'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
++        },
++        'contenttypes.contenttype': {
++            'Meta': {'unique_together': "(('app_label', 'model'),)", 'db_table': "'django_content_type'"},
++            'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
++            'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
++        },
++        'reviewsapp.architecture': {
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'tag': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '16'})
++        },
++        'reviewsapp.consumer': {
++            'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'key': ('django.db.models.fields.CharField', [], {'max_length': '64'}),
++            'secret': ('django.db.models.fields.CharField', [], {'default': "'QQcOHuhpGcauNucDPiRTuWKsVbFWrr'", 'max_length': '255', 'blank': 'True'}),
++            'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}),
++            'user': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'oauth_consumer'", 'unique': 'True', 'to': "orm['auth.User']"})
++        },
++        'reviewsapp.nonce': {
++            'consumer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Consumer']"}),
++            'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'nonce': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255'}),
++            'token': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Token']"})
++        },
++        'reviewsapp.repository': {
++            'distroseries': ('django.db.models.fields.SlugField', [], {'max_length': '25', 'db_index': 'True'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'origin': ('django.db.models.fields.SlugField', [], {'max_length': '100', 'db_index': 'True'})
++        },
++        'reviewsapp.review': {
++            'app_name': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '100', 'blank': 'True'}),
++            'architecture': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Architecture']"}),
++            'date_created': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 29, 1, 431193)'}),
++            'date_deleted': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'blank': 'True'}),
++            'hide': ('django.db.models.fields.BooleanField', [], {'default': 'False', 'db_index': 'True', 'blank': 'True'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'language': ('django.db.models.fields.SlugField', [], {'max_length': '10', 'db_index': 'True'}),
++            'rating': ('django.db.models.fields.IntegerField', [], {}),
++            'repository': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Repository']"}),
++            'review_text': ('django.db.models.fields.CharField', [], {'max_length': '5000'}),
++            'reviewer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}),
++            'softwareitem': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.SoftwareItem']"}),
++            'summary': ('django.db.models.fields.CharField', [], {'max_length': '80'}),
++            'usefulness_favorable': ('django.db.models.fields.IntegerField', [], {'default': '0'}),
++            'usefulness_percentage': ('django.db.models.fields.IntegerField', [], {'default': '0'}),
++            'usefulness_total': ('django.db.models.fields.IntegerField', [], {'default': '0'}),
++            'version': ('django.db.models.fields.CharField', [], {'max_length': '100'})
++        },
++        'reviewsapp.reviewmoderation': {
++            'date_created': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 29, 1, 491649)'}),
++            'date_moderated': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'blank': 'True'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'moderation_text': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
++            'moderator': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}),
++            'review': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Review']"}),
++            'status': ('django.db.models.fields.SmallIntegerField', [], {'default': '0'})
++        },
++        'reviewsapp.reviewmoderationflag': {
++            'date_created': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 29, 1, 630446)'}),
++            'description': ('django.db.models.fields.TextField', [], {}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'review_moderation': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.ReviewModeration']"}),
++            'summary': ('django.db.models.fields.CharField', [], {'max_length': '200'}),
++            'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True'})
++        },
++        'reviewsapp.rnrsettings': {
++            'blacklist_words': ('django.db.models.fields.TextField', [], {'default': "''"}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'moderation_mode': ('django.db.models.fields.CharField', [], {'default': "'passive'", 'max_length': '8'})
++        },
++        'reviewsapp.softwareitem': {
++            'date_ratings_updated': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 28, 59, 374540)'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'package_name': ('django.db.models.fields.SlugField', [], {'max_length': '100', 'db_index': 'True'}),
++            'ratings_average': ('django.db.models.fields.DecimalField', [], {'default': '0', 'max_digits': '3', 'decimal_places': '2'}),
++            'ratings_total': ('django.db.models.fields.IntegerField', [], {'default': '0'})
++        },
++        'reviewsapp.softwareiteminorigin': {
++            'Meta': {'unique_together': "(('softwareitem', 'origin'),)"},
++            'date_ratings_updated': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 28, 59, 374540)'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'origin': ('django.db.models.fields.SlugField', [], {'max_length': '100', 'db_index': 'True'}),
++            'ratings_average': ('django.db.models.fields.DecimalField', [], {'default': '0', 'max_digits': '3', 'decimal_places': '2'}),
++            'ratings_total': ('django.db.models.fields.IntegerField', [], {'default': '0'}),
++            'softwareitem': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.SoftwareItem']"})
++        },
++        'reviewsapp.softwareiteminrepository': {
++            'Meta': {'unique_together': "(('softwareitem', 'repository'),)"},
++            'date_ratings_updated': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 28, 59, 374540)'}),
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'ratings_average': ('django.db.models.fields.DecimalField', [], {'default': '0', 'max_digits': '3', 'decimal_places': '2'}),
++            'ratings_total': ('django.db.models.fields.IntegerField', [], {'default': '0'}),
++            'repository': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Repository']"}),
++            'softwareitem': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.SoftwareItem']"})
++        },
++        'reviewsapp.token': {
++            'consumer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Consumer']"}),
++            'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
++            'name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
++            'token': ('django.db.models.fields.CharField', [], {'default': "'xFzxdTdlozOgtDGjUHucpzkQBIAUakPHtqfwhEgXLrCjQYvfqm'", 'max_length': '50', 'primary_key': 'True'}),
++            'token_secret': ('django.db.models.fields.CharField', [], {'default': "'brHDYrMqjlzNiHVQdWSWDIqrtflQLWGVcSwDZCGszrsqLuApYN'", 'max_length': '50'}),
++            'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'})
++        },
++        'reviewsapp.usefulness': {
++            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
++            'review': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Review']"}),
++            'useful': ('django.db.models.fields.BooleanField', [], {'default': 'False', 'db_index': 'True', 'blank': 'True'}),
++            'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"})
++        }
++    }
++
++    complete_apps = ['reviewsapp']
 === 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-06 01:20:55 +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)
      # 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,14 @@
      def reviewer_displayname(self):
          return self.reviewer.get_full_name()
++
++    def delete(self):
++        """Delete a review submitted by the user"""
++        self.hide = True
++        self.date_deleted = datetime.utcnow()
++        self.save()
++        self.softwareitem_in_repository.update_stats()
++        return self
      def flag(self, user, summary, description):
          """Flag an object as possibly requiring moderation.
@@ -378,7 +387,6 @@
      class Meta:
          app_label = 'reviewsapp'
--
  class Usefulness(models.Model):
      review = models.ForeignKey(Review, db_index=True)
      user = models.ForeignKey(User)
 === modified file 'src/reviewsapp/tests/factory.py'
 --- src/reviewsapp/tests/factory.py	2011-03-22 16:28:23 +0000
 +++ src/reviewsapp/tests/factory.py	2011-05-06 01:20:55 +0000
@@ -163,7 +163,7 @@
      def makeReview(self, reviewer=None, software_item=None, repository=None,
                     version=None, rating="5", summary=None, review_text=None,
                     date_created=None, language=None, hide=False, num_flags=0,
--                   architecture='i386', app_name=''):
++                   architecture='i386', app_name='', deleted=False):
          if reviewer is None:
              reviewer = self.makeUser()
          if software_item is None:
@@ -180,6 +180,11 @@
              date_created = datetime.utcnow()
          if language is None:
              language = self.getUniqueString(prefix='lang')
++        if deleted:
++            hide = True
++            date_deleted = datetime.utcnow()
++        else:
++            date_deleted = None
          architecture, created = Architecture.objects.get_or_create(
              tag=architecture)
@@ -189,7 +194,7 @@
              version=version, rating=rating, summary=summary,
              review_text=review_text, language=language, hide=hide,
              date_created=date_created, architecture=architecture,
--            app_name=app_name)
++            app_name=app_name, date_deleted=date_deleted)
          review.softwareitem_in_repository.update_stats()
          for flag_count in range(num_flags):
              review.flag(
@@ -200,9 +205,9 @@
      def makeReviewModeration(self, num_flags=1,
                               status=ReviewModeration.PENDING_STATUS,
--                             language=None, distro_series=None):
++                             language=None, distro_series=None, deleted_review=False):
          repository = self.makeRepository(distro_series=distro_series)
--        review = self.makeReview(language=language, repository=repository)
++        review = self.makeReview(language=language, repository=repository, deleted=deleted_review)
          review_moderation = ReviewModeration.objects.create(review=review)
          for flag_count in range(num_flags):
              review.flag(
 === 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-06 01:20:55 +0000
@@ -28,10 +28,12 @@
      'SubmitReviewHandlerTestCase',
      'UsefulnessHandlerTestCase',
      'ShowUsefulnessHandlerTestCase',
++    'DeleteReviewHandlerTestCase',
+     ]
  import httplib
++from django.http import Http404
  from datetime import datetime, timedelta
  from urllib import quote_plus
@@ -51,6 +53,7 @@
      ShowReviewsHandler,
      SubmitUsefulnessHandler,
      ShowUsefulnessHandler,
++    DeleteReviewHandler
+ )
  from reviewsapp.models import (
      Repository,
@@ -837,6 +840,61 @@
          self.assertEqual(httplib.OK, first.status_code)
          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()
++        response = self._delete_review_id(review.id, review.reviewer)
++        self.assertEqual(review.id, response.id)
++        self.assertEqual(True,response.hide)
++        self.assertNotEqual(None, response.date_deleted)
++
++    def test_non_existent_review(self):
++        review = self.factory.makeReview()
++        self.assertRaises(Http404, self._delete_review_id, review.id+1,
++            review.reviewer)
++
++    def test_already_deleted(self):
++        review = self.factory.makeReview()
++
++        response_good = self._delete_review_id(review.id, review.reviewer)
++        self.assertEqual(review, response_good)
++        #try to delete a second time and check it raises a 404 error
++        self.assertRaises(Http404, self._delete_review_id, review.id,
++            review.reviewer)
++
++    def test_wrong_user(self):
++        review = self.factory.makeReview()
++        wrong_user = self.factory.makeUser(username='deleter')
++        self.assertRaises(Http404, self._delete_review_id, review.id,
++            wrong_user)
++
++    def test_delete_updates_stats(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=5)
++        review2 = self.factory.makeReview(software_item=software_item, rating=3)
++        software_item = SoftwareItem.objects.get(id=software_item.id)
++
++        # 2 reviews with ratings of 5 + 3 = 8 (average of 4)
++        self.assertEqual(2, software_item.ratings_total)
++        self.assertEqual(4, software_item.ratings_average)
++
++        self._delete_review_id(review2.id, review2.reviewer)
++        software_item = SoftwareItem.objects.get(id=software_item.id)
++
++        # 1 review with rating 5 = average of 5
++        self.assertEqual(1, software_item.ratings_total)
++        self.assertEqual(5, software_item.ratings_average)
++
  class FlagReviewHandlerTestCase(TestCaseWithFactory):
      def _flag_review_id(self, review_id, user=None, data=None):
 === modified file 'src/reviewsapp/tests/test_rnrclient.py'
 --- src/reviewsapp/tests/test_rnrclient.py	2011-04-19 12:32:23 +0000
 +++ src/reviewsapp/tests/test_rnrclient.py	2011-05-06 01:20:55 +0000
@@ -29,6 +29,7 @@
      'RnRReviewStatsTestCase',
      'RnRServerStatusTestCase',
      'RnRSubmitReviewTestCase',
++    'RnRDeleteReviewTestCase',
      'UsefulnessAPITestCase',
+     ]
@@ -246,6 +247,31 @@
          self.assertEqual(review_detail.distroseries, review.distroseries())
          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)
++
++        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 RnRSubmitReviewTestCase(RnRTxBaseTestCase):
 === modified file 'src/reviewsapp/tests/test_views.py'
 --- src/reviewsapp/tests/test_views.py	2011-04-20 16:47:48 +0000
 +++ src/reviewsapp/tests/test_views.py	2011-05-06 01:20:55 +0000
@@ -80,6 +80,18 @@
          self.assertEqual(302, response.status_code)
          self.assertTrue('/openid/login' in response['Location'])
++
++    def test_moderation_excludes_deleted(self):
++        review_moderation = self.factory.makeReviewModeration()
++        review_moderation_deleted = self.factory.makeReviewModeration(deleted_review=True)
++
++        response = self._request_url_as_moderator()
++
++        review_moderations = response.context['review_moderations']
++
++        self.assertEqual(1, len(review_moderations))
++        self.assertEqual(review_moderation, review_moderations[0])
++        self.assertNotEqual(review_moderation_deleted, review_moderations[0])
      def _request_url_as_moderator(self, data=None, user=None):
          # Helper to request the url as a moderator.
 === modified file 'src/reviewsapp/views/moderation.py'
 --- src/reviewsapp/views/moderation.py	2011-03-31 11:09:37 +0000
 +++ src/reviewsapp/views/moderation.py	2011-05-06 01:20:55 +0000
@@ -58,7 +58,8 @@
  def reviewmoderation_list(request):
      """A filterable list of review moderations with a useful default."""
--    review_moderations = ReviewModeration.objects.all()
++    #don't present reviews for moderation if they've been deleted
++    review_moderations = ReviewModeration.objects.filter(review__date_deleted=None)
      next_url = reverse('rnr-reviewmoderation-list')
      # By default we'll be displaying pending moderations.