launchpad-reviewers team mailing list archive

[Ines Almeida <mp+489721@xxxxxxxxxxxxxxxxxx>]

Looks good, added a couple of questions

Diff comments:

> diff --git a/lib/lp/bugs/browser/bugtask.py b/lib/lp/bugs/browser/bugtask.py
> index 081825d..91e3314 100644
> --- a/lib/lp/bugs/browser/bugtask.py
> +++ b/lib/lp/bugs/browser/bugtask.py
> @@ -343,23 +346,18 @@ class BugTargetTraversalMixin:
>          # anonymous user is presented with a login screen at the correct URL,
>          # rather than making it look as though this task was "not found",
>          # because it was filtered out by privacy-aware code.
> -        is_external_package = IExternalPackage.providedBy(context)
> +
> +        # Check if it uses +external url
> +        is_external = IExternalURL.providedBy(context)
> +
>          for bugtask in bug.bugtasks:
>              target = bugtask.target
>  
> -            if is_external_package:
> -                # +external url lacks necessary data, so we only match
> -                # distribution and sourcepackagename, then using +bugktask we
> -                # can jump to the right one
> -                if (
> -                    IExternalPackage.providedBy(target)
> -                    and target.sourcepackagename == context.sourcepackagename
> -                    and target.distribution == context.distribution
> -                ):
> +            if is_external:
> +                if context.isMatching(target):

Nice, this looks way cleaner

> +                    # Security proxy the object on the way out
>                      return getUtility(IBugTaskSet).get(bugtask.id)
> -
>              elif target == context:
> -                # Security proxy the object on the way out
>                  return getUtility(IBugTaskSet).get(bugtask.id)
>  
>          # If we've come this far, there's no task for the requested context.
> @@ -1883,22 +1881,45 @@ def bugtask_sort_key(bugtask):
>              None,
>              None,
>          )
> +    # Version should only be compared to items with same object type
>      elif ISourcePackage.providedBy(bugtask.target):
>          key = (
>              bugtask.target.sourcepackagename.name,
>              bugtask.target.distribution.displayname,
> +            None,

Why do we need to added these None here, but not in the `IDistributionSourcePackage` section above?

> +            None,
> +            Version(bugtask.target.distroseries.version),
> +            None,
> +        )
> +    elif IExternalPackageSeries.providedBy(bugtask.target):
> +        key = (
> +            bugtask.target.sourcepackagename.name,
> +            bugtask.target.distribution.displayname,
> +            bugtask.target.packagetype,
> +            bugtask.target.channel,
>              Version(bugtask.target.distroseries.version),
>              None,
>              None,
>              None,
>          )
>      elif IProduct.providedBy(bugtask.target):
> -        key = (None, None, None, bugtask.target.displayname, None, None)
> +        key = (
> +            None,
> +            None,
> +            None,
> +            None,
> +            None,
> +            bugtask.target.displayname,
> +            None,
> +            None,
> +        )
>      elif IProductSeries.providedBy(bugtask.target):
>          key = (
>              None,
>              None,
>              None,
> +            None,
> +            None,
>              bugtask.target.product.displayname,
>              bugtask.target.name,
>              None,
> diff --git a/lib/lp/bugs/browser/tests/test_bugtask_navigation.py b/lib/lp/bugs/browser/tests/test_bugtask_navigation.py
> index e30f0ce..bb41781 100644
> --- a/lib/lp/bugs/browser/tests/test_bugtask_navigation.py
> +++ b/lib/lp/bugs/browser/tests/test_bugtask_navigation.py
> @@ -162,15 +187,50 @@ class TestBugTaskTraversal(TestCaseWithFactory):
>              ),
>          )
>  
> +    def test_traversal_to_default_external_package_series_bugtask(self):
> +        # Test that a traversing to a bug with an external package series
> +        # as default bugtask redirects to the bug's default bugtask using
> +        # +bugtask/id.
> +        bug = self.factory.makeBug(target=self.ep)
> +        bug_url = canonical_url(bug, rootsite="bugs")
> +
> +        # We need to create a bugtask in the distribution before creating it in
> +        # the distroseries
> +        eps_bugtask = self.factory.makeBugTask(bug=bug, target=self.eps)
> +
> +        # Deleting the distribution bugtask to change the default one
> +        login_person(bug.owner)
> +        bug.default_bugtask.delete()
> +        self.assertEqual(eps_bugtask, bug.default_bugtask)
> +
> +        obj, view, request = test_traverse(bug_url)
> +        view()

