From 5879df5c03f5e2f75a2442436bda43b0e3bb0dc7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 May 2019 10:36:41 +0100 Subject: [PATCH 1/3] Move `as_dict` property to its own method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This helps declutter the init method, and is one less thing to create in memory when it’s not needed. --- app/utils.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/utils.py b/app/utils.py index 83774dc06..a3a82295d 100644 --- a/app/utils.py +++ b/app/utils.py @@ -226,7 +226,10 @@ class Spreadsheet(): def __init__(self, csv_data, filename=''): self.filename = filename self.as_csv_data = csv_data - self.as_dict = { + + @property + def as_dict(self): + return { 'file_name': self.filename, 'data': self.as_csv_data } From 55d4810c4b8e5001d5ae7513177b8af8f4a16187 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 May 2019 10:50:45 +0100 Subject: [PATCH 2/3] =?UTF-8?q?Don=E2=80=99t=20convert=20Excel=20reports?= =?UTF-8?q?=20to=20CSV=20before=20output?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 42 ++++++++++++++------- tests/app/main/views/test_platform_admin.py | 8 ++-- tests/app/test_utils.py | 16 ++++++++ 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/app/utils.py b/app/utils.py index a3a82295d..c129697cb 100644 --- a/app/utils.py +++ b/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): diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index 632902a98..44a5bb81f 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/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], ] diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index cb5a85b1c..6a3ded471 100644 --- a/tests/app/test_utils.py +++ b/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, [ From 20e774ca33e6510acd778fcc531301a4f51bd3f5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 8 May 2019 10:17:20 +0100 Subject: [PATCH 3/3] Use keyword argument for explicitness --- app/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/utils.py b/app/utils.py index c129697cb..6bf6a6679 100644 --- a/app/utils.py +++ b/app/utils.py @@ -280,7 +280,7 @@ class Spreadsheet(): extension = cls.get_extension(filename) if extension == 'csv': - return cls(Spreadsheet.normalise_newlines(file_content), filename=filename) + return cls(csv_data=Spreadsheet.normalise_newlines(file_content), filename=filename) if extension == 'tsv': file_content = StringIO(