Merge pull request #2953 from alphagov/numbers-in-excel-reports

Don’t convert Excel reports to CSV before output

app/utils.py

@@ -223,14 +223,33 @@ 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
-        self.as_dict = {
+
+        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):
+        return {
             'file_name': self.filename,
             '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
@@ -245,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=''):
@@ -257,7 +272,7 @@ class Spreadsheet():
             zip(
                 *sorted(dictionary.items(), key=lambda pair: pair[0])
             ),
-            filename
+            filename=filename,
         )
 
     @classmethod
@@ -265,7 +280,7 @@ class Spreadsheet():
         extension = cls.get_extension(filename)
 
         if extension == 'csv':
-            return cls(Spreadsheet.normalise_newlines(file_content), filename)
+            return cls(csv_data=Spreadsheet.normalise_newlines(file_content), filename=filename)
 
         if extension == 'tsv':
             file_content = StringIO(
@@ -281,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, [