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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
