From d78c98970d20b03e9687d3d5212b60a16a50fd58 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 15:30:32 +0100 Subject: [PATCH 01/10] Removed this date. Wasn't integrated. Data was incorrect and didn't respect it. No API calls ensured it was true. --- app/templates/views/dashboard/all-template-statistics.html | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/templates/views/dashboard/all-template-statistics.html b/app/templates/views/dashboard/all-template-statistics.html index bb3a7d0dc..829d67d19 100644 --- a/app/templates/views/dashboard/all-template-statistics.html +++ b/app/templates/views/dashboard/all-template-statistics.html @@ -10,9 +10,6 @@

Templates sent

-
- 1 April 2016 to date -
{% include 'views/dashboard/template-statistics.html' %} From 55c4443a05f0d130991e03bf0b74c2c766931811 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 15:30:57 +0100 Subject: [PATCH 02/10] Admin app uses the new API response formats. --- app/main/views/dashboard.py | 33 +++--- .../template_statistics_api_client.py | 2 + .../views/dashboard/template-statistics.html | 10 +- tests/app/main/views/test_dashboard.py | 100 ++++++++---------- tests/conftest.py | 15 +-- 5 files changed, 73 insertions(+), 87 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 67588672a..eabb386c9 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -66,11 +66,12 @@ def template_history(service_id): template_statistics = aggregate_usage( template_statistics_client.get_template_statistics_for_service(service_id) ) + return render_template( 'views/dashboard/all-template-statistics.html', template_statistics=template_statistics, most_used_template_count=max( - [row['usage_count'] for row in template_statistics] or [0] + [row['count'] for row in template_statistics] or [0] ) ) @@ -98,34 +99,28 @@ def weekly(service_id): def aggregate_usage(template_statistics): - - immutable_template = namedtuple('Template', ['template_type', 'name', 'id']) - # grouby requires the list to be sorted by template first statistics_sorted_by_template = sorted( - ( - ( - immutable_template(**row['template']), - row['usage_count'] - ) - for row in template_statistics - ), - key=lambda items: items[0] + template_statistics, + key=lambda template_statistic: template_statistic['template_name'] ) - # then group and sort the result by usage - return sorted( + totals = sorted( ( { - 'usage_count': sum(usage[1] for usage in usages), - 'template': template + 'count': sum(usage['count'] for usage in usages), + 'template_name': template_name, + 'template_id': template_id, + 'template_type': template_type } - for template, usages in groupby(statistics_sorted_by_template, lambda items: items[0]) + for (template_name, template_id, template_type), usages in groupby(statistics_sorted_by_template, lambda items: (items['template_name'], items['template_id'], items['template_type'])) # noqa ), - key=lambda row: row['usage_count'], + key=lambda row: row['count'], reverse=True ) + return totals + def get_dashboard_partials(service_id): @@ -149,7 +144,7 @@ def get_dashboard_partials(service_id): 'views/dashboard/template-statistics.html', template_statistics=template_statistics, most_used_template_count=max( - [row['usage_count'] for row in template_statistics] or [0] + [row['count'] for row in template_statistics] or [0] ), ), 'has_template_statistics': bool(template_statistics), diff --git a/app/notify_client/template_statistics_api_client.py b/app/notify_client/template_statistics_api_client.py index b0d54b22c..7829340d2 100644 --- a/app/notify_client/template_statistics_api_client.py +++ b/app/notify_client/template_statistics_api_client.py @@ -16,12 +16,14 @@ class TemplateStatisticsApiClient(BaseAPIClient): params = {} if limit_days is not None: params['limit_days'] = limit_days + return self.get( url='/service/{}/template-statistics'.format(service_id), params=params )['data'] def get_template_statistics_for_template(self, service_id, template_id): + return self.get( url='/service/{}/template-statistics/{}'.format(service_id, template_id) )['data'] diff --git a/app/templates/views/dashboard/template-statistics.html b/app/templates/views/dashboard/template-statistics.html index 819b759cd..11c7d677b 100644 --- a/app/templates/views/dashboard/template-statistics.html +++ b/app/templates/views/dashboard/template-statistics.html @@ -17,18 +17,18 @@ ) %} {% call row_heading() %} - {{ item.template.name }} + {{ item.template_name }} - {{ message_count_label(1, item.template.template_type, suffix='template')|capitalize }} + {{ message_count_label(1, item.template_type, suffix='template')|capitalize }} {% endcall %} {% call field() %} {% if template_statistics|length > 1 %} - + {{ big_number( - item.usage_count, + item.count, smallest=True ) }} @@ -36,7 +36,7 @@ {% else %} {{ big_number( - item.usage_count, + item.count, smallest=True ) }} diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 1be535cbd..d620a6a9f 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -14,48 +14,39 @@ from tests.conftest import SERVICE_ONE_ID stub_template_stats = [ { - 'template': { - 'name': 'Brine Shrimp', - 'template_type': 'sms', - 'id': 1 - }, - 'id': '6005e192-4738-4962-beec-ebd982d0b03f', - 'day': '2016-04-06', - 'usage_count': 6, - 'service': '1491b86f-c950-48f5-bed1-2a55df027ecb' + 'template_type': 'sms', + 'template_name': 'one', + 'template_id': 'id-1', + 'count': 100, + 'day': '2016-01-01' }, { - 'template': { - 'name': 'Pickle feet', - 'template_type': 'sms', - 'id': 2 - }, - 'id': '0bd529cd-a0fd-43e5-80ee-b95ef6b0d51f', - 'day': '2016-04-06', - 'usage_count': 6, - 'service': '1491b86f-c950-48f5-bed1-2a55df027ecb' - }, + 'template_type': 'email', + 'template_name': 'two', + 'template_id': 'id-2', + 'count': 200, + 'day': '2016-01-01' + }, { - 'template': { - 'name': 'Brine Shrimp', - 'template_type': 'sms', - 'id': 1 - }, - 'id': '24531628-ffff-4082-a443-9f6db5af83d9', - 'day': '2016-04-05', - 'usage_count': 7, - 'service': '1491b86f-c950-48f5-bed1-2a55df027ecb' - }, + 'template_type': 'sms', + 'template_name': 'one', + 'template_id': 'id-1', + 'count': 300, + 'day': '2016-01-02' + }, { - 'template': { - 'name': 'Pickle feet', - 'template_type': 'sms', - 'id': 2 - }, - 'id': '0bd529cd-a0fd-43e5-80ee-b95ef6b0d51f', - 'day': '2016-03-06', - 'usage_count': 200, - 'service': '1491b86f-c950-48f5-bed1-2a55df027ecb' + 'template_type': 'sms', + 'template_name': 'one', + 'template_id': 'id-1', + 'count': 400, + 'day': '2016-01-02' + }, + { + 'template_type': 'email', + 'template_name': 'two', + 'template_id': 'id-2', + 'count': 500, + 'day': '2016-01-03' }, ] @@ -83,7 +74,6 @@ def test_get_started( response = client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) # mock_get_service_templates_when_no_templates_exist.assert_called_once_with(SERVICE_ONE_ID) - print(response.get_data(as_text=True)) assert response.status_code == 200 assert 'Get started' in response.get_data(as_text=True) @@ -147,13 +137,13 @@ def test_should_show_recent_templates_on_dashboard(app_, assert len(table_rows) == 2 - assert 'Pickle feet' in table_rows[0].find_all('th')[0].text + assert 'one' in table_rows[0].find_all('th')[0].text assert 'Text message template' in table_rows[0].find_all('th')[0].text - assert '206' in table_rows[0].find_all('td')[0].text + assert '800' in table_rows[0].find_all('td')[0].text - assert 'Brine Shrimp' in table_rows[1].find_all('th')[0].text - assert 'Text message template' in table_rows[1].find_all('th')[0].text - assert '13' in table_rows[1].find_all('td')[0].text + assert 'two' in table_rows[1].find_all('th')[0].text + assert 'Email template' in table_rows[1].find_all('th')[0].text + assert '700' in table_rows[1].find_all('td')[0].text def test_should_show_all_templates_on_template_statistics_page( @@ -186,13 +176,13 @@ def test_should_show_all_templates_on_template_statistics_page( assert len(table_rows) == 2 - assert 'Pickle feet' in table_rows[0].find_all('th')[0].text + assert 'one' in table_rows[0].find_all('th')[0].text assert 'Text message template' in table_rows[0].find_all('th')[0].text - assert '206' in table_rows[0].find_all('td')[0].text + assert '800' in table_rows[0].find_all('td')[0].text - assert 'Brine Shrimp' in table_rows[1].find_all('th')[0].text - assert 'Text message template' in table_rows[1].find_all('th')[0].text - assert '13' in table_rows[1].find_all('td')[0].text + assert 'two' in table_rows[1].find_all('th')[0].text + assert 'Email template' in table_rows[1].find_all('th')[0].text + assert '700' in table_rows[1].find_all('td')[0].text @freeze_time("2016-01-01 11:09:00.061258") @@ -409,10 +399,14 @@ def test_aggregate_template_stats(): assert len(expected) == 2 for item in expected: - if item['template'].id == 1: - assert item['usage_count'] == 13 - elif item['template'].id == 2: - assert item['usage_count'] == 206 + if item['template_name'] == 'one': + assert item['count'] == 800 + assert item['template_id'] == 'id-1' + assert item['template_type'] == 'sms' + elif item['template_name'] == 'two': + assert item['count'] == 700 + assert item['template_id'] == 'id-2' + assert item['template_type'] == 'email' def test_service_dashboard_updates_gets_dashboard_totals(mocker, diff --git a/tests/conftest.py b/tests/conftest.py index 25bde5c81..604bd1f60 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1068,16 +1068,11 @@ def mock_remove_user_from_service(mocker): def mock_get_template_statistics(mocker, service_one, fake_uuid): template = template_json(service_one['id'], fake_uuid, "Test template", "sms", "Something very interesting") data = { - "usage_count": 1, - "template": { - "name": template['name'], - "template_type": template['template_type'], - "id": template['id'] - }, - "service": template['service'], - "id": str(generate_uuid()), - "day": "2016-04-04", - "updated_at": "2016-04-04T12:00:00.000000+00:00" + "count": 1, + "template_name": template['name'], + "template_type": template['template_type'], + "template_id": template['id'], + "day": "2016-04-04" } def _get_stats(service_id, limit_days=None): From f59e05fb1a159d31a7454d93f5c4190141595f3a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 22 Aug 2016 12:35:34 +0100 Subject: [PATCH 03/10] Handle the API migration If it;s the new format API hide the results - Temp fix until new Admin code ships. Should be live for 5 mins. --- app/notify_client/template_statistics_api_client.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/notify_client/template_statistics_api_client.py b/app/notify_client/template_statistics_api_client.py index b0d54b22c..7ffbd291a 100644 --- a/app/notify_client/template_statistics_api_client.py +++ b/app/notify_client/template_statistics_api_client.py @@ -16,11 +16,17 @@ class TemplateStatisticsApiClient(BaseAPIClient): params = {} if limit_days is not None: params['limit_days'] = limit_days - return self.get( + + response = self.get( url='/service/{}/template-statistics'.format(service_id), params=params )['data'] + if len(response) > 0 and 'template' in response[0]: + return response + + return [] + def get_template_statistics_for_template(self, service_id, template_id): return self.get( url='/service/{}/template-statistics/{}'.format(service_id, template_id) From 380a6526c4c27b80cbb706be2d8ed72ddb6793af Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 22 Aug 2016 12:52:05 +0100 Subject: [PATCH 04/10] Changed some more instances of "confirmation" code to "security" code --- app/templates/views/text-not-received-2.html | 4 ++-- app/templates/views/text-not-received.html | 4 ++-- app/templates/views/user-profile/confirm.html | 2 +- app/templates/views/verify-mobile.html | 4 ++-- tests/app/main/views/test_code_not_received.py | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/templates/views/text-not-received-2.html b/app/templates/views/text-not-received-2.html index 4e10ac956..3a49f39a7 100644 --- a/app/templates/views/text-not-received-2.html +++ b/app/templates/views/text-not-received-2.html @@ -12,7 +12,7 @@ Check your mobile number – GOV.UK Notify

