From ceef77b2af976c92a497dcaeee7cb044d2abb4e0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 24 Jun 2016 10:15:20 +0100 Subject: [PATCH 1/3] =?UTF-8?q?Fix=20=E2=80=98help=E2=80=99=20appearing=20?= =?UTF-8?q?when=20it=20shouldn=E2=80=99t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Steps to reproduce: - make a template with a placeholder - click ‘send yourself a test’ - leave fields blank - click ‘check’ - see error, click ‘back’ Expected: previous page Actual: previous page with blue help sidebar When the URL contains `help=0`, `request.args.get('help')` returns '0'. Doing `if '0':` is the same as doing any `if :` which returns `True`. So we should only display the help when the help query parameter is: - not missing - AND a string that isn’t `'0'` --- app/main/views/send.py | 2 +- tests/app/main/views/test_send.py | 33 ++++++++++++++++++++++++++----- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 94c7dc978..4615491a9 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -243,7 +243,7 @@ def check_messages(service_id, template_type, upload_id): ) if request.args.get('from_test') and len(template.placeholders): - extra_args = {'help': 1} if request.args.get('help') else {} + extra_args = {'help': 1} if request.args.get('help', '0') != '0' else {} back_link = url_for( '.send_test', service_id=service_id, template_id=template.id, **extra_args ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 58fbe8efd..8c69ddc16 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -5,6 +5,7 @@ from io import BytesIO from os import path from glob import glob from bs4 import BeautifulSoup +from functools import partial from flask import url_for from tests import validate_route_permission from datetime import datetime @@ -630,6 +631,23 @@ def test_check_messages_back_link_with_help(app_, assert url_for('.send_test', service_id=fake_uuid, template_id=fake_uuid, help=1) in content +@pytest.mark.parametrize( + 'extra_args,expected_url', + [ + ( + dict(), + partial(url_for, '.send_test') + ), + ( + dict(help='0'), + partial(url_for, '.send_test') + ), + ( + dict(help='2'), + partial(url_for, '.send_test', help='1') + ) + ] +) def test_check_messages_back_link_without_help(app_, api_user_active, mock_login, @@ -640,7 +658,9 @@ def test_check_messages_back_link_without_help(app_, mock_has_permissions, mock_get_service_statistics_for_day, mock_s3_download, - fake_uuid): + fake_uuid, + extra_args, + expected_url): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -654,11 +674,14 @@ def test_check_messages_back_link_without_help(app_, service_id=fake_uuid, upload_id=fake_uuid, template_type='sms', - from_test=True) - ) + from_test=True, + **extra_args + )) assert response.status_code == 200 - content = response.get_data(as_text=True) - assert url_for('.send_test', service_id=fake_uuid, template_id=fake_uuid) in content + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert ( + page.findAll('a', {'class': 'page-footer-back-link'})[0]['href'] + ) == expected_url(service_id=fake_uuid, template_id=fake_uuid) def test_send_and_check_page_renders_if_no_statistics( From 4809a26ffe4a9a3e984be8bef0ebdcd26b31e760 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 24 Jun 2016 10:19:59 +0100 Subject: [PATCH 2/3] Remove redundant test This test is no replicated in the one below, so is no longer needed. --- tests/app/main/views/test_send.py | 32 ------------------------------- 1 file changed, 32 deletions(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 8c69ddc16..ea52f62e1 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -599,38 +599,6 @@ def test_route_choose_template_manage_api_keys_permissions(mocker, assert len(links) == 1 -def test_check_messages_back_link_with_help(app_, - api_user_active, - mock_login, - mock_get_user_by_email, - mock_get_users_by_service, - mock_get_service, - mock_get_service_template_with_placeholders, - mock_has_permissions, - mock_get_service_statistics_for_day, - mock_s3_download, - fake_uuid): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - with client.session_transaction() as session: - session['upload_data'] = {'original_file_name': 'valid.csv', - 'template_id': fake_uuid, - 'notification_count': 1, - 'valid': True} - response = client.get(url_for( - 'main.check_messages', - service_id=fake_uuid, - upload_id=fake_uuid, - template_type='sms', - from_test=True, - help=2) - ) - assert response.status_code == 200 - content = response.get_data(as_text=True) - assert url_for('.send_test', service_id=fake_uuid, template_id=fake_uuid, help=1) in content - - @pytest.mark.parametrize( 'extra_args,expected_url', [ From f0ae43db2ce0d6b55ede8569de372d8011d3876b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 24 Jun 2016 10:25:35 +0100 Subject: [PATCH 3/3] Remove gnarly indentation By combining the two `with` statements. No functional changes, split into a separate commit for easier reviewing. --- tests/app/main/views/test_send.py | 59 ++++++++++++++++--------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index ea52f62e1..753a205f5 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -616,35 +616,36 @@ def test_route_choose_template_manage_api_keys_permissions(mocker, ) ] ) -def test_check_messages_back_link_without_help(app_, - api_user_active, - mock_login, - mock_get_user_by_email, - mock_get_users_by_service, - mock_get_service, - mock_get_service_template_with_placeholders, - mock_has_permissions, - mock_get_service_statistics_for_day, - mock_s3_download, - fake_uuid, - extra_args, - expected_url): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - with client.session_transaction() as session: - session['upload_data'] = {'original_file_name': 'valid.csv', - 'template_id': fake_uuid, - 'notification_count': 1, - 'valid': True} - response = client.get(url_for( - 'main.check_messages', - service_id=fake_uuid, - upload_id=fake_uuid, - template_type='sms', - from_test=True, - **extra_args - )) +def test_check_messages_back_link( + app_, + api_user_active, + mock_login, + mock_get_user_by_email, + mock_get_users_by_service, + mock_get_service, + mock_get_service_template_with_placeholders, + mock_has_permissions, + mock_get_service_statistics_for_day, + mock_s3_download, + fake_uuid, + extra_args, + expected_url +): + with app_.test_request_context(), app_.test_client() as client: + client.login(api_user_active) + with client.session_transaction() as session: + session['upload_data'] = {'original_file_name': 'valid.csv', + 'template_id': fake_uuid, + 'notification_count': 1, + 'valid': True} + response = client.get(url_for( + 'main.check_messages', + service_id=fake_uuid, + upload_id=fake_uuid, + template_type='sms', + from_test=True, + **extra_args + )) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert (