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

Revision history for this message
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.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.

« Back to merge proposal