On Fri, Jul 17, 2009 at 11:00 AM, Tim Penhey<email address hidden> wrote:
> Review: Needs Information
>> bazaar_identity has been changed to return 'lp:ubuntu/package' for source
>> package branches associated to the development focus package.
>
> My first thought on reading this was "for just the release pocket?", but I
> guess we'll see.
>

Yeah, just for the release pocket.

>> === modified file 'lib/lp/code/interfaces/branch.py'
>> --- lib/lp/code/interfaces/branch.py  2009-07-15 01:45:35 +0000
>> +++ lib/lp/code/interfaces/branch.py  2009-07-16 04:47:24 +0000
>> @@ -1131,6 +1130,10 @@
>>              'series': use_series.name}
>>
>>      if branch.sourcepackage is not None:
>> +        distro_package = branch.sourcepackage.distribution_sourcepackage
>> +        linked_branch = ICanHasLinkedBranch(distro_package)
>> +        if linked_branch.branch == branch:
>> +            return lp_prefix + linked_branch.bzr_identity
>
> What object traversal is really going on here?  And how will it impact getting
> bazaar_identities for branch listings?
>
> branch.sourcepackage -> No other queries
> .distribution_sourcepackage -> Other queries?

No queries, it's a virtual object.

> Adaption -> other queries?
>

No queries in adaptation either.

However, getting the bzr_identity from the adapted object requires
loading the currentseries from the DistroSeries table. You probably
want to have a look at Distribution.currentseries anyway to resolve
some of your other questions.

If possible, I'd rather land this branch now with some performance
issues than in 2 months time.

>> === modified file 'lib/lp/code/model/linkedbranch.py'
>> --- lib/lp/code/model/linkedbranch.py 2009-05-15 03:16:18 +0000
>> +++ lib/lp/code/model/linkedbranch.py 2009-07-16 05:38:41 +0000
>> @@ -30,6 +33,16 @@
>>          """See `ICanHasLinkedBranch`."""
>>          return self.product_series.branch
>>
>> +    @property
>> +    def bzr_identity(self):
>> +        """See `ICanHasLinkedBranch`."""
>> +        return '/'.join(
>> +            [self.product_series.product.name, self.product_series.name])
>
> I think that the bzr_identity here (and in following adapters) should really
> have the lp: prefix too (from the config variable).  Otherwise bzr_identity
> functions in different places return different types of strings.
>

Fair enough, although in some ways I'd prefer keeping the current
behaviour and picking a different name. It seems wrong to imbue this
code with knowledge of the name of the server.

Oh, I see later on you suggest the names 'bzr_path' and
'bzr_identity_path' for a property with the current behaviour. I like
that.

Changed.

> Also, it shouldn't really return anything if there is not a branch set.
>

Why do you say that?

>> @@ -45,6 +58,15 @@
>>          """See `ICanHasLinkedBranch`."""
>>          return ICanHasLinkedBranch(self.product.development_focus).branch
>>
>> +    @property
>> +    def bzr_identity(self):
>> +        """See `ICanHasLinkedBranch`."""
>> +        return self.product.name
>> +
>> +    def setBranch(self, branch, registrant=None):
>> +        """See `ICanHasLinkedBranch`."""
>> +
> ICanHasLinkedBranch(self.product.development_focus).setBranch(branch)
>

Your wrapping here is very weird.

> I think you should also pass the registrant through.  If we start to record
> it, it'll save changing this later.
>

Good point. Changed.

>> +
>>
>>  class PackageLinkedBranch:
>>      """Implement a linked branch for a source package pocket."""
>> @@ -61,3 +83,50 @@
>>          package = self.suite_sourcepackage.sourcepackage
>>          pocket = self.suite_sourcepackage.pocket
>>          return package.getBranch(pocket)
>> +
>> +    @property
>> +    def bzr_identity(self):
>> +        """See `ICanHasLinkedBranch`."""
>> +        return self.suite_sourcepackage.path
>> +
>> +    def setBranch(self, branch, registrant):
>> +        """See `ICanHasLinkedBranch`."""
>> +        package = self.suite_sourcepackage.sourcepackage
>> +        pocket = self.suite_sourcepackage.pocket
>> +        package.setBranch(pocket, branch, registrant)
>> +
>> +
>> +class DistributionPackageLinkedBranch:
>> +    """Implement a linked branch for an `IDistributionSourcePackage`."""
>> +
>> +    adapts(IDistributionSourcePackage)
>> +    implements(ICanHasLinkedBranch)
>> +
>> +    def __init__(self, distribution_sourcepackage):
>> +        self._distribution_sourcepackage = distribution_sourcepackage
>
> Why use a leading underscore here?  None of the other adpaters do this.
>

Because all of the other adapters ought to do it.

> If you think strongly that we should be using "private" variable, then go for
> consistence and change the other adapters too.
>

Done.

>> +
>> +    @property
>> +    def branch(self):
>> +        """See `ICanHasLinkedBranch`."""
>> +        development_package = (
>> +            self._distribution_sourcepackage.development_version)
>> +        if development_package is None:
>> +            return None
>> +        suite_sourcepackage = development_package.getSuiteSourcePackage(
>> +            PackagePublishingPocket.RELEASE)
>> +        return ICanHasLinkedBranch(suite_sourcepackage).branch
>> +
>> +    @property
>> +    def bzr_identity(self):
>> +        """See `ICanHasLinkedBranch`."""
>> +        return '/'.join(
>> +            [self._distribution_sourcepackage.distribution.name,
>> +             self._distribution_sourcepackage.sourcepackagename.name])
>> +
>> +    def setBranch(self, branch, registrant):
>> +        """See `ICanHasLinkedBranch`."""
>> +        development_package = (
>> +            self._distribution_sourcepackage.development_version)
>
> In the branch property you check to see if the developmen_package is None, but
> here you don't.  I'm assuming that either you want to check here, or not check
> above.  Which is it?
>

Well, what this really means is that I'm missing a test. If
development_package is None, we can't set the branch, but there's no
defined behaviour for what *does* happen.

I've added a test to make sure this raises NoSuchDistroSeries -- I
couldn't think of a better exception -- and have changed the code
accordingly.

>
>> +        suite_sourcepackage = development_package.getSuiteSourcePackage(
>> +            PackagePublishingPocket.RELEASE)
>> +        ICanHasLinkedBranch(suite_sourcepackage).setBranch(branch,
> registrant)
>>
>> === modified file 'lib/lp/code/model/tests/test_branch.py'
>> --- lib/lp/code/model/tests/test_branch.py    2009-07-08 04:57:51 +0000
>> +++ lib/lp/code/model/tests/test_branch.py    2009-07-16 03:22:24 +0000
>> @@ -418,9 +418,10 @@
>>          # If a branch is the development focus branch for a product, then
> it's
>>          # bzr identity is lp:product.
>>          branch = self.factory.makeProductBranch()
>> -        product = branch.product
>> -        removeSecurityProxy(product).development_focus.branch = branch
>> -        self.assertBzrIdentity(branch, product.name)
>> +        product = removeSecurityProxy(branch.product)
>> +        linked_branch = ICanHasLinkedBranch(product)
>> +        linked_branch.setBranch(branch)
>> +        self.assertBzrIdentity(branch, linked_branch.bzr_identity)
>
> Changing the bzr_identity of the linked_branch to be complete should just mean
> changing the assertBzrIdentity :)
>

Well, only because I've changed _all_ of the tests here to use
ICanHasLinkedBranch.