Check your mobile number

-

Check your mobile phone number is correct and then resend the confirmation code.

+

Check your mobile phone number is correct and then resend the security code.

@@ -21,7 +21,7 @@ Check your mobile number – GOV.UK Notify

- Resend confirmation code + Resend security code

diff --git a/app/templates/views/text-not-received.html b/app/templates/views/text-not-received.html index e6bab7546..4ee286f08 100644 --- a/app/templates/views/text-not-received.html +++ b/app/templates/views/text-not-received.html @@ -12,11 +12,11 @@ Check your mobile number – GOV.UK Notify

Check your mobile number

-

Check your mobile phone number is correct and then resend the confirmation code.

+

Check your mobile phone number is correct and then resend the security code.

{{ textbox(form.mobile_number) }} - {{ page_footer("Resend confirmation code") }} + {{ page_footer("Resend security code") }}
diff --git a/app/templates/views/user-profile/confirm.html b/app/templates/views/user-profile/confirm.html index 069fe0e7e..0a4d8db32 100644 --- a/app/templates/views/user-profile/confirm.html +++ b/app/templates/views/user-profile/confirm.html @@ -13,7 +13,7 @@ GOV.UK Notify | Service settings

- We’ve sent a code to your new {{ thing }}. + We’ve sent a security code to your new {{ thing }}.

