Don’t convert Excel reports to CSV before output

Converting Python data to CSV makes every field a string. This means
that in the report we return to the user every field will be a string,
even if it’s come from an `int` type in Python. This is because the CSV
‘standard’ doesn’t support any kind of typing.

Excel does support types for fields, so we can make our reports more
useful by preserving these types. This is particularly relevant in the
report we generate for performance platform, which needs the `count`
column to be a number type.

This commit adds extra code paths to the `Spreadsheet` class which mean
that it can be instantiated from either CSV data or a list of Python
data. Previously we were converting the Python data to CSV as an
intermediate step, before instantiating the class.

app/utils.py

@@ -223,9 +223,15 @@ class Spreadsheet():
 
     allowed_file_extensions = ['csv', 'xlsx', 'xls', 'ods', 'xlsm', 'tsv']
 
-    def __init__(self, csv_data, filename=''):
+    def __init__(self, csv_data=None, rows=None, filename=''):
+
         self.filename = filename
-        self.as_csv_data = csv_data
+
+        if csv_data and rows:
+            raise TypeError('Spreadsheet must be created from either rows or CSV data')
+
+        self._csv_data = csv_data or ''
+        self._rows = rows or []
 
     @property
     def as_dict(self):
@@ -234,6 +240,16 @@ class Spreadsheet():
             'data': self.as_csv_data
         }
 
+    @property
+    def as_csv_data(self):
+        if not self._csv_data:
+            with StringIO() as converted:
+                output = csv.writer(converted)
+                for row in self._rows:
+                    output.writerow(row)
+                self._csv_data = converted.getvalue()
+        return self._csv_data
+
     @classmethod
     def can_handle(cls, filename):
         return cls.get_extension(filename) in cls.allowed_file_extensions
@@ -248,11 +264,7 @@ class Spreadsheet():
 
     @classmethod
     def from_rows(cls, rows, filename=''):
-        with StringIO() as converted:
-            output = csv.writer(converted)
-            for row in rows:
-                output.writerow(row)
-            return cls(converted.getvalue(), filename)
+        return cls(rows=rows, filename=filename)
 
     @classmethod
     def from_dict(cls, dictionary, filename=''):
@@ -260,7 +272,7 @@ class Spreadsheet():
             zip(
                 *sorted(dictionary.items(), key=lambda pair: pair[0])
             ),
-            filename
+            filename=filename,
         )
 
     @classmethod
@@ -268,7 +280,7 @@ class Spreadsheet():
         extension = cls.get_extension(filename)
 
         if extension == 'csv':
-            return cls(Spreadsheet.normalise_newlines(file_content), filename)
+            return cls(Spreadsheet.normalise_newlines(file_content), filename=filename)
 
         if extension == 'tsv':
             file_content = StringIO(
@@ -284,11 +296,13 @@ class Spreadsheet():
 
     @property
     def as_rows(self):
-        return list(csv.reader(
-            self.as_csv_data.strip().splitlines(),
-            quoting=csv.QUOTE_MINIMAL,
-            skipinitialspace=True,
-        ))
+        if not self._rows:
+            self._rows = list(csv.reader(
+                self._csv_data.strip().splitlines(),
+                quoting=csv.QUOTE_MINIMAL,
+                skipinitialspace=True,
+            ))
+        return self._rows
 
     @property
     def as_excel_file(self):

tests/app/main/views/test_platform_admin.py

@@ -989,12 +989,12 @@ def test_get_performance_platform_report(client, platform_admin_user, mocker):
     mocker.patch(
         'app.service_api_client.get_live_services_data',
         return_value={'data': [
-            {'service_id': 1, 'service_name': 'jessie the oak tree', 'organisation_name': 'Forest',
+            {'service_id': 'abc123', 'service_name': 'jessie the oak tree', 'organisation_name': 'Forest',
                 'consent_to_research': True, 'contact_name': 'Forest fairy', 'organisation_type': 'Ecosystem',
                 'contact_email': 'forest.fairy@digital.cabinet-office.gov.uk', 'contact_mobile': '+447700900986',
                 'live_date': 'Sat, 29 Mar 2014 00:00:00 GMT', 'sms_volume_intent': 100, 'email_volume_intent': 50,
                 'letter_volume_intent': 20, 'sms_totals': 300, 'email_totals': 1200, 'letter_totals': 0},
-            {'service_id': 2, 'service_name': 'james the pine tree', 'organisation_name': 'Forest',
+            {'service_id': 'def456', 'service_name': 'james the pine tree', 'organisation_name': 'Forest',
                 'consent_to_research': None, 'contact_name': None, 'organisation_type': 'Ecosystem',
                 'contact_email': None, 'contact_mobile': None,
                 'live_date': None, 'sms_volume_intent': None, 'email_volume_intent': 60,
@@ -1008,6 +1008,6 @@ def test_get_performance_platform_report(client, platform_admin_user, mocker):
         file_stream=response.get_data(),
     ) == [
         ['service_id', 'agency', 'service_name', '_timestamp', 'service', 'count'],
-        ['1', 'Forest', 'jessie the oak tree', '2014-03-29T00:00:00Z', 'govuk-notify', '1'],
-        ['2', 'Forest', 'james the pine tree', '', 'govuk-notify', '1'],
+        ['abc123', 'Forest', 'jessie the oak tree', '2014-03-29T00:00:00Z', 'govuk-notify', 1],
+        ['def456', 'Forest', 'james the pine tree', '', 'govuk-notify', 1],
     ]

tests/app/test_utils.py

@@ -140,6 +140,22 @@ def test_can_create_spreadsheet_from_dict_with_filename():
     assert Spreadsheet.from_dict({}, filename='empty.csv').as_dict['file_name'] == "empty.csv"
 
 
+@pytest.mark.parametrize('args, kwargs', (
+    (
+        ('hello', ['hello']),
+        {},
+    ),
+    (
+        (),
+        {'csv_data': 'hello', 'rows': ['hello']}
+    ),
+))
+def test_spreadsheet_checks_for_bad_arguments(args, kwargs):
+    with pytest.raises(TypeError) as exception:
+        Spreadsheet(*args, **kwargs)
+    assert str(exception.value) == 'Spreadsheet must be created from either rows or CSV data'
+
+
 @pytest.mark.parametrize('created_by_name, expected_content', [
     (
         None, [