launchpad-reviewers team mailing list archive

[Ines Almeida <mp+489379@xxxxxxxxxxxxxxxxxx>]

Added a few comments and questions.

I had never tried adding this sort of new URL traverse, great job finding all of these out. It all makes sense even if at times it feels a little over complicated - I think it's all due to how these things are set up in such a flexible way

Diff comments:

> diff --git a/lib/lp/bugs/browser/bugtask.py b/lib/lp/bugs/browser/bugtask.py
> index 9ec5f1a..68d90f1 100644
> --- a/lib/lp/bugs/browser/bugtask.py
> +++ b/lib/lp/bugs/browser/bugtask.py
> @@ -287,6 +288,27 @@ def get_visible_comments(comments, user=None):
>      return visible_comments
>  
>  
> +@implementer(ICanonicalUrlData)
> +class BugTaskURL:
> +    """External package URL creation rules."""

Afaics this isn't just for ExternalPackages - I'd update this to say it's a general bugtask URL creation rules, with an extra sentence mentioning that for external packages we do X

> +
> +    rootsite = "bugs"
> +
> +    def __init__(self, context):
> +        self.context = context
> +
> +    @property
> +    def inside(self):
> +        return self.context.target
> +
> +    @property
> +    def path(self):
> +        if IExternalPackage.providedBy(self.context.target):
> +            return f"+bug/{self.context.bug.id}/+bugtask/{self.context.id}"
> +
> +        return "+bug/%s" % self.context.bug.id
> +
> +
>  class BugTargetTraversalMixin:
>      """Mix-in in class that provides .../+bug/NNN traversal."""
>  
> @@ -320,29 +342,24 @@ 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.
> -
> -        externalpackage_bugtask = None
> +        is_external_package = IExternalPackage.providedBy(context)
>  
>          for bugtask in bug.bugtasks:
> -            if bugtask.target == context:
> -                # Security proxy this object on the way out.
> -                return getUtility(IBugTaskSet).get(bugtask.id)
> -
> +            target = bugtask.target
>              if (
> -                externalpackage_bugtask is None
> -                and IExternalPackage.providedBy(bugtask.target)
> +                target == context
> +                or is_external_package
> +                and IExternalPackage.providedBy(target)
> +                and target.sourcepackagename == context.sourcepackagename
>              ):
> -                # enriqueesanchz 2025-07-15 TODO: set +external urls for
> -                # ExternalPackages. Currently we select the first
> -                # ExternalPackage that appears if we don't have other match
> -                externalpackage_bugtask = getUtility(IBugTaskSet).get(
> -                    bugtask.id
> -                )
> +                # for ExternalPackage we select the first package whose name is

Why? It feels like we might as well return if `target == context` else return the general one. This seems to be adding complexity without a real purpose. Can you explain why this would be needed?

> +                # equal to the one from the context and later we will target
> +                # other bugtask if required
>  
> -        # If we've come this far, there's no task for the requested context.
> -        if externalpackage_bugtask:
> -            return externalpackage_bugtask
> +                # Security proxy this 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.
>          # If we are attempting to navigate past the non-existent bugtask,
>          # we raise NotFound error. eg +delete or +edit etc.
>          # Otherwise we are simply navigating to a non-existent task and so we
> @@ -365,6 +382,18 @@ class BugTaskNavigation(Navigation):
>  
>      usedfor = IBugTask
>  
> +    @stepthrough("+bugtask")
> +    def traverse_bugtask(self, id):
> +        bugtask = getUtility(IBugTaskSet).get(int(id))
> +        # Jumping to another bug is not allowed
> +        if bugtask.bug.id != self.context.bug.id:
> +            return
> +        # Jumping to another sourcepackagename is not allowed
> +        if bugtask.sourcepackagename != self.context.sourcepackagename:

What about distribution?

> +            return
> +
> +        return bugtask
> +
>      @stepthrough("attachments")
>      def traverse_attachments(self, name):
>          """traverse to an attachment by id."""
> diff --git a/lib/lp/bugs/browser/tests/test_buglisting.py b/lib/lp/bugs/browser/tests/test_buglisting.py
> index f11f586..8f0d5fb 100644
> --- a/lib/lp/bugs/browser/tests/test_buglisting.py
> +++ b/lib/lp/bugs/browser/tests/test_buglisting.py
> @@ -50,6 +54,23 @@ class TestBugTaskSearchListingPage(BrowserTestCase):
>              extract_text(top_portlet[0]),
>          )
>  
> +    def test_externalpackage_unknown_bugtracker_message(self):

Can you add a quick comment in each test explaining what it's testing?

