launchpad-reviewers team mailing list archive

[Enrique Sánchez <mp+494829@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> 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}"

Sure, done, forgot to do it before. Let me know what you think about it as it seems we are creating here the same logic we have for urls in the model code.

> +            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)

- Afaik, `BugTaskFlat` comes as a flat copy of the `BugTask`.
- `BugTask.channel` will be created based on `ExternalPackage` or `ExternalPackageSeries` `channel` attribute.
- `ExternalPackage` and `ExternalPackageSeries` use the `lp.services.channel.channel_string_to_list()` that I've reused as it is used in many places. This method accepts a string as an argument but also a list/tuple. It will always return a tuple of 3 elements `(track, risk, branch)`.

You can check `channel` columns in the db, for example `SourcePackagePublishingHistory.channel` is doing the same.

It's true that we don't have a db constraint for this in any place (afaik)

PD: Added a comment to make this clearer

> +
>      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)

Replied above ^

> +        else:
> +            channel = risk

I will update to be more clear, but as I said above the `channel_string_to_list()` is very flexible and allows us to do it. Also, I prefer to make it look similar to what the channel is at the end (tuple 3 elements).

> +
> +        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]

Always a tuple of 3 elements.

> +        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)

True, thanks! :). To be more clear this is a ValueError (something in the channel is invalid by using chars not allowed, non existing risks...) -> we are returning a 404 as discussed in MM (although a 400 would be more precise).

Added a comment to explain it clearer.

> +
>  
>  @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)

answer above ^ :)

> +
> +        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.