launchpad-reviewers team mailing list archive

[Ines Almeida <mp+494829@xxxxxxxxxxxxxxxxxx>]

Looking good, looks like a really nice cleanup and clean solution
Left a few comments

Diff comments:

> diff --git a/lib/lp/bugs/browser/bugtask.py b/lib/lp/bugs/browser/bugtask.py
> index 270167d..9c65b9d 100644
> --- a/lib/lp/bugs/browser/bugtask.py
> +++ b/lib/lp/bugs/browser/bugtask.py
> @@ -347,17 +341,9 @@ class BugTargetTraversalMixin:
>          # rather than making it look as though this task was "not found",
>          # because it was filtered out by privacy-aware code.
>  
> -        # Check if it uses +external url
> -        is_external = IExternalURL.providedBy(context)
> -
>          for bugtask in bug.bugtasks:
>              target = bugtask.target
> -
> -            if is_external:
> -                if context.isMatching(target):
> -                    # Security proxy the object on the way out
> -                    return getUtility(IBugTaskSet).get(bugtask.id)
> -            elif target == context:

It feels nice to remove some of these specialized things, nice!

> +            if target == context:
>                  return getUtility(IBugTaskSet).get(bugtask.id)
>  
>          # If we've come this far, there's no task for the requested context.
> diff --git a/lib/lp/bugs/browser/tests/test_bugtask_navigation.py b/lib/lp/bugs/browser/tests/test_bugtask_navigation.py
> index 8cf2537..a208eeb 100644
> --- a/lib/lp/bugs/browser/tests/test_bugtask_navigation.py
> +++ b/lib/lp/bugs/browser/tests/test_bugtask_navigation.py
> @@ -132,36 +131,40 @@ class TestBugTaskTraversal(TestCaseWithFactory):
>  
>          # Check externalpackage urls
>          for bugtask in bugtasks[:3]:
> -            self.assertEqual(
> -                canonical_url(bugtask),
> -                "http://bugs.launchpad.test/%s/+%s/%s/+bug/%d/";
> -                "+bugtask/%s"
> -                % (
> -                    bugtask.distribution.name,
> -                    bugtask.target.packagetype.name.lower(),
> -                    bugtask.target.name,
> -                    bugtask.bug.id,
> -                    bugtask.id,
> -                ),
> +            target = bugtask.target
> +            track, risk, branch = target.channel
> +            # Helper to build the channel URL part
> +            channel_url = f"/+track/{track}/+risk/{risk}"

Maybe create an aux method to build the URL to be reused below as well?

> +            if branch is not None:
> +                channel_url += f"/+branch/{branch}"
> +
> +            expected_url = (
> +                f"http://bugs.launchpad.test/{target.distribution.name}";
> +                f"/+{target.packagetype.name.lower()}/{target.name}"
> +                f"{channel_url}/+bug/{bugtask.bug.id}"
>              )
> +            self.assertEqual(expected_url, canonical_url(bugtask))
> +
>              obj, _, _ = test_traverse(canonical_url(bugtask))
>              self.assertEqual(bugtask, obj)
>  
>          # Check externalpackageseries urls
>          for bugtask in bugtasks[3:]:
> -            self.assertEqual(
> -                canonical_url(bugtask),
> -                "http://bugs.launchpad.test/%s/%s/+%s/%s/+bug/%d/";
> -                "+bugtask/%s"
> -                % (
> -                    bugtask.target.distribution.name,
> -                    bugtask.distroseries.name,
> -                    bugtask.target.packagetype.name.lower(),
> -                    bugtask.target.name,
> -                    bugtask.bug.id,
> -                    bugtask.id,
> -                ),
> +            target = bugtask.target
> +            track, risk, branch = target.channel
> +            # Helper to build the channel URL part
> +            channel_url = f"/+track/{track}/+risk/{risk}"
> +            if branch is not None:
> +                channel_url += f"/+branch/{branch}"
> +
> +            expected_url = (
> +                f"http://bugs.launchpad.test/{target.distribution.name}";
> +                f"/{target.distroseries.name}"
> +                f"/+{target.packagetype.name.lower()}/{target.name}"
> +                f"{channel_url}/+bug/{bugtask.bug.id}"
>              )
> +            self.assertEqual(expected_url, canonical_url(bugtask))
> +
>              obj, _, _ = test_traverse(canonical_url(bugtask))
>              self.assertEqual(bugtask, obj)
>  
> diff --git a/lib/lp/bugs/model/bugtasksearch.py b/lib/lp/bugs/model/bugtasksearch.py
> index 2af1ee7..6e9b98d 100644
> --- a/lib/lp/bugs/model/bugtasksearch.py
> +++ b/lib/lp/bugs/model/bugtasksearch.py
> @@ -349,6 +349,9 @@ def _build_query(params):
>          if where_cond is not None:
>              extra_clauses.append(where_cond)
>  
> +    if params.channel is not None:
> +        extra_clauses.append(BugTaskFlat.channel == params.channel)

You likely thought about this, but just to be sure: do channels always look the same or do we allow it being ["1.6", "stable"] vs "1.6/stable" vs {"track": "1.6", "risk": "stable"}?
If they are always the same, than this equality is fine, otherwise we should consider a function to check equality

> +
>      if params.status is not None:
>          extra_clauses.append(
>              _build_status_clause(BugTaskFlat.status, params.status)
> diff --git a/lib/lp/registry/browser/externalpackage.py b/lib/lp/registry/browser/externalpackage.py
> index 6ec7657..1b398d8 100644
> --- a/lib/lp/registry/browser/externalpackage.py
> +++ b/lib/lp/registry/browser/externalpackage.py
> @@ -46,10 +53,56 @@ class ExternalPackageFacets(StandardLaunchpadFacets):
>      ]
>  
>  
> +class ExternalPackageNavigationMixin:
> +    """Provides traversal for +track, +risk, and +branch."""
> +
> +    def _get_package_for_channel(self, channel):
> +        """
> +        Get the external package or package series for a given channel.
> +
> +        Subclasses must implement this.
> +        """
> +        raise NotImplementedError
> +
> +    @stepthrough("+track")
> +    def traverse_track(self, track):
> +        stack = self.request.getTraversalStack()
> +        if len(stack) >= 2 and stack[-1] == "+risk":
> +            risk = stack[-2]
> +        else:
> +            # If no risk provided, default to stable
> +            risk = "stable"
> +
> +        channel = (track, risk)
> +        return self._get_package_for_channel(channel)
> +
> +    @stepthrough("+risk")
> +    def traverse_risk(self, risk):
> +        if self.context.channel:
> +            channel = (self.context.channel[0], risk)

Is there any case where the channel exists but is not a list?

> +        else:
> +            channel = risk

I don't get this - how could a channel be just the risk?
I also see a "None/stable" below, so might be something I'm missing
Maybe the naming could be updated to be more clear?

> +
> +        return self._get_package_for_channel(channel)
> +
> +    @stepthrough("+branch")
> +    def traverse_branch(self, branch):
> +        if self.context.channel:
> +            track, risk = self.context.channel[0], self.context.channel[1]

Is it possible that `context.channel` has only the track and this fails?

> +        else:
> +            # If no track/risk provided, default to None/stable
> +            track = None
> +            risk = "stable"
> +
> +        channel = (track, risk, branch)
> +        return self._get_package_for_channel(channel)
> +
> +
>  class ExternalPackageNavigation(
>      Navigation,
>      BugTargetTraversalMixin,
>      StructuralSubscriptionTargetTraversalMixin,
> +    ExternalPackageNavigationMixin,
>  ):
>      usedfor = IExternalPackage
>  
> @@ -57,6 +110,14 @@ class ExternalPackageNavigation(
>      def redirect_editbugcontact(self):
>          return "+subscribe"
>  
> +    def _get_package_for_channel(self, channel):
> +        try:
> +            return self.context.distribution.getExternalPackage(
> +                self.context.name, self.context.packagetype, channel
> +            )
> +        except ValueError:
> +            raise NotFound(self.context, channel)

I think we are missing a test for the NotFound use case here where the URL points to an external package that doesn't exist

> +
>  
>  @implementer(ICanonicalUrlData)
>  class ExternalPackageURL:
> diff --git a/lib/lp/registry/browser/externalpackageseries.py b/lib/lp/registry/browser/externalpackageseries.py
> index 4221ef8..a9a54ae 100644
> --- a/lib/lp/registry/browser/externalpackageseries.py
> +++ b/lib/lp/registry/browser/externalpackageseries.py
> @@ -91,4 +102,15 @@ class ExternalPackageSeriesURL:
>      @property
>      def path(self):
>          packagetype = self.context.packagetype.name.lower()
> -        return f"+{packagetype}/{self.context.name}"
> +
> +        track, risk, branch = self.context.channel or (None, None, None)

Can we be sure that they are always 3 in `context.channel`?
I know I asked about this in other places above, so the answer might be the same :)

> +
> +        channel_url = ""
> +        if track:
> +            channel_url = f"/+track/{track}"
> +        if risk:
> +            channel_url = channel_url + f"/+risk/{risk}"
> +        if branch:
> +            channel_url = channel_url + f"/+branch/{branch}"
> +
> +        return f"+{packagetype}/{self.context.name}{channel_url}"


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