From 0ea68f9c6d6bd808597c2cc64deaa185cbda93fb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 27 Oct 2017 10:56:03 +0100 Subject: [PATCH 1/5] fix linter errors --- app/main/forms.py | 4 ++-- app/main/views/dashboard.py | 8 ++++---- tests/app/main/views/test_conversation.py | 2 +- tests/app/main/views/test_letter_jobs.py | 20 ++++++++++---------- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index b6a5aaf38..33712c932 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -506,7 +506,7 @@ class ServiceSmsSender(Form): ] ) - def validate_sms_sender(form, field): + def validate_sms_sender(self, field): if field.data and not re.match(r'^[a-zA-Z0-9\s]+$', field.data): raise ValidationError('Use letters and numbers only') @@ -520,7 +520,7 @@ class ServiceLetterContactBlockForm(Form): ) is_default = BooleanField("Set as your default address") - def validate_letter_contact_block(form, field): + def validate_letter_contact_block(self, field): line_count = field.data.strip().count('\n') if line_count >= 10: raise ValidationError( diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index f4d5dad19..ce63fdc00 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -80,7 +80,7 @@ def template_history(service_id): months = [ { - 'name': YYYY_MM_to_datetime(month).strftime('%B'), + 'name': yyyy_mm_to_datetime(month).strftime('%B'), 'templates_used': aggregate_usage( format_template_stats_to_list(stats.get(month)), sort_key='requested_count' ), @@ -310,14 +310,14 @@ def format_monthly_stats_to_list(historical_stats): return sorted(( dict( date=key, - future=YYYY_MM_to_datetime(key) > datetime.utcnow(), - name=YYYY_MM_to_datetime(key).strftime('%B'), + future=yyyy_mm_to_datetime(key) > datetime.utcnow(), + name=yyyy_mm_to_datetime(key).strftime('%B'), **aggregate_status_types(value) ) for key, value in historical_stats.items() ), key=lambda x: x['date']) -def YYYY_MM_to_datetime(string): +def yyyy_mm_to_datetime(string): return datetime(int(string[0:4]), int(string[5:7]), 1) diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index bc5e2eabd..dba58a14c 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -50,7 +50,7 @@ def test_get_user_phone_number_when_only_outbound_exists(mocker): mock_get_notification.assert_called_once_with('service', 'notification') -def test_get_user_phone_number_raises_if_both_API_requests_fail(mocker): +def test_get_user_phone_number_raises_if_both_api_requests_fail(mocker): mock_get_inbound_sms = mocker.patch( 'app.main.views.conversation.service_api_client.get_inbound_sms_by_id', side_effect=HTTPError, diff --git a/tests/app/main/views/test_letter_jobs.py b/tests/app/main/views/test_letter_jobs.py index 9d45b07e4..ac071d2d5 100644 --- a/tests/app/main/views/test_letter_jobs.py +++ b/tests/app/main/views/test_letter_jobs.py @@ -31,7 +31,7 @@ valid_letter_jobs = [ send_letter_jobs_response = {"response": "Task created to send files to DVLA"} -class letter_jobs_header(IntEnum): +class LetterJobsHeader(IntEnum): SERVICE_NAME = 0 JOB_ID = 1 NOTIFICATION_COUNT = 2 @@ -58,15 +58,15 @@ def test_get_letter_jobs_returns_list_of_all_letter_jobs(logged_in_platform_admi for row_pos in range(len(rows)): cols = rows[row_pos].find_all('td') - assert valid_letter_jobs[row_pos]['service_name']['name'] == cols[letter_jobs_header.SERVICE_NAME].text - assert valid_letter_jobs[row_pos]['id'] == cols[letter_jobs_header.JOB_ID].text - assert valid_letter_jobs[row_pos]['notification_count'] == int(cols[letter_jobs_header.NOTIFICATION_COUNT].text) - assert valid_letter_jobs[row_pos]['job_status'] == cols[letter_jobs_header.JOB_STATUS].text + assert valid_letter_jobs[row_pos]['service_name']['name'] == cols[LetterJobsHeader.SERVICE_NAME].text + assert valid_letter_jobs[row_pos]['id'] == cols[LetterJobsHeader.JOB_ID].text + assert valid_letter_jobs[row_pos]['notification_count'] == int(cols[LetterJobsHeader.NOTIFICATION_COUNT].text) + assert valid_letter_jobs[row_pos]['job_status'] == cols[LetterJobsHeader.JOB_STATUS].text assert format_datetime_short( - valid_letter_jobs[row_pos]['created_at']) == cols[letter_jobs_header.CREATED_AT].text + valid_letter_jobs[row_pos]['created_at']) == cols[LetterJobsHeader.CREATED_AT].text if not (valid_letter_jobs[row_pos]['job_status'] == 'ready to send' or valid_letter_jobs[row_pos]['job_status'] == 'sent to dvla'): - assert 'disabled' in str(cols[letter_jobs_header.CHECKBOX]) + assert 'disabled' in str(cols[LetterJobsHeader.CHECKBOX]) def test_post_letter_jobs_select_1_letter_job_submits_1_job(logged_in_platform_admin_client, mocker): @@ -92,9 +92,9 @@ def test_post_letter_jobs_select_1_letter_job_submits_1_job(logged_in_platform_a colr1 = rows[1].find_all('td') colr2 = rows[2].find_all('td') - assert colr0[letter_jobs_header.TEMP_STATUS].text == "sending" - assert colr1[letter_jobs_header.TEMP_STATUS].text == "" - assert colr2[letter_jobs_header.TEMP_STATUS].text == "" + assert colr0[LetterJobsHeader.TEMP_STATUS].text == "sending" + assert colr1[LetterJobsHeader.TEMP_STATUS].text == "" + assert colr2[LetterJobsHeader.TEMP_STATUS].text == "" message = page.find('p', attrs={'id': 'message'}).text assert "Task created to send files to DVLA" in message From 151605180a45ebd2fd0eec07d4b880830b30b2ce Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 27 Oct 2017 11:44:40 +0100 Subject: [PATCH 2/5] Add email auth platform admin toggle also added tests to check visibility, url, and button content --- app/main/views/service_settings.py | 8 +++ app/templates/views/service-settings.html | 8 ++- .../test_service_setting_permissions.py | 66 +++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 tests/app/main/views/service_settings/test_service_setting_permissions.py diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 1dbe5ba0f..63eb21c31 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -302,6 +302,14 @@ def service_switch_can_send_sms(service_id): return redirect(url_for('.service_settings', service_id=service_id)) +@main.route("/services//service-settings/email-auth") +@login_required +@user_has_permissions(admin_override=True) +def service_switch_email_auth(service_id): + switch_service_permissions(service_id, 'email_auth') + return redirect(url_for('.service_settings', service_id=service_id)) + + @main.route("/services//service-settings/archive", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_settings', admin_override=True) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index a03640f06..6e7f63739 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -261,6 +261,11 @@ {% endif %} {% endif %} +
  • + + {{ 'Stop email auth' if 'email_auth' in current_service.permissions else 'Allow email auth' }} + +
    • {% if current_service.active %}
  • @@ -272,8 +277,7 @@ Suspend service
    • - {% endif %} - {% if not current_service.active %} + {% else %}
  • Resume service diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py new file mode 100644 index 000000000..b78c1f11d --- /dev/null +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -0,0 +1,66 @@ +import pytest +from flask import url_for + +from tests.conftest import client_request as client_request_factory + + +@pytest.fixture +def platform_admin_request(logged_in_platform_admin_client): + return client_request_factory(logged_in_platform_admin_client) + + +@pytest.mark.parametrize('service_fields, endpoint, kwargs, text', [ + ({'restricted': True}, '.service_switch_live', {}, 'Make service live'), + ({'restricted': False}, '.service_switch_live', {}, 'Revert service to trial mode'), + + ({'research_mode': True}, '.service_switch_research_mode', {}, 'Take service out of research mode'), + ({'research_mode': False}, '.service_switch_research_mode', {}, 'Put into research mode'), + + ({'permissions': ['email']}, '.service_switch_can_send_email', {}, 'Stop sending emails'), + ({'permissions': []}, '.service_switch_can_send_email', {}, 'Allow to send emails'), + + ({'permissions': ['letter']}, '.service_switch_can_send_letters', {}, 'Stop sending letters'), + ({'permissions': []}, '.service_switch_can_send_letters', {}, 'Allow to send letters'), + + ({'permissions': ['sms']}, '.service_switch_can_send_sms', {}, 'Stop sending sms'), + ({'permissions': []}, '.service_switch_can_send_sms', {}, 'Allow to send sms'), + + ({'permissions': ['sms', 'inbound_sms']}, '.service_set_inbound_number', {'set_inbound_sms': False}, 'Stop inbound sms'), # noqa + ({'permissions': ['sms']}, '.service_set_inbound_number', {'set_inbound_sms': True}, 'Allow inbound sms'), + + ({'active': True}, '.archive_service', {}, 'Archive service'), + ({'active': True}, '.suspend_service', {}, 'Suspend service'), + ({'active': False}, '.resume_service', {}, 'Resume service'), +]) +def test_service_setting_toggles_show(platform_admin_request, service_one, service_fields, endpoint, kwargs, text): + button_url = url_for(endpoint, **kwargs, service_id=service_one['id']) + service_one.update(service_fields) + page = platform_admin_request.get('main.service_settings', service_id=service_one['id']) + assert page.find('a', {'class': 'button', 'href': button_url}).text.strip() == text + + +@pytest.mark.parametrize('permissions', [ + ['inbound_sms'], [] +]) +def test_service_settings_doesnt_show_inbound_options_if_sms_disabled(platform_admin_request, service_one, permissions): + service_one['permissions'] = permissions + page = platform_admin_request.get('main.service_settings', service_id=service_one['id']) + toggles = page.find_all('a', {'class': 'button'}) + assert not any(button for button in toggles if 'inbound sms' in button.text) + + +@pytest.mark.parametrize('service_fields, hidden_button_text', [ + # if no sms permission, inbound sms shouldn't show + ({'permissions': ['inbound_sms']}, 'Stop inbound sms'), + ({'permissions': []}, 'Allow inbound sms'), + + # can't archive or suspend inactive service. Can't resume active service. + ({'active': False}, 'Archive service'), + ({'active': False}, 'Suspend service'), + ({'active': True}, 'Resume service'), +]) +def test_service_setting_toggles_dont_show(platform_admin_request, service_one, service_fields, hidden_button_text): + service_one.update(service_fields) + page = platform_admin_request.get('main.service_settings', service_id=service_one['id']) + toggles = page.find_all('a', {'class': 'button'}) + assert not any(button for button in toggles if hidden_button_text in button.text) From ba07fcff2d6558f17d777b70b881e2b293afe147 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 27 Oct 2017 11:59:50 +0100 Subject: [PATCH 3/5] move all mocks and things to the fixture to avoid duplication --- .../test_service_setting_permissions.py | 30 ++++++++++++++----- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py index b78c1f11d..1192699db 100644 --- a/tests/app/main/views/service_settings/test_service_setting_permissions.py +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -1,3 +1,5 @@ +import functools + import pytest from flask import url_for @@ -5,8 +7,16 @@ from tests.conftest import client_request as client_request_factory @pytest.fixture -def platform_admin_request(logged_in_platform_admin_client): - return client_request_factory(logged_in_platform_admin_client) +def get_service_settings_page( + logged_in_platform_admin_client, + service_one, + mock_get_inbound_number_for_service, + mock_get_letter_organisations, + no_reply_to_email_addresses, + no_letter_contact_blocks, +): + platform_admin_request = client_request_factory(logged_in_platform_admin_client) + return functools.partial(platform_admin_request.get, 'main.service_settings', service_id=service_one['id']) @pytest.mark.parametrize('service_fields, endpoint, kwargs, text', [ @@ -32,19 +42,23 @@ def platform_admin_request(logged_in_platform_admin_client): ({'active': True}, '.suspend_service', {}, 'Suspend service'), ({'active': False}, '.resume_service', {}, 'Resume service'), ]) -def test_service_setting_toggles_show(platform_admin_request, service_one, service_fields, endpoint, kwargs, text): +def test_service_setting_toggles_show(get_service_settings_page, service_one, service_fields, endpoint, kwargs, text): button_url = url_for(endpoint, **kwargs, service_id=service_one['id']) service_one.update(service_fields) - page = platform_admin_request.get('main.service_settings', service_id=service_one['id']) + page = get_service_settings_page() assert page.find('a', {'class': 'button', 'href': button_url}).text.strip() == text @pytest.mark.parametrize('permissions', [ ['inbound_sms'], [] ]) -def test_service_settings_doesnt_show_inbound_options_if_sms_disabled(platform_admin_request, service_one, permissions): +def test_service_settings_doesnt_show_inbound_options_if_sms_disabled( + get_service_settings_page, + service_one, + permissions +): service_one['permissions'] = permissions - page = platform_admin_request.get('main.service_settings', service_id=service_one['id']) + page = get_service_settings_page() toggles = page.find_all('a', {'class': 'button'}) assert not any(button for button in toggles if 'inbound sms' in button.text) @@ -59,8 +73,8 @@ def test_service_settings_doesnt_show_inbound_options_if_sms_disabled(platform_a ({'active': False}, 'Suspend service'), ({'active': True}, 'Resume service'), ]) -def test_service_setting_toggles_dont_show(platform_admin_request, service_one, service_fields, hidden_button_text): +def test_service_setting_toggles_dont_show(get_service_settings_page, service_one, service_fields, hidden_button_text): service_one.update(service_fields) - page = platform_admin_request.get('main.service_settings', service_id=service_one['id']) + page = get_service_settings_page() toggles = page.find_all('a', {'class': 'button'}) assert not any(button for button in toggles if hidden_button_text in button.text) From 69b766b3c9a4489711fe9d95c5c66b769cf78510 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 27 Oct 2017 13:10:12 +0100 Subject: [PATCH 4/5] test on python3.5 --- docker/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/Dockerfile b/docker/Dockerfile index 41768df4f..7123937e2 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -1,4 +1,4 @@ -FROM python:3.4-slim +FROM python:3.5-slim ARG HTTP_PROXY ARG HTTPS_PROXY From fcef77738c185895c9123bfefed7410cbe0f3956 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 27 Oct 2017 14:34:38 +0100 Subject: [PATCH 5/5] add test to make sure all buttons are plat admin only --- .../test_service_setting_permissions.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py index 1192699db..e5c60e3f4 100644 --- a/tests/app/main/views/service_settings/test_service_setting_permissions.py +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -78,3 +78,16 @@ def test_service_setting_toggles_dont_show(get_service_settings_page, service_on page = get_service_settings_page() toggles = page.find_all('a', {'class': 'button'}) assert not any(button for button in toggles if hidden_button_text in button.text) + + +def test_normal_user_doesnt_see_any_toggle_buttons( + client_request, + service_one, + mock_get_inbound_number_for_service, + mock_get_letter_organisations, + no_reply_to_email_addresses, + no_letter_contact_blocks, +): + page = client_request.get('main.service_settings', service_id=service_one['id']) + toggles = page.find('a', {'class': 'button'}) + assert toggles is None