Code review comment for lp:~james-w/launchpad/package-merge-proposal-permissions

Revision history for this message
Jonathan Lange (jml) wrote :

Good catch. This is very much how the permission should have been designed in the first place. A few small points:

 * Remove the is_admin and is_bzr_expert checks from the merge proposal permission checks. They are already got with the new permission you add.
 * The new test should be test_package... not test_packge...
 * Why do you set "self.permission_set"?
 * Why do you have an "if" statement in a test? It smells like the behaviour varies from run to run, which is bad for a test.
 * Ideally, there would be two unit tests, with the second round of assertions being split out into a second test.

review: Needs Fixing

« Back to merge proposal