From b885ce9cf4c2297a43a93c95353084f858add553 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 8 Nov 2016 15:37:25 +0000 Subject: [PATCH 1/5] clean up some usage of active in tests and remove it from service_api_client.create_service (created services are always active) --- app/main/views/add_service.py | 1 - tests/app/main/views/test_add_service.py | 2 -- tests/app/main/views/test_service_settings.py | 6 +----- tests/conftest.py | 16 +++++----------- 4 files changed, 6 insertions(+), 19 deletions(-) diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 3f8a5e064..2869788ce 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -41,7 +41,6 @@ def _add_invited_user_to_service(invited_user): def _create_service(service_name, email_from): service_id = service_api_client.create_service(service_name=service_name, - active=False, message_limit=current_app.config['DEFAULT_SERVICE_LIMIT'], restricted=True, user_id=session['user_id'], diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index a1639663b..537ccad58 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -40,7 +40,6 @@ def test_should_add_service_and_redirect_to_tour_when_no_services(app_, assert mock_get_services_with_no_services.called mock_create_service.assert_called_once_with( service_name='testing the post', - active=False, message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], restricted=True, user_id=api_user_active.id, @@ -73,7 +72,6 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service(app_ assert mock_get_services.called mock_create_service.assert_called_once_with( service_name='testing the post', - active=False, message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], restricted=True, user_id=api_user_active.id, diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index ae0484b28..4090ec366 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -545,11 +545,7 @@ def test_switch_service_from_research_mode_to_normal( with app_.test_request_context(): with app_.test_client() as client: service = service_json( - "1234", - "Test Service", - [active_user_with_permissions.id], - message_limit=1000, - active=False, + users=[active_user_with_permissions.id], restricted=True, research_mode=True ) diff --git a/tests/conftest.py b/tests/conftest.py index e9f7cc8f9..2d3d14120 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -67,9 +67,7 @@ def fake_uuid(): @pytest.fixture(scope='function') def mock_get_service(mocker, api_user_active): def _get(service_id): - service = service_json( - service_id, "Test Service", [api_user_active.id], message_limit=50, - active=False, restricted=True) + service = service_json(service_id, users=[api_user_active.id], message_limit=50) return {'data': service} return mocker.patch('app.service_api_client.get_service', side_effect=_get) @@ -125,10 +123,7 @@ def mock_get_live_service(mocker, api_user_active): def _get(service_id): service = service_json( service_id, - "Test Service", - [api_user_active.id], - message_limit=1000, - active=False, + users=[api_user_active.id], restricted=False) return {'data': service} @@ -137,10 +132,9 @@ def mock_get_live_service(mocker, api_user_active): @pytest.fixture(scope='function') def mock_create_service(mocker): - def _create(service_name, active, message_limit, restricted, user_id, email_from): + def _create(service_name, message_limit, restricted, user_id, email_from): service = service_json( - 101, service_name, [user_id], message_limit=message_limit, - active=active, restricted=restricted, email_from=email_from) + 101, service_name, [user_id], message_limit=message_limit, restricted=restricted, email_from=email_from) return service['id'] return mocker.patch( @@ -152,7 +146,7 @@ def mock_update_service(mocker): def _update(service_id, **kwargs): service = service_json( service_id, - **{key: kwargs.get(key) for key in [ + **{key: kwargs[key] for key in kwargs if key in [ 'name', 'users', 'message_limit', From 08881e5bd1fe0f28e88e90799603a45fc16e197c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 9 Nov 2016 14:33:35 +0000 Subject: [PATCH 2/5] add get_active_services method * all current invocations of get_services now call get_active_services EXCEPT for platform admin page (where we want to see inactive services * cleaned up parameter names and unpacking (since *params is unhelpful) * fixed incorrect kwarg name in conftest --- app/main/views/add_service.py | 2 +- app/main/views/choose_service.py | 4 ++-- app/main/views/platform_admin.py | 1 + app/main/views/sign_in.py | 2 +- app/main/views/two_factor.py | 2 +- app/notify_client/service_api_client.py | 11 +++++++++-- tests/conftest.py | 14 +++++++------- 7 files changed, 22 insertions(+), 14 deletions(-) diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 2869788ce..a9731c32b 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -68,7 +68,7 @@ def add_service(): service_name = form.name.data service_id = _create_service(service_name, email_from) - if (len(service_api_client.get_services({'user_id': session['user_id']}).get('data', [])) > 1): + if (len(service_api_client.get_active_services({'user_id': session['user_id']}).get('data', [])) > 1): return redirect(url_for('main.service_dashboard', service_id=service_id)) example_sms_template = service_api_client.create_service_template( diff --git a/app/main/views/choose_service.py b/app/main/views/choose_service.py index d5cd2e6b9..d1a58f68d 100644 --- a/app/main/views/choose_service.py +++ b/app/main/views/choose_service.py @@ -12,7 +12,7 @@ def choose_service(): return render_template( 'views/choose-service.html', services=[ServicesBrowsableItem(x) for x in - service_api_client.get_services({'user_id': current_user.id})['data']], + service_api_client.get_active_services({'user_id': current_user.id})['data']], can_add_service=is_gov_user(current_user.email_address) ) @@ -23,7 +23,7 @@ def show_all_services_or_dashboard(): if not current_user.is_authenticated: return redirect(url_for('.index')) - services = service_api_client.get_services({'user_id': current_user.id})['data'] + services = service_api_client.get_active_services({'user_id': current_user.id})['data'] if 1 == len(services): return redirect(url_for('.service_dashboard', service_id=services[0]['id'])) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 3d613f458..a5485b988 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -13,6 +13,7 @@ from app.statistics_utils import get_formatted_percentage @login_required @user_has_permissions(admin_override=True) def platform_admin(): + # specifically DO get inactive services services = service_api_client.get_services({'detailed': True})['data'] return render_template( 'views/platform-admin.html', diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index c93dfd847..1ad7a2521 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -55,7 +55,7 @@ def sign_in(): user.is_active: confirm_login() - services = service_api_client.get_services({'user_id': str(user.id)}).get('data', []) + services = service_api_client.get_active_services({'user_id': str(user.id)}).get('data', []) if (len(services) == 1): return redirect(url_for('main.service_dashboard', service_id=services[0]['id'])) else: diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index c4d609d64..770d2cf4b 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -26,7 +26,7 @@ def two_factor(): if form.validate_on_submit(): try: user = user_api_client.get_user(user_id) - services = service_api_client.get_services({'user_id': str(user_id)}).get('data', []) + services = service_api_client.get_active_services({'user_id': str(user_id)}).get('data', []) # Check if coming from new password page if 'password' in session['user_details']: user.set_password(session['user_details']['password']) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 9766d876c..af71b6469 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -57,11 +57,18 @@ class ServiceAPIClient(BaseAPIClient): '/service/{0}'.format(service_id), params=params) - def get_services(self, *params): + def get_services(self, params_dict=None): """ Retrieve a list of services. """ - return self.get('/service', *params) + return self.get('/service', params=params_dict) + + def get_active_services(self, params_dict=None): + """ + Retrieve a list of active services. + """ + params_dict['only_active'] = True + return self.get_services(params_dict) def update_service( self, diff --git a/tests/conftest.py b/tests/conftest.py index 2d3d14120..93a9c8813 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -187,7 +187,7 @@ def mock_get_services(mocker, fake_uuid, user=None): if user is None: user = active_user_with_permissions(fake_uuid) - def _create(user_id=None): + def _get_services(params_dict=None): service_one = service_json( SERVICE_ONE_ID, "service_one", [user.id], 1000, True, False) service_two = service_json( @@ -195,7 +195,7 @@ def mock_get_services(mocker, fake_uuid, user=None): return {'data': [service_one, service_two]} return mocker.patch( - 'app.service_api_client.get_services', side_effect=_create) + 'app.service_api_client.get_services', side_effect=_get_services) @pytest.fixture(scope='function') @@ -203,11 +203,11 @@ def mock_get_services_with_no_services(mocker, fake_uuid, user=None): if user is None: user = active_user_with_permissions(fake_uuid) - def _create(user_id=None): + def _get_services(params_dict=None): return {'data': []} return mocker.patch( - 'app.service_api_client.get_services', side_effect=_create) + 'app.service_api_client.get_services', side_effect=_get_services) @pytest.fixture(scope='function') @@ -215,13 +215,13 @@ def mock_get_services_with_one_service(mocker, fake_uuid, user=None): if user is None: user = api_user_active(fake_uuid) - def _create(user_id=None): + def _get_services(params_dict=None): return {'data': [service_json( SERVICE_ONE_ID, "service_one", [user.id], 1000, True, False )]} return mocker.patch( - 'app.service_api_client.get_services', side_effect=_create) + 'app.service_api_client.get_services', side_effect=_get_services) @pytest.fixture(scope='function') @@ -825,7 +825,7 @@ def mock_login(mocker, mock_get_user, mock_update_user, mock_events): def _verify_code(user_id, code, code_type): return True, '' - def _no_services(user_id=None): + def _no_services(params_dict=None): return {'data': []} return ( From 805639a2e148cb70cd92c3bbc0e78604cdd0eee5 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 16 Nov 2016 11:44:29 +0000 Subject: [PATCH 3/5] add test for choose services and related redirects to it it'll just show the add service button if you only have archived services --- app/notify_client/service_api_client.py | 4 +- tests/app/main/views/test_choose_services.py | 22 ++++-- tests/app/main/views/test_sign_in.py | 72 ++++++++++++-------- 3 files changed, 61 insertions(+), 37 deletions(-) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index af71b6469..379bbcb14 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -16,13 +16,13 @@ class ServiceAPIClient(BaseAPIClient): self.service_id = application.config['ADMIN_CLIENT_USER_NAME'] self.api_key = application.config['ADMIN_CLIENT_SECRET'] - def create_service(self, service_name, active, message_limit, restricted, user_id, email_from): + def create_service(self, service_name, message_limit, restricted, user_id, email_from): """ Create a service and return the json. """ data = { "name": service_name, - "active": active, + "active": True, "message_limit": message_limit, "user_id": user_id, "restricted": restricted, diff --git a/tests/app/main/views/test_choose_services.py b/tests/app/main/views/test_choose_services.py index d4399490a..2ca0dbb28 100644 --- a/tests/app/main/views/test_choose_services.py +++ b/tests/app/main/views/test_choose_services.py @@ -1,4 +1,4 @@ -from flask import url_for, session +from flask import url_for def test_should_show_choose_services_page(app_, @@ -18,13 +18,26 @@ def test_should_show_choose_services_page(app_, assert mock_get_services.called assert services['data'][0]['name'] in resp_data assert services['data'][1]['name'] in resp_data - assert 'List all services' not in resp_data + + +def test_should_show_choose_services_page_if_no_services( + client, + mock_login, + api_user_active, +): + # if users last service has been archived there'll be no services + # mock_login already patches get_services to return no data + client.login(api_user_active) + response = client.get(url_for('main.choose_service')) + assert response.status_code == 200 + resp_data = response.get_data(as_text=True) + assert 'Choose service' in resp_data + assert 'Add a new service' 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 ): @@ -41,9 +54,7 @@ def test_redirect_if_only_one_service( 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: @@ -57,7 +68,6 @@ def test_redirect_if_multiple_services( def test_redirect_if_service_in_session( app_, mock_login, - mock_get_user, api_user_active, mock_get_services, mock_get_service diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index f57eac9ee..418098eed 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -12,15 +12,9 @@ def test_render_sign_in_returns_sign_in_template(app_): assert 'Forgot your password?' in response.get_data(as_text=True) -def test_logged_in_user_redirects_to_choose_service(app_, - api_user_active, - mock_get_user): - - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - response = client.get(url_for('main.sign_in')) - assert response.location == url_for('main.choose_service', _external=True) +def test_logged_in_user_redirects_to_choose_service(logged_in_client): + response = logged_in_client.get(url_for('main.sign_in')) + assert response.location == url_for('main.choose_service', _external=True) def test_process_sign_in_return_2fa_template(app_, @@ -88,24 +82,44 @@ def test_should_attempt_redirect_when_user_is_pending(app_, assert response.status_code == 302 -def test_not_fresh_session_login(app_, - api_user_active, - mock_login, - mock_get_user_by_email, - mock_verify_password, - mock_get_services_with_one_service): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - with client.session_transaction() as session: - assert session['_fresh'] - session['_fresh'] = False - # This should skip the two factor - response = client.post( - url_for('main.sign_in'), data={ - 'email_address': api_user_active.email_address, - 'password': 'val1dPassw0rd!'}) +def test_not_fresh_session_login_redirects_to_dashboard( + client, + api_user_active, + mock_login, + mock_get_user_by_email, + mock_verify_password, + mock_get_services_with_one_service +): + client.login(api_user_active) + with client.session_transaction() as session: + assert session['_fresh'] + session['_fresh'] = False + # This should skip the two factor + response = client.post( + url_for('main.sign_in'), data={ + 'email_address': api_user_active.email_address, + 'password': 'val1dPassw0rd!'}) + assert response.status_code == 302 + service_dct = mock_get_services_with_one_service(api_user_active.id)['data'][0] + assert response.location == url_for( + 'main.service_dashboard', service_id=service_dct['id'], _external=True) + + def test_not_fresh_session_login_redirects_to_choose_service( + client, + api_user_active, + mock_login, + mock_get_user_by_email, + mock_verify_password, + mock_get_services + ): + client.login(api_user_active) + with client.session_transaction() as session: + assert session['_fresh'] + session['_fresh'] = False + # This should skip the two factor + response = client.post( + url_for('main.sign_in'), data={ + 'email_address': api_user_active.email_address, + 'password': 'val1dPassw0rd!'}) assert response.status_code == 302 - service_dct = mock_get_services_with_one_service(api_user_active.id)['data'][0] - assert response.location == url_for( - 'main.service_dashboard', service_id=service_dct['id'], _external=True) + assert response.location == url_for('main.choose_service', _external=True) From a2b8e91ae7e7f5d7c652b539e166fdebb8173fb0 Mon Sep 17 00:00:00 2001 From: bandesz Date: Wed, 23 Nov 2016 12:40:39 +0000 Subject: [PATCH 4/5] Exclude cache directory from pep8 --- setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index af94d5aee..95eee0b08 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,6 +1,6 @@ [pep8] max-line-length = 120 -exclude = ./migrations,./venv,./venv3,./node_modules,./bower_components +exclude = ./migrations,./venv,./venv3,./node_modules,./bower_components,./cache [tool:pytest] norecursedirs = node_modules bower_components From 2734f273692a9e1fd4948a9c555d0d426e33f607 Mon Sep 17 00:00:00 2001 From: bandesz Date: Wed, 23 Nov 2016 12:41:07 +0000 Subject: [PATCH 5/5] Add python-dev, libffi-dev, libssl-dev to the Docker build image --- docker/Dockerfile-build | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docker/Dockerfile-build b/docker/Dockerfile-build index 49a36e251..2ce5a28bd 100644 --- a/docker/Dockerfile-build +++ b/docker/Dockerfile-build @@ -18,6 +18,9 @@ RUN \ zlib1g-dev \ zip \ rlwrap \ + python-dev \ + libffi-dev \ + libssl-dev \ && echo "Install nodejs" \ && cd /tmp \