| Message ID | 20170130225944.12507-4-stephen@that.guru |
|---|---|
| State | Superseded |
| Headers | show |
I've fat-fingered the initial response to this message and accidentally omitted the list - here's the info for posterity. Stephen Finucane <stephen@that.guru> writes: > On Thu, 2017-02-02 at 17:07 +1100, Daniel Axtens wrote: >> Hi Stephen, >> >> I'm not much of a fan of: >> >> - mixing cleanup with refactoring, > > I thought as much when sending this, but did it anyway. I'll break out > the import refactoring into a separate patch for v2. > >> - calling this patch a 'views:' patch, given that the view should be >> exactly the same before and after, > > +1 > >> - adding code to __init__.py > > That entire file needs to be broken (I've already started on it), but > for now that's where the other '_to_mbox' files live. I figure it's > better to be consistent now and leave any larger refactoring to later > changes. > >> - testing a view that is a very thin wrapper around the refactored >> method, rather than testing the refactored method directly. I >> would >> be more OK if you were testing anything the view adds (like the >> content disposition or something like that), but you're only >> testing >> the content, which is just the result of the function. > > Also agree on this point, but alas this is how the existing tests in > that file all function. I'd like to refactor that entire file, but I > would much rather get 2.0 out before doing so :) > > Stephen > >> But having said that, these are all nits, and I'm overall quite OK >> with >> the idea and patch. >> >> Regards, >> Daniel >> >> > Signed-off-by: Stephen Finucane <stephen@that.guru> >> > --- >> > patchwork/tests/test_bundles.py | 20 ++++++++++++++++++++ >> > patchwork/views/__init__.py | 4 ++++ >> > patchwork/views/bundle.py | 26 ++++++++++++++++---------- >> > 3 files changed, 40 insertions(+), 10 deletions(-) >> > >> > diff --git a/patchwork/tests/test_bundles.py >> > b/patchwork/tests/test_bundles.py >> > index c185110..2e34174 100644 >> > --- a/patchwork/tests/test_bundles.py >> > +++ b/patchwork/tests/test_bundles.py >> > @@ -42,6 +42,11 @@ def bundle_url(bundle): >> > 'username': bundle.owner.username, 'bundlename': >> > bundle.name}) >> > >> > >> > +def bundle_mbox_url(bundle): >> > + return reverse('bundle-mbox', kwargs={ >> > + 'username': bundle.owner.username, 'bundlename': >> > bundle.name}) >> > + >> > + >> > class BundleListTest(TestCase): >> > >> > def setUp(self): >> > @@ -120,6 +125,21 @@ class BundleViewTest(BundleTestBase): >> > pos = next_pos >> > >> > >> > +class BundleMboxTest(BundleTestBase): >> > + >> > + def test_empty_bundle(self): >> > + response = self.client.get(bundle_mbox_url(self.bundle)) >> > + self.assertEqual(response.status_code, 200) >> > + self.assertEqual(response.content, '') >> > + >> > + def test_non_empty_bundle(self): >> > + self.bundle.append_patch(self.patches[0]) >> > + >> > + response = self.client.get(bundle_mbox_url(self.bundle)) >> > + self.assertEqual(response.status_code, 200) >> > + self.assertNotEqual(response.content, '') >> > + >> > + >> > class BundleUpdateTest(BundleTestBase): >> > >> > def test_no_action(self): >> > diff --git a/patchwork/views/__init__.py >> > b/patchwork/views/__init__.py >> > index db53cdf..298e0e2 100644 >> > --- a/patchwork/views/__init__.py >> > +++ b/patchwork/views/__init__.py >> > @@ -408,3 +408,7 @@ def patch_to_mbox(patch): >> > mail = mail.as_string(True) >> > >> > return mail >> > + >> > + >> > +def bundle_to_mbox(bundle): >> > + return '\n'.join([patch_to_mbox(p) for p in >> > bundle.ordered_patches()]) >> > diff --git a/patchwork/views/bundle.py b/patchwork/views/bundle.py >> > index aea828d..ec8016b 100644 >> > --- a/patchwork/views/bundle.py >> > +++ b/patchwork/views/bundle.py >> > @@ -17,18 +17,25 @@ >> > # along with Patchwork; if not, write to the Free Software >> > # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111- >> > 1307 USA >> > >> > -from __future__ import absolute_import >> > - >> > from django.contrib.auth.decorators import login_required >> > import django.core.urlresolvers >> > -from django.http import (HttpResponse, HttpResponseRedirect, >> > - HttpResponseNotFound) >> > -from django.shortcuts import render, get_object_or_404 >> > +from django.http import HttpResponse >> > +from django.http import HttpResponseNotFound >> > +from django.http import HttpResponseRedirect >> > +from django.shortcuts import get_object_or_404 >> > +from django.shortcuts import render >> > >> > from patchwork.filters import DelegateFilter >> > -from patchwork.forms import BundleForm, DeleteBundleForm >> > -from patchwork.models import Patch, Bundle, BundlePatch, Project >> > -from patchwork.views import generic_list, patch_to_mbox, >> > get_patch_ids >> > +from patchwork.forms import BundleForm >> > +from patchwork.forms import DeleteBundleForm >> > +from patchwork.models import Bundle >> > +from patchwork.models import BundlePatch >> > +from patchwork.models import Patch >> > +from patchwork.models import Project >> > +from patchwork.views import bundle_to_mbox >> > +from patchwork.views import generic_list >> > +from patchwork.views import get_patch_ids >> > +from patchwork.views import patch_to_mbox >> > >> > >> > @login_required >> > @@ -136,8 +143,7 @@ def bundle_mbox(request, username, bundlename): >> > response = HttpResponse(content_type='text/plain') >> > response['Content-Disposition'] = \ >> > 'attachment; filename=bundle-%d-%s.mbox' % (bundle.id, >> > bundle.name) >> > - response.write('\n'.join( >> > - [patch_to_mbox(p) for p in bundle.ordered_patches()])) >> > + response.write(bundle_to_mbox(bundle)) >> > >> > return response >> > >> > -- >> > 2.9.3 >> > >> > _______________________________________________ >> > Patchwork mailing list >> > Patchwork@lists.ozlabs.org >> > https://lists.ozlabs.org/listinfo/patchwork
diff --git a/patchwork/tests/test_bundles.py b/patchwork/tests/test_bundles.py index c185110..2e34174 100644 --- a/patchwork/tests/test_bundles.py +++ b/patchwork/tests/test_bundles.py @@ -42,6 +42,11 @@ def bundle_url(bundle): 'username': bundle.owner.username, 'bundlename': bundle.name}) +def bundle_mbox_url(bundle): + return reverse('bundle-mbox', kwargs={ + 'username': bundle.owner.username, 'bundlename': bundle.name}) + + class BundleListTest(TestCase): def setUp(self): @@ -120,6 +125,21 @@ class BundleViewTest(BundleTestBase): pos = next_pos +class BundleMboxTest(BundleTestBase): + + def test_empty_bundle(self): + response = self.client.get(bundle_mbox_url(self.bundle)) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.content, '') + + def test_non_empty_bundle(self): + self.bundle.append_patch(self.patches[0]) + + response = self.client.get(bundle_mbox_url(self.bundle)) + self.assertEqual(response.status_code, 200) + self.assertNotEqual(response.content, '') + + class BundleUpdateTest(BundleTestBase): def test_no_action(self): diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py index db53cdf..298e0e2 100644 --- a/patchwork/views/__init__.py +++ b/patchwork/views/__init__.py @@ -408,3 +408,7 @@ def patch_to_mbox(patch): mail = mail.as_string(True) return mail + + +def bundle_to_mbox(bundle): + return '\n'.join([patch_to_mbox(p) for p in bundle.ordered_patches()]) diff --git a/patchwork/views/bundle.py b/patchwork/views/bundle.py index aea828d..ec8016b 100644 --- a/patchwork/views/bundle.py +++ b/patchwork/views/bundle.py @@ -17,18 +17,25 @@ # along with Patchwork; if not, write to the Free Software # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA -from __future__ import absolute_import - from django.contrib.auth.decorators import login_required import django.core.urlresolvers -from django.http import (HttpResponse, HttpResponseRedirect, - HttpResponseNotFound) -from django.shortcuts import render, get_object_or_404 +from django.http import HttpResponse +from django.http import HttpResponseNotFound +from django.http import HttpResponseRedirect +from django.shortcuts import get_object_or_404 +from django.shortcuts import render from patchwork.filters import DelegateFilter -from patchwork.forms import BundleForm, DeleteBundleForm -from patchwork.models import Patch, Bundle, BundlePatch, Project -from patchwork.views import generic_list, patch_to_mbox, get_patch_ids +from patchwork.forms import BundleForm +from patchwork.forms import DeleteBundleForm +from patchwork.models import Bundle +from patchwork.models import BundlePatch +from patchwork.models import Patch +from patchwork.models import Project +from patchwork.views import bundle_to_mbox +from patchwork.views import generic_list +from patchwork.views import get_patch_ids +from patchwork.views import patch_to_mbox @login_required @@ -136,8 +143,7 @@ def bundle_mbox(request, username, bundlename): response = HttpResponse(content_type='text/plain') response['Content-Disposition'] = \ 'attachment; filename=bundle-%d-%s.mbox' % (bundle.id, bundle.name) - response.write('\n'.join( - [patch_to_mbox(p) for p in bundle.ordered_patches()])) + response.write(bundle_to_mbox(bundle)) return response
This includes unit tests to validate correct behavior and prevent regressions. Signed-off-by: Stephen Finucane <stephen@that.guru> --- patchwork/tests/test_bundles.py | 20 ++++++++++++++++++++ patchwork/views/__init__.py | 4 ++++ patchwork/views/bundle.py | 26 ++++++++++++++++---------- 3 files changed, 40 insertions(+), 10 deletions(-)