[PATCH 4/6] views: Add 'series' parameter to '/mbox' endpoint

[Daniel Axtens]

Hi Stephen,

> This allows a user to download dependencies for a patch without having
> to do it manually.

I'm not sold on the idea of calling these dependencies and allowing
downloads of partial series. It just seems to present the user with an
dazzling array of ways to shoot themselves in the foot - for example I
can imagine a lot of users clicking at the start of a series or midway
into a series and downloading an mbox, not realising that if they want
the entire series of patches they need to explicitly find the final
patch and download the mbox from there... What was your rationale or
inteded use-case for this? (I'm happier to expose it programatically, I
guess, it's just more complexity we have to carry around...)

  
>  def patch_to_mbox(patch):
> +    """Get an mbox representation of a single patch.
> +
> +    Arguments:
> +        patch: The Patch object to convert.
> +
> +    Returns:
> +        A string for the mbox file.
> +    """

Yay for docstrings, but maybe you should mention them in the
commit message, or even put them in a patch with other cleanups?

>      postscript_re = re.compile('\n-{2,3} ?\n')
>      body = ''
>  

> +
> +def series_patch_to_mbox(patch, series_num):
> +    """Get an mbox representation of a patch with dependencies.
> +
> +    Arguments:
> +        patch: The Patch object to convert.
> +        series_num: The series number to retrieve dependencies from.
> +
> +    Returns:
> +        A string for the mbox file.
> +    """
> +    try:
> +        series_num = int(series_num)
> +    except ValueError:
> +        # TODO(stephenfin): Perhaps we should raise a HTTP code here?
Yes, we should - silently failing is not a great way to go.
I think both this case and the series not found should probably both
return a 404, preferably with a friendly message of some sort?
(see
e.g. http://stackoverflow.com/questions/20922543/correct-http-status-code-for-existent-resource-but-non-existent-entity )
> +        return ''
> +
> +    try:
> +        series = patch.series.get(id=series_num)
> +    except Series.DoesNotExist:
> +        return ''
> +
> +    mbox = []
> +
> +    # get the series-ified patch
> +    number = series.seriespatch_set.get(patch=patch).number
> +    for dep in series.seriespatch_set.filter(number__lt=number):
> +        mbox.append(patch_to_mbox(dep.patch))
Are these guaranteed to be ordered even in the event of out of order
arrival? I honestly can't remember - I think they are?

> +
> +    mbox.append(patch_to_mbox(patch))
> +
> +    return '\n'.join(mbox)


>  def patch_mbox(request, patch_id):
>      patch = get_object_or_404(Patch, id=patch_id)
> -    response = HttpResponse(content_type="text/plain")
> -    response.write(patch_to_mbox(patch))
> +    series_num = request.GET.get('series')
> +
> +    response = HttpResponse(content_type='text/plain')
> +    if series_num:
> +        response.write(series_patch_to_mbox(patch, series_num))
> +    else:
> +        response.write(patch_to_mbox(patch))
>      response['Content-Disposition'] = 'attachment; filename=' + \
>          patch.filename.replace(';', '').replace('\n', '')
I notice we don't sanitise bundle names the same way we sanitise this
filename (see one of your previous patches). Should we? Are the name
characters for bundles arbitrary?

Regards,
Daniel