launchpad-reviewers team mailing list archive

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

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
> @@ -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,

We need Version objects to be compared against `Version` objects. As the `IExternalPackageSeries` will have `sourcepackagename`, `distribution`, `packagetype` and `channel`, we need to add the `Version` as the fifth element. That's why we need to add None in `ISourcePackage` so the Version is the fifth element.
In `IDistributionSourcePackage` we don't need to add this None because we only have `sourcepackagename` and `distribution`, but no `Version`. Other keys, like `IProduct` one, needs also these None as they were already using the fifth element.

If you are wondering why some keys have less elements than others, it does not matter as we are later doing a list sorting and it will treat the remaining elements as None.

This is the solution I've found to be coherent with the existing code, if you have any other idea I would be happy to work on it :)

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

In our case, we need to execute `view()` to actually be able to get the correct status code

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


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