What does this `view()` do here? Is it relevant for the test?

> +        naked_view = removeSecurityProxy(view)
> +        self.assertEqual(303, request.response.getStatus())
> +        self.assertEqual(
> +            naked_view.target,
> +            canonical_url(bug.default_bugtask, rootsite="bugs"),
> +        )
> +        self.assertEqual(
> +            removeSecurityProxy(view).target,
> +            "http://bugs.launchpad.test/%s/%s/+external/%s/+bug/%d/+bugtask/%s";
> +            % (
> +                bug.default_bugtask.target.distribution.name,
> +                bug.default_bugtask.distroseries.name,
> +                bug.default_bugtask.target.name,
> +                bug.default_bugtask.bug.id,
> +                bug.default_bugtask.id,
> +            ),
> +        )
> +
>      def test_traversal_to_default_external_package_bugtask_on_api(self):
>          # Traversing to a bug with an external package as default task
>          # redirects to the +bugtask/id also in the API.
> -        ep = self.factory.makeExternalPackage()
> -        bug = self.factory.makeBug(target=ep)
> +        bug = self.factory.makeBug(target=self.ep)
>          obj, view, request = test_traverse(
>              "http://api.launchpad.test/1.0/%s/+bug/%d";
>              % (
> -                removeSecurityProxy(ep).distribution.name,
> +                removeSecurityProxy(self.ep).distribution.name,
>                  bug.default_bugtask.bug.id,
>              )
>          )
> diff --git a/lib/lp/registry/browser/externalpackageseries.py b/lib/lp/registry/browser/externalpackageseries.py
> new file mode 100644
> index 0000000..9e8f1a6
> --- /dev/null
> +++ b/lib/lp/registry/browser/externalpackageseries.py
> @@ -0,0 +1,70 @@
> +# Copyright 2009-2025 Canonical Ltd.  This software is licensed under the

nit: chnage this to Copyright 2025 Canonical (no need for a range, and add the up-to-date year)

> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__all__ = [
> +    "ExternalPackageSeriesBreadcrumb",
> +    "ExternalPackageSeriesNavigation",
> +    "ExternalPackageSeriesFacets",
> +]
> +
> +
> +from zope.interface import implementer
> +
> +from lp.app.interfaces.headings import IHeadingBreadcrumb
> +from lp.bugs.browser.bugtask import BugTargetTraversalMixin
> +from lp.bugs.browser.structuralsubscription import (
> +    StructuralSubscriptionTargetTraversalMixin,
> +)
> +from lp.registry.interfaces.externalpackageseries import IExternalPackageSeries
> +from lp.services.webapp import (
> +    Navigation,
> +    StandardLaunchpadFacets,
> +    canonical_url,
> +    redirection,
> +    stepto,
> +)
> +from lp.services.webapp.breadcrumb import Breadcrumb
> +from lp.services.webapp.interfaces import IMultiFacetedBreadcrumb
> +
> +
> +@implementer(IHeadingBreadcrumb, IMultiFacetedBreadcrumb)
> +class ExternalPackageSeriesBreadcrumb(Breadcrumb):
> +    """Builds a breadcrumb for an `IExternalPackageSeries`."""
> +
> +    rootsite = "bugs"
> +
> +    @property
> +    def text(self):
> +        return "%s external package in %s" % (
> +            self.context.sourcepackagename.name,
> +            self.context.distroseries.named_version,
> +        )
> +
> +
> +class ExternalPackageSeriesFacets(StandardLaunchpadFacets):
> +    usedfor = IExternalPackageSeries
> +    enable_only = [
> +        "bugs",
> +    ]
> +
> +
> +class ExternalPackageSeriesNavigation(
> +    Navigation,
> +    BugTargetTraversalMixin,
> +    StructuralSubscriptionTargetTraversalMixin,
> +):
> +    usedfor = IExternalPackageSeries
> +
> +    @redirection("+editbugcontact")
> +    def redirect_editbugcontact(self):
> +        return "+subscribe"
> +
> +    @stepto("+filebug")
> +    def filebug(self):
> +        """Redirect to the IExternalPackage +filebug page."""
> +        external_package = self.context.distribution_sourcepackage
> +
> +        redirection_url = canonical_url(external_package, view_name="+filebug")
> +        if self.request.form.get("no-redirect") is not None:
> +            redirection_url += "?no-redirect"
> +        return self.redirectSubTree(redirection_url, status=303)
> diff --git a/lib/lp/registry/interfaces/externalpackageseries.py b/lib/lp/registry/interfaces/externalpackageseries.py
> new file mode 100644
> index 0000000..8aef040
> --- /dev/null
> +++ b/lib/lp/registry/interfaces/externalpackageseries.py
> @@ -0,0 +1,90 @@
> +# Copyright 2009, 2025 Canonical Ltd.  This software is licensed under the