> +        ep = self._makeExternalPackage()
> +        url = canonical_url(ep, rootsite="bugs")
> +        browser = self.getUserBrowser(url)
> +        top_portlet = find_tags_by_class(browser.contents, "top-portlet")
> +        self.assertTrue(
> +            len(top_portlet) > 0, "Tag with class=top-portlet not found"
> +        )
> +        # An external package from url will use unknown type
> +        self.assertTextMatchesExpressionIgnoreWhitespace(
> +            """
> +            ep - Unknown in Ep-distro
> +            does not use Launchpad for bug tracking.
> +            Getting started with bug tracking in Launchpad.""",
> +            extract_text(top_portlet[0]),
> +        )
> +
>      def test_distributionsourcepackage_unknown_bugtracker_no_button(self):
>          # A DistributionSourcePackage whose Distro does not use
>          # Launchpad for bug tracking should not show the "Report a bug"
> diff --git a/lib/lp/bugs/browser/tests/test_bugtask.py b/lib/lp/bugs/browser/tests/test_bugtask.py
> index 8d665f0..6a3ed4b 100644
> --- a/lib/lp/bugs/browser/tests/test_bugtask.py
> +++ b/lib/lp/bugs/browser/tests/test_bugtask.py
> @@ -873,6 +875,25 @@ class TestBugTasksTableView(TestCaseWithFactory):
>          foo_ociproject = self.factory.makeOCIProject(pillar=foo)
>          barix_ociproject = self.factory.makeOCIProject(pillar=barix)
>  
> +        foo_ep_snap = self.factory.makeExternalPackage(foo_spn)

I'd generally rather we use kwargs when it's not obvious what the args are, makes it quicker to read what the test is doing IMO.
So I'd use 
```
foo_ep_snap = self.factory.makeExternalPackage(sourcepackagename=foo_spn)
```

Same below

