Style changes and refactoring from review

Adding tests for pdf rendering and tif exporting

mfr/extensions/pdf/exceptions.py

@@ -2,11 +2,11 @@
 
 
 class PillowImageError(ExporterError):
-    """The Image related errors raised from a :class:`mfr.extentions.image`
+    """The Image related errors raised from a :class:`mfr.extentions.pdf`
     and relating to the Pillow Library should inherit from PillowImageError
     """
 
-    __TYPE = 'image_pillow'
+    __TYPE = 'pdf_pillow'
 
     def __init__(self, message, *args, export_format: str='', detected_format: str='',
                  original_exception: Exception=None, **kwargs):

mfr/extensions/pdf/export.py

@@ -1,12 +1,13 @@
 import os
-
 import imghdr
-from PIL import Image
+from http import HTTPStatus
 
+from PIL import Image
 from reportlab.pdfgen import canvas
 
 from mfr.core import extension
 from mfr.extensions.pdf import exceptions
+from mfr.extensions.pdf.settings import EXPORT_MAX_PAGES
 
 
 class PdfExporter(extension.BaseExporter):
@@ -17,69 +18,66 @@ def __init__(self, *args, **kwargs):
         self.metrics.add('pil_version', Image.VERSION)
 
     def tiff_to_pdf(self, tiff_img, max_size):
-        max_pages = 40
         width, height = tiff_img.size
         c = canvas.Canvas(self.output_file_path)
         c.setPageSize((max_size[0], max_size[1]))
 
         page = 0
 
         # This seems to be the only way to write this loop at the moment
-        while True:
+        while page < EXPORT_MAX_PAGES:
             try:
                 tiff_img.seek(page)
             except EOFError:
                 break
 
             # Center the image and draw it in the canvas
-            c.drawInlineImage(tiff_img, (max_size[0] - width) // 2, (max_size[1] - height) // 2, anchor='c')
-            # move to the next page
+            c.drawInlineImage(tiff_img, (max_size[0] - width) // 2,
+                             (max_size[1] - height) // 2, anchor='c')
             c.showPage()
-
-            if max_pages and page > max_pages:
-                break
             page += 1
 
         c.save()
 
     def export(self):
         parts = self.format.split('.')
-        type = parts[-1].lower()
+        export_type = parts[-1].lower()
         max_size = [int(x) for x in parts[0].split('x')] if len(parts) == 2 else None
 
         self.metrics.merge({
-            'type': type,
+            'type': export_type,
             'max_size_w': max_size[0],
             'max_size_h': max_size[1],
         })
         try:
             image = Image.open(self.source_file_path)
             if max_size:
-                # resize the image to the w/h maximum specified
+                # Resize the image to the w/h maximum specified
                 ratio = min(max_size[0] / image.size[0], max_size[1] / image.size[1])
                 self.metrics.add('ratio', ratio)
                 if ratio < 1:
-                    image = image.resize((round(image.size[0] * ratio), round(image.size[1] * ratio)), Image.ANTIALIAS)
-
+                    image = image.resize((round(image.size[0] * ratio),
+                            round(image.size[1] * ratio)), Image.ANTIALIAS)
             self.tiff_to_pdf(image, max_size)
             image.close()
 
         except (UnicodeDecodeError, IOError) as err:
             name, extension = os.path.splitext(os.path.split(self.source_file_path)[-1])
             raise exceptions.PillowImageError(
                 'Unable to export the file as a {}, please check that the '
-                'file is a valid image.'.format(type),
-                export_format=type,
+                'file is a valid image.'.format(export_type),
+                export_format=export_type,
                 detected_format=imghdr.what(self.source_file_path),
                 original_exception=err,
-                code=400,
+                code=HTTPStatus.BAD_REQUEST,
             )
-        except(ValueError) as err:
+        # This is due to a bug with pillow. Once its upgraded to 4.3, this case can go away
+        except ValueError as err:
             name, extension = os.path.splitext(os.path.split(self.source_file_path)[-1])
             raise exceptions.PillowImageError(
                 'Unable to open file. Please check that it is a valid tiff file',
-                export_format=type,
+                export_format=export_type,
                 detected_format=imghdr.what(self.source_file_path),
                 original_exception=err,
-                code=400,
+                code=HTTPStatus.BAD_REQUEST,
             )

mfr/extensions/pdf/render.py

@@ -19,15 +19,17 @@ def render(self):
             return self.TEMPLATE.render(base=self.assets_url, url=self.metadata.download_url)
 
         exported_url = furl.furl(self.export_url)
-        if settings.EXPORT_MAXIMUM_SIZE and settings.EXPORT_TYPE:
-            exported_url.args['format'] = '{}.{}'.format(settings.EXPORT_MAXIMUM_SIZE, settings.EXPORT_TYPE)
-        elif settings.EXPORT_TYPE:
-            exported_url.args['format'] = settings.EXPORT_TYPE
-        else:
-            return self.TEMPLATE.render(base=self.assets_url, url=self.metadata.download_url)
+        if settings.EXPORT_TYPE:
+            if settings.EXPORT_MAXIMUM_SIZE:
+                exported_url.args['format'] = '{}.{}'.format(settings.EXPORT_MAXIMUM_SIZE,
+                                                             settings.EXPORT_TYPE)
+            else:
+                exported_url.args['format'] = settings.EXPORT_TYPE
+
+            self.metrics.add('needs_export', True)
+            return self.TEMPLATE.render(base=self.assets_url, url=exported_url.url)
 
-        self.metrics.add('needs_export', True)
-        return self.TEMPLATE.render(base=self.assets_url, url=exported_url.url)
+        return self.TEMPLATE.render(base=self.assets_url, url=self.metadata.download_url)
 
     @property
     def file_required(self):

mfr/extensions/pdf/settings.py

@@ -5,4 +5,7 @@
 
 EXPORT_TYPE = config.get('EXPORT_TYPE', 'pdf')
 EXPORT_MAXIMUM_SIZE = config.get('EXPORT_MAXIMUM_SIZE', '1200x1200')
-EXPORT_EXCLUSIONS = config.get('EXPORT_EXCLUSIONS', ['.pdf', ])
+
+# supports multiple files in the form of a space separated string
+EXPORT_EXCLUSIONS = config.get('EXPORT_EXCLUSIONS', '.pdf').split(' ')
+EXPORT_MAX_PAGES = int(config.get('EXPORT_MAX_PAGES', 40))

requirements.txt

@@ -23,6 +23,8 @@ pydocx==0.7.0
 # Image
 Pillow==2.8.2
 psd-tools==1.4
+# This should enhance Pillows ability to work with tif files, but it may not do anything
+libtiff==0.4.1
 
 # IPython
 ipython==5.2.2

tests/extensions/pdf/test_exporter.py

@@ -0,0 +1,80 @@
+import os
+import shutil
+import binascii
+
+import pytest
+
+from mfr.extensions.pdf import (settings,
+                                exceptions,
+                                PdfExporter)
+
+
+BASE = os.path.dirname(os.path.abspath(__file__))
+# Should be the first 4 bytes of a pdf file
+PDF_SIG = b'25504446'
+
+
+@pytest.fixture
+def directory(tmpdir):
+    return str(tmpdir)
+
+
+@pytest.fixture(scope="function", autouse=True)
+def setup_filesystem(directory):
+    shutil.rmtree(directory)
+    os.makedirs(directory, exist_ok=True)
+
+
+class TestPdfExporter:
+    '''
+    The PdfExporter uses the `report-lab` library to turn tiffs into pdfs.
+    However, opening and verifying pdfs for testing purposes is a paid feature of
+    `report-lab`. Instead we will manually open them and just check the signature.
+    '''
+
+    @pytest.mark.parametrize("file_name", [
+        ('test.tif'),
+        ('test_multipage.tif'),
+        ('test_ratio.tif'),
+    ])
+    def test_single_page_tiff(self, directory, file_name):
+        source_file_path = os.path.join(BASE, 'files', file_name)
+        output_file_path = os.path.join(directory, 'test.{}'.format(settings.EXPORT_TYPE))
+        format = '{}.{}'.format(settings.EXPORT_MAXIMUM_SIZE, settings.EXPORT_TYPE)
+        exporter = PdfExporter(source_file_path=source_file_path, ext='.tif',
+                               output_file_path=output_file_path, format=format)
+
+        assert not os.path.exists(output_file_path)
+
+        exporter.export()
+
+        assert os.path.exists(output_file_path)
+        # Open file to check that the exported PDF contains the proper signature
+        with open(output_file_path, 'rb') as file:
+            # the first 4 bytes contain the signature
+            assert binascii.hexlify(file.read(4)) == PDF_SIG
+
+    def test_broken_tiff(self, directory):
+        # Once Pillow is updated to 4.3, this test will fail and can be removed
+        source_file_path = os.path.join(BASE, 'files', 'test_broken.tif')
+        output_file_path = os.path.join(directory, 'test.{}'.format(settings.EXPORT_TYPE))
+        format = '{}.{}'.format(settings.EXPORT_MAXIMUM_SIZE, settings.EXPORT_TYPE)
+        exporter = PdfExporter(source_file_path=source_file_path, ext='.tif',
+                               output_file_path=output_file_path, format=format)
+
+        assert not os.path.exists(output_file_path)
+
+        with pytest.raises(exceptions.PillowImageError):
+            exporter.export()
+
+    def test_bad_tiff(self, directory):
+        source_file_path = os.path.join(BASE, 'files', 'invalid.tif')
+        output_file_path = os.path.join(directory, 'test.{}'.format(settings.EXPORT_TYPE))
+        format = '{}.{}'.format(settings.EXPORT_MAXIMUM_SIZE, settings.EXPORT_TYPE)
+        exporter = PdfExporter(source_file_path=source_file_path, ext='.tif',
+                               output_file_path=output_file_path, format=format)
+
+        assert not os.path.exists(output_file_path)
+
+        with pytest.raises(exceptions.PillowImageError):
+            exporter.export()

tests/extensions/pdf/test_renderer.py

@@ -1,25 +1,40 @@
+import furl
 import pytest
 
+from mfr.extensions.pdf import (settings,
+                                PdfRenderer)
 from mfr.core.provider import ProviderMetadata
 
-from mfr.extensions.pdf import PdfRenderer
-
 
 @pytest.fixture
 def metadata():
-    return ProviderMetadata('test', '.pdf', 'text/plain', '1234', 'http://wb.osf.io/file/test.pdf?token=1234')
+    return ProviderMetadata('test', '.pdf', 'text/plain', '1234',
+                            'http://wb.osf.io/file/test.pdf?token=1234')
 
+@pytest.fixture
+def tif_metadata():
+    return ProviderMetadata('test', '.tif', 'text/plain', '1234',
+                            'http://wb.osf.io/file/test.tif?token=1234')
 
 @pytest.fixture
 def file_path():
     return '/tmp/test.pdf'
 
+@pytest.fixture
+def tif_file_path():
+    return '/tmp/test.tif'
+
 
 @pytest.fixture
 def url():
     return 'http://osf.io/file/test.pdf'
 
 
+@pytest.fixture
+def tif_url():
+    return 'http://osf.io/file/test.tif'
+
+
 @pytest.fixture
 def assets_url():
     return 'http://mfr.osf.io/assets'
@@ -34,6 +49,10 @@ def export_url():
 def renderer(metadata, file_path, url, assets_url, export_url):
     return PdfRenderer(metadata, file_path, url, assets_url, export_url)
 
+@pytest.fixture
+def tif_renderer(tif_metadata, tif_file_path, tif_url, assets_url, export_url):
+    return PdfRenderer(tif_metadata, tif_file_path, tif_url, assets_url, export_url)
+
 
 class TestPdfRenderer:
 
@@ -42,3 +61,13 @@ def test_render_pdf(self, renderer, metadata, assets_url):
         assert '<base href="{}/{}/web/" target="_blank">'.format(assets_url, 'pdf') in body
         assert '<div id="viewer" class="pdfViewer"></div>' in body
         assert 'DEFAULT_URL = \'{}\''.format(metadata.download_url) in body
+
+    def test_render_tif(self, tif_renderer, assets_url):
+        exported_url = furl.furl(tif_renderer.export_url)
+        exported_url.args['format'] = '{}.{}'.format(settings.EXPORT_MAXIMUM_SIZE,
+                                                            settings.EXPORT_TYPE)
+
+        body = tif_renderer.render()
+        assert '<base href="{}/{}/web/" target="_blank">'.format(assets_url, 'pdf') in body
+        assert '<div id="viewer" class="pdfViewer"></div>' in body
+        assert 'DEFAULT_URL = \'{}\''.format(exported_url.url) in body