[PATCH v2 5/5] views: Add support for boolean 'series' parameters

[Daniel Axtens]

Stephen Finucane <stephen at that.guru> writes:

> Previously, we allowed users to download patch mboxes with dependencies
> included using a 'series' parameter. This accepted either a numeric ID,
> corresponding to the ID of the patch series that dependencies should be
> included from, or a wildcard value ('*').
>
>     /patch/{patchID}/mbox/?series=123
>     /patch/{patchID}/mbox/?series=*
>
> With switch to a 1:N series-patch relationship, this is clearly
> unnecessary now but must be retained to avoid breaking users. However,
> that doesn't mean we can't things a little clearer. Add support for
> boolean parameters, which make more sense for this kind of relationship:
>
>   /patch/{patchID}/mbox/?series=true
>   /patch/{patchID}/mbox/?series=1
>   /patch/{patchID}/mbox/?series=false
>   /patch/{patchID}/mbox/?series=0
>
> Signed-off-by: Stephen Finucane <stephen at that.guru>
> ---
> TODO: I'm not sure if I should do this or introduce a new parameter
> (maybe 'dependencies'?). Thoughts?

So my question is:

  "Previously, specifying a wrong series number would lead to getting
   no patches. Is it a problem if specifying a wrong series number now
   leads to you getting the series patches?"

I would say that the answer to that question is "No, if you specify
?series=<literally anything>", you should get the whole series.

Therefore, I'd make the whole thing simpler by dropping the 0/false
thing and just returning the series if ?series is present.

Regards,
Daniel

> ---
>  patchwork/tests/test_mboxviews.py | 17 +++++++++++++++++
>  patchwork/views/patch.py          |  2 +-
>  patchwork/views/utils.py          |  2 +-
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/patchwork/tests/test_mboxviews.py b/patchwork/tests/test_mboxviews.py
> index 2add3950..9d941bf8 100644
> --- a/patchwork/tests/test_mboxviews.py
> +++ b/patchwork/tests/test_mboxviews.py
> @@ -237,6 +237,23 @@ class MboxSeriesDependencies(TestCase):
>          self.assertContains(response, patch_a.content)
>          self.assertContains(response, patch_b.content)
>  
> +    def test_patch_with_boolean_series(self):
> +        _, patch_a, patch_b = self._create_patches()
> +
> +        for value in ('true', '1'):
> +            response = self.client.get('%s?series=%s' % (
> +                reverse('patch-mbox', args=[patch_b.id]), value))
> +
> +            self.assertContains(response, patch_a.content)
> +            self.assertContains(response, patch_b.content)
> +
> +        for value in ('false', '0'):
> +            response = self.client.get('%s?series=%s' % (
> +                reverse('patch-mbox', args=[patch_b.id]), value))
> +
> +            self.assertNotContains(response, patch_a.content)
> +            self.assertContains(response, patch_b.content)
> +
>      def test_patch_with_invalid_series(self):
>          series, patch_a, patch_b = self._create_patches()
>  
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index 9fed4312..91b98e38 100644
> --- a/patchwork/views/patch.py
> +++ b/patchwork/views/patch.py
> @@ -144,7 +144,7 @@ def patch_mbox(request, patch_id):
>      series_id = request.GET.get('series')
>  
>      response = HttpResponse(content_type='text/plain')
> -    if series_id:
> +    if series_id and series_id.lower() not in ('false', '0'):
>          if not patch.series:
>              raise Http404('Patch does not have an associated series. This is '
>                            'because the patch was processed with an older '
> diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
> index bfcf6aba..4644c621 100644
> --- a/patchwork/views/utils.py
> +++ b/patchwork/views/utils.py
> @@ -141,7 +141,7 @@ def series_patch_to_mbox(patch, series_id):
>      Returns:
>          A string for the mbox file.
>      """
> -    if series_id != '*':
> +    if series_id.lower() not in ('*', 'true', '1'):
>          try:
>              series_id = int(series_id)
>          except ValueError:
> -- 
> 2.17.1
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork