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

> 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