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

[Stephen Finucane]

On Sat, 2018-09-22 at 01:34 +1000, Daniel Axtens wrote:
> 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?"

"No patches" might be a bit misleading: it would lead you to getting a
HTTP 404.

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

The concern I have here is that we'd encode bad behaviour in clients.
If '?series=6' is used on a Patchwork 2.0 or 2.1 instance where the
given patch is associated with series 8, they would see an error on the
older versions but not the newer ones. As a result, I've continued to
check this behaviour. It's also the reason why I'm thinking it would be
best to move away from this parameter altogether.

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

Hmm, that makes sense. Looking at various RFCs, it seems there's no
reason arguments need to be key-value pairs. That being said, given
that behaviour has changed, do we want to use a new parameter to
highlight this fact?

Stephen

> 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