>>      def test_linked_to_series_and_dev_focus(self):
>> @@ -448,33 +450,54 @@
>>          # branch for a series, the bzr identity will be the storter of the
> two
>>          # URLs.
>>          branch = self.factory.makeProductBranch()
>> -        product = branch.product
>> -        removeSecurityProxy(product).development_focus.branch = branch
>> -        series = self.factory.makeProductSeries(product=product)
>> -        series.branch = branch
>> -        self.assertBzrIdentity(branch, product.name)
>> +        series = self.factory.makeProductSeries(product=branch.product)
>> +        product_link = ICanHasLinkedBranch(
>> +            removeSecurityProxy(branch.product))
>> +        series_link = ICanHasLinkedBranch(series)
>> +        product_link.setBranch(branch)
>> +        series_link.setBranch(branch)
>> +        self.assertBzrIdentity(branch, product_link.bzr_identity)
>>
>>      def test_junk_branch_always_unique_name(self):
>>          # For junk branches, the bzr identity is always based on the unique
>>          # name of the branch, even if it's linked to a product, product
> series
>>          # or whatever.
>>          branch = self.factory.makePersonalBranch()
>> -        product = self.factory.makeProduct()
>> -        removeSecurityProxy(product).development_focus.branch = branch
>> +        product = removeSecurityProxy(self.factory.makeProduct())
>> +        ICanHasLinkedBranch(product).setBranch(branch)
>
> This one has always bothered me.  We should not allow junk branches to be
> linked rather than just saying that their links don't show it.  What do you
> think?
>

As a general principle, I think we should do as little special-casing
of +junk as possible. It always comes back to bite us.

I personally think that we can do without this special case at all.
However, if pressed, I'd favour simply preventing junk branches from
being linked. I think it's for another branch though.

>>          self.assertBzrIdentity(branch, branch.unique_name)
>>
>> -    def test_linked_to_package_release(self):
>> -        # If a branch is linked to the release pocket of a package, then
> the
>> +    def test_linked_to_package(self):
>> +        # If a branch is linked to a pocket of a package, then the
>>          # bzr identity is the path to that package.
>>          branch = self.factory.makePackageBranch()
>> +        # Have to pick something that's not RELEASE in order to guarantee
> that
>> +        # it's not the dev focus source package.
>> +        pocket = PackagePublishingPocket.BACKPORTS
>> +        linked_branch = ICanHasLinkedBranch(
>> +            branch.sourcepackage.getSuiteSourcePackage(pocket))
>>          registrant = getUtility(
>>              ILaunchpadCelebrities).ubuntu_branches.teamowner
>>          login_person(registrant)
>
> To save with the login, logout dance, how about we just remove the security
> proxy as with the other tests, but still pass the registrant through.
>

Doesn't work. Internally, the link is made by code that looks like:
  getUtility(ISeriesSourcePackageBranchSet).new(branch, pocket,
distroseries, sourcepackagename)

getUtility guarantees that the object is secured, and 'new' requires
lp.Edit permissions.

>> -        branch.sourcepackage.setBranch(
>> -            PackagePublishingPocket.RELEASE, branch, registrant)
>> +        linked_branch.setBranch(branch, registrant)
>>          logout()
>>          login(ANONYMOUS)
>> -        self.assertBzrIdentity(branch, branch.sourcepackage.path)
>> +        self.assertBzrIdentity(branch, linked_branch.bzr_identity)
>> +
>> +    def test_linked_to_dev_package(self):
>> +        # If a branch is linked to the development focus version of a
> package
>> +        # then the bzr identity is distro/package.
>> +        sourcepackage = self.factory.makeSourcePackage()
>> +        distro_package = sourcepackage.distribution_sourcepackage
>> +        branch = self.factory.makePackageBranch(
>> +            sourcepackage=distro_package.development_version)
>> +        linked_branch = ICanHasLinkedBranch(distro_package)
>> +        registrant = getUtility(
>> +            ILaunchpadCelebrities).ubuntu_branches.teamowner
>> +        run_with_login(
>> +            registrant,
>> +            linked_branch.setBranch, branch, registrant)
>
> Again, here, lets not complicate the test by worrying about who is doing the
> linking.  We aren't testing the security policies here, so let's just remove
> the security proxy.
>

Ditto.

