Review: Needs Information

Like the implementation of getAllowedInformationTypes() on IBranch, should we 
allow admins to access to all information types? If not, I'd like to see a 
comment in the docstring stating why the user parameter is ignored. Also, the 
interface calls the parameter 'user', the class calls it 'who'. These should be 
consistent.

It is really the case that there's no model support for Proprietary? If a 
project is a commercial project, then Proprietary is allowed isn't it? (so long 
as the feature flag allows it; well, assuming the feature flag is not deleted 
in this branch). If we take this approach, then there's no need to delete an 
number of the tests. I'd rather not delete code only to add it back later.

With the removal of the items from the json cache for +filebug on project 
groups, did you test this this fully locally? I can't recall if there was an 
issue at one stage with not having the cache fully populated, perhaps there 
wasn't, I'm not sure.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-informationtype-refactor/+merge/117002
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Reply via email to