launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

On 27/07/12 22:51, Ian Booth wrote:
> 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's currently hardcoded to always return the same value, so there's no
need for admins to have an escape hatch. But InformationTypePortletMixin
needed the same signature on both types, so I left the unused argument.
The branch escape hatch will probably be temporary, in which case the
argument might disappear entirely once the migration is complete. Good
point on the name, though -- that was a bad copy+paste.

> 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.

The existing model support was very limited, unused, and didn't fit with
project configuration. I felt it was probably easier to tear the few
existing bits of it down and bring it back up in its final form, given
that it's never been used on production.

There's a few lines left in transitionToInformationType to handle it
incorrectly (Proprietary is forbidden from being set through the API,
but the method otherwise allows it on any bug affecting a single pillar,
even if there's not even a commercial subscription). The main bit of the
Proprietary implementation was in InformationTypeVocabulary, which
worked off has_commercial_subscription rather than project
configuration, and tended to poke around in the Bugs model far more than
a supposedly agnostic Registry vocab should.

There were only a few tests for Proprietary, and they all tended to be
integration tests that were probably at the wrong level, and they all
just verified that Proprietary shows up if there's a commercial
subscription. It wasn't feasible to fully implement bug_sharing_policy
in this branch, so the tests had no obvious way to enable Proprietary.

> 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.

Yeah, I was wary, but it all works fine. The JS that wants it doesn't
even get close to being run, as the form sends you off to
Product:+filebug before it has a chance.


-- 
https://code.launchpad.net/~wgrant/launchpad/bug-informationtype-refactor/+merge/117002
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.