{{ textbox(form_field) }} diff --git a/app/templates/views/verify-mobile.html b/app/templates/views/verify-mobile.html index 2ed9dbdd1..f10427872 100644 --- a/app/templates/views/verify-mobile.html +++ b/app/templates/views/verify-mobile.html @@ -10,10 +10,10 @@ Confirm your mobile number – GOV.UK Notify

Confirm your mobile number

-

We've sent you a confirmation code by text message.

+

We've sent you a security code by text message.

-

diff --git a/tests/app/main/views/test_code_not_received.py b/tests/app/main/views/test_code_not_received.py index 271b6ee81..675885111 100644 --- a/tests/app/main/views/test_code_not_received.py +++ b/tests/app/main/views/test_code_not_received.py @@ -66,7 +66,7 @@ def test_should_render_correct_resend_template_for_pending_user(app_, page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.string == 'Check your mobile number' - expected = 'Check your mobile phone number is correct and then resend the confirmation code.' + expected = 'Check your mobile phone number is correct and then resend the security code.' message = page.find_all('p')[1].text assert message == expected assert page.find('form').input['value'] == api_user_pending.mobile_number From 63fa77fde75a017373d8b1a9a1aa6a52000723f0 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 22 Aug 2016 13:14:35 +0100 Subject: [PATCH 05/10] Admin app now in line with new API response for template statistics - dropped the day level aggregate. Now grouped by template only, --- app/main/views/dashboard.py | 19 +-- tests/app/main/views/test_dashboard.py | 166 ++++++++++--------------- 2 files changed, 69 insertions(+), 116 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index eabb386c9..0762a0d3a 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -99,28 +99,11 @@ def weekly(service_id): def aggregate_usage(template_statistics): - # grouby requires the list to be sorted by template first - statistics_sorted_by_template = sorted( + return sorted( template_statistics, key=lambda template_statistic: template_statistic['template_name'] ) - totals = sorted( - ( - { - 'count': sum(usage['count'] for usage in usages), - 'template_name': template_name, - 'template_id': template_id, - 'template_type': template_type - } - for (template_name, template_id, template_type), usages in groupby(statistics_sorted_by_template, lambda items: (items['template_name'], items['template_id'], items['template_type'])) # noqa - ), - key=lambda row: row['count'], - reverse=True - ) - - return totals - def get_dashboard_partials(service_id): diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index d620a6a9f..fe53b26e8 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -11,61 +11,36 @@ from app.main.views.dashboard import get_dashboard_totals, format_weekly_stats_t from tests import validate_route_permission from tests.conftest import SERVICE_ONE_ID - stub_template_stats = [ { 'template_type': 'sms', 'template_name': 'one', 'template_id': 'id-1', - 'count': 100, - 'day': '2016-01-01' + 'count': 100 }, { 'template_type': 'email', 'template_name': 'two', 'template_id': 'id-2', - 'count': 200, - 'day': '2016-01-01' - }, - { - 'template_type': 'sms', - 'template_name': 'one', - 'template_id': 'id-1', - 'count': 300, - 'day': '2016-01-02' - }, - { - 'template_type': 'sms', - 'template_name': 'one', - 'template_id': 'id-1', - 'count': 400, - 'day': '2016-01-02' - }, - { - 'template_type': 'email', - 'template_name': 'two', - 'template_id': 'id-2', - 'count': 500, - 'day': '2016-01-03' - }, + 'count': 200 + } ] def test_get_started( - app_, - mocker, - api_user_active, - mock_get_service, - mock_get_service_templates_when_no_templates_exist, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_get_jobs, - mock_has_permissions, - mock_get_detailed_service, - mock_get_usage + app_, + mocker, + api_user_active, + mock_get_service, + mock_get_service_templates_when_no_templates_exist, + mock_get_user, + mock_get_user_by_email, + mock_login, + mock_get_jobs, + mock_has_permissions, + mock_get_detailed_service, + mock_get_usage ): - mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -79,18 +54,18 @@ def test_get_started( def test_get_started_is_hidden_once_templates_exist( - app_, - mocker, - api_user_active, - mock_get_service, - mock_get_service_templates, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_get_jobs, - mock_has_permissions, - mock_get_detailed_service, - mock_get_usage + app_, + mocker, + api_user_active, + mock_get_service, + mock_get_service_templates, + mock_get_user, + mock_get_user_by_email, + mock_login, + mock_get_jobs, + mock_has_permissions, + mock_get_detailed_service, + mock_get_usage ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -115,7 +90,6 @@ def test_should_show_recent_templates_on_dashboard(app_, mock_has_permissions, mock_get_detailed_service, mock_get_usage): - mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -139,26 +113,25 @@ def test_should_show_recent_templates_on_dashboard(app_, assert 'one' in table_rows[0].find_all('th')[0].text assert 'Text message template' in table_rows[0].find_all('th')[0].text - assert '800' in table_rows[0].find_all('td')[0].text + assert '100' in table_rows[0].find_all('td')[0].text assert 'two' in table_rows[1].find_all('th')[0].text assert 'Email template' in table_rows[1].find_all('th')[0].text - assert '700' in table_rows[1].find_all('td')[0].text + assert '200' in table_rows[1].find_all('td')[0].text def test_should_show_all_templates_on_template_statistics_page( - app_, - mocker, - api_user_active, - mock_get_service, - mock_get_service_templates, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_get_jobs, - mock_has_permissions + app_, + mocker, + api_user_active, + mock_get_service, + mock_get_service_templates, + mock_get_user, + mock_get_user_by_email, + mock_login, + mock_get_jobs, + mock_has_permissions ): - mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -178,30 +151,29 @@ def test_should_show_all_templates_on_template_statistics_page( assert 'one' in table_rows[0].find_all('th')[0].text assert 'Text message template' in table_rows[0].find_all('th')[0].text - assert '800' in table_rows[0].find_all('td')[0].text + assert '100' in table_rows[0].find_all('td')[0].text assert 'two' in table_rows[1].find_all('th')[0].text assert 'Email template' in table_rows[1].find_all('th')[0].text - assert '700' in table_rows[1].find_all('td')[0].text + assert '200' in table_rows[1].find_all('td')[0].text @freeze_time("2016-01-01 11:09:00.061258") def test_should_show_recent_jobs_on_dashboard( - app_, - mocker, - api_user_active, - mock_get_service, - mock_get_service_templates, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_get_template_statistics, - mock_get_detailed_service, - mock_get_jobs, - mock_has_permissions, - mock_get_usage + app_, + mocker, + api_user_active, + mock_get_service, + mock_get_service_templates, + mock_get_user, + mock_get_user_by_email, + mock_login, + mock_get_template_statistics, + mock_get_detailed_service, + mock_get_jobs, + mock_has_permissions, + mock_get_usage ): - with app_.test_request_context(), app_.test_client() as client: client.login(api_user_active) response = client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) @@ -216,10 +188,10 @@ def test_should_show_recent_jobs_on_dashboard( assert len(table_rows) == 4 for index, filename in enumerate(( - "export 1/1/2016.xls", - "all email addresses.xlsx", - "applicants.ods", - "thisisatest.csv", + "export 1/1/2016.xls", + "all email addresses.xlsx", + "applicants.ods", + "thisisatest.csv", )): assert filename in table_rows[index].find_all('th')[0].text assert 'Uploaded 1 January at 11:09' in table_rows[index].find_all('th')[0].text @@ -249,7 +221,6 @@ def test_menu_send_messages(mocker, mock_get_template_statistics, mock_get_detailed_service, mock_get_usage): - with app_.test_request_context(): resp = _test_dashboard_menu( mocker, @@ -261,11 +232,11 @@ def test_menu_send_messages(mocker, assert url_for( 'main.choose_template', service_id=service_one['id'], - template_type='email')in page + template_type='email') in page assert url_for( 'main.choose_template', service_id=service_one['id'], - template_type='sms')in page + template_type='sms') in page assert url_for('main.manage_users', service_id=service_one['id']) in page assert url_for('main.service_settings', service_id=service_one['id']) not in page @@ -398,15 +369,14 @@ def test_aggregate_template_stats(): expected = aggregate_usage(copy.deepcopy(stub_template_stats)) assert len(expected) == 2 - for item in expected: - if item['template_name'] == 'one': - assert item['count'] == 800 - assert item['template_id'] == 'id-1' - assert item['template_type'] == 'sms' - elif item['template_name'] == 'two': - assert item['count'] == 700 - assert item['template_id'] == 'id-2' - assert item['template_type'] == 'email' + assert expected[0]['template_name'] == 'one' + assert expected[0]['count'] == 100 + assert expected[0]['template_id'] == 'id-1' + assert expected[0]['template_type'] == 'sms' + assert expected[1]['template_name'] == 'two' + assert expected[1]['count'] == 200 + assert expected[1]['template_id'] == 'id-2' + assert expected[1]['template_type'] == 'email' def test_service_dashboard_updates_gets_dashboard_totals(mocker, From 1e254415d2f00d85de085f7ff0fb1279932e08ed Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Aug 2016 13:30:03 +0100 Subject: [PATCH 06/10] Text message not Sms --- app/main/forms.py | 2 +- tests/app/main/test_validators.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index c1f50a089..820d47c9c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -337,7 +337,7 @@ class ServiceSmsSender(Form): def validate_sms_sender(form, field): import re if field.data and not re.match('^[a-zA-Z0-9\s]+$', field.data): - raise ValidationError('Sms text message sender can only contain alpha-numeric characters') + raise ValidationError('Text message sender can only contain alpha-numeric characters') class ServiceBrandingOrg(Form): diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index f8b1a6490..5ea1300ca 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -141,4 +141,4 @@ def test_sms_sender_form_validation(app_, mock_get_user_by_email): form.sms_sender.data = '###########' form.validate() - assert 'Sms text message sender can only contain alpha-numeric characters' == form.errors['sms_sender'][0] + assert 'Text message sender can only contain alpha-numeric characters' == form.errors['sms_sender'][0] From d893a67fc2c97c15cef40a6668646a13e0dd347b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Aug 2016 15:47:25 +0100 Subject: [PATCH 07/10] Another content change from Gwen _Missed this one because it was in a different email_ We don't use 'we'. It's better to say 'An email has been sent to...' --- app/templates/views/registration-continue.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/registration-continue.html b/app/templates/views/registration-continue.html index ab3e54275..e2d6e2be4 100644 --- a/app/templates/views/registration-continue.html +++ b/app/templates/views/registration-continue.html @@ -7,7 +7,7 @@ {% block maincolumn_content %}

Check your email​

-

We’ve sent an email to {{ session['user_details']['email'] }}.

+

An email has been sent to {{ session['user_details']['email'] }}.

Click the link in the email to continue your registration.

{% endblock %} From 54e4311d1bdf4e44635b220e9bfdfba5c75c34fa Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 22 Aug 2016 16:56:43 +0100 Subject: [PATCH 08/10] Merged in the defensive code by mistake --- app/notify_client/template_statistics_api_client.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/notify_client/template_statistics_api_client.py b/app/notify_client/template_statistics_api_client.py index e1965166f..7829340d2 100644 --- a/app/notify_client/template_statistics_api_client.py +++ b/app/notify_client/template_statistics_api_client.py @@ -17,16 +17,11 @@ class TemplateStatisticsApiClient(BaseAPIClient): if limit_days is not None: params['limit_days'] = limit_days - response = self.get( + return self.get( url='/service/{}/template-statistics'.format(service_id), params=params )['data'] - if len(response) > 0 and 'template' in response[0]: - return response - - return [] - def get_template_statistics_for_template(self, service_id, template_id): return self.get( From 26ff449b77a53625d77df6412a61e792772c8c5a Mon Sep 17 00:00:00 2001 From: bandesz Date: Fri, 12 Aug 2016 11:23:50 +0100 Subject: [PATCH 09/10] Create Docker build image, build project with Docker --- .gitignore | 3 + .travis.yml | 2 +- Makefile | 128 ++++++++++++++++++++++++++++ app/{version.py => version.py.dist} | 3 +- appspec.yml | 16 ++-- deploy-exclude.lst | 9 ++ docker/Dockerfile-build | 36 ++++++++ docker/Makefile | 10 +++ scripts/common_functions.sh | 39 ++++++++- scripts/deregister_from_elb.sh | 4 +- scripts/register_with_elb.sh | 6 +- scripts/run_tests.sh | 3 + scripts/update_version_file.sh | 7 -- 13 files changed, 244 insertions(+), 22 deletions(-) create mode 100644 Makefile rename app/{version.py => version.py.dist} (56%) create mode 100644 deploy-exclude.lst create mode 100644 docker/Dockerfile-build create mode 100644 docker/Makefile delete mode 100755 scripts/update_version_file.sh diff --git a/.gitignore b/.gitignore index 2ff4784bb..8d5a836e2 100644 --- a/.gitignore +++ b/.gitignore @@ -70,3 +70,6 @@ app/templates/govuk_template.html npm-debug.log environment.sh +.envrc + +app/version.py diff --git a/.travis.yml b/.travis.yml index e353329e3..9a0a62f10 100644 --- a/.travis.yml +++ b/.travis.yml @@ -18,6 +18,7 @@ after_success: - coveralls - ./scripts/trigger-dependent-build.sh script: +- make generate-version-file - npm run build - ./scripts/run_tests.sh notifications: @@ -88,7 +89,6 @@ deploy: region: eu-west-1 on: *2 before_deploy: - - ./scripts/update_version_file.sh - rm -rf node_modules bower_components app/assets - zip -r --exclude=*__pycache__* notifications-admin * - mkdir -p dpl_cd_upload diff --git a/Makefile b/Makefile new file mode 100644 index 000000000..f046f9c04 --- /dev/null +++ b/Makefile @@ -0,0 +1,128 @@ +.DEFAULT_GOAL := help +SHELL := /bin/bash +DATE = $(shell date +%Y-%m-%d:%H:%M:%S) + +PIP_ACCEL_CACHE ?= ${CURDIR}/cache/pip-accel +APP_VERSION_FILE = app/version.py + +GIT_BRANCH = $(shell git symbolic-ref --short HEAD 2> /dev/null || echo "detached") +GIT_COMMIT ?= $(shell git rev-parse HEAD) + +DOCKER_BUILDER_IMAGE_NAME = govuk/notify-admin-builder + +BUILD_TAG ?= notifications-admin-manual +BUILD_NUMBER ?= 0 +DEPLOY_BUILD_NUMBER ?= ${BUILD_NUMBER} +BUILD_URL ?= + +DOCKER_CONTAINER_PREFIX = ${USER}-${BUILD_TAG} + +.PHONY: help +help: + @cat $(MAKEFILE_LIST) | grep -E '^[a-zA-Z_-]+:.*?## .*$$' | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' + +.PHONY: venv +venv: venv/bin/activate ## Create virtualenv if it does not exist + +venv/bin/activate: + test -d venv || virtualenv venv + ./venv/bin/pip install pip-accel + +.PHONY: check-env-vars +check-env-vars: ## Check mandatory environment variables + $(if ${DEPLOY_ENV},,$(error Must specify DEPLOY_ENV)) + $(if ${DNS_NAME},,$(error Must specify DNS_NAME)) + $(if ${AWS_ACCESS_KEY_ID},,$(error Must specify AWS_ACCESS_KEY_ID)) + $(if ${AWS_SECRET_ACCESS_KEY},,$(error Must specify AWS_SECRET_ACCESS_KEY)) + +.PHONY: development +development: ## Set environment to development + $(eval export DEPLOY_ENV=development) + $(eval export DNS_NAME="notify.works") + @true + +.PHONY: staging +staging: ## Set environment to staging + $(eval export DEPLOY_ENV=staging) + $(eval export DNS_NAME="staging-notify.works") + @true + +.PHONY: production +production: ## Set environment to production + $(eval export DEPLOY_ENV=production) + $(eval export DNS_NAME="notifications.service.gov.uk") + @true + +.PHONY: dependencies +dependencies: venv ## Install build dependencies + npm set progress=false + npm install + npm rebuild node-sass + mkdir -p ${PIP_ACCEL_CACHE} + PIP_ACCEL_CACHE=${PIP_ACCEL_CACHE} ./venv/bin/pip-accel install -r requirements_for_test.txt + +.PHONY: generate-version-file +generate-version-file: ## Generates the app version file + @echo -e "__travis_commit__ = \"${GIT_COMMIT}\"\n__time__ = \"${DATE}\"\n__travis_job_number__ = \"${BUILD_NUMBER}\"\n__travis_job_url__ = \"${BUILD_URL}\"" > ${APP_VERSION_FILE} + +.PHONY: build +build: dependencies generate-version-file ## Build project + npm run build + +.PHONY: build-codedeploy-artifact +build-codedeploy-artifact: ## Build the deploy artifact for CodeDeploy + mkdir -p target + zip -r -x@deploy-exclude.lst target/notifications-admin.zip * + +.PHONY: upload-codedeploy-artifact ## Upload the deploy artifact for CodeDeploy +upload-codedeploy-artifact: check-env-vars + aws s3 cp --region eu-west-1 target/notifications-admin.zip s3://${DNS_NAME}-codedeploy/notifications-admin-${DEPLOY_BUILD_NUMBER}.zip + +.PHONY: test +test: venv ## Run tests + ./scripts/run_tests.sh + +.PHONY: deploy +deploy: check-env-vars ## Upload deploy artifacts to S3 and trigger CodeDeploy + aws deploy create-deployment --application-name notify-admin --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-admin --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-admin-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 + +.PHONY: coverage +coverage: venv ## Create coverage report + ./venv/bin/coveralls + +.PHONY: prepare-docker-build-image +prepare-docker-build-image: ## Prepare the Docker builder image + mkdir -p ${PIP_ACCEL_CACHE} + make -C docker build-build-image + +.PHONY: build-with-docker +build-with-docker: prepare-docker-build-image ## Build inside a Docker container + docker run -i --rm \ + --name "${DOCKER_CONTAINER_PREFIX}-build" \ + -v `pwd`:/var/project \ + -v ${PIP_ACCEL_CACHE}:/var/project/cache/pip-accel \ + ${DOCKER_BUILDER_IMAGE_NAME} \ + make build + +.PHONY: test-with-docker +test-with-docker: prepare-docker-build-image ## Run tests inside a Docker container + docker run -i --rm \ + --name "${DOCKER_CONTAINER_PREFIX}-test" \ + -v `pwd`:/var/project \ + ${DOCKER_BUILDER_IMAGE_NAME} \ + make test + +.PHONY: coverage-with-docker +coverage-with-docker: prepare-docker-build-image ## Generates coverage report inside a Docker container + docker run -i --rm \ + --name "${DOCKER_CONTAINER_PREFIX}-coverage" \ + -v `pwd`:/var/project \ + ${DOCKER_BUILDER_IMAGE_NAME} \ + make coverage + +.PHONY: clean-docker-containers +clean-docker-containers: ## Clean up any remaining docker containers + docker rm -f $(shell docker ps -q -f "name=${DOCKER_CONTAINER_PREFIX}") 2> /dev/null || true + +clean: + rm -rf node_modules cache target venv .coverage diff --git a/app/version.py b/app/version.py.dist similarity index 56% rename from app/version.py rename to app/version.py.dist index fc30d1fca..295df522a 100644 --- a/app/version.py +++ b/app/version.py.dist @@ -1,3 +1,4 @@ __travis_commit__ = "" -__time__ = "2016-07-05:14:44:52" +__time__ = "" __travis_job_number__ = "" +__travis_job_url__ = "" diff --git a/appspec.yml b/appspec.yml index c663536ab..882459f2e 100644 --- a/appspec.yml +++ b/appspec.yml @@ -1,11 +1,11 @@ ---- -files: - - +--- +files: + - destination: /home/notify-app/notifications-admin source: / -hooks: - AfterInstall: - - +hooks: + AfterInstall: + - location: scripts/aws_install_dependencies.sh runas: root timeout: 300 @@ -27,11 +27,9 @@ hooks: location: scripts/deregister_from_elb.sh runas: ubuntu timeout: 300 - - + - location: scripts/aws_stop_app.sh runas: root timeout: 300 os: linux version: 0.0 - - diff --git a/deploy-exclude.lst b/deploy-exclude.lst new file mode 100644 index 000000000..5eaf61748 --- /dev/null +++ b/deploy-exclude.lst @@ -0,0 +1,9 @@ +*__pycache__* +*.git* +*app/assets* +*bower_components* +*cache* +*docker* +*node_modules* +*target* +*venv* diff --git a/docker/Dockerfile-build b/docker/Dockerfile-build new file mode 100644 index 000000000..105f1fdca --- /dev/null +++ b/docker/Dockerfile-build @@ -0,0 +1,36 @@ +FROM python:3.4-slim + +ENV PYTHONUNBUFFERED=1 \ + DEBIAN_FRONTEND=noninteractive \ + NODEJS_VERSION=6.3.1-1nodesource1~jessie1 + +RUN \ + echo "Install base packages" \ + && apt-get update \ + && apt-get install -y --no-install-recommends \ + apt-transport-https \ + make \ + curl \ + git \ + build-essential \ + libxml2-dev \ + libxslt-dev \ + zlib1g-dev \ + zip \ + rlwrap \ + + && echo "Install nodejs" \ + && cd /tmp \ + && curl -sSLO https://deb.nodesource.com/node_6.x/pool/main/n/nodejs/nodejs_${NODEJS_VERSION}_amd64.deb \ + && dpkg -i /tmp/nodejs_${NODEJS_VERSION}_amd64.deb \ + + && echo "Clean up" \ + && rm -rf /var/lib/apt/lists/* /tmp/* + +RUN \ + echo "Install global pip packages" \ + && pip install \ + virtualenv \ + awscli + +WORKDIR /var/project diff --git a/docker/Makefile b/docker/Makefile new file mode 100644 index 000000000..ea3add478 --- /dev/null +++ b/docker/Makefile @@ -0,0 +1,10 @@ +.DEFAULT_GOAL := help +SHELL := /bin/bash + +.PHONY: help +help: + @cat $(MAKEFILE_LIST) | grep -E '^[a-zA-Z_-]+:.*?## .*$$' | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' + +.PHONY: build-build-image +build-build-image: + docker build -f Dockerfile-build -t govuk/notify-admin-builder . diff --git a/scripts/common_functions.sh b/scripts/common_functions.sh index 09cdbe4e0..b0f6694d9 100644 --- a/scripts/common_functions.sh +++ b/scripts/common_functions.sh @@ -183,7 +183,7 @@ get_instance_health_elb() { ;; *) msg "Instance '$instance_id' not part of ELB '$elb_name'" - return 0 + return 1 esac fi } @@ -345,3 +345,40 @@ get_instance_id() { curl -s http://169.254.169.254/latest/meta-data/instance-id return $? } + +# Usage: get_instance_name_from_tags +# +# Looks up tags for the given instance, extracting the 'name' +# returns or error_exit +get_instance_name_from_tags() { + local instance_id=$1 + + local instance_name=$($AWS_CLI ec2 describe-tags \ + --filters "Name=resource-id,Values=${instance_id}" \ + --query Tags[0].Value \ + --output text) + if [ $? != 0 ]; then + error_exit "Couldn't get instance name for '$instance_id'" + fi + echo $instance_name + return $? +} + +ELB_NAME="" + +get_elb_name_for_instance_name() { + local instance_name=$1 + + declare -A elb_to_instance_mapping + + elb_to_instance_mapping['notify-admin']='notify-admin' + + elb_to_instance_mapping['notify_admin']='notify-admin-elb' + + local elb_name=${elb_to_instance_mapping[${instance_name}]} + if [ -z $elb_name ]; then + msg "No ELB for instance ${instance_name}" + else + ELB_NAME=$elb_name + fi +} diff --git a/scripts/deregister_from_elb.sh b/scripts/deregister_from_elb.sh index 25650196b..62bab1612 100755 --- a/scripts/deregister_from_elb.sh +++ b/scripts/deregister_from_elb.sh @@ -28,7 +28,9 @@ msg "Started $(basename $0) at $(/bin/date "+%F %T")" start_sec=$(/bin/date +%s.%N) msg "Getting relevant load balancer" -get_elb_list $INSTANCE_ID "notify-admin-elb" +INSTANCE_NAME=$(get_instance_name_from_tags $INSTANCE_ID) +get_elb_name_for_instance_name $INSTANCE_NAME +get_elb_list $INSTANCE_ID $ELB_NAME msg "Checking that user set at least one load balancer" if test -z "$ELB_LIST"; then diff --git a/scripts/register_with_elb.sh b/scripts/register_with_elb.sh index 9385716d4..7cc78006c 100755 --- a/scripts/register_with_elb.sh +++ b/scripts/register_with_elb.sh @@ -28,8 +28,10 @@ msg "Started $(basename $0) at $(/bin/date "+%F %T")" start_sec=$(/bin/date +%s.%N) msg "Getting relevant load balancer" -get_elb_list $INSTANCE_ID "notify-admin-elb" - +INSTANCE_NAME=$(get_instance_name_from_tags $INSTANCE_ID) +get_elb_name_for_instance_name $INSTANCE_NAME +ELB_LIST=$ELB_NAME +get_elb_list $INSTANCE_ID $ELB_NAME msg "Checking that user set at least one load balancer" if test -z "$ELB_LIST"; then diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index b1d6333f3..9bd70ae62 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -24,6 +24,9 @@ function display_result { fi } +if [ -d venv ]; then + source ./venv/bin/activate +fi pep8 . display_result $? 1 "Code style check" diff --git a/scripts/update_version_file.sh b/scripts/update_version_file.sh deleted file mode 100755 index 231c210b7..000000000 --- a/scripts/update_version_file.sh +++ /dev/null @@ -1,7 +0,0 @@ -#!/bin/bash -# -# Update the version file of the project from the Travis build details -# -sed -i -e "s/__travis_commit__ =.*/__travis_commit__ = \"$TRAVIS_COMMIT\"/g" ./app/version.py -sed -i -e "s/__travis_job_number__ =.*/__travis_job_number__ = \"$TRAVIS_BUILD_NUMBER\"/g" ./app/version.py -sed -i -e "s/__time__ =.*/__time__ = \"$(date +%Y-%m-%d:%H:%M:%S)\"/g" ./app/version.py \ No newline at end of file From 4dec7a79399e028facd9d980e39b17bc765c7e66 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 23 Aug 2016 09:14:21 +0100 Subject: [PATCH 10/10] Changed order of templates activity page from alphabetical to desc count. --- app/main/views/dashboard.py | 3 +- tests/app/main/views/test_dashboard.py | 40 +++++++++++++------------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 0762a0d3a..7ce396923 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -101,7 +101,8 @@ def weekly(service_id): def aggregate_usage(template_statistics): return sorted( template_statistics, - key=lambda template_statistic: template_statistic['template_name'] + key=lambda template_statistic: template_statistic['count'], + reverse=True ) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index fe53b26e8..4cf56ed1a 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -111,13 +111,13 @@ def test_should_show_recent_templates_on_dashboard(app_, assert len(table_rows) == 2 - assert 'one' in table_rows[0].find_all('th')[0].text - assert 'Text message template' in table_rows[0].find_all('th')[0].text - assert '100' in table_rows[0].find_all('td')[0].text + assert 'two' in table_rows[0].find_all('th')[0].text + assert 'Email template' in table_rows[0].find_all('th')[0].text + assert '200' in table_rows[0].find_all('td')[0].text - assert 'two' in table_rows[1].find_all('th')[0].text - assert 'Email template' in table_rows[1].find_all('th')[0].text - assert '200' in table_rows[1].find_all('td')[0].text + assert 'one' in table_rows[1].find_all('th')[0].text + assert 'Text message template' in table_rows[1].find_all('th')[0].text + assert '100' in table_rows[1].find_all('td')[0].text def test_should_show_all_templates_on_template_statistics_page( @@ -149,13 +149,13 @@ def test_should_show_all_templates_on_template_statistics_page( assert len(table_rows) == 2 - assert 'one' in table_rows[0].find_all('th')[0].text - assert 'Text message template' in table_rows[0].find_all('th')[0].text - assert '100' in table_rows[0].find_all('td')[0].text + assert 'two' in table_rows[0].find_all('th')[0].text + assert 'Email template' in table_rows[0].find_all('th')[0].text + assert '200' in table_rows[0].find_all('td')[0].text - assert 'two' in table_rows[1].find_all('th')[0].text - assert 'Email template' in table_rows[1].find_all('th')[0].text - assert '200' in table_rows[1].find_all('td')[0].text + assert 'one' in table_rows[1].find_all('th')[0].text + assert 'Text message template' in table_rows[1].find_all('th')[0].text + assert '100' in table_rows[1].find_all('td')[0].text @freeze_time("2016-01-01 11:09:00.061258") @@ -369,14 +369,14 @@ def test_aggregate_template_stats(): expected = aggregate_usage(copy.deepcopy(stub_template_stats)) assert len(expected) == 2 - assert expected[0]['template_name'] == 'one' - assert expected[0]['count'] == 100 - assert expected[0]['template_id'] == 'id-1' - assert expected[0]['template_type'] == 'sms' - assert expected[1]['template_name'] == 'two' - assert expected[1]['count'] == 200 - assert expected[1]['template_id'] == 'id-2' - assert expected[1]['template_type'] == 'email' + assert expected[0]['template_name'] == 'two' + assert expected[0]['count'] == 200 + assert expected[0]['template_id'] == 'id-2' + assert expected[0]['template_type'] == 'email' + assert expected[1]['template_name'] == 'one' + assert expected[1]['count'] == 100 + assert expected[1]['template_id'] == 'id-1' + assert expected[1]['template_type'] == 'sms' def test_service_dashboard_updates_gets_dashboard_totals(mocker,