nit: same about copyright (and a few others below)

> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""External Package Series interface."""
> +
> +__all__ = [
> +    "IExternalPackageSeries",
> +]
> +
> +from lazr.restful.declarations import exported, exported_as_webservice_entry
> +from lazr.restful.fields import Reference
> +from zope.interface import Attribute
> +from zope.schema import TextLine
> +
> +from lp import _
> +from lp.app.interfaces.launchpad import IHeadingContext
> +from lp.bugs.interfaces.bugtarget import IBugTarget, IHasOfficialBugTags
> +from lp.registry.interfaces.distribution import IDistribution
> +from lp.registry.interfaces.distroseries import IDistroSeries
> +from lp.registry.interfaces.externalpackage import IExternalURL
> +from lp.registry.interfaces.role import IHasDrivers
> +
> +
> +@exported_as_webservice_entry(as_of="beta")
> +class IExternalPackageSeriesView(
> +    IHeadingContext,
> +    IBugTarget,
> +    IHasOfficialBugTags,
> +    IHasDrivers,
> +    IExternalURL,
> +):
> +    """`IExternalPackageSeries` attributes that require launchpad.View."""
> +
> +    packagetype = Attribute("The package type")
> +
> +    channel = Attribute("The package channel")
> +
> +    display_channel = TextLine(title=_("Display channel name"), readonly=True)
> +
> +    distribution = exported(
> +        Reference(IDistribution, title=_("The distribution."))
> +    )
> +    distroseries = exported(
> +        Reference(IDistroSeries, title=_("The distroseries."))
> +    )
> +    sourcepackagename = Attribute("The source package name.")
> +
> +    distribution_sourcepackage = Attribute(
> +        "The IExternalPackage for this external package series."
> +    )
> +
> +    name = exported(
> +        TextLine(title=_("The source package name as text"), readonly=True)
> +    )
> +    display_name = exported(
> +        TextLine(title=_("Display name for this package."), readonly=True)
> +    )
> +    displayname = Attribute("Display name (deprecated)")
> +    title = exported(
> +        TextLine(title=_("Title for this package."), readonly=True)
> +    )
> +
> +    drivers = Attribute("The drivers for the distroseries.")
> +
> +    def isMatching(other):
> +        """See `IExternalURL`."""
> +
> +    def __eq__(other):
> +        """IExternalPackageSeries comparison method.
> +
> +        ExternalPackageSeries compare equal only if their fields compare equal.
> +        """
> +
> +    def __ne__(other):
> +        """IExternalPackageSeries comparison method.
> +
> +        External packages compare not equal if either of their
> +        fields compare not equal.
> +        """
> +
> +
> +@exported_as_webservice_entry(as_of="beta")
> +class IExternalPackageSeries(
> +    IExternalPackageSeriesView,
> +):
> +    """Represents an ExternalPackage in a distroseries.
> +
> +    Create IExternalPackageSeries by invoking
> +    `IDistroSeries.getExternalPackageSeries()`.
> +    """


-- 
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/489721
Your team Launchpad code reviewers is requested to review the proposed merge of ~enriqueesanchz/launchpad:add-external-package-series into launchpad:master.