>> +        self.assertBzrIdentity(branch, linked_branch.bzr_identity)
>> === modified file 'lib/lp/code/model/tests/test_branchlookup.py'
>> --- lib/lp/code/model/tests/test_branchlookup.py      2009-06-02 13:52:48 +0000
>> +++ lib/lp/code/model/tests/test_branchlookup.py      2009-07-16 05:52:44 +0000
>> @@ -455,6 +470,24 @@
>>              (branch, 'foo/bar/baz'),
>>              self.branch_lookup.getByLPPath(path))
>>
>> +    def test_resolve_distro_package_branch(self):
>> +        # getByLPPath returns the branch associated with the distribution
>> +        # source package referred to by the path.
>> +        sourcepackage = self.factory.makeSourcePackage()
>> +        branch =
> self.factory.makePackageBranch(sourcepackage=sourcepackage)
>> +        distro_package = sourcepackage.distribution_sourcepackage
>> +        ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
>> +        registrant = ubuntu_branches.teamowner
>> +        run_with_login(
>> +            registrant,
>> +            ICanHasLinkedBranch(distro_package).setBranch, branch,
> registrant)
>> +        self.assertEqual(
>> +            (branch, None),
>> +            self.branch_lookup.getByLPPath(
>> +                '%s/%s' % (
>> +                    distro_package.distribution.name,
>> +                    distro_package.sourcepackagename.name)))
>> +
>>      def test_no_product_series_branch(self):
>>          # getByLPPath raises `NoLinkedBranch` if there's no branch
> registered
>>          # linked to the requested series.
>> @@ -501,6 +534,12 @@
>>              self.branch_lookup.getByLPPath, distribution.name)
>>          self.assertEqual(distribution, exception.component)
>>
>> +    def test_distribution_with_no_series(self):
>> +        distro_package = self.factory.makeDistributionSourcePackage()
>> +        path = ICanHasLinkedBranch(distro_package).bzr_identity
>> +        self.assertRaises(
>> +            NoLinkedBranch, self.branch_lookup.getByLPPath, path)
>> +
>>      def test_project_linked_branch(self):
>>          # Projects cannot have linked branches, so `getByLPPath` raises a
>>          # `CannotHaveLinkedBranch` error if we try to get the linked branch
>>
>> === modified file 'lib/lp/code/model/tests/test_linkedbranch.py'
>> --- lib/lp/code/model/tests/test_linkedbranch.py      2009-05-24 18:09:51 +0000
>> +++ lib/lp/code/model/tests/test_linkedbranch.py      2009-07-16 05:38:41 +0000
>> @@ -18,11 +18,11 @@
>>  from lp.testing import run_with_login, TestCaseWithFactory
>>
>>
>> -class TestLinkedBranch(TestCaseWithFactory):
>> +class TestProductSeriesLinkedBranch(TestCaseWithFactory):
>>
>>      layer = DatabaseFunctionalLayer
>>
>> -    def test_product_series(self):
>> +    def test_branch(self):
>>          # The linked branch of a product series is its branch attribute.
>>          product_series = self.factory.makeProductSeries()
>>          product_series.branch = self.factory.makeProductBranch(
>> @@ -30,7 +30,29 @@
>>          self.assertEqual(
>>              product_series.branch,
> ICanHasLinkedBranch(product_series).branch)
>>
>> -    def test_product(self):
>> +    def test_setBranch(self):
>> +        # setBranch sets the linked branch of the product series.
>> +        product_series = self.factory.makeProductSeries()
>> +        branch = self.factory.makeProductBranch(
>> +            product=product_series.product)
>> +        ICanHasLinkedBranch(product_series).setBranch(branch)
>> +        self.assertEqual(branch, product_series.branch)
>> +
>> +    def test_bzr_identity(self):
>> +        # The bzr_identity of a product series linked branch is
>> +        # product/product_series.
>> +        product_series = self.factory.makeProductSeries()
>> +        bzr_identity = '%s/%s' % (
>> +            product_series.product.name, product_series.name)
>> +        self.assertEqual(
>> +            bzr_identity, ICanHasLinkedBranch(product_series).bzr_identity)
>
> If you are against changing the implementation of bzr_identity for
> ICanHasLinkedBranch then perhaps change it to bzr_identity_path or bzr_path?
>

Yes. I'll rename it to bzr_path. See earlier comment.

>> === modified file 'lib/lp/registry/interfaces/distributionsourcepackage.py'
>> --- lib/lp/registry/interfaces/distributionsourcepackage.py   2009-07-10
> 15:38:46 +0000
>> +++ lib/lp/registry/interfaces/distributionsourcepackage.py   2009-07-16
> 04:48:10 +0000
>> @@ -68,6 +68,10 @@
>>          "The list of all releases of this source package "
>>          "in this distribution.")
>>
>> +    development_version = Attribute(
>> +        'The development version of this source package. None if there is
> no '
>> +        'such package.')
>
> When exactly would None occur?
>

