From 8ca783254169950165b3ae119efa81d555fe2e87 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sun, 19 Jun 2016 09:04:21 +0100 Subject: [PATCH 1/3] Filter test messages from jobs on the dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While test messages technically have a file and are a job, there’s not much reason to ever revisit them. So all they end up doing is cluttering the dashboard and making it harder to find the actual files you’ve actually uploaded from your computer. So this commit: - abstracts the name of test messages into config - filters out any files whose filename represents a test message - adds some more thorough tests for the jobs on the dashboard --- app/main/views/dashboard.py | 8 +++-- app/main/views/send.py | 2 +- config.py | 1 + tests/app/main/views/test_dashboard.py | 44 ++++++++++++++++++++++++++ tests/conftest.py | 10 ++++-- 5 files changed, 60 insertions(+), 5 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index ea4130e42..5e5cafc8c 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -7,7 +7,8 @@ from flask import ( redirect, url_for, session, - jsonify + jsonify, + current_app ) from flask_login import login_required @@ -169,5 +170,8 @@ def get_dashboard_statistics_for_service(service_id): 'sms_allowance_remaining': max(0, (sms_free_allowance - sms_sent)), 'sms_chargeable': max(0, sms_sent - sms_free_allowance), 'sms_rate': sms_rate, - 'jobs': add_rate_to_jobs(job_api_client.get_job(service_id, limit_days=7)['data']) + 'jobs': add_rate_to_jobs(filter( + lambda job: job['original_file_name'] != current_app.config['TEST_MESSAGE_FILENAME'], + job_api_client.get_job(service_id, limit_days=7)['data'] + )) } diff --git a/app/main/views/send.py b/app/main/views/send.py index 0070ec048..77c9f550a 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -144,7 +144,7 @@ def get_example_csv(service_id, template_id): @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_test(service_id, template_id): - file_name = 'Test message' + file_name = current_app.config['TEST_MESSAGE_FILENAME'] template = Template( service_api_client.get_service_template(service_id, template_id)['data'], diff --git a/config.py b/config.py index 9f83d1912..8b2e125ec 100644 --- a/config.py +++ b/config.py @@ -40,6 +40,7 @@ class Config(object): DESKPRO_DEPT_ID = os.environ['DESKPRO_DEPT_ID'] DESKPRO_ASSIGNED_AGENT_TEAM_ID = os.environ['DESKPRO_ASSIGNED_AGENT_TEAM_ID'] ACTIVITY_STATS_LIMIT_DAYS = 7 + TEST_MESSAGE_FILENAME = 'Test message' EMAIL_DOMAIN_REGEXES = [ "gov\.uk", diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index bec5783d3..ca607cc50 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -2,6 +2,7 @@ import copy from flask import url_for from bs4 import BeautifulSoup +from freezegun import freeze_time from tests import validate_route_permission from tests.conftest import SERVICE_ONE_ID @@ -140,6 +141,49 @@ def test_should_show_all_templates_on_template_statistics_page( assert '13' 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_service_statistics, + mock_get_aggregate_service_statistics, + mock_get_user, + mock_get_user_by_email, + mock_login, + mock_get_template_statistics, + 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)) + + mock_get_jobs.assert_called_once_with(SERVICE_ONE_ID, limit_days=7) + assert response.status_code == 200 + + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + table_rows = page.find_all('tbody')[1].find_all('tr') + + assert "Test message" not in page.text + assert len(table_rows) == 4 + + for index, filename in enumerate(( + "export 1/1/2016.xls", + "all email addresses.xlsx", + "applicants.ods", + "thisisatest.csv", + )): + for string in (filename, 'Uploaded 1 January at 11:09'): + assert string in table_rows[index].find_all('th')[0].text + for column_index, count in enumerate((1, 0, 0)): + assert table_rows[index].find_all('td')[column_index].text.strip() == str(count) + + def _test_dashboard_menu(mocker, app_, usr, service, permissions): with app_.test_request_context(): with app_.test_client() as client: diff --git a/tests/conftest.py b/tests/conftest.py index 845f6ee82..9540ea86c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -805,8 +805,14 @@ def mock_get_job(mocker, api_user_active): def mock_get_jobs(mocker, api_user_active): def _get_jobs(service_id, limit_days=None): data = [] - for i in range(5): - job_data = job_json(service_id, api_user_active) + for filename in [ + "Test message", + "export 1/1/2016.xls", + "all email addresses.xlsx", + "applicants.ods", + "thisisatest.csv", + ]: + job_data = job_json(service_id, api_user_active, original_file_name=filename) data.append(job_data) return {"data": data} From 741607019958a559094b8e5196a3c6beceab9d95 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sun, 19 Jun 2016 09:18:19 +0100 Subject: [PATCH 2/3] Refactor job conftests to eliminate temp variables These tests were assigning something to a variable, then immediately returning that variable. Why not just return the thing itself? --- tests/conftest.py | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 9540ea86c..ddee3ae45 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -779,7 +779,7 @@ def mock_check_verify_code_code_expired(mocker): @pytest.fixture(scope='function') def mock_create_job(mocker, api_user_active): def _create(job_id, service_id, template_id, file_name, notification_count): - job_data = job_json( + return job_json( service_id, api_user_active, job_id=job_id, @@ -787,7 +787,6 @@ def mock_create_job(mocker, api_user_active): bucket_name='service-{}-notify'.format(job_id), original_file_name='{}.csv'.format(job_id), notification_count=notification_count) - return job_data return mocker.patch('app.job_api_client.create_job', side_effect=_create) @@ -795,8 +794,7 @@ def mock_create_job(mocker, api_user_active): @pytest.fixture(scope='function') def mock_get_job(mocker, api_user_active): def _get_job(service_id, job_id): - job_data = job_json(service_id, api_user_active, job_id=job_id) - return {"data": job_data} + return {"data": job_json(service_id, api_user_active, job_id=job_id)} return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) @@ -804,17 +802,16 @@ def mock_get_job(mocker, api_user_active): @pytest.fixture(scope='function') def mock_get_jobs(mocker, api_user_active): def _get_jobs(service_id, limit_days=None): - data = [] - for filename in [ - "Test message", - "export 1/1/2016.xls", - "all email addresses.xlsx", - "applicants.ods", - "thisisatest.csv", - ]: - job_data = job_json(service_id, api_user_active, original_file_name=filename) - data.append(job_data) - return {"data": data} + return {"data": [ + job_json(service_id, api_user_active, original_file_name=filename) + for filename in ( + "Test message", + "export 1/1/2016.xls", + "all email addresses.xlsx", + "applicants.ods", + "thisisatest.csv", + ) + ]} return mocker.patch('app.job_api_client.get_job', side_effect=_get_jobs) From b7e58b0a5b33a1ca44d7de95f36ad508ca1f7df4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Jun 2016 10:07:37 +0100 Subject: [PATCH 3/3] Unroll unneccessary for loop --- tests/app/main/views/test_dashboard.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index ca607cc50..3ccb58fd8 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -178,8 +178,8 @@ def test_should_show_recent_jobs_on_dashboard( "applicants.ods", "thisisatest.csv", )): - for string in (filename, 'Uploaded 1 January at 11:09'): - assert string in table_rows[index].find_all('th')[0].text + 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 for column_index, count in enumerate((1, 0, 0)): assert table_rows[index].find_all('td')[column_index].text.strip() == str(count)