Code review comment for lp:~aaronp/rnr-server/delete-review

> 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