Code review comment for lp:~stevenk/launchpad/db-add-derivedistroseries-api

Hi Steven,

I don't really understand what a derived distro series is, but it
sounds good, and triggering a job to do it seems neat.

I have quite a few comments, some trivial, but some do need work. I'm
mostly concerned with the way authorization is handled and with test
coverage.

Gavin.

[1]

+ @operation_parameters(
+ name=TextLine(
+ title=_("The name of the distroseries to derive."),
+ required=True),
...

Here, and with the other parameters, consider using copy_field() to
copy the relevant fields from IDistroSeries instead of defining them
again. Some of them provide extra validation, and it ensures that
things remain in sync.

[2]

+ status=TextLine(
+ title=_("Status the derived distroseries to created is."),

s/created/create/

[3]

+ if not user.inTeam('soyuz-team'):
+ raise Unauthorized

Access should be configured in ZCML, see configure.zcml in
lib/lp/registry/.

You could:

- Write tests to ensure that only members of ~soyuz-team can access
  deriveDistroSeries().

- Define a new security adapter that only permits, say, launchpad.Edit
  to ~soyuz-team,

- Define a new interface, IDistroSeriesDerivation, with only
  deriveDistroSeries() in it, and get IDistroSeries to inherit from
  it.

- Add something like the following to the ZCML:

    <require
        permission="launchpad.Edit"
        interface="lp.registry.interfaces.distroseries.IDistroSeriesDerivation"/>

[4]

+ #if self.distribution is ubuntu and not user.in_tech_board
+ #elsif not user a driver

I guess you can drop this.

[5]

+ for param in (displayname, summary, description, version):
+ if not param:

Avoid using implicit truthiness :) Or falsiness. Instead:

    if param is None:
        raise ...

Or, depending on what you need:

    if param is None or len(param) == 0:
        raise ...

[6]

Back to to the parameters:

+ arches=List(
+ title=_("The list of arches to copy to the derived "
+ "distroseries."),
+ required=False),

Is "arches" a standard term? If not, consider expanding this to
"architectures", or whatever it's meant to be, "archbishops" perhaps.

[7]

+ child = getUtility(IDistroSeriesSet).new(

I'm probably missing something, but IDistroSeriesSet does not define a
new() method, and it's not implemented on DistroSeriesSet either. That
makes me suspect that this part of the conditional is not tested.

[8]

+ raise DerivationError(
+ "DistroSeries %s parent series isn't %s" % (
+ child.name, self.name))

Perhaps this exception could explain why this is a requirement too. Or
is it blindingly obvious to someone who expects to use this function.

[9]

+ ids = InitialiseDistroSeries(child)

Can you use a longer and more meaningful variable name?

[10]

+We can't call .deriveDistroSeries() with a distroseries that isn't the

Trailing whitespace.