When there's no current series for the distribution. Updated to say so.

>> === modified file 'lib/lp/registry/model/distributionsourcepackage.py'
>> --- lib/lp/registry/model/distributionsourcepackage.py        2009-07-10 15:38:46
> +0000
>> +++ lib/lp/registry/model/distributionsourcepackage.py        2009-07-16 04:48:10
> +0000
>> @@ -89,6 +89,14 @@
>>              self.sourcepackagename.name, self.distribution.displayname)
>>
>>      @property
>> +    def development_version(self):
>> +        """See `IDistributionSourcePackage`."""
>> +        series = self.distribution.currentseries
>
> If a distribution has at least one series, is currentseries always set?
>

Yes.

For evidence of this, see the implementation of Distribution.currentseries.

>> +        if series is None:
>> +            return None
>> +        return series.getSourcePackage(self.sourcepackagename)
>> +
>> +    @property
>>      def _self_in_database(self):
>>          """Return the equivalent database-backed record of self."""
>>          # XXX: allenap 2008-11-13 bug=297736: This is a temporary
>> === modified file 'lib/lp/registry/tests/test_distributionsourcepackage.py'
>> --- lib/lp/registry/tests/test_distributionsourcepackage.py   2009-07-10
> 15:38:46 +0000
>> +++ lib/lp/registry/tests/test_distributionsourcepackage.py   2009-07-15
> 07:55:56 +0000
>> @@ -109,5 +109,25 @@
>>
>>          self.assertEqual(related_archive_names, ['gedit-nightly'])
>>
>> +    def test_development_version(self):
>> +        # IDistributionSourcePackage.development_version is the
> ISourcePackage
>> +        # for the current series of the distribution.
>> +        dsp = self.factory.makeDistributionSourcePackage()
>> +        series =
> self.factory.makeDistroRelease(distribution=dsp.distribution)
>> +        self.assertEqual(series, dsp.distribution.currentseries)
>> +        development_version =
> dsp.distribution.currentseries.getSourcePackage(
>> +            dsp.sourcepackagename)
>> +        self.assertEqual(development_version, dsp.development_version)
>> +
>> +    def test_development_version_no_current_series(self):
>> +        # IDistributionSourcePackage.development_version is the
> ISourcePackage
>> +        # for the current series of the distribution.
>> +        dsp = self.factory.makeDistributionSourcePackage()
>> +        currentseries = dsp.distribution.currentseries
>> +        # The current series is None by default.
>
> The interesting question becomes "is it ever None if there are any distro
> series?"
>

See above.

>> +        self.assertIs(None, currentseries)
>> +        self.assertEqual(None, dsp.development_version)
>> +
>> +
>>  def test_suite():
>>      return unittest.TestLoader().loadTestsFromName(__name__)
>>
>
>
> On the whole, a nice branch to review.
>

Thanks.

> The main questions I have resolve around the currentseries for a distribution,
> and the actual content of ICanHasLinkedBranch.bzr_identity.
>

Cool. I hope they're addressed here.

jml