> +        foo_ep_charm = self.factory.makeExternalPackage(
> +            foo_spn, packagetype=ExternalPackageType.CHARM
> +        )
> +        foo_ep_charm_candidate = self.factory.makeExternalPackage(
> +            foo_spn,
> +            packagetype=ExternalPackageType.CHARM,
> +            channel=("12.1", "candidate"),
> +        )
> +        bar_ep_snap = self.factory.makeExternalPackage(bar_spn)
> +        bar_ep_rock = self.factory.makeExternalPackage(
> +            bar_spn, packagetype=ExternalPackageType.ROCK
> +        )
> +        bar_ep_rock_candidate = self.factory.makeExternalPackage(
> +            bar_spn,
> +            packagetype=ExternalPackageType.ROCK,
> +            channel=("12.1", "candidate"),
> +        )
> +
>          expected_targets = [
>              bar,
>              bar.getSeries("0.0"),
> diff --git a/lib/lp/bugs/browser/tests/test_bugtask_navigation.py b/lib/lp/bugs/browser/tests/test_bugtask_navigation.py
> index 356f05d..7eb1f6b 100644
> --- a/lib/lp/bugs/browser/tests/test_bugtask_navigation.py
> +++ b/lib/lp/bugs/browser/tests/test_bugtask_navigation.py
> @@ -58,3 +58,87 @@ class TestBugTaskTraversal(TestCaseWithFactory):
>              "http://api.launchpad.test/1.0/%s/+bug/%d";
>              % (bug.default_bugtask.target.name, bug.default_bugtask.bug.id),
>          )
> +
> +    def test_traversal_to_external_package_bugtask(self):
> +        # Test that traversal using +bugtask/id works
> +        bug = self.factory.makeBug()
> +        ep = self.factory.makeExternalPackage()
> +        bugtask = self.factory.makeBugTask(bug=bug, target=ep)
> +        bugtask_url = canonical_url(bugtask)
> +        ep_2 = self.factory.makeExternalPackage()
> +        bugtask_2 = self.factory.makeBugTask(bug=bug, target=ep_2)
> +        bugtask_url_2 = canonical_url(bugtask_2)
> +        self.assertEqual(
> +            bugtask_url,
> +            "http://bugs.launchpad.test/%s/+external/%s/+bug/%d/+bugtask/%s";

Could we ensure that if a bug contains multiple external packages with the same spn and distribution, if we try to target the last one we don't go to the first external package in the list?

> +            % (
> +                removeSecurityProxy(bugtask).distribution.name,
> +                removeSecurityProxy(bugtask).target.name,
> +                removeSecurityProxy(bugtask).bug.id,
> +                removeSecurityProxy(bugtask).id,
> +            ),
> +        )
> +        self.assertEqual(
> +            bugtask_url_2,
> +            "http://bugs.launchpad.test/%s/+external/%s/+bug/%d/+bugtask/%s";
> +            % (
> +                removeSecurityProxy(bugtask_2).distribution.name,
> +                removeSecurityProxy(bugtask_2).target.name,
> +                removeSecurityProxy(bugtask_2).bug.id,
> +                removeSecurityProxy(bugtask_2).id,
> +            ),
> +        )
> +        obj, _, _ = test_traverse(bugtask_url)
> +        obj_2, _, _ = test_traverse(bugtask_url_2)
> +        self.assertEqual(bugtask, obj)
> +        self.assertEqual(bugtask_2, obj_2)
> +        self.assertEqual(ep, obj.target)
> +        self.assertEqual(ep_2, obj_2.target)
> +
> +    def test_traversal_to_default_external_package_bugtask(self):
> +        # Test that a traversing to a bug with an external package as default
> +        # bugtask redirects to the bug's default bugtask using +bugtask/id.
> +        ep = self.factory.makeExternalPackage()
> +        bug = self.factory.makeBug(target=ep)
> +        bug_url = canonical_url(bug, rootsite="bugs")
> +        obj, view, request = test_traverse(bug_url)
> +        view()
> +        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/+external/%s/+bug/%d/+bugtask/%s";
> +            % (
> +                bug.default_bugtask.distribution.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)
> +        obj, view, request = test_traverse(
> +            "http://api.launchpad.test/1.0/%s/+bug/%d";
> +            % (
> +                removeSecurityProxy(ep).distribution.name,
> +                bug.default_bugtask.bug.id,
> +            )
> +        )
> +        self.assertEqual(
> +            removeSecurityProxy(view).target,
> +            "http://api.launchpad.test/1.0/%s/+external/%s/+bug/%d/+bugtask/%s";
> +            % (
> +                bug.default_bugtask.distribution.name,
> +                bug.default_bugtask.target.name,
> +                bug.default_bugtask.bug.id,
> +                bug.default_bugtask.id,
> +            ),
> +        )
> diff --git a/lib/lp/registry/browser/externalpackage.py b/lib/lp/registry/browser/externalpackage.py
> new file mode 100644
> index 0000000..36842ef
> --- /dev/null
> +++ b/lib/lp/registry/browser/externalpackage.py
> @@ -0,0 +1,92 @@
> +# Copyright 2009-2025 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__all__ = [
> +    "ExternalPackageBreadcrumb",
> +    "ExternalPackageNavigation",
> +    "ExternalPackageURL",
> +    "ExternalPackageFacets",
> +]
> +
> +
> +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.code.browser.vcslisting import TargetDefaultVCSNavigationMixin
> +from lp.registry.interfaces.externalpackage import IExternalPackage
> +from lp.services.webapp import Navigation, StandardLaunchpadFacets, redirection
> +from lp.services.webapp.breadcrumb import Breadcrumb
> +from lp.services.webapp.interfaces import (
> +    ICanonicalUrlData,
> +    IMultiFacetedBreadcrumb,
> +)
> +from lp.services.webapp.menu import Link
> +from lp.services.webhooks.browser import WebhookTargetNavigationMixin
> +from lp.translations.browser.customlanguagecode import (
> +    HasCustomLanguageCodesTraversalMixin,
> +)
> +
> +
> +@implementer(ICanonicalUrlData)
> +class ExternalPackageURL:
> +    """External package URL creation rules."""
> +
> +    rootsite = None
> +
> +    def __init__(self, context):
> +        self.context = context
> +
> +    @property
> +    def inside(self):
> +        return self.context.distribution
> +
> +    @property
> +    def path(self):
> +        return "+external/%s" % self.context.name
> +
> +
> +@implementer(IHeadingBreadcrumb, IMultiFacetedBreadcrumb)
> +class ExternalPackageBreadcrumb(Breadcrumb):
> +    """Builds a breadcrumb for an `IExternalPackage`."""
> +
> +    rootsite = "bugs"
> +
> +    @property
> +    def text(self):
> +        return "%s external package" % self.context.sourcepackagename.name
> +
> +
> +class ExternalPackageFacets(StandardLaunchpadFacets):
> +    usedfor = IExternalPackage
> +    enable_only = [
> +        "bugs",
> +    ]
> +
> +
> +class ExternalPackageLinksMixin:
> +    def new_bugs(self):
> +        base_path = "+bugs"
> +        get_data = "?field.status:list=NEW"
> +        return Link(base_path + get_data, "New bugs", site="bugs")
> +
> +
> +class ExternalPackageNavigation(
> +    Navigation,
> +    BugTargetTraversalMixin,
> +    HasCustomLanguageCodesTraversalMixin,
> +    TargetDefaultVCSNavigationMixin,
> +    StructuralSubscriptionTargetTraversalMixin,
> +    WebhookTargetNavigationMixin,
> +):
> +    usedfor = IExternalPackage
> +
> +    @redirection("+editbugcontact")
> +    def redirect_editbugcontact(self):
> +        return "+subscribe"
> +
> +    def traverse(self, name):
> +        return None

What does this do? Does it mean that from external package page we can't go into a subpage?

> diff --git a/lib/lp/registry/model/externalpackage.py b/lib/lp/registry/model/externalpackage.py
> index d97390d..f7bfaa8 100644
> --- a/lib/lp/registry/model/externalpackage.py
> +++ b/lib/lp/registry/model/externalpackage.py
> @@ -147,6 +147,23 @@ class ExternalPackage(
>          """See `IBugTarget`."""
>          return self.distribution
>  
> +    @property
> +    def bug_reporting_guidelines(self):
> +        return
> +
> +    @property
> +    def content_templates(self):
> +        return
> +
> +    @property
> +    def bug_reported_acknowledgement(self):
> +        """See `IBugTarget`."""
> +        return self.distribution.bug_reported_acknowledgement
> +
>      def _getOfficialTagClause(self):
>          """See `IBugTarget`."""
>          return self.distribution._getOfficialTagClause()
> +
> +    def _customizeSearchParams(self, search_params):
> +        """Customize `search_params` for this distribution source package."""

Replace `source` with `external`

> +        search_params.setExternalPackage(self)


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