Code review comment for lp:~wgrant/launchpad/export-bug-nominations

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Well done!

Good branch, though there are some little warts that came up on IRC and
which you fixed interactively:

 * Don't return None when looking up a malformed id if a well-formed id
   that doesn't belong to any bug nomination reaises NotFoundError.

 * Don't over-test isApproved() in the doctest, though it was fine as a
   transitional check during development. It's already covered in
   bug-nominations.txt.

 * Better not export canApprove() for any person, because it may expose
   private team memberships. Just make it apply to the current user:
   "Can I approve this?"

 * Test tearDown belongs just below setUp.

 * Don't derive a test case from another, but lift the commonality up
   into a mixin class.

 * You inherited a few small pieces of lint; should be easy to clean up.

With that out of the way, r=me.

Jeroen

review: Approve

« Back to merge proposal