Merge lp:~aaronp/rnr-server/delete-review into lp:rnr-server
- delete-review
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Michael Nelson |
Approved revision: | 187 |
Merged at revision: | 183 |
Proposed branch: | lp:~aaronp/rnr-server/delete-review |
Merge into: | lp:rnr-server |
Diff against target: |
502 lines (+294/-8) 10 files modified
src/reviewsapp/api/handlers.py (+9/-0) src/reviewsapp/api/urls.py (+9/-1) src/reviewsapp/forms.py (+3/-1) src/reviewsapp/migrations/0010_add_review_date_deleted.py (+157/-0) src/reviewsapp/models/reviews.py (+9/-1) src/reviewsapp/tests/factory.py (+9/-4) src/reviewsapp/tests/test_handlers.py (+58/-0) src/reviewsapp/tests/test_rnrclient.py (+26/-0) src/reviewsapp/tests/test_views.py (+12/-0) src/reviewsapp/views/moderation.py (+2/-1) |
To merge this branch: | bzr merge lp:~aaronp/rnr-server/delete-review |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Nelson (community) | Approve | ||
Aaron Peachey (community) | Needs Resubmitting | ||
Review via email:
|
Commit message
[r=noodles][bug=778301] Enable rnrclient method for user to delete their own review.
Description of the change
add's functionality for a user to self-delete a review they have submitted.
Adds a new field in the Review model to hold a date/time of deletion (where null = not deleted) and uses existing hide flag.

Michael Nelson (michael.nelson) wrote : | # |

Michael Nelson (michael.nelson) wrote : | # |
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/
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://
}}}
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.

Aaron Peachey (aaronp) wrote : | # |
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

Michael Nelson (michael.nelson) wrote : | # |
> 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.reviewmod
review_
to:
review_
(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.
>
> 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

Aaron Peachey (aaronp) wrote : | # |
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.

Aaron Peachey (aaronp) wrote : | # |
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
- 184. By Aaron Peachey
-
* 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

Michael Nelson (michael.nelson) wrote : | # |
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:
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:/
> 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)
<email address hidden>
IRC nick: noodles (noodles775 on Freenode)
Ubuntu - Linux for human beings | www.ubuntu.com | www.canonical.com
- 185. By Aaron Peachey
-
add migration to bring in new date_deleted field to Review

Aaron Peachey (aaronp) wrote : | # |
Easy, thanks Michael. I've pushed the migration now too.
- 186. By Aaron Peachey
-
- test_rnrclient.py: add test for delete review in rnrclient
- api/handlers.py: return date_deleted in review response

Aaron Peachey (aaronp) wrote : | # |
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(
+
+ @validate(
+ @returns_json
+ def delete_review(self, review_id):
+ """Delete a review"""
+ return self._post(
+ scheme=

Michael Nelson (michael.nelson) wrote : | # |
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.
2) AFAICS, the two exceptions you created are no longer necessary? (ReviewAlreadyD
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_
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.

Aaron Peachey (aaronp) wrote : | # |
> 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.
> 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?
> (ReviewAlreadyD
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_
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
- 187. By Aaron Peachey
-
- models/reviews.py: remove unused exceptions
- tests/test_handlers. py: tweak tests and add test
to check stats are updated on delete

Aaron Peachey (aaronp) wrote : | # |
All changes made. (Re-)Re-submitted.
Thanks

Michael Nelson (michael.nelson) wrote : | # |
Excellent, thanks Aaron! I'll land the rnrclient change first then this one.
Preview Diff
1 | === modified file 'src/reviewsapp/api/handlers.py' | |||
2 | --- src/reviewsapp/api/handlers.py 2011-04-12 10:43:30 +0000 | |||
3 | +++ src/reviewsapp/api/handlers.py 2011-05-06 01:20:55 +0000 | |||
4 | @@ -27,6 +27,7 @@ | |||
5 | 27 | 'ShowReviewsHandler', | 27 | 'ShowReviewsHandler', |
6 | 28 | 'SubmitReviewHandler', | 28 | 'SubmitReviewHandler', |
7 | 29 | 'ShowUsefulnessHandler', | 29 | 'ShowUsefulnessHandler', |
8 | 30 | 'DeleteReviewHandler', | ||
9 | 30 | ] | 31 | ] |
10 | 31 | 32 | ||
11 | 32 | import urllib | 33 | import urllib |
12 | @@ -37,6 +38,7 @@ | |||
13 | 37 | HttpResponse, | 38 | HttpResponse, |
14 | 38 | HttpResponseBadRequest, | 39 | HttpResponseBadRequest, |
15 | 39 | HttpResponseNotFound, | 40 | HttpResponseNotFound, |
16 | 41 | HttpResponseForbidden, | ||
17 | 40 | ) | 42 | ) |
18 | 41 | from django.shortcuts import get_object_or_404 | 43 | from django.shortcuts import get_object_or_404 |
19 | 42 | from django.utils import simplejson | 44 | from django.utils import simplejson |
20 | @@ -174,6 +176,7 @@ | |||
21 | 174 | 'summary', | 176 | 'summary', |
22 | 175 | 'review_text', | 177 | 'review_text', |
23 | 176 | 'hide', | 178 | 'hide', |
24 | 179 | 'date_deleted', | ||
25 | 177 | 'version', | 180 | 'version', |
26 | 178 | 'usefulness_total', | 181 | 'usefulness_total', |
27 | 179 | 'usefulness_favorable', | 182 | 'usefulness_favorable', |
28 | @@ -236,6 +239,12 @@ | |||
29 | 236 | 239 | ||
30 | 237 | return review | 240 | return review |
31 | 238 | 241 | ||
32 | 242 | class DeleteReviewHandler(BaseHandler): | ||
33 | 243 | allowed_methods = ('POST',) | ||
34 | 244 | |||
35 | 245 | def create(self, request, review_id): | ||
36 | 246 | review = get_object_or_404(Review, id=review_id, reviewer=request.user, date_deleted=None) | ||
37 | 247 | return review.delete() | ||
38 | 239 | 248 | ||
39 | 240 | class FlagReviewHandler(BaseHandler): | 249 | class FlagReviewHandler(BaseHandler): |
40 | 241 | allowed_methods = ('POST',) | 250 | allowed_methods = ('POST',) |
41 | 242 | 251 | ||
42 | === modified file 'src/reviewsapp/api/urls.py' | |||
43 | --- src/reviewsapp/api/urls.py 2011-03-23 16:02:43 +0000 | |||
44 | +++ src/reviewsapp/api/urls.py 2011-05-06 01:20:55 +0000 | |||
45 | @@ -30,7 +30,8 @@ | |||
46 | 30 | SubmitReviewHandler, | 30 | SubmitReviewHandler, |
47 | 31 | SubmitUsefulnessHandler, | 31 | SubmitUsefulnessHandler, |
48 | 32 | RetrieveReviewHandler, | 32 | RetrieveReviewHandler, |
50 | 33 | ShowUsefulnessHandler | 33 | ShowUsefulnessHandler, |
51 | 34 | DeleteReviewHandler | ||
52 | 34 | ) | 35 | ) |
53 | 35 | from reviewsapp.auth import SSOOAuthAuthentication | 36 | from reviewsapp.auth import SSOOAuthAuthentication |
54 | 36 | from reviewsapp.api.decorators import dont_vary | 37 | from reviewsapp.api.decorators import dont_vary |
55 | @@ -72,6 +73,8 @@ | |||
56 | 72 | authentication=auth) | 73 | authentication=auth) |
57 | 73 | submit_usefulness_resource = Resource(handler=SubmitUsefulnessHandler, | 74 | submit_usefulness_resource = Resource(handler=SubmitUsefulnessHandler, |
58 | 74 | authentication=auth) | 75 | authentication=auth) |
59 | 76 | delete_review_resource = Resource(handler=DeleteReviewHandler, | ||
60 | 77 | authentication=auth) | ||
61 | 75 | 78 | ||
62 | 76 | urlpatterns = patterns('', | 79 | urlpatterns = patterns('', |
63 | 77 | # get status of the service (usually just "ok", might be "read-only") | 80 | # get status of the service (usually just "ok", might be "read-only") |
64 | @@ -135,4 +138,9 @@ | |||
65 | 135 | # POST /1.0/reviews/1/+report-review/ | 138 | # POST /1.0/reviews/1/+report-review/ |
66 | 136 | url(r'^1.0/reviews/(?P<review_id>\d+)/flags/$', | 139 | url(r'^1.0/reviews/(?P<review_id>\d+)/flags/$', |
67 | 137 | flag_review_resource), | 140 | flag_review_resource), |
68 | 141 | |||
69 | 142 | # delete own reviews | ||
70 | 143 | # POST /1.0/reviews/delete/100 | ||
71 | 144 | url(r'^1.0/reviews/delete/(?P<review_id>\d+)/$', | ||
72 | 145 | delete_review_resource), | ||
73 | 138 | ) | 146 | ) |
74 | 139 | 147 | ||
75 | === modified file 'src/reviewsapp/forms.py' | |||
76 | --- src/reviewsapp/forms.py 2011-03-18 17:07:30 +0000 | |||
77 | +++ src/reviewsapp/forms.py 2011-05-06 01:20:55 +0000 | |||
78 | @@ -92,10 +92,12 @@ | |||
79 | 92 | 92 | ||
80 | 93 | if not settings.ALLOW_MULTIPLE_REVIEWS_FOR_TESTING: | 93 | if not settings.ALLOW_MULTIPLE_REVIEWS_FOR_TESTING: |
81 | 94 | # Ensure a user cannot submit a second review for the same app | 94 | # Ensure a user cannot submit a second review for the same app |
82 | 95 | # unless their first one has been deleted by them | ||
83 | 95 | previous_reviews = Review.objects.filter( | 96 | previous_reviews = Review.objects.filter( |
84 | 96 | reviewer=self.instance.reviewer, | 97 | reviewer=self.instance.reviewer, |
85 | 97 | softwareitem=self.cleaned_data['software_item'], | 98 | softwareitem=self.cleaned_data['software_item'], |
87 | 98 | repository=self.cleaned_data['repository']) | 99 | repository=self.cleaned_data['repository'], |
88 | 100 | date_deleted=None) | ||
89 | 99 | if previous_reviews: | 101 | if previous_reviews: |
90 | 100 | raise forms.ValidationError( | 102 | raise forms.ValidationError( |
91 | 101 | "A user cannot create multiple reviews for an app.") | 103 | "A user cannot create multiple reviews for an app.") |
92 | 102 | 104 | ||
93 | === added file 'src/reviewsapp/migrations/0010_add_review_date_deleted.py' | |||
94 | --- src/reviewsapp/migrations/0010_add_review_date_deleted.py 1970-01-01 00:00:00 +0000 | |||
95 | +++ src/reviewsapp/migrations/0010_add_review_date_deleted.py 2011-05-06 01:20:55 +0000 | |||
96 | @@ -0,0 +1,157 @@ | |||
97 | 1 | |||
98 | 2 | from south.db import db | ||
99 | 3 | from django.db import models | ||
100 | 4 | from reviewsapp.models import * | ||
101 | 5 | |||
102 | 6 | class Migration: | ||
103 | 7 | |||
104 | 8 | def forwards(self, orm): | ||
105 | 9 | # Adding field 'Review.date_deleted' | ||
106 | 10 | db.add_column('reviewsapp_review', 'date_deleted', orm['reviewsapp.review:date_deleted']) | ||
107 | 11 | |||
108 | 12 | def backwards(self, orm): | ||
109 | 13 | # Deleting field 'Review.date_deleted' | ||
110 | 14 | db.delete_column('reviewsapp_review', 'date_deleted') | ||
111 | 15 | |||
112 | 16 | models = { | ||
113 | 17 | 'auth.group': { | ||
114 | 18 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
115 | 19 | 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), | ||
116 | 20 | 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) | ||
117 | 21 | }, | ||
118 | 22 | 'auth.permission': { | ||
119 | 23 | 'Meta': {'unique_together': "(('content_type', 'codename'),)"}, | ||
120 | 24 | 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), | ||
121 | 25 | 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), | ||
122 | 26 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
123 | 27 | 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) | ||
124 | 28 | }, | ||
125 | 29 | 'auth.user': { | ||
126 | 30 | 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), | ||
127 | 31 | 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), | ||
128 | 32 | 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), | ||
129 | 33 | 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), | ||
130 | 34 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
131 | 35 | 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True', 'blank': 'True'}), | ||
132 | 36 | 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False', 'blank': 'True'}), | ||
133 | 37 | 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False', 'blank': 'True'}), | ||
134 | 38 | 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), | ||
135 | 39 | 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), | ||
136 | 40 | 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), | ||
137 | 41 | 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), | ||
138 | 42 | 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) | ||
139 | 43 | }, | ||
140 | 44 | 'contenttypes.contenttype': { | ||
141 | 45 | 'Meta': {'unique_together': "(('app_label', 'model'),)", 'db_table': "'django_content_type'"}, | ||
142 | 46 | 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), | ||
143 | 47 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
144 | 48 | 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), | ||
145 | 49 | 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) | ||
146 | 50 | }, | ||
147 | 51 | 'reviewsapp.architecture': { | ||
148 | 52 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
149 | 53 | 'tag': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '16'}) | ||
150 | 54 | }, | ||
151 | 55 | 'reviewsapp.consumer': { | ||
152 | 56 | 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), | ||
153 | 57 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
154 | 58 | 'key': ('django.db.models.fields.CharField', [], {'max_length': '64'}), | ||
155 | 59 | 'secret': ('django.db.models.fields.CharField', [], {'default': "'QQcOHuhpGcauNucDPiRTuWKsVbFWrr'", 'max_length': '255', 'blank': 'True'}), | ||
156 | 60 | 'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), | ||
157 | 61 | 'user': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'oauth_consumer'", 'unique': 'True', 'to': "orm['auth.User']"}) | ||
158 | 62 | }, | ||
159 | 63 | 'reviewsapp.nonce': { | ||
160 | 64 | 'consumer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Consumer']"}), | ||
161 | 65 | 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), | ||
162 | 66 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
163 | 67 | 'nonce': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255'}), | ||
164 | 68 | 'token': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Token']"}) | ||
165 | 69 | }, | ||
166 | 70 | 'reviewsapp.repository': { | ||
167 | 71 | 'distroseries': ('django.db.models.fields.SlugField', [], {'max_length': '25', 'db_index': 'True'}), | ||
168 | 72 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
169 | 73 | 'origin': ('django.db.models.fields.SlugField', [], {'max_length': '100', 'db_index': 'True'}) | ||
170 | 74 | }, | ||
171 | 75 | 'reviewsapp.review': { | ||
172 | 76 | 'app_name': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '100', 'blank': 'True'}), | ||
173 | 77 | 'architecture': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Architecture']"}), | ||
174 | 78 | 'date_created': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 29, 1, 431193)'}), | ||
175 | 79 | 'date_deleted': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'blank': 'True'}), | ||
176 | 80 | 'hide': ('django.db.models.fields.BooleanField', [], {'default': 'False', 'db_index': 'True', 'blank': 'True'}), | ||
177 | 81 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
178 | 82 | 'language': ('django.db.models.fields.SlugField', [], {'max_length': '10', 'db_index': 'True'}), | ||
179 | 83 | 'rating': ('django.db.models.fields.IntegerField', [], {}), | ||
180 | 84 | 'repository': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Repository']"}), | ||
181 | 85 | 'review_text': ('django.db.models.fields.CharField', [], {'max_length': '5000'}), | ||
182 | 86 | 'reviewer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}), | ||
183 | 87 | 'softwareitem': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.SoftwareItem']"}), | ||
184 | 88 | 'summary': ('django.db.models.fields.CharField', [], {'max_length': '80'}), | ||
185 | 89 | 'usefulness_favorable': ('django.db.models.fields.IntegerField', [], {'default': '0'}), | ||
186 | 90 | 'usefulness_percentage': ('django.db.models.fields.IntegerField', [], {'default': '0'}), | ||
187 | 91 | 'usefulness_total': ('django.db.models.fields.IntegerField', [], {'default': '0'}), | ||
188 | 92 | 'version': ('django.db.models.fields.CharField', [], {'max_length': '100'}) | ||
189 | 93 | }, | ||
190 | 94 | 'reviewsapp.reviewmoderation': { | ||
191 | 95 | 'date_created': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 29, 1, 491649)'}), | ||
192 | 96 | 'date_moderated': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'blank': 'True'}), | ||
193 | 97 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
194 | 98 | 'moderation_text': ('django.db.models.fields.TextField', [], {'blank': 'True'}), | ||
195 | 99 | 'moderator': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), | ||
196 | 100 | 'review': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Review']"}), | ||
197 | 101 | 'status': ('django.db.models.fields.SmallIntegerField', [], {'default': '0'}) | ||
198 | 102 | }, | ||
199 | 103 | 'reviewsapp.reviewmoderationflag': { | ||
200 | 104 | 'date_created': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 29, 1, 630446)'}), | ||
201 | 105 | 'description': ('django.db.models.fields.TextField', [], {}), | ||
202 | 106 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
203 | 107 | 'review_moderation': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.ReviewModeration']"}), | ||
204 | 108 | 'summary': ('django.db.models.fields.CharField', [], {'max_length': '200'}), | ||
205 | 109 | 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True'}) | ||
206 | 110 | }, | ||
207 | 111 | 'reviewsapp.rnrsettings': { | ||
208 | 112 | 'blacklist_words': ('django.db.models.fields.TextField', [], {'default': "''"}), | ||
209 | 113 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
210 | 114 | 'moderation_mode': ('django.db.models.fields.CharField', [], {'default': "'passive'", 'max_length': '8'}) | ||
211 | 115 | }, | ||
212 | 116 | 'reviewsapp.softwareitem': { | ||
213 | 117 | 'date_ratings_updated': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 28, 59, 374540)'}), | ||
214 | 118 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
215 | 119 | 'package_name': ('django.db.models.fields.SlugField', [], {'max_length': '100', 'db_index': 'True'}), | ||
216 | 120 | 'ratings_average': ('django.db.models.fields.DecimalField', [], {'default': '0', 'max_digits': '3', 'decimal_places': '2'}), | ||
217 | 121 | 'ratings_total': ('django.db.models.fields.IntegerField', [], {'default': '0'}) | ||
218 | 122 | }, | ||
219 | 123 | 'reviewsapp.softwareiteminorigin': { | ||
220 | 124 | 'Meta': {'unique_together': "(('softwareitem', 'origin'),)"}, | ||
221 | 125 | 'date_ratings_updated': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 28, 59, 374540)'}), | ||
222 | 126 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
223 | 127 | 'origin': ('django.db.models.fields.SlugField', [], {'max_length': '100', 'db_index': 'True'}), | ||
224 | 128 | 'ratings_average': ('django.db.models.fields.DecimalField', [], {'default': '0', 'max_digits': '3', 'decimal_places': '2'}), | ||
225 | 129 | 'ratings_total': ('django.db.models.fields.IntegerField', [], {'default': '0'}), | ||
226 | 130 | 'softwareitem': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.SoftwareItem']"}) | ||
227 | 131 | }, | ||
228 | 132 | 'reviewsapp.softwareiteminrepository': { | ||
229 | 133 | 'Meta': {'unique_together': "(('softwareitem', 'repository'),)"}, | ||
230 | 134 | 'date_ratings_updated': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2011, 5, 5, 13, 28, 59, 374540)'}), | ||
231 | 135 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
232 | 136 | 'ratings_average': ('django.db.models.fields.DecimalField', [], {'default': '0', 'max_digits': '3', 'decimal_places': '2'}), | ||
233 | 137 | 'ratings_total': ('django.db.models.fields.IntegerField', [], {'default': '0'}), | ||
234 | 138 | 'repository': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Repository']"}), | ||
235 | 139 | 'softwareitem': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.SoftwareItem']"}) | ||
236 | 140 | }, | ||
237 | 141 | 'reviewsapp.token': { | ||
238 | 142 | 'consumer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Consumer']"}), | ||
239 | 143 | 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), | ||
240 | 144 | 'name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}), | ||
241 | 145 | 'token': ('django.db.models.fields.CharField', [], {'default': "'xFzxdTdlozOgtDGjUHucpzkQBIAUakPHtqfwhEgXLrCjQYvfqm'", 'max_length': '50', 'primary_key': 'True'}), | ||
242 | 146 | 'token_secret': ('django.db.models.fields.CharField', [], {'default': "'brHDYrMqjlzNiHVQdWSWDIqrtflQLWGVcSwDZCGszrsqLuApYN'", 'max_length': '50'}), | ||
243 | 147 | 'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}) | ||
244 | 148 | }, | ||
245 | 149 | 'reviewsapp.usefulness': { | ||
246 | 150 | 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), | ||
247 | 151 | 'review': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['reviewsapp.Review']"}), | ||
248 | 152 | 'useful': ('django.db.models.fields.BooleanField', [], {'default': 'False', 'db_index': 'True', 'blank': 'True'}), | ||
249 | 153 | 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) | ||
250 | 154 | } | ||
251 | 155 | } | ||
252 | 156 | |||
253 | 157 | complete_apps = ['reviewsapp'] | ||
254 | 0 | 158 | ||
255 | === modified file 'src/reviewsapp/models/reviews.py' | |||
256 | --- src/reviewsapp/models/reviews.py 2011-03-22 15:43:40 +0000 | |||
257 | +++ src/reviewsapp/models/reviews.py 2011-05-06 01:20:55 +0000 | |||
258 | @@ -298,6 +298,7 @@ | |||
259 | 298 | language = models.SlugField(max_length=10) | 298 | language = models.SlugField(max_length=10) |
260 | 299 | hide = models.BooleanField( | 299 | hide = models.BooleanField( |
261 | 300 | 'Hide review from the UI', db_index=True, default=False) | 300 | 'Hide review from the UI', db_index=True, default=False) |
262 | 301 | date_deleted=models.DateTimeField(blank=True, null=True) | ||
263 | 301 | # The following get updated when a usefulness vote is added | 302 | # The following get updated when a usefulness vote is added |
264 | 302 | usefulness_total = models.IntegerField(default=0) | 303 | usefulness_total = models.IntegerField(default=0) |
265 | 303 | usefulness_favorable = models.IntegerField(default=0) | 304 | usefulness_favorable = models.IntegerField(default=0) |
266 | @@ -340,6 +341,14 @@ | |||
267 | 340 | 341 | ||
268 | 341 | def reviewer_displayname(self): | 342 | def reviewer_displayname(self): |
269 | 342 | return self.reviewer.get_full_name() | 343 | return self.reviewer.get_full_name() |
270 | 344 | |||
271 | 345 | def delete(self): | ||
272 | 346 | """Delete a review submitted by the user""" | ||
273 | 347 | self.hide = True | ||
274 | 348 | self.date_deleted = datetime.utcnow() | ||
275 | 349 | self.save() | ||
276 | 350 | self.softwareitem_in_repository.update_stats() | ||
277 | 351 | return self | ||
278 | 343 | 352 | ||
279 | 344 | def flag(self, user, summary, description): | 353 | def flag(self, user, summary, description): |
280 | 345 | """Flag an object as possibly requiring moderation. | 354 | """Flag an object as possibly requiring moderation. |
281 | @@ -378,7 +387,6 @@ | |||
282 | 378 | class Meta: | 387 | class Meta: |
283 | 379 | app_label = 'reviewsapp' | 388 | app_label = 'reviewsapp' |
284 | 380 | 389 | ||
285 | 381 | |||
286 | 382 | class Usefulness(models.Model): | 390 | class Usefulness(models.Model): |
287 | 383 | review = models.ForeignKey(Review, db_index=True) | 391 | review = models.ForeignKey(Review, db_index=True) |
288 | 384 | user = models.ForeignKey(User) | 392 | user = models.ForeignKey(User) |
289 | 385 | 393 | ||
290 | === modified file 'src/reviewsapp/tests/factory.py' | |||
291 | --- src/reviewsapp/tests/factory.py 2011-03-22 16:28:23 +0000 | |||
292 | +++ src/reviewsapp/tests/factory.py 2011-05-06 01:20:55 +0000 | |||
293 | @@ -163,7 +163,7 @@ | |||
294 | 163 | def makeReview(self, reviewer=None, software_item=None, repository=None, | 163 | def makeReview(self, reviewer=None, software_item=None, repository=None, |
295 | 164 | version=None, rating="5", summary=None, review_text=None, | 164 | version=None, rating="5", summary=None, review_text=None, |
296 | 165 | date_created=None, language=None, hide=False, num_flags=0, | 165 | date_created=None, language=None, hide=False, num_flags=0, |
298 | 166 | architecture='i386', app_name=''): | 166 | architecture='i386', app_name='', deleted=False): |
299 | 167 | if reviewer is None: | 167 | if reviewer is None: |
300 | 168 | reviewer = self.makeUser() | 168 | reviewer = self.makeUser() |
301 | 169 | if software_item is None: | 169 | if software_item is None: |
302 | @@ -180,6 +180,11 @@ | |||
303 | 180 | date_created = datetime.utcnow() | 180 | date_created = datetime.utcnow() |
304 | 181 | if language is None: | 181 | if language is None: |
305 | 182 | language = self.getUniqueString(prefix='lang') | 182 | language = self.getUniqueString(prefix='lang') |
306 | 183 | if deleted: | ||
307 | 184 | hide = True | ||
308 | 185 | date_deleted = datetime.utcnow() | ||
309 | 186 | else: | ||
310 | 187 | date_deleted = None | ||
311 | 183 | 188 | ||
312 | 184 | architecture, created = Architecture.objects.get_or_create( | 189 | architecture, created = Architecture.objects.get_or_create( |
313 | 185 | tag=architecture) | 190 | tag=architecture) |
314 | @@ -189,7 +194,7 @@ | |||
315 | 189 | version=version, rating=rating, summary=summary, | 194 | version=version, rating=rating, summary=summary, |
316 | 190 | review_text=review_text, language=language, hide=hide, | 195 | review_text=review_text, language=language, hide=hide, |
317 | 191 | date_created=date_created, architecture=architecture, | 196 | date_created=date_created, architecture=architecture, |
319 | 192 | app_name=app_name) | 197 | app_name=app_name, date_deleted=date_deleted) |
320 | 193 | review.softwareitem_in_repository.update_stats() | 198 | review.softwareitem_in_repository.update_stats() |
321 | 194 | for flag_count in range(num_flags): | 199 | for flag_count in range(num_flags): |
322 | 195 | review.flag( | 200 | review.flag( |
323 | @@ -200,9 +205,9 @@ | |||
324 | 200 | 205 | ||
325 | 201 | def makeReviewModeration(self, num_flags=1, | 206 | def makeReviewModeration(self, num_flags=1, |
326 | 202 | status=ReviewModeration.PENDING_STATUS, | 207 | status=ReviewModeration.PENDING_STATUS, |
328 | 203 | language=None, distro_series=None): | 208 | language=None, distro_series=None, deleted_review=False): |
329 | 204 | repository = self.makeRepository(distro_series=distro_series) | 209 | repository = self.makeRepository(distro_series=distro_series) |
331 | 205 | review = self.makeReview(language=language, repository=repository) | 210 | review = self.makeReview(language=language, repository=repository, deleted=deleted_review) |
332 | 206 | review_moderation = ReviewModeration.objects.create(review=review) | 211 | review_moderation = ReviewModeration.objects.create(review=review) |
333 | 207 | for flag_count in range(num_flags): | 212 | for flag_count in range(num_flags): |
334 | 208 | review.flag( | 213 | review.flag( |
335 | 209 | 214 | ||
336 | === modified file 'src/reviewsapp/tests/test_handlers.py' | |||
337 | --- src/reviewsapp/tests/test_handlers.py 2011-04-12 07:44:43 +0000 | |||
338 | +++ src/reviewsapp/tests/test_handlers.py 2011-05-06 01:20:55 +0000 | |||
339 | @@ -28,10 +28,12 @@ | |||
340 | 28 | 'SubmitReviewHandlerTestCase', | 28 | 'SubmitReviewHandlerTestCase', |
341 | 29 | 'UsefulnessHandlerTestCase', | 29 | 'UsefulnessHandlerTestCase', |
342 | 30 | 'ShowUsefulnessHandlerTestCase', | 30 | 'ShowUsefulnessHandlerTestCase', |
343 | 31 | 'DeleteReviewHandlerTestCase', | ||
344 | 31 | ] | 32 | ] |
345 | 32 | 33 | ||
346 | 33 | 34 | ||
347 | 34 | import httplib | 35 | import httplib |
348 | 36 | from django.http import Http404 | ||
349 | 35 | from datetime import datetime, timedelta | 37 | from datetime import datetime, timedelta |
350 | 36 | from urllib import quote_plus | 38 | from urllib import quote_plus |
351 | 37 | 39 | ||
352 | @@ -51,6 +53,7 @@ | |||
353 | 51 | ShowReviewsHandler, | 53 | ShowReviewsHandler, |
354 | 52 | SubmitUsefulnessHandler, | 54 | SubmitUsefulnessHandler, |
355 | 53 | ShowUsefulnessHandler, | 55 | ShowUsefulnessHandler, |
356 | 56 | DeleteReviewHandler | ||
357 | 54 | ) | 57 | ) |
358 | 55 | from reviewsapp.models import ( | 58 | from reviewsapp.models import ( |
359 | 56 | Repository, | 59 | Repository, |
360 | @@ -837,6 +840,61 @@ | |||
361 | 837 | self.assertEqual(httplib.OK, first.status_code) | 840 | self.assertEqual(httplib.OK, first.status_code) |
362 | 838 | self.assertEqual(httplib.OK, second.status_code) | 841 | self.assertEqual(httplib.OK, second.status_code) |
363 | 839 | 842 | ||
364 | 843 | class DeleteReviewHandlerTestCase(TestCaseWithFactory): | ||
365 | 844 | def _delete_review_id(self, review_id, user=None): | ||
366 | 845 | if user is None: | ||
367 | 846 | user = self.factory.makeUser() | ||
368 | 847 | request = Mock() | ||
369 | 848 | request.POST = None | ||
370 | 849 | request.user = user | ||
371 | 850 | handler = DeleteReviewHandler() | ||
372 | 851 | return handler.create(request, review_id) | ||
373 | 852 | |||
374 | 853 | def test_delete_review(self): | ||
375 | 854 | review = self.factory.makeReview() | ||
376 | 855 | response = self._delete_review_id(review.id, review.reviewer) | ||
377 | 856 | self.assertEqual(review.id, response.id) | ||
378 | 857 | self.assertEqual(True,response.hide) | ||
379 | 858 | self.assertNotEqual(None, response.date_deleted) | ||
380 | 859 | |||
381 | 860 | def test_non_existent_review(self): | ||
382 | 861 | review = self.factory.makeReview() | ||
383 | 862 | self.assertRaises(Http404, self._delete_review_id, review.id+1, | ||
384 | 863 | review.reviewer) | ||
385 | 864 | |||
386 | 865 | def test_already_deleted(self): | ||
387 | 866 | review = self.factory.makeReview() | ||
388 | 867 | |||
389 | 868 | response_good = self._delete_review_id(review.id, review.reviewer) | ||
390 | 869 | self.assertEqual(review, response_good) | ||
391 | 870 | #try to delete a second time and check it raises a 404 error | ||
392 | 871 | self.assertRaises(Http404, self._delete_review_id, review.id, | ||
393 | 872 | review.reviewer) | ||
394 | 873 | |||
395 | 874 | def test_wrong_user(self): | ||
396 | 875 | review = self.factory.makeReview() | ||
397 | 876 | wrong_user = self.factory.makeUser(username='deleter') | ||
398 | 877 | self.assertRaises(Http404, self._delete_review_id, review.id, | ||
399 | 878 | wrong_user) | ||
400 | 879 | |||
401 | 880 | def test_delete_updates_stats(self): | ||
402 | 881 | software_item = self.factory.makeSoftwareItem(package_name='compiz-core', | ||
403 | 882 | ratings_total=0, ratings_average=0) | ||
404 | 883 | review = self.factory.makeReview(software_item=software_item, rating=5) | ||
405 | 884 | review2 = self.factory.makeReview(software_item=software_item, rating=3) | ||
406 | 885 | software_item = SoftwareItem.objects.get(id=software_item.id) | ||
407 | 886 | |||
408 | 887 | # 2 reviews with ratings of 5 + 3 = 8 (average of 4) | ||
409 | 888 | self.assertEqual(2, software_item.ratings_total) | ||
410 | 889 | self.assertEqual(4, software_item.ratings_average) | ||
411 | 890 | |||
412 | 891 | self._delete_review_id(review2.id, review2.reviewer) | ||
413 | 892 | software_item = SoftwareItem.objects.get(id=software_item.id) | ||
414 | 893 | |||
415 | 894 | # 1 review with rating 5 = average of 5 | ||
416 | 895 | self.assertEqual(1, software_item.ratings_total) | ||
417 | 896 | self.assertEqual(5, software_item.ratings_average) | ||
418 | 897 | |||
419 | 840 | 898 | ||
420 | 841 | class FlagReviewHandlerTestCase(TestCaseWithFactory): | 899 | class FlagReviewHandlerTestCase(TestCaseWithFactory): |
421 | 842 | def _flag_review_id(self, review_id, user=None, data=None): | 900 | def _flag_review_id(self, review_id, user=None, data=None): |
422 | 843 | 901 | ||
423 | === modified file 'src/reviewsapp/tests/test_rnrclient.py' | |||
424 | --- src/reviewsapp/tests/test_rnrclient.py 2011-04-19 12:32:23 +0000 | |||
425 | +++ src/reviewsapp/tests/test_rnrclient.py 2011-05-06 01:20:55 +0000 | |||
426 | @@ -29,6 +29,7 @@ | |||
427 | 29 | 'RnRReviewStatsTestCase', | 29 | 'RnRReviewStatsTestCase', |
428 | 30 | 'RnRServerStatusTestCase', | 30 | 'RnRServerStatusTestCase', |
429 | 31 | 'RnRSubmitReviewTestCase', | 31 | 'RnRSubmitReviewTestCase', |
430 | 32 | 'RnRDeleteReviewTestCase', | ||
431 | 32 | 'UsefulnessAPITestCase', | 33 | 'UsefulnessAPITestCase', |
432 | 33 | ] | 34 | ] |
433 | 34 | 35 | ||
434 | @@ -246,6 +247,31 @@ | |||
435 | 246 | self.assertEqual(review_detail.distroseries, review.distroseries()) | 247 | self.assertEqual(review_detail.distroseries, review.distroseries()) |
436 | 247 | invalidate_paginated_reviews_for(review) | 248 | invalidate_paginated_reviews_for(review) |
437 | 248 | 249 | ||
438 | 250 | class RnRDeleteReviewTestCase(RnRTxBaseTestCase): | ||
439 | 251 | |||
440 | 252 | def test_delete_review(self): | ||
441 | 253 | user = self.factory.makeUser() | ||
442 | 254 | review = self.factory.makeReview(reviewer=user) | ||
443 | 255 | token, consumer = self.factory.makeOAuthTokenAndConsumer(user=user) | ||
444 | 256 | |||
445 | 257 | auth = OAuthAuthorizer(token.token, token.token_secret, consumer.key, | ||
446 | 258 | consumer.secret) | ||
447 | 259 | |||
448 | 260 | lp_verify_packagename_method = ( | ||
449 | 261 | 'reviewsapp.utilities.WebServices.' | ||
450 | 262 | 'lp_verify_packagename_in_repository') | ||
451 | 263 | |||
452 | 264 | mock_verify_package = Mock() | ||
453 | 265 | mock_verify_package.return_value = True | ||
454 | 266 | |||
455 | 267 | api = RatingsAndReviewsAPI(auth=auth) | ||
456 | 268 | |||
457 | 269 | with patch(lp_verify_packagename_method, mock_verify_package): | ||
458 | 270 | response = api.delete_review(review_id=review.id) | ||
459 | 271 | |||
460 | 272 | self.assertEqual(review.id, response['id']) | ||
461 | 273 | self.assertNotEqual(None, response['date_deleted']) | ||
462 | 274 | |||
463 | 249 | 275 | ||
464 | 250 | class RnRSubmitReviewTestCase(RnRTxBaseTestCase): | 276 | class RnRSubmitReviewTestCase(RnRTxBaseTestCase): |
465 | 251 | 277 | ||
466 | 252 | 278 | ||
467 | === modified file 'src/reviewsapp/tests/test_views.py' | |||
468 | --- src/reviewsapp/tests/test_views.py 2011-04-20 16:47:48 +0000 | |||
469 | +++ src/reviewsapp/tests/test_views.py 2011-05-06 01:20:55 +0000 | |||
470 | @@ -80,6 +80,18 @@ | |||
471 | 80 | 80 | ||
472 | 81 | self.assertEqual(302, response.status_code) | 81 | self.assertEqual(302, response.status_code) |
473 | 82 | self.assertTrue('/openid/login' in response['Location']) | 82 | self.assertTrue('/openid/login' in response['Location']) |
474 | 83 | |||
475 | 84 | def test_moderation_excludes_deleted(self): | ||
476 | 85 | review_moderation = self.factory.makeReviewModeration() | ||
477 | 86 | review_moderation_deleted = self.factory.makeReviewModeration(deleted_review=True) | ||
478 | 87 | |||
479 | 88 | response = self._request_url_as_moderator() | ||
480 | 89 | |||
481 | 90 | review_moderations = response.context['review_moderations'] | ||
482 | 91 | |||
483 | 92 | self.assertEqual(1, len(review_moderations)) | ||
484 | 93 | self.assertEqual(review_moderation, review_moderations[0]) | ||
485 | 94 | self.assertNotEqual(review_moderation_deleted, review_moderations[0]) | ||
486 | 83 | 95 | ||
487 | 84 | def _request_url_as_moderator(self, data=None, user=None): | 96 | def _request_url_as_moderator(self, data=None, user=None): |
488 | 85 | # Helper to request the url as a moderator. | 97 | # Helper to request the url as a moderator. |
489 | 86 | 98 | ||
490 | === modified file 'src/reviewsapp/views/moderation.py' | |||
491 | --- src/reviewsapp/views/moderation.py 2011-03-31 11:09:37 +0000 | |||
492 | +++ src/reviewsapp/views/moderation.py 2011-05-06 01:20:55 +0000 | |||
493 | @@ -58,7 +58,8 @@ | |||
494 | 58 | def reviewmoderation_list(request): | 58 | def reviewmoderation_list(request): |
495 | 59 | """A filterable list of review moderations with a useful default.""" | 59 | """A filterable list of review moderations with a useful default.""" |
496 | 60 | 60 | ||
498 | 61 | review_moderations = ReviewModeration.objects.all() | 61 | #don't present reviews for moderation if they've been deleted |
499 | 62 | review_moderations = ReviewModeration.objects.filter(review__date_deleted=None) | ||
500 | 62 | next_url = reverse('rnr-reviewmoderation-list') | 63 | next_url = reverse('rnr-reviewmoderation-list') |
501 | 63 | 64 | ||
502 | 64 | # By default we'll be displaying pending moderations. | 65 | # By default we'll be displaying pending moderations. |
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: /code.launchpad .net/~aaronp/ rnr-server/ delete- review/ +merge/ 59658
https:/
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' api/handlers. py 2011-04-12 10:43:30 +0000 api/handlers. py 2011-05-02 13:21:35 +0000 sHandler' , ewHandler' , nessHandler' , ndler',
--- src/reviewsapp/
+++ src/reviewsapp/
@@ -27,6 +27,7 @@
'ShowReview
'SubmitRevi
'ShowUseful
+ 'DeleteReviewHa
]
import urllib eBadRequest, eNotFound, bidden,
@@ -37,6 +38,7 @@
HttpResponse,
HttpRespons
HttpRespons
+ HttpResponseFor
)
from django.shortcuts import get_object_or_404
from django.utils import simplejson
@@ -236,6 +238,23 @@
return review
+class DeleteReviewHan dler(BaseHandle r):
+ 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.
+ objects. get(id= review_ id) DoesNotExist: Found(' No matching review found')
+ def create(self, request, review_id):
+ try:
+ review = Review.
+ except Review.
+ return HttpResponseNot
We should be able to simplify the above with a single:
get_object_ or_404( Review, id=review_id)
+ delete( request. user)
+ try:
+ deleted = review.
+ ...