Code review comment for lp:~brian-murray/launchpad/bug-556489-distro-struct-sub

Hi, Brian.

I've had a chance to review the code itself now. Thanks for the great job here you did refactoring the tests! Well done!

I do, however, see a couple problems that need to be addressed before this is ready to land.

First, you do a great job of avoiding sample data in the test except for your admin test, which uses Foo Bar. Your commented out attempt at using a with block and logging in the admin celebrity is the right way to go. You can reference the currently logged in user inside that block. Look in lp.testing.tests.test_login and look at test_with_celebrity_logged_in and getLoggedInPerson for ideas of how to do this. If you need more help working this out, ping me on IRC.

Second, this work ensures that addBugSubscription won't succeed, but the user will still have a subscribe to bug mail link, which will then OOPS if that click-path is followed and the user tries to subscribe. The code that checks that the user can subscribe should be refactored into a helper method which can be called from the model and the view so that the link is never presented in the first place. See lp.bugs.browser.bugtask.BugTaskSearchListingMenu.subscribe for where to do this check in the view.

Again, feel free to ping me if you need to talk more about these changes.

Cheers,
deryck