From f921a2dcb685c51995e5543bac5f021f9c55ecc3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 24 Mar 2016 13:48:11 +0000 Subject: [PATCH 1/7] Strip trailing CSV rows, relax phone validation Implements (and depends on): - https://github.com/alphagov/notifications-utils/pull/14 - https://github.com/alphagov/notifications-utils/pull/15 --- requirements.txt | 2 +- tests/app/main/views/test_send.py | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/requirements.txt b/requirements.txt index b1c315f8d..d37ae37df 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,4 +14,4 @@ Pygments==2.0.2 git+https://github.com/alphagov/notifications-python-client.git@0.3.1#egg=notifications-python-client==0.3.1 -git+https://github.com/alphagov/notifications-utils.git@3.1.1#egg=notifications-utils==3.1.1 +git+https://github.com/alphagov/notifications-utils.git@3.1.3#egg=notifications-utils==3.1.3 diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index f45ed0c17..4dd42030b 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -247,11 +247,8 @@ def test_check_messages_should_revalidate_file_when_uploading_file( 'app.main.views.send.s3download', return_value=""" phone number,name,,, - ++44 7700 900981,test1,,, - +44 7700 900981,test2,,, - ,,, - ,,, \t \t - + 123,test1,,, + 123,test2,,, """ ) with app_.test_request_context(): From eb279c88d44238d19bb0fe940a77388b68b1fec0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 29 Mar 2016 10:39:01 +0100 Subject: [PATCH 2/7] =?UTF-8?q?Only=20show=20=E2=80=98Choose=20service?= =?UTF-8?q?=E2=80=99=20if=20multiple=20services?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the user clicks ‘GOV.UK Notify’ in the header, they should, by default, be redirected to the dashboard for their service. They should only see the ‘Choose service’ page if they have multiple services. This also allows some logic to be factored out of the template, so one route now handles all this redirection. In the future we might want to keep the last-used service in the session, and always redirect to that. But for now, this should fix most of the confusion for first-time users. --- app/main/views/choose_service.py | 22 +++++++++-- app/templates/admin_template.html | 7 +--- tests/app/main/views/test_choose_services.py | 41 ++++++++++++++++++++ 3 files changed, 61 insertions(+), 9 deletions(-) diff --git a/app/main/views/choose_service.py b/app/main/views/choose_service.py index 39b0b9f8e..5ddaf61b0 100644 --- a/app/main/views/choose_service.py +++ b/app/main/views/choose_service.py @@ -1,13 +1,29 @@ from flask import (render_template, redirect, url_for, session) from flask_login import login_required, current_user -from app.main.dao import services_dao +from app.main.dao.services_dao import ServicesBrowsableItem +from app import service_api_client from app.main import main @main.route("/services") @login_required def choose_service(): - services = services_dao.get_services(current_user.id) return render_template( 'views/choose-service.html', - services=[services_dao.ServicesBrowsableItem(x) for x in services['data']]) + services=[ServicesBrowsableItem(x) for x in service_api_client.get_services()['data']] + ) + + +@main.route("/services-or-dashboard") +def show_all_services_or_dashboard(): + + if current_user.is_authenticated(): + + services = service_api_client.get_services()['data'] + + if 1 == len(services): + return redirect(url_for('.service_dashboard', service_id=services[0]['id'])) + else: + return redirect(url_for('.choose_service')) + + return redirect(url_for('main.index')) diff --git a/app/templates/admin_template.html b/app/templates/admin_template.html index 573b3f1b2..a371476db 100644 --- a/app/templates/admin_template.html +++ b/app/templates/admin_template.html @@ -59,12 +59,7 @@ {% set global_header_text = "GOV.UK Notify" %} - -{% if not current_user.is_authenticated() %} - {% set homepage_url = url_for('main.index') %} -{% else %} - {% set homepage_url = url_for('main.choose_service') %} -{% endif %} +{% set homepage_url = url_for('main.show_all_services_or_dashboard') %} {% block content %}
diff --git a/tests/app/main/views/test_choose_services.py b/tests/app/main/views/test_choose_services.py index 02b0845a5..7f99c2e26 100644 --- a/tests/app/main/views/test_choose_services.py +++ b/tests/app/main/views/test_choose_services.py @@ -23,6 +23,47 @@ def test_should_show_choose_services_page(app_, assert 'List all services' not in resp_data +def test_redirect_if_only_one_service( + app_, + mock_login, + mock_get_user, + api_user_active, + mock_get_services_with_one_service +): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + response = client.get(url_for('main.show_all_services_or_dashboard')) + + service = mock_get_services_with_one_service.side_effect()['data'][0] + assert response.status_code == 302 + assert response.location == url_for('main.service_dashboard', service_id=service['id'], _external=True) + + +def test_redirect_if_multiple_services( + app_, + mock_login, + mock_get_user, + api_user_active, + mock_get_services +): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + response = client.get(url_for('main.show_all_services_or_dashboard')) + + assert response.status_code == 302 + assert response.location == url_for('main.choose_service', _external=True) + + +def test_should_redirect_if_not_logged_in(app_): + with app_.test_request_context(): + with app_.test_client() as client: + response = client.get(url_for('main.show_all_services_or_dashboard')) + assert response.status_code == 302 + assert response.location == url_for('main.index', _external=True) + + def test_should_show_all_services_for_platform_admin_user(app_, platform_admin_user, mock_get_services, From 352f169fb1400cdf8e8986e0ea84469c7d4faefd Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 29 Mar 2016 12:13:36 +0100 Subject: [PATCH 3/7] If user is pending it means they have not verified email yet Added better checking on re use of consumed verification link. --- app/main/views/sign_in.py | 7 ++--- app/main/views/verify.py | 14 +++++++--- tests/app/main/views/test_sign_in.py | 14 +++++----- tests/app/main/views/test_verify.py | 40 +++++++++++++++++++++------- 4 files changed, 52 insertions(+), 23 deletions(-) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index f678afc40..c9aa103a8 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -31,6 +31,9 @@ def sign_in(): if form.validate_on_submit(): user = user_api_client.get_user_by_email_or_none(form.email_address.data) user = _get_and_verify_user(user, form.password.data) + if user and user.state == 'pending': + flash("You haven't verified your email or mobile number yet.") + return redirect(url_for('main.sign_in')) if user: # Remember me login if not login_fresh() and \ @@ -45,9 +48,7 @@ def sign_in(): return redirect(url_for('main.choose_service')) session['user_details'] = {"email": user.email_address, "id": user.id} - if user.state == 'pending': - return redirect(url_for('.verify')) - elif user.is_active(): + if user.is_active(): user_api_client.send_verify_code(user.id, 'sms', user.mobile_number) if request.args.get('next'): return redirect(url_for('.two_factor', next=request.args.get('next'))) diff --git a/app/main/views/verify.py b/app/main/views/verify.py index 9c701fdc0..5bee7b0f3 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -6,7 +6,8 @@ from flask import ( session, url_for, current_app, - flash + flash, + abort ) from itsdangerous import SignatureExpired @@ -55,10 +56,17 @@ def verify_email(token): token_data = json.loads(token_data) verified = user_api_client.check_verify_code(token_data['user_id'], token_data['secret_code'], 'email') + user = user_api_client.get_user(token_data['user_id']) + if not user: + abort(404) + + if user.is_active(): + flash("You have already verified your email address.") + return redirect(url_for('main.sign_in')) + + session['user_details'] = {"email": user.email_address, "id": user.id} if verified[0]: - user = user_api_client.get_user(token_data['user_id']) user_api_client.send_verify_code(user.id, 'sms', user.mobile_number) - session['user_details'] = {"email": user.email_address, "id": user.id} return redirect('verify') else: if verified[1] == 'Code has expired': diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index c3cc669d1..6d2a150c4 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -1,9 +1,5 @@ - -from datetime import datetime - -from app.main.dao import users_dao - from flask import url_for +from bs4 import BeautifulSoup def test_render_sign_in_returns_sign_in_template(app_): @@ -75,9 +71,11 @@ def test_should_return_redirect_when_user_is_pending(app_, response = app_.test_client().post( url_for('main.sign_in'), data={ 'email_address': 'pending_user@example.gov.uk', - 'password': 'val1dPassw0rd!'}) - assert response.status_code == 302 - assert response.location == url_for('main.verify', _external=True) + 'password': 'val1dPassw0rd!'}, follow_redirects=True) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string == 'Sign in' + flash_banner = page.find('div', class_='banner-dangerous').string.strip() + assert flash_banner == "You haven't verified your email or mobile number yet." def test_not_fresh_session_login(app_, diff --git a/tests/app/main/views/test_verify.py b/tests/app/main/views/test_verify.py index 4f756ced2..0fe35c533 100644 --- a/tests/app/main/views/test_verify.py +++ b/tests/app/main/views/test_verify.py @@ -70,18 +70,18 @@ def test_should_return_200_when_sms_code_is_wrong(app_, def test_verify_email_redirects_to_verify_if_token_valid(app_, mocker, - api_user_active, - mock_get_user, + api_user_pending, + mock_get_user_pending, mock_send_verify_code, mock_check_verify_code): import json - token_data = {"user_id": api_user_active.id, "secret_code": 12345} + token_data = {"user_id": api_user_pending.id, "secret_code": 12345} mocker.patch('utils.url_safe_token.check_token', return_value=json.dumps(token_data)) with app_.test_request_context(): with app_.test_client() as client: with client.session_transaction() as session: - session['user_details'] = {'email_address': api_user_active.email_address, 'id': api_user_active.id} + session['user_details'] = {'email_address': api_user_pending.email_address, 'id': api_user_pending.id} response = client.get(url_for('main.verify_email', token='notreal')) @@ -91,7 +91,7 @@ def test_verify_email_redirects_to_verify_if_token_valid(app_, def test_verify_email_redirects_to_email_sent_if_token_expired(app_, mocker, - api_user_active, + api_user_pending, mock_check_verify_code): from itsdangerous import SignatureExpired mocker.patch('utils.url_safe_token.check_token', side_effect=SignatureExpired('expired')) @@ -99,7 +99,7 @@ def test_verify_email_redirects_to_email_sent_if_token_expired(app_, with app_.test_request_context(): with app_.test_client() as client: with client.session_transaction() as session: - session['user_details'] = {'email_address': api_user_active.email_address, 'id': api_user_active.id} + session['user_details'] = {'email_address': api_user_pending.email_address, 'id': api_user_pending.id} response = client.get(url_for('main.verify_email', token='notreal')) @@ -109,8 +109,8 @@ def test_verify_email_redirects_to_email_sent_if_token_expired(app_, def test_verify_email_redirects_to_email_sent_if_token_used(app_, mocker, - api_user_active, - mock_get_user, + api_user_pending, + mock_get_user_pending, mock_send_verify_code, mock_check_verify_code_code_expired): from itsdangerous import SignatureExpired @@ -119,9 +119,31 @@ def test_verify_email_redirects_to_email_sent_if_token_used(app_, with app_.test_request_context(): with app_.test_client() as client: with client.session_transaction() as session: - session['user_details'] = {'email_address': api_user_active.email_address, 'id': api_user_active.id} + session['user_details'] = {'email_address': api_user_pending.email_address, 'id': api_user_pending.id} response = client.get(url_for('main.verify_email', token='notreal')) assert response.status_code == 302 assert response.location == url_for('main.resend_email_verification', _external=True) + + +def test_verify_email_redirects_to_sign_in_if_user_active(app_, + mocker, + api_user_active, + mock_get_user, + mock_send_verify_code, + mock_check_verify_code): + import json + token_data = {"user_id": api_user_active.id, "secret_code": 12345} + mocker.patch('utils.url_safe_token.check_token', return_value=json.dumps(token_data)) + + with app_.test_request_context(): + with app_.test_client() as client: + with client.session_transaction() as session: + session['user_details'] = {'email_address': api_user_active.email_address, 'id': api_user_active.id} + + response = client.get(url_for('main.verify_email', token='notreal'), follow_redirects=True) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.text == 'Sign in' + flash_banner = page.find('div', class_='banner-dangerous').string.strip() + assert flash_banner == "You have already verified your email address." From db24a633c121246af99af97ee2506ce6b8fa04b0 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 29 Mar 2016 13:21:51 +0100 Subject: [PATCH 4/7] Better flash message for users with active accounts who click on verification link again. --- app/main/views/verify.py | 2 +- tests/app/main/views/test_verify.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/views/verify.py b/app/main/views/verify.py index 5bee7b0f3..b076bb5c6 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -61,7 +61,7 @@ def verify_email(token): abort(404) if user.is_active(): - flash("You have already verified your email address.") + flash("That verification link has expired.") return redirect(url_for('main.sign_in')) session['user_details'] = {"email": user.email_address, "id": user.id} diff --git a/tests/app/main/views/test_verify.py b/tests/app/main/views/test_verify.py index 0fe35c533..58b4f6301 100644 --- a/tests/app/main/views/test_verify.py +++ b/tests/app/main/views/test_verify.py @@ -146,4 +146,4 @@ def test_verify_email_redirects_to_sign_in_if_user_active(app_, page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.text == 'Sign in' flash_banner = page.find('div', class_='banner-dangerous').string.strip() - assert flash_banner == "You have already verified your email address." + assert flash_banner == "That verification link has expired." From f3fd5f6b15b9841111819d9526cf80b20b6ababd Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 29 Mar 2016 15:59:53 +0100 Subject: [PATCH 5/7] After sending sms or email to self or batch then then back button does not take user to check page, but rather the start of sending either sms or email. --- app/main/views/send.py | 29 ++++++++++++++++++----------- app/templates/views/check.html | 2 +- tests/app/main/views/test_send.py | 27 +-------------------------- 3 files changed, 20 insertions(+), 38 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 9f252292d..0f57271ee 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -98,6 +98,12 @@ def choose_template(service_id, template_type): @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_messages(service_id, template_id): + service = services_dao.get_service_by_id_or_404(service_id) + template = Template( + templates_dao.get_service_template_or_404(service_id, template_id)['data'], + prefix=service['name'] + ) + form = CsvUploadForm() if form.validate_on_submit(): try: @@ -117,18 +123,13 @@ def send_messages(service_id, template_id): } return redirect(url_for('.check_messages', service_id=service_id, - upload_id=upload_id)) + upload_id=upload_id, + template_type=template.template_type)) except ValueError as e: flash('There was a problem uploading: {}'.format(form.file.data.filename)) flash(str(e)) return redirect(url_for('.send_messages', service_id=service_id, template_id=template_id)) - service = services_dao.get_service_by_id_or_404(service_id) - template = Template( - templates_dao.get_service_template_or_404(service_id, template_id)['data'], - prefix=service['name'] - ) - return render_template( 'views/send.html', template=template, @@ -184,12 +185,14 @@ def send_message_to_self(service_id, template_id): 'data': output.getvalue() } upload_id = str(uuid.uuid4()) + s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION']) session['upload_data'] = {"template_id": template_id, "original_file_name": filedata['file_name']} return redirect(url_for('.check_messages', service_id=service_id, - upload_id=upload_id)) + upload_id=upload_id, + template_type=template.template_type)) @main.route("/services//send//from-api", methods=['GET']) @@ -214,10 +217,13 @@ def send_from_api(service_id, template_id): ) -@main.route("/services//check/", methods=['GET']) +@main.route("/services///check/", methods=['GET']) @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') -def check_messages(service_id, upload_id): +def check_messages(service_id, template_type, upload_id): + + if not session.get('upload_data'): + return redirect(url_for('main.choose_template', service_id=service_id, template_type=template_type)) service = services_dao.get_service_by_id_or_404(service_id) @@ -266,11 +272,12 @@ def check_messages(service_id, upload_id): send_button_text=get_send_button_text(template.template_type, session['upload_data']['notification_count']), service_id=service_id, service=service, + upload_id=upload_id, form=CsvUploadForm() ) -@main.route("/services//check/", methods=['POST']) +@main.route("/services//start-job/", methods=['POST']) @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') def start_job(service_id, upload_id): diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 018f0f491..0bd132ea9 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -64,7 +64,7 @@ {% if errors %} {{file_upload(form.file, button_text='Re-upload your file')}} {% else %} -
+ Back diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 4dd42030b..5fe295062 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -40,31 +40,6 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( assert 'Re-upload your file' in content -def test_send_test_message_to_self( - app_, - mocker, - api_user_active, - mock_login, - mock_get_service, - mock_get_service_template, - mock_s3_upload, - mock_has_permissions -): - - expected_data = {'data': ['phone number', '+4412341234'], 'file_name': 'Test run'} - mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') - - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - response = client.get( - url_for('main.send_message_to_self', service_id=12345, template_id=54321), - follow_redirects=True - ) - assert response.status_code == 200 - mock_s3_upload.assert_called_with(ANY, '12345', expected_data, 'eu-west-1') - - def test_send_test_message_to_self( app_, mocker, @@ -260,7 +235,7 @@ def test_check_messages_should_revalidate_file_when_uploading_file( 'notification_count': job_data['notification_count'], 'valid': True} response = client.post( - url_for('main.check_messages', service_id=service_id, upload_id=job_data['id']), + url_for('main.start_job', service_id=service_id, upload_id=job_data['id']), data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')}, content_type='multipart/form-data', follow_redirects=True From 0bbe35cd7dcba493ef962d28ff324768a43452d4 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 29 Mar 2016 16:52:33 +0100 Subject: [PATCH 6/7] Reintroduced send sms to self test --- tests/app/main/views/test_send.py | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 5fe295062..8646cf3be 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -40,7 +40,32 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( assert 'Re-upload your file' in content -def test_send_test_message_to_self( +def test_send_test_sms_message_to_self( + app_, + mocker, + api_user_active, + mock_login, + mock_get_service, + mock_get_service_template, + mock_s3_upload, + mock_has_permissions +): + + expected_data = {'data': 'phone number\r\n+4412341234\r\n', 'file_name': 'Test run'} + mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') + + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + response = client.get( + url_for('main.send_message_to_self', service_id=12345, template_id=54321), + follow_redirects=True + ) + assert response.status_code == 200 + mock_s3_upload.assert_called_with(ANY, '12345', expected_data, 'eu-west-1') + + +def test_send_test_email_message_to_self( app_, mocker, api_user_active, From 08d4e1318593a8f3023af4a326eee2dceb02a4ea Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 29 Mar 2016 16:59:06 +0100 Subject: [PATCH 7/7] Updated to use client instead of dao --- app/main/views/send.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 0f57271ee..f766f7394 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -26,8 +26,10 @@ from app.main.uploader import ( s3download ) from app.main.dao import templates_dao -from app.main.dao import services_dao -from app import job_api_client +from app import ( + job_api_client, + service_api_client +) from app.utils import user_has_permissions, get_errors_for_csv @@ -70,7 +72,7 @@ def get_page_headings(template_type): admin_override=True, or_=True) def choose_template(service_id, template_type): - service = services_dao.get_service_by_id_or_404(service_id) + service = service_api_client.get_service(service_id)['data'] if template_type not in ['email', 'sms']: abort(404) @@ -98,7 +100,7 @@ def choose_template(service_id, template_type): @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_messages(service_id, template_id): - service = services_dao.get_service_by_id_or_404(service_id) + service = service_api_client.get_service(service_id)['data'] template = Template( templates_dao.get_service_template_or_404(service_id, template_id)['data'], prefix=service['name'] @@ -225,7 +227,7 @@ def check_messages(service_id, template_type, upload_id): if not session.get('upload_data'): return redirect(url_for('main.choose_template', service_id=service_id, template_type=template_type)) - service = services_dao.get_service_by_id_or_404(service_id) + service = service_api_client.get_service(service_id)['data'] contents = s3download(service_id, upload_id) if not contents: @@ -283,7 +285,7 @@ def check_messages(service_id, template_type, upload_id): def start_job(service_id, upload_id): upload_data = session['upload_data'] - services_dao.get_service_by_id_or_404(service_id) + service = service_api_client.get_service(service_id)['data'] if request.files or not upload_data.get('valid'): # The csv was invalid, validate the csv again