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.
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. eleted and ReviewNotUsersOwn) deleted)
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.