From e3b1813ed7e9f5753c8c1df40a553d99e1ebfa9f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Jan 2020 14:14:38 +0000 Subject: [PATCH 01/36] Add a test for the no notifications message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This wasn’t tested before. --- tests/app/main/views/test_jobs.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 2b3e8f707..477794706 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -294,7 +294,27 @@ def test_should_show_job_in_progress( service_id=service_one['id'], job_id=fake_uuid, ) - assert page.find('p', {'class': 'hint'}).text.strip() == 'Report is 50% complete…' + assert page.select_one('p.hint').text.strip() == 'Report is 50% complete…' + + +def test_should_show_job_without_notifications( + client_request, + service_one, + active_user_with_permissions, + mock_get_service_template, + mock_get_job_in_progress, + mocker, + mock_get_notifications_with_no_notifications, + mock_get_service_data_retention, + fake_uuid, +): + page = client_request.get( + 'main.view_job', + service_id=service_one['id'], + job_id=fake_uuid, + ) + assert page.select_one('p.hint').text.strip() == 'Report is 50% complete…' + assert page.select_one('tbody').text.strip() == 'No messages to show' @freeze_time("2016-01-01 11:09:00.061258") From d2f976c8eececffa5d6d9c5e466091ffd91527d5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Dec 2019 16:25:09 +0000 Subject: [PATCH 02/36] =?UTF-8?q?Don=E2=80=99t=20show=20progress=20once=20?= =?UTF-8?q?notifications=20are=20gone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We work out how complete a job’s processing is by looking at how many notifications have been created. Later, we deleted all the notifications, according to the data retention schedule. This makes it look like the job has gone back to 0% processed. This commit accounts for this by not showing the % complete message once a finished job has had its notifications deleted. --- .../partials/jobs/notifications.html | 2 +- tests/app/main/views/test_jobs.py | 21 +++++++++++++++++++ tests/conftest.py | 3 ++- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index 7ff70cc7f..b11b396cb 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -28,7 +28,7 @@ {% if template.template_type == 'letter' %}
{% endif %} - {% if percentage_complete < 100 %} + {% if percentage_complete < 100 and job.job_status != 'finished' %}

Report is {{ "{:.0f}%".format(percentage_complete * 0.99) }} complete…

diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 477794706..fe68a16b9 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -317,6 +317,27 @@ def test_should_show_job_without_notifications( assert page.select_one('tbody').text.strip() == 'No messages to show' +def test_should_show_old_job( + client_request, + service_one, + active_user_with_permissions, + mock_get_service_template, + mock_get_job, + mocker, + mock_get_notifications_with_no_notifications, + mock_get_service_data_retention, + fake_uuid, +): + page = client_request.get( + 'main.view_job', + service_id=service_one['id'], + job_id=fake_uuid, + ) + assert not page.select('p.hint') + assert not page.select('a[download]') + assert page.select_one('tbody').text.strip() == 'No messages to show' + + @freeze_time("2016-01-01 11:09:00.061258") def test_should_show_letter_job( client_request, diff --git a/tests/conftest.py b/tests/conftest.py index c5e4b371f..d99f974fc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1818,7 +1818,8 @@ def mock_get_job_in_progress(mocker, api_user_active): return {"data": job_json( service_id, api_user_active, job_id=job_id, notification_count=10, - notifications_requested=5 + notifications_requested=5, + job_status='processing', )} return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) From 9446b35e3b127e4b4f0765762122c4480c8bdcaa Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Dec 2019 16:37:50 +0000 Subject: [PATCH 03/36] Add a more accurate missing notifications message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Notifications could be missing because: - none have been created yet - they’ve been deleted This commit adds separate error messages for each case, rather than a less helpful generic one. --- app/main/views/jobs.py | 1 + app/templates/partials/jobs/notifications.html | 2 +- tests/app/main/views/test_jobs.py | 6 ++++-- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index ba4972a3b..b7f9a5d70 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -480,6 +480,7 @@ def get_job_partials(job, template): job=job, template=template, template_version=job['template_version'], + service_data_retention_days=service_data_retention_days, ), 'status': render_template( 'partials/jobs/status.html', diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index b11b396cb..29ee92311 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -47,7 +47,7 @@ notifications, caption=uploaded_file_name, caption_visible=False, - empty_message="No messages to show", + empty_message='These messages have been deleted because they were sent more than {} days ago'.format(service_data_retention_days) if job.job_status == 'finished' else 'No messages to show yet…', field_headings=[ 'Recipient', 'Status' diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index fe68a16b9..7832f8b20 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -314,7 +314,7 @@ def test_should_show_job_without_notifications( job_id=fake_uuid, ) assert page.select_one('p.hint').text.strip() == 'Report is 50% complete…' - assert page.select_one('tbody').text.strip() == 'No messages to show' + assert page.select_one('tbody').text.strip() == 'No messages to show yet…' def test_should_show_old_job( @@ -335,7 +335,9 @@ def test_should_show_old_job( ) assert not page.select('p.hint') assert not page.select('a[download]') - assert page.select_one('tbody').text.strip() == 'No messages to show' + assert page.select_one('tbody').text.strip() == ( + 'These messages have been deleted because they were sent more than 7 days ago' + ) @freeze_time("2016-01-01 11:09:00.061258") From 93d2d47f4e0862708a26d4b6001978b754451dbc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Dec 2019 16:53:45 +0000 Subject: [PATCH 04/36] =?UTF-8?q?Don=E2=80=99t=20show=20blue=20boxes=20onc?= =?UTF-8?q?e=20notifications=20have=20gone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit You can click the blue boxes to filter the list of notifications. Once the notifications have gone there’s nothing to filter, so we should just show the numbers but without them being clickable. --- app/main/views/jobs.py | 5 ++++- app/templates/partials/count.html | 17 +++++++++++++++-- tests/app/main/views/test_jobs.py | 13 +++++++++++++ 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index b7f9a5d70..147078e5c 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -446,7 +446,10 @@ def get_job_partials(job, template): counts = render_template( 'partials/count.html', counts=_get_job_counts(job), - status=filter_args['status'] + status=filter_args['status'], + notifications_deleted=( + job['job_status'] == 'finished' and not notifications['notifications'] + ), ) service_data_retention_days = current_service.get_days_of_retention(template['template_type']) can_letter_job_be_cancelled = False diff --git a/app/templates/partials/count.html b/app/templates/partials/count.html index eb2990161..c66b6bff2 100644 --- a/app/templates/partials/count.html +++ b/app/templates/partials/count.html @@ -1,5 +1,18 @@ +{% from "components/big-number.html" import big_number %} {% from "components/pill.html" import pill %} -
- {{ pill(counts, request.args.get('status', '')) }} +
+ {% if notifications_deleted %} +
+ {% for label, query_param, url, count in counts %} +
+ {{ big_number(count, label, smaller=True) }} +
+ {% endfor %} +
+ {% else %} +
+ {{ pill(counts, request.args.get('status', '')) }} +
+ {% endif %}
diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 7832f8b20..62c78641f 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -294,6 +294,12 @@ def test_should_show_job_in_progress( service_id=service_one['id'], job_id=fake_uuid, ) + assert [ + normalize_spaces(link.text) + for link in page.select('.pill a') + ] == [ + '10 sending', '0 delivered', '0 failed' + ] assert page.select_one('p.hint').text.strip() == 'Report is 50% complete…' @@ -313,6 +319,12 @@ def test_should_show_job_without_notifications( service_id=service_one['id'], job_id=fake_uuid, ) + assert [ + normalize_spaces(link.text) + for link in page.select('.pill a') + ] == [ + '10 sending', '0 delivered', '0 failed' + ] assert page.select_one('p.hint').text.strip() == 'Report is 50% complete…' assert page.select_one('tbody').text.strip() == 'No messages to show yet…' @@ -333,6 +345,7 @@ def test_should_show_old_job( service_id=service_one['id'], job_id=fake_uuid, ) + assert not page.select('.pill a') assert not page.select('p.hint') assert not page.select('a[download]') assert page.select_one('tbody').text.strip() == ( From b822914e23f977c2e13aefec897e426617c1f1e0 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Mon, 6 Jan 2020 10:55:47 +0000 Subject: [PATCH 05/36] Send asset path to Header component The SVG fallback image for the GOV.UK logo starts its path as '/assets/images' by default unless you send in an alternative root. This sends in our static asset host root to stop the image 404'ing on browsers that load it. --- app/templates/admin_template.html | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/templates/admin_template.html b/app/templates/admin_template.html index feefab8da..896dc6efd 100644 --- a/app/templates/admin_template.html +++ b/app/templates/admin_template.html @@ -125,7 +125,8 @@ "homepageUrl": url_for('main.show_accounts_or_dashboard'), "productName": "Notify", "navigation": navigation, - "navigationClasses": "govuk-header__navigation--end" + "navigationClasses": "govuk-header__navigation--end", + "assetsPath": asset_path + "images" }) }} {% endblock %} From 6726ad31b9b13e59197507e0416f8ceeca5caa04 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Mon, 6 Jan 2020 11:40:12 +0000 Subject: [PATCH 06/36] Add test for SVG fallback image path It's the only resource that uses the `asset_path` template variable in `template.njk` so this checks that works as expected. From feedback on the associated pull request: https://github.com/alphagov/notifications-admin/pull/3241#issuecomment-571102394 --- tests/app/main/views/test_index.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index 2e652200b..d5c744c0e 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -177,6 +177,15 @@ def test_css_is_served_from_correct_path(client_request): ][index]) +def test_resources_that_use_asset_path_variable_have_correct_path(client_request): + + page = client_request.get('main.documentation') # easy static page + + logo_svg_fallback = page.select_one('.govuk-header__logotype-crown-fallback-image') + + assert logo_svg_fallback['src'].startswith('https://static.example.com/images/govuk-logotype-crown.png') + + @pytest.mark.parametrize('extra_args, email_branding_retrieved', ( ( {}, From 1bb0b6b2f11c316c474c1e74bca773c050002ab9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jan 2020 12:56:43 +0000 Subject: [PATCH 07/36] Fix incorrect assertion for letter job CSV link The statement `page.select('[download=download]')` was returning an empty list because the link on the page has an empty `download` attribute, rather than it being set to the string `download`. This commit: - updates the assertion to find the element on the page - makes the test more specific by checking where the link goes to, not just its exists or lack thereof --- tests/app/main/views/test_jobs.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 2b3e8f707..0b386baae 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -329,7 +329,11 @@ def test_should_show_letter_job( assert normalize_spaces(page.select('.keyline-block')[1].text) == ( '6 January Estimated delivery date' ) - assert page.select('[download=download]') == [] + assert page.select_one('a[download]')['href'] == url_for( + 'main.view_job_csv', + service_id=SERVICE_ONE_ID, + job_id=fake_uuid, + ) assert page.select('.hint') == [] get_notifications.assert_called_with( From 024812b49a01828b086d021685881f10b3defee6 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 7 Jan 2020 09:58:16 +0000 Subject: [PATCH 08/36] Turn on compatibility mode for GOVUK Frontend Automatically fixes a load of known issues that arise when you use GOVUK Frontend at the same time as GOVUK Frontend Toolkit, GOVUK Template and GOVUK Elements. --- app/assets/stylesheets/govuk-frontend/_all.scss | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/assets/stylesheets/govuk-frontend/_all.scss b/app/assets/stylesheets/govuk-frontend/_all.scss index 9ecefd894..e91af0467 100644 --- a/app/assets/stylesheets/govuk-frontend/_all.scss +++ b/app/assets/stylesheets/govuk-frontend/_all.scss @@ -2,6 +2,13 @@ // Included to allow us to only include the components we need // All imports come from node_modules/govuk-frontend +// turn on compatibility mode for all legacy frameworks +// https://github.com/alphagov/govuk-frontend/blob/master/docs/installation/compatibility.md#turn-on-compatibility-mode +// to be removed when these frameworks are removed. +$govuk-compatibility-govukfrontendtoolkit: true; +$govuk-compatibility-govuktemplate: true; +$govuk-compatibility-govukelements: true; + // set asset URL root to match that of application $govuk-assets-path: "/static/"; From 91adadfed094ff8ecc0be547f460b2cc56f663c2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Jan 2020 11:54:10 +0000 Subject: [PATCH 09/36] dont reassign os.environ os.environ is an `environ` object, not a dict. by only interacting with it through builtin functions we can ensure it remains properly accessible to third party libraries which might interact with it in different ways. See https://github.com/alphagov/notifications-api/commit/d2441466 for more detail --- tests/app/test_cloudfoundry_config.py | 7 +++---- tests/app/test_config.py | 15 +++++++++------ tests/conftest.py | 5 +++-- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index dfd2ccdf7..d03aa29d4 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -6,12 +6,11 @@ from app.cloudfoundry_config import extract_cloudfoundry_config @pytest.fixture -def cloudfoundry_environ(monkeypatch): - monkeypatch.setenv('VCAP_APPLICATION', '{"space_name":"🚀🌌"}') +def cloudfoundry_environ(os_environ): + os.environ['VCAP_APPLICATION'] = '{"space_name":"🚀🌌"}' -@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_extract_cloudfoundry_config_populates_other_vars(): +def test_extract_cloudfoundry_config_populates_other_vars(cloudfoundry_environ): extract_cloudfoundry_config() assert os.environ['NOTIFY_ENVIRONMENT'] == '🚀🌌' diff --git a/tests/app/test_config.py b/tests/app/test_config.py index 4d0c7387d..166e5d277 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -12,21 +12,24 @@ def cf_conf(): @pytest.fixture -def reload_config(): +def reload_config(os_environ): """ Reset config, by simply re-running config.py from a fresh environment """ old_env = os.environ.copy() + os.environ.clear() yield - os.environ = old_env + os.environ.clear() + for k, v in old_env.items(): + os.environ[k] = v importlib.reload(config) -def test_load_cloudfoundry_config_if_available(monkeypatch, reload_config): +def test_load_cloudfoundry_config_if_available(reload_config): os.environ['API_HOST_NAME'] = 'env' - monkeypatch.setenv('VCAP_APPLICATION', 'some json blob') + os.environ['VCAP_APPLICATION'] = 'some json blob' with mock.patch('app.cloudfoundry_config.extract_cloudfoundry_config', side_effect=cf_conf) as cf_config: # reload config so that its module level code (ie: all of it) is re-instantiated @@ -38,10 +41,10 @@ def test_load_cloudfoundry_config_if_available(monkeypatch, reload_config): assert config.Config.API_HOST_NAME == 'cf' -def test_load_config_if_cloudfoundry_not_available(monkeypatch, reload_config): +def test_load_config_if_cloudfoundry_not_available(reload_config): os.environ['API_HOST_NAME'] = 'env' - monkeypatch.delenv('VCAP_APPLICATION', raising=False) + os.environ.pop('VCAP_APPLICATION', None) with mock.patch('app.cloudfoundry_config.extract_cloudfoundry_config') as cf_config: # reload config so that its module level code (ie: all of it) is re-instantiated diff --git a/tests/conftest.py b/tests/conftest.py index c5e4b371f..b78bb5ce6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2827,9 +2827,10 @@ def os_environ(): """ # for use whenever you expect code to edit environment variables old_env = os.environ.copy() - os.environ = {} + os.environ.clear() yield - os.environ = old_env + for k, v in old_env.items(): + os.environ[k] = v @pytest.fixture From e5b2d81d227c0b4aa61e72c55a1aa0845493f054 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Jan 2020 11:57:38 +0000 Subject: [PATCH 10/36] increase reply to address validation timeout on preview Celery/SQS underperforms in low-traffic environments. Tasks will sit on celery queues for several seconds before getting picked up if they're the only thing on the queue. This is observable in our test environments like preview and staging, but we've got enough load on production that this isn't an issue. When we validate reply to email addresses, we expect a delivery receipt to have been processed within 45 seconds of the button being pressed. On preview, we often observe times over that, possibly due to the several queues involved in sending an email and processing its receipt. So, to ensure that functional tests can pass (when we don't really care how fast things are, just that the flow doesn't break), bump this timeout up to 120 seconds on preview. The functional tests were waiting for 120 seconds for the reply to address to be validated anyway. --- app/config.py | 5 +++++ app/main/views/service_settings.py | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index ad8951baf..cd694be4b 100644 --- a/app/config.py +++ b/app/config.py @@ -72,6 +72,8 @@ class Config(object): ACTIVITY_STATS_LIMIT_DAYS = 7 TEST_MESSAGE_FILENAME = 'Report' + REPLY_TO_EMAIL_ADDRESS_VALIDATION_TIMEOUT = 45 + NOTIFY_ENVIRONMENT = 'development' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-local' MOU_BUCKET_NAME = 'local-mou' @@ -147,6 +149,9 @@ class Preview(Config): ASSET_DOMAIN = 'static.notify.works' ASSET_PATH = 'https://static.notify.works/' + # On preview, extend the validation timeout to allow more leniency when running functional tests + REPLY_TO_EMAIL_ADDRESS_VALIDATION_TIMEOUT = 120 + class Staging(Config): SHOW_STYLEGUIDE = False diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 8596a431c..79447ebe5 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -478,7 +478,8 @@ def get_service_verify_reply_to_address_partials(service_id, notification_id): created_at_no_tz = notification["created_at"][:-6] seconds_since_sending = (datetime.utcnow() - datetime.strptime(created_at_no_tz, '%Y-%m-%dT%H:%M:%S.%f')).seconds if notification["status"] in FAILURE_STATUSES or ( - notification["status"] in SENDING_STATUSES and seconds_since_sending > 45 + notification["status"] in SENDING_STATUSES and + seconds_since_sending > current_app.config['REPLY_TO_EMAIL_ADDRESS_VALIDATION_TIMEOUT'] ): verification_status = "failure" form.email_address.data = notification['to'] From d939c19222c69efdca328d1158614ec0b55a308d Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 7 Jan 2020 12:55:21 +0000 Subject: [PATCH 11/36] Remove govuk_template compatibility flag We removed govuk_template when we moved to the GOVUK Frontend template.njk for our base layout. The flag was originally turned on under the assumption that the global CSS govuk_template adds would be present in our cascade. It fixes issues that CSS causes with the GOVUK Frontend CSS further down. This was mostly wrong, as we did remove the govuk_template code, but our situation is a bit different because when we removed it, we copied across some global styles it introduces to prevent problems with our own CSS. One of the side-effects of turning on this flag was that the Transport font was not being applied. This turns the flag off again, which replaces the font, and hard-codes in the fix having this flag would have brought in: a darker colour for link text when focused. --- app/assets/stylesheets/globals.scss | 5 ----- app/assets/stylesheets/govuk-frontend/_all.scss | 1 - 2 files changed, 6 deletions(-) diff --git a/app/assets/stylesheets/globals.scss b/app/assets/stylesheets/globals.scss index 8db4a7fc7..8863601cd 100644 --- a/app/assets/stylesheets/globals.scss +++ b/app/assets/stylesheets/globals.scss @@ -67,11 +67,6 @@ a { background-color: $focus-colour; outline: 3px solid $focus-colour; } - - /* Make links slightly darker when focused to improve contrast. */ - &:link:focus { - color: darken( $link-colour, 2.5%) - } } // Each selector, and then the whole block when only one remains, to be removed when the diff --git a/app/assets/stylesheets/govuk-frontend/_all.scss b/app/assets/stylesheets/govuk-frontend/_all.scss index e91af0467..1f013e2c6 100644 --- a/app/assets/stylesheets/govuk-frontend/_all.scss +++ b/app/assets/stylesheets/govuk-frontend/_all.scss @@ -6,7 +6,6 @@ // https://github.com/alphagov/govuk-frontend/blob/master/docs/installation/compatibility.md#turn-on-compatibility-mode // to be removed when these frameworks are removed. $govuk-compatibility-govukfrontendtoolkit: true; -$govuk-compatibility-govuktemplate: true; $govuk-compatibility-govukelements: true; // set asset URL root to match that of application From ee9c36c1023a567b536508dad528042913279bce Mon Sep 17 00:00:00 2001 From: karlchillmaid Date: Tue, 7 Jan 2020 15:03:13 +0000 Subject: [PATCH 12/36] Update roadmap content --- app/templates/views/roadmap.html | 42 +++++++++++++++++--------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/app/templates/views/roadmap.html b/app/templates/views/roadmap.html index 4512f31c4..6bf623106 100644 --- a/app/templates/views/roadmap.html +++ b/app/templates/views/roadmap.html @@ -13,37 +13,39 @@

Notify is in public beta. This means it’s fully operational and supported, but we’re still adding new features. The roadmap is a guide to what we have planned, but some things might change.

You can contact us if you have any questions about the roadmap or suggestions for new features.

-

July to September 2019

- -
    -
  • Upload your own letters instead of using Notify templates
    • -
  • Get automated updates about returned and undelivered letters
    • -
- -

October to December 2019

- -
    -
  • Send letters internationally
    • -
  • Upload a mail merge of letters instead of using Notify templates
    • -
-

January to March 2020

    -
  • Bilingual letter templates for services that need to provide information in 2 languages
    • -
  • Save draft templates
    • +
  • Let services send letters to international addresses
    • +
  • Automate the removal of inactive users
-

April 2020 onwards

+

April to June 2020

+ +
    +
  • Add two-factor authentication to files sent by email
    • +
  • Let services add forms to letter templates
    • +
+ +

July to September 2020

+ +
    +
  • Add large print letter templates
    • +
  • Add multilingual letter templates so services can provide information in several languages
    • +
  • Publish design patterns to help teams send effective messages
    • +
  • Let services save contact lists
    • +
+ +

October 2020 onwards

    -
  • Other ways to pay for Notify
    • -
  • Send emails from NHS and Parliament email addresses
    • +
  • Start sending emails from NHS and Parliament email addresses
  • Link shortening service
    • -
  • Two-factor authentication for accessing files sent by email
  • Specify an expiry period for undeliverable text messages
    • +
  • Let users save draft templates
  • Distribute the delivery of a batch of messages over several hours
    • +
  • Explore other ways to pay for Notify
{% endblock %} From adededcf2164e5e4d59145ad5976eb06c23b8125 Mon Sep 17 00:00:00 2001 From: karlchillmaid Date: Tue, 7 Jan 2020 15:18:03 +0000 Subject: [PATCH 13/36] Update content --- app/templates/views/roadmap.html | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/templates/views/roadmap.html b/app/templates/views/roadmap.html index 6bf623106..6d62eb987 100644 --- a/app/templates/views/roadmap.html +++ b/app/templates/views/roadmap.html @@ -17,14 +17,14 @@
  • Let services send letters to international addresses
    • -
  • Automate the removal of inactive users
    • +
  • Automate the removal of inactive users and API keys

April to June 2020

  • Add two-factor authentication to files sent by email
    • -
  • Let services add forms to letter templates
    • +
  • Let services include forms with letter templates

July to September 2020

@@ -32,8 +32,8 @@
  • Add large print letter templates
  • Add multilingual letter templates so services can provide information in several languages
    • -
  • Publish design patterns to help teams send effective messages
    • -
  • Let services save contact lists
    • +
  • Publish guidance and examples to help teams send effective messages
    • +
  • Let services save emergency contact lists for business continuity

October 2020 onwards

@@ -41,7 +41,7 @@
  • Start sending emails from NHS and Parliament email addresses
    • -
  • Link shortening service
    • +
  • Add a link shortening service
  • Specify an expiry period for undeliverable text messages
  • Let users save draft templates
  • Distribute the delivery of a batch of messages over several hours
    • From c889801f4fa2ec3b34b80d62b150d4e03a4c089a Mon Sep 17 00:00:00 2001 From: karlchillmaid Date: Tue, 7 Jan 2020 15:32:12 +0000 Subject: [PATCH 14/36] Update content --- app/templates/views/roadmap.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/roadmap.html b/app/templates/views/roadmap.html index 6d62eb987..085f408bf 100644 --- a/app/templates/views/roadmap.html +++ b/app/templates/views/roadmap.html @@ -23,8 +23,8 @@

    April to June 2020

      -
    • Add two-factor authentication to files sent by email
      • -
    • Let services include forms with letter templates
      • +
    • Use two-factor authentication to protect files sent by email
      • +
    • Let services attach forms to letter templates

    July to September 2020

    From 82a71c67dff76a94c14fdc213452db3bf8f10bd1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Jan 2020 17:47:17 +0000 Subject: [PATCH 15/36] Make button more buttony `role=button` for users of voice control software `draggable=false` as per https://github.com/alphagov/govuk-frontend/pull/1020 --- app/templates/views/signedout.html | 2 +- tests/app/main/views/test_index.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index ea9e7db75..8f6906b89 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -31,7 +31,7 @@ Try GOV.UK Notify now if you work in central government, a local authority, or the NHS.

    - + Create an account or sign in if you’ve used diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index d5c744c0e..2b5232178 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -21,6 +21,10 @@ def test_non_logged_in_user_can_see_homepage( 'Send emails, text messages and letters to your users' ) + assert page.select_one('a[role=button][draggable=false]')['href'] == url_for( + 'main.register' + ) + assert page.select_one('meta[name=description]')['content'].strip() == ( 'GOV.UK Notify lets you send emails, text messages and letters ' 'to your users. Try it now if you work in central government, a ' From a602ce5937edb5a6a374c4cbf1e0b4cfc1b9fb39 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 6 Jan 2020 14:42:52 +0000 Subject: [PATCH 16/36] Stop calling mock_get_no_api_keys and api_keys_mock like functions --- tests/app/main/views/test_activity.py | 70 ++++++++++++--------------- tests/conftest.py | 3 +- 2 files changed, 31 insertions(+), 42 deletions(-) diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index d9942086a..dfc7f0897 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -14,8 +14,6 @@ from tests.conftest import ( create_active_caseworking_user, create_active_user_view_permissions, create_active_user_with_permissions, - mock_get_api_keys, - mock_get_no_api_keys, mock_get_notifications, normalize_spaces, ) @@ -420,43 +418,12 @@ def test_search_recipient_form( assert field['value'] == expected_search_box_contents -@pytest.mark.parametrize(( - 'message_type,' - 'api_keys_mock,' - 'expected_search_box_label,' -), [ - ( - None, - mock_get_no_api_keys, - 'Search by email address or phone number', - ), - ( - None, - mock_get_api_keys, - 'Search by email address, phone number or reference', - ), - ( - 'sms', - mock_get_no_api_keys, - 'Search by phone number', - ), - ( - 'sms', - mock_get_api_keys, - 'Search by phone number or reference', - ), - ( - 'email', - mock_get_no_api_keys, - 'Search by email address', - ), - ( - 'email', - mock_get_api_keys, - 'Search by email address or reference', - ), +@pytest.mark.parametrize('message_type, expected_search_box_label', [ + (None, 'Search by email address, phone number or reference'), + ('sms', 'Search by phone number or reference'), + ('email', 'Search by email address or reference'), ]) -def test_api_users_are_told_they_can_search_by_reference( +def test_api_users_are_told_they_can_search_by_reference_when_service_has_api_keys( client_request, mocker, fake_uuid, @@ -465,9 +432,32 @@ def test_api_users_are_told_they_can_search_by_reference( mock_get_service_data_retention, message_type, expected_search_box_label, - api_keys_mock, + mock_get_api_keys, +): + page = client_request.get( + 'main.view_notifications', + service_id=SERVICE_ONE_ID, + message_type=message_type, + ) + assert page.select_one('label[for=to]').text.strip() == expected_search_box_label + + +@pytest.mark.parametrize('message_type, expected_search_box_label', [ + (None, 'Search by email address or phone number'), + ('sms', 'Search by phone number'), + ('email', 'Search by email address'), +]) +def test_api_users_are_not_told_they_can_search_by_reference_when_service_has_no_api_keys( + client_request, + mocker, + fake_uuid, + mock_get_notifications, + mock_get_service_statistics, + mock_get_service_data_retention, + message_type, + expected_search_box_label, + mock_get_no_api_keys, ): - api_keys_mock(mocker, fake_uuid) page = client_request.get( 'main.view_notifications', service_id=SERVICE_ONE_ID, diff --git a/tests/conftest.py b/tests/conftest.py index 7dcf601d9..5e55b5817 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1687,8 +1687,7 @@ def mock_get_api_keys(mocker, fake_uuid): @pytest.fixture(scope='function') -# Second argument added so can be used interchangeably with `mock_get_api_keys` -def mock_get_no_api_keys(mocker, _=None): +def mock_get_no_api_keys(mocker): def _get_keys(service_id): keys = {'apiKeys': []} return keys From f93df53ed37c2bdefa460c968e04af65e47736ef Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 6 Jan 2020 15:29:21 +0000 Subject: [PATCH 17/36] Stop calling more fixtures as functions This commits stops calling more fixtures as if they were functions in order to reduce the number of errors with Pytest 5. --- tests/app/main/views/test_dashboard.py | 42 ++++++++++++++------ tests/app/main/views/test_email_branding.py | 4 +- tests/app/main/views/test_send.py | 44 +++++++++++---------- tests/app/main/views/test_templates.py | 6 +-- 4 files changed, 58 insertions(+), 38 deletions(-) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 7d0ebc05c..a31f5bb89 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -27,8 +27,6 @@ from tests.conftest import ( SERVICE_ONE_ID, create_active_caseworking_user, create_active_user_view_permissions, - mock_get_inbound_sms_summary, - mock_get_inbound_sms_summary_with_no_messages, normalize_spaces, ) @@ -207,11 +205,7 @@ def test_inbound_messages_not_visible_to_service_without_permissions( assert mock_get_inbound_sms_summary.called is False -@pytest.mark.parametrize('inbound_summary_mock, expected_text', [ - (mock_get_inbound_sms_summary_with_no_messages, '0 text messages received'), - (mock_get_inbound_sms_summary, '99 text messages received latest message just now'), -]) -def test_inbound_messages_shows_count_of_messages( +def test_inbound_messages_shows_count_of_messages_when_there_are_messages( client_request, mocker, service_one, @@ -221,19 +215,41 @@ def test_inbound_messages_shows_count_of_messages( mock_get_template_statistics, mock_get_usage, mock_get_free_sms_fragment_limit, - inbound_summary_mock, - expected_text + mock_get_inbound_sms_summary, ): - service_one['permissions'] = ['inbound_sms'] - inbound_summary_mock(mocker) - page = client_request.get( 'main.service_dashboard', service_id=SERVICE_ONE_ID, ) - assert normalize_spaces(page.select('.big-number-meta-wrapper')[0].text) == expected_text + assert normalize_spaces( + page.select('.big-number-meta-wrapper')[0].text + ) == '99 text messages received latest message just now' + assert page.select('.big-number-meta-wrapper a')[0]['href'] == url_for( + 'main.inbox', service_id=SERVICE_ONE_ID + ) + + +def test_inbound_messages_shows_count_of_messages_when_there_are_no_messages( + client_request, + mocker, + service_one, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_service_statistics, + mock_get_template_statistics, + mock_get_usage, + mock_get_free_sms_fragment_limit, + mock_get_inbound_sms_summary_with_no_messages, +): + service_one['permissions'] = ['inbound_sms'] + page = client_request.get( + 'main.service_dashboard', + service_id=SERVICE_ONE_ID, + ) + + assert normalize_spaces(page.select('.big-number-meta-wrapper')[0].text) == '0 text messages received' assert page.select('.big-number-meta-wrapper a')[0]['href'] == url_for( 'main.inbox', service_id=SERVICE_ONE_ID ) diff --git a/tests/app/main/views/test_email_branding.py b/tests/app/main/views/test_email_branding.py index 89e9052f9..92b4c236b 100644 --- a/tests/app/main/views/test_email_branding.py +++ b/tests/app/main/views/test_email_branding.py @@ -7,7 +7,7 @@ from flask import url_for from notifications_python_client.errors import HTTPError from app.s3_client.s3_logo_client import EMAIL_LOGO_LOCATION_STRUCTURE, TEMP_TAG -from tests.conftest import mock_get_email_branding, normalize_spaces +from tests.conftest import create_email_branding, normalize_spaces def test_email_branding_page_shows_full_branding_list( @@ -231,7 +231,7 @@ def test_deletes_previous_temp_logo_after_uploading_logo( fake_uuid ): if has_data: - mock_get_email_branding(mocker, fake_uuid) + mocker.patch('app.email_branding_client.get_email_branding', return_value=create_email_branding(fake_uuid)) with platform_admin_client.session_transaction() as session: user_id = session["user_id"] diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 87d13aedd..3e44a10b0 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -45,8 +45,6 @@ from tests.conftest import ( multiple_sms_senders, multiple_sms_senders_no_inbound, multiple_sms_senders_with_diff_default, - no_reply_to_email_addresses, - no_sms_senders, normalize_spaces, ) @@ -204,26 +202,12 @@ def test_sender_session_is_present_after_selected( assert session['sender_id'] == '1234' -@pytest.mark.parametrize('template_mock, sender_data', [ - ( - mock_get_service_email_template, - no_reply_to_email_addresses, - ), - ( - mock_get_service_template, - no_sms_senders - ) -]) -def test_set_sender_redirects_if_no_sender_data( +def test_set_sender_redirects_if_no_reply_to_email_addresses( client_request, - service_one, fake_uuid, - template_mock, - sender_data, - mocker + mock_get_service_email_template, + no_reply_to_email_addresses, ): - template_mock(mocker) - sender_data(mocker) client_request.get( '.set_sender', service_id=SERVICE_ONE_ID, @@ -231,7 +215,27 @@ def test_set_sender_redirects_if_no_sender_data( _expected_status=302, _expected_url=url_for( '.send_one_off', - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _external=True, + ) + ) + + +def test_set_sender_redirects_if_no_sms_senders( + client_request, + fake_uuid, + mock_get_service_template, + no_sms_senders, +): + client_request.get( + '.set_sender', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _expected_status=302, + _expected_url=url_for( + '.send_one_off', + service_id=SERVICE_ONE_ID, template_id=fake_uuid, _external=True, ) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 969370d57..fd6f2cfd2 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -551,8 +551,8 @@ def test_edit_letter_template_postage_page_displays_correctly( service_one, fake_uuid, mocker, + mock_get_service_letter_template, ): - mock_get_service_letter_template(mocker) page = client_request.get( 'main.edit_template_postage', service_id=SERVICE_ONE_ID, @@ -586,12 +586,12 @@ def test_edit_letter_templates_postage_updates_postage( client_request, service_one, mocker, - fake_uuid + fake_uuid, + mock_get_service_letter_template, ): mock_update_template_postage = mocker.patch( 'app.main.views.templates.service_api_client.update_service_template_postage' ) - mock_get_service_letter_template(mocker) client_request.post( 'main.edit_template_postage', From a3978f8a5f7521bd1fa3c7e07a013b3d638e7016 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 7 Jan 2020 10:47:00 +0000 Subject: [PATCH 18/36] Use fixtures that get services in Pytest 5 compatible way --- tests/app/main/views/test_api_integration.py | 21 +++++----- tests/app/main/views/test_send.py | 41 ++++++++++---------- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py index 5ba9a68c1..eb078db7c 100644 --- a/tests/app/main/views/test_api_integration.py +++ b/tests/app/main/views/test_api_integration.py @@ -11,10 +11,7 @@ from tests.conftest import ( SERVICE_ONE_ID, mock_get_empty_service_callback_api, mock_get_empty_service_inbound_api, - mock_get_live_service, mock_get_notifications, - mock_get_service, - mock_get_service_with_letters, mock_get_valid_service_callback_api, mock_get_valid_service_inbound_api, normalize_spaces, @@ -210,8 +207,8 @@ def test_should_show_api_keys_page( mock_get_api_keys.assert_called_once_with(SERVICE_ONE_ID) -@pytest.mark.parametrize('service_mock, expected_options', [ - (mock_get_service, [ +@pytest.mark.parametrize('restricted, can_send_letters, expected_options', [ + (True, False, [ ( 'Live – sends to anyone ' 'Not available because your service is in trial mode' @@ -219,12 +216,12 @@ def test_should_show_api_keys_page( 'Team and whitelist – limits who you can send to', 'Test – pretends to send messages', ]), - (mock_get_live_service, [ + (False, False, [ 'Live – sends to anyone', 'Team and whitelist – limits who you can send to', 'Test – pretends to send messages', ]), - (mock_get_service_with_letters, [ + (False, True, [ 'Live – sends to anyone', ( 'Team and whitelist – limits who you can send to ' @@ -238,10 +235,16 @@ def test_should_show_create_api_key_page( mocker, api_user_active, mock_get_api_keys, - service_mock, + restricted, + can_send_letters, expected_options, + service_one, ): - service_mock(mocker, api_user_active) + service_one['restricted'] = restricted + if can_send_letters: + service_one['permissions'].append('letter') + + mocker.patch('app.service_api_client.get_service', return_value={'data': service_one}) page = client_request.get('main.create_api_key', service_id=SERVICE_ONE_ID) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 3e44a10b0..bfc1a1a0f 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -34,9 +34,6 @@ from tests.conftest import ( SERVICE_ONE_ID, create_active_caseworking_user, create_active_user_with_permissions, - mock_get_international_service, - mock_get_live_service, - mock_get_service, mock_get_service_email_template, mock_get_service_letter_template, mock_get_service_template, @@ -2110,9 +2107,9 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_get_service_statistics.assert_called_once_with(service_one['id'], today_only=True) -@pytest.mark.parametrize('service_mock, should_allow_international', [ - (mock_get_service, False), - (mock_get_international_service, True), +@pytest.mark.parametrize('international_sms_permission, should_allow_international', [ + (False, False), + (True, True), ]) def test_upload_csvfile_with_international_validates( mocker, @@ -2126,11 +2123,14 @@ def test_upload_csvfile_with_international_validates( mock_get_job_doesnt_exist, mock_get_jobs, fake_uuid, - service_mock, + international_sms_permission, should_allow_international, + service_one, ): + if international_sms_permission: + service_one['permissions'] += ('sms', 'international_sms') + mocker.patch('app.service_api_client.get_service', return_value={'data': service_one}) - service_mock(mocker, api_user_active) mocker.patch('app.main.views.send.s3download', return_value='') mock_recipients = mocker.patch( 'app.main.views.send.RecipientCSV', @@ -2737,9 +2737,9 @@ def test_check_messages_shows_trial_mode_error( ) -@pytest.mark.parametrize('service_mock, error_should_be_shown', [ - (mock_get_service, True), - (mock_get_live_service, False), +@pytest.mark.parametrize('restricted, error_should_be_shown', [ + (True, True), + (False, False), ]) @pytest.mark.parametrize('number_of_rows, expected_error_message', [ (1, 'You cannot send this letter'), @@ -2748,7 +2748,7 @@ def test_check_messages_shows_trial_mode_error( ]) def test_check_messages_shows_trial_mode_error_for_letters( client_request, - api_user_active, + service_one, mock_get_service_letter_template, mock_has_permissions, mock_get_users_by_service, @@ -2758,12 +2758,13 @@ def test_check_messages_shows_trial_mode_error_for_letters( mock_s3_set_metadata, fake_uuid, mocker, - service_mock, + restricted, error_should_be_shown, number_of_rows, expected_error_message, ): - service_mock(mocker, api_user_active) + service_one['restricted'] = restricted + mocker.patch('app.service_api_client.get_service', return_value={'data': service_one}) mocker.patch('app.main.views.send.s3download', return_value='\n'.join( ['address_line_1,address_line_2,postcode,'] + @@ -3092,10 +3093,7 @@ def test_letters_from_csv_files_dont_have_download_link( assert not page.select('a[download]') -@pytest.mark.parametrize('service_mock', ( - mock_get_service, - mock_get_live_service, -)) +@pytest.mark.parametrize('restricted', [True, False]) def test_one_off_letters_have_download_link( client_request, mocker, @@ -3105,10 +3103,11 @@ def test_one_off_letters_have_download_link( fake_uuid, mock_get_users_by_service, mock_get_service_statistics, - service_mock, + restricted, + service_one, ): - - service_mock(mocker, api_user_active) + service_one['restricted'] = restricted + mocker.patch('app.service_api_client.get_service', return_value={'data': service_one}) mocker.patch( 'app.main.views.send.get_page_count_for_letter', From 68983a21d27df3b43f0649337c255b9f813f0860 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 7 Jan 2020 11:44:35 +0000 Subject: [PATCH 19/36] Use fixtures for callbacks in a Pytest 5 compatible way --- tests/__init__.py | 2 + tests/app/main/views/test_api_integration.py | 217 ++++++++++--------- 2 files changed, 121 insertions(+), 98 deletions(-) diff --git a/tests/__init__.py b/tests/__init__.py index 06ecf9c75..a8df289c4 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -150,6 +150,8 @@ def service_json( users = [] if permissions is None: permissions = ['email', 'sms'] + if service_callback_api is None: + service_callback_api = [] if inbound_api is None: inbound_api = [] return { diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py index eb078db7c..5254f3ee2 100644 --- a/tests/app/main/views/test_api_integration.py +++ b/tests/app/main/views/test_api_integration.py @@ -1,7 +1,6 @@ import uuid from collections import OrderedDict from unittest.mock import call -from uuid import uuid4 import pytest from flask import url_for @@ -9,11 +8,7 @@ from flask import url_for from tests import sample_uuid, validate_route_permission from tests.conftest import ( SERVICE_ONE_ID, - mock_get_empty_service_callback_api, - mock_get_empty_service_inbound_api, mock_get_notifications, - mock_get_valid_service_callback_api, - mock_get_valid_service_inbound_api, normalize_spaces, ) @@ -522,16 +517,6 @@ def test_callback_forms_validation( assert error_msgs == expected_errors -@pytest.mark.parametrize('existing_values, callback_ids, delete_should_be_called', [ - (( - mock_get_valid_service_callback_api, - mock_get_valid_service_inbound_api, - ), [uuid4()], True), - (( - mock_get_empty_service_callback_api, - mock_get_empty_service_inbound_api, - ), [], False), -]) @pytest.mark.parametrize('bearer_token', ['', 'some-bearer-token']) @pytest.mark.parametrize('endpoint, expected_delete_url', [ ( @@ -546,22 +531,18 @@ def test_callback_forms_validation( def test_callback_forms_can_be_cleared( client_request, service_one, - existing_values, - callback_ids, - delete_should_be_called, endpoint, expected_delete_url, bearer_token, mocker, + fake_uuid, + mock_get_valid_service_callback_api, + mock_get_valid_service_inbound_api, ): - - service_one['service_callback_api'] = callback_ids - service_one['inbound_api'] = callback_ids - mocked_delete = mocker.patch('app.service_api_client.delete') - for mock_get_callback in existing_values: - mock_get_callback(mocker) - + service_one['service_callback_api'] = [fake_uuid] + service_one['inbound_api'] = [fake_uuid] service_one['permissions'] = ['inbound_sms'] + mocked_delete = mocker.patch('app.service_api_client.delete') page = client_request.post( endpoint, @@ -579,14 +560,53 @@ def test_callback_forms_can_be_cleared( assert not page.select(".error-message") - if delete_should_be_called: - mocked_delete.assert_called_once_with( - expected_delete_url.format( - service_one['id'], callback_ids[0] - ) + mocked_delete.assert_called_once_with( + expected_delete_url.format( + service_one['id'], fake_uuid ) - else: - assert mocked_delete.call_args_list == [] + ) + + +@pytest.mark.parametrize('bearer_token', ['', 'some-bearer-token']) +@pytest.mark.parametrize('endpoint, expected_delete_url', [ + ( + 'main.delivery_status_callback', + '/service/{}/delivery-receipt-api/{}', + ), + ( + 'main.received_text_messages_callback', + '/service/{}/inbound-api/{}', + ), +]) +def test_callback_forms_can_be_cleared_when_callback_and_inbound_apis_are_empty( + client_request, + service_one, + endpoint, + expected_delete_url, + bearer_token, + mocker, + mock_get_empty_service_callback_api, + mock_get_empty_service_inbound_api, +): + service_one['permissions'] = ['inbound_sms'] + mocked_delete = mocker.patch('app.service_api_client.delete') + + page = client_request.post( + endpoint, + service_id=service_one['id'], + _data={ + 'url': '', + 'bearer_token': bearer_token, + }, + _expected_redirect=url_for( + 'main.api_callbacks', + service_id=service_one['id'], + _external=True, + ) + ) + + assert not page.select(".error-message") + assert mocked_delete.call_args_list == [] @pytest.mark.parametrize('has_inbound_sms, expected_link', [ @@ -699,28 +719,14 @@ def test_create_delivery_status_and_receive_text_message_callbacks( ) -@pytest.mark.parametrize('endpoint, fixture', [ - ('main.delivery_status_callback', mock_get_valid_service_callback_api), - ('main.received_text_messages_callback', mock_get_valid_service_inbound_api), -]) -def test_update_delivery_status_and_receive_text_message_callbacks( +def test_update_delivery_status_callback_details( client_request, service_one, - mocker, - mock_get_notifications, - mock_update_service_inbound_api, mock_update_service_callback_api, - endpoint, - fixture, + mock_get_valid_service_callback_api, fake_uuid, ): - if endpoint == 'main.received_text_messages_callback': - service_one['inbound_api'] = [fake_uuid] - service_one['permissions'] = ['inbound_sms'] - else: - service_one['service_callback_api'] = [fake_uuid] - - fixture(mocker) + service_one['service_callback_api'] = [fake_uuid] data = { 'url': "https://test.url.com/", @@ -729,73 +735,88 @@ def test_update_delivery_status_and_receive_text_message_callbacks( } client_request.post( - endpoint, + 'main.delivery_status_callback', service_id=service_one['id'], _data=data, ) - if endpoint == 'main.received_text_messages_callback': - mock_update_service_inbound_api.assert_called_once_with( - service_one['id'], - url="https://test.url.com/", - bearer_token="1234567890", - user_id=fake_uuid, - inbound_api_id=fake_uuid, - ) - else: - mock_update_service_callback_api.assert_called_once_with( - service_one['id'], - url="https://test.url.com/", - bearer_token="1234567890", - user_id=fake_uuid, - callback_api_id=fake_uuid - ) + mock_update_service_callback_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + callback_api_id=fake_uuid + ) -@pytest.mark.parametrize('endpoint, data, fixture', [ - ( - 'main.delivery_status_callback', - {"url": "https://hello2.gov.uk", "bearer_token": "bearer_token_set"}, - mock_get_valid_service_callback_api - ), - ( - 'main.received_text_messages_callback', - {"url": "https://hello3.gov.uk", "bearer_token": "bearer_token_set"}, - mock_get_valid_service_inbound_api - ), -]) -def test_update_delivery_status_and_receive_text_message_callbacks_without_changes_do_not_update( +def test_update_receive_text_message_callback_details( client_request, service_one, - mocker, - mock_get_notifications, - mock_update_service_callback_api, mock_update_service_inbound_api, - data, - fixture, - endpoint, + mock_get_valid_service_inbound_api, fake_uuid, ): - if endpoint == 'main.received_text_messages_callback': - service_one['inbound_api'] = [fake_uuid] - service_one['permissions'] = ['inbound_sms'] - else: - service_one['service_callback_api'] = [fake_uuid] + service_one['inbound_api'] = [fake_uuid] + service_one['permissions'] = ['inbound_sms'] - fixture(mocker) - - data['user_id'] = fake_uuid + data = { + 'url': "https://test.url.com/", + 'bearer_token': '1234567890', + 'user_id': fake_uuid + } client_request.post( - endpoint, + 'main.received_text_messages_callback', service_id=service_one['id'], _data=data, ) - if endpoint == 'main.received_text_messages_callback': - assert mock_update_service_inbound_api.called is False - else: - assert mock_update_service_callback_api.called is False + mock_update_service_inbound_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + inbound_api_id=fake_uuid, + ) + + +def test_update_delivery_status_callback_without_changes_does_not_update( + client_request, + service_one, + mock_update_service_callback_api, + fake_uuid, + mock_get_valid_service_callback_api, +): + service_one['service_callback_api'] = [fake_uuid] + data = {"user_id": fake_uuid, "url": "https://hello2.gov.uk", "bearer_token": "bearer_token_set"} + + client_request.post( + 'main.delivery_status_callback', + service_id=service_one['id'], + _data=data, + ) + + assert mock_update_service_callback_api.called is False + + +def test_update_receive_text_message_callback_without_changes_does_not_update( + client_request, + service_one, + mock_update_service_inbound_api, + fake_uuid, + mock_get_valid_service_inbound_api, +): + service_one['inbound_api'] = [fake_uuid] + service_one['permissions'] = ['inbound_sms'] + data = {"user_id": fake_uuid, "url": "https://hello3.gov.uk", "bearer_token": "bearer_token_set"} + + client_request.post( + 'main.received_text_messages_callback', + service_id=service_one['id'], + _data=data, + ) + + assert mock_update_service_inbound_api.called is False @pytest.mark.parametrize('service_callback_api, delivery_url, expected_1st_table_row', [ From 9e3f21cd393edb5edfeff0e5e67d1d2970279ffe Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 7 Jan 2020 15:24:33 +0000 Subject: [PATCH 20/36] Stop calling mock_get_notications as if it's a function By creating a new function in conftest.py, `create_notifications`, which can be used instead. --- tests/app/main/views/test_activity.py | 54 +++++++++----------- tests/app/main/views/test_api_integration.py | 14 ++--- tests/app/main/views/test_conversation.py | 11 ++-- tests/app/main/views/test_jobs.py | 19 +++---- tests/conftest.py | 36 +++++++++++++ 5 files changed, 78 insertions(+), 56 deletions(-) diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index dfc7f0897..b2c7fe550 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -14,7 +14,7 @@ from tests.conftest import ( create_active_caseworking_user, create_active_user_view_permissions, create_active_user_with_permissions, - mock_get_notifications, + create_notifications, normalize_spaces, ) @@ -265,19 +265,15 @@ def test_download_not_available_to_users_without_dashboard( def test_letters_with_status_virus_scan_failed_shows_a_failure_description( mocker, - active_user_with_permissions, client_request, service_one, mock_get_service_statistics, mock_get_service_data_retention, mock_get_api_keys, ): - mock_get_notifications( - mocker, - active_user_with_permissions, - is_precompiled_letter=True, - noti_status='virus-scan-failed' - ) + notifications = create_notifications(template_type='letter', status='virus-scan-failed', is_precompiled_letter=True) + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) + page = client_request.get( 'main.view_notifications', service_id=service_one['id'], @@ -294,7 +290,6 @@ def test_letters_with_status_virus_scan_failed_shows_a_failure_description( ]) def test_should_not_show_preview_link_for_precompiled_letters_in_virus_states( mocker, - active_user_with_permissions, client_request, service_one, mock_get_service_statistics, @@ -302,12 +297,9 @@ def test_should_not_show_preview_link_for_precompiled_letters_in_virus_states( mock_get_no_api_keys, letter_status, ): - mock_get_notifications( - mocker, - active_user_with_permissions, - is_precompiled_letter=True, - noti_status=letter_status - ) + notifications = create_notifications(template_type='letter', status=letter_status) + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) + page = client_request.get( 'main.view_notifications', service_id=service_one['id'], @@ -571,16 +563,14 @@ def test_html_contains_notification_id( def test_html_contains_links_for_failed_notifications( client_request, - active_user_with_permissions, mock_get_service_statistics, mock_get_service_data_retention, mock_get_no_api_keys, mocker, ): - mock_get_notifications(mocker, - active_user_with_permissions, - diff_template_type="sms", - noti_status='technical-failure') + notifications = create_notifications(status='technical-failure') + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) + response = client_request.get( 'main.view_notifications', service_id=SERVICE_ONE_ID, @@ -596,18 +586,18 @@ def test_html_contains_links_for_failed_notifications( def test_redacts_templates_that_should_be_redacted( client_request, mocker, - active_user_with_permissions, mock_get_service_statistics, mock_get_service_data_retention, mock_get_no_api_keys, ): - mock_get_notifications( - mocker, - active_user_with_permissions, - template_content="hello ((name))", + notifications = create_notifications( + status='technical-failure', + content='hello ((name))', personalisation={'name': 'Jo'}, redact_personalisation=True, ) + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) + page = client_request.get( 'main.view_notifications', service_id=SERVICE_ONE_ID, @@ -680,7 +670,6 @@ def test_big_numbers_and_search_dont_show_for_letters( def test_sending_status_hint_displays_correctly_on_notifications_page( client_request, service_one, - active_user_with_permissions, mock_get_service_statistics, mock_get_service_data_retention, mock_get_no_api_keys, @@ -690,7 +679,8 @@ def test_sending_status_hint_displays_correctly_on_notifications_page( single_line, mocker ): - mock_get_notifications(mocker, True, diff_template_type=message_type, noti_status=status) + notifications = create_notifications(template_type=message_type, status=status) + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) page = client_request.get( 'main.view_notifications', @@ -709,17 +699,19 @@ def test_sending_status_hint_displays_correctly_on_notifications_page( def test_should_expected_hint_for_letters( client_request, service_one, - active_user_with_permissions, mock_get_service_statistics, mock_get_service_data_retention, mock_get_no_api_keys, mocker, - fake_uuid, is_precompiled_letter, expected_hint ): - mock_get_notifications( - mocker, active_user_with_permissions, is_precompiled_letter=is_precompiled_letter) + notifications = create_notifications( + template_type='letter', + subject=expected_hint, + is_precompiled_letter=is_precompiled_letter, + ) + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) page = client_request.get( 'main.view_notifications', diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py index 5254f3ee2..675dfee7b 100644 --- a/tests/app/main/views/test_api_integration.py +++ b/tests/app/main/views/test_api_integration.py @@ -8,7 +8,7 @@ from flask import url_for from tests import sample_uuid, validate_route_permission from tests.conftest import ( SERVICE_ONE_ID, - mock_get_notifications, + create_notifications, normalize_spaces, ) @@ -73,13 +73,13 @@ def test_should_show_api_page_with_no_notifications( ]) def test_letter_notifications_should_have_link_to_view_letter( client_request, - api_user_active, mock_has_permissions, mocker, template_type, link_text, ): - mock_get_notifications(mocker, api_user_active, diff_template_type=template_type) + notifications = create_notifications(template_type=template_type) + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) page = client_request.get( 'main.api_integration', service_id=SERVICE_ONE_ID, @@ -93,13 +93,13 @@ def test_letter_notifications_should_have_link_to_view_letter( ]) def test_should_not_have_link_to_view_letter_for_precompiled_letters_in_virus_states( client_request, - api_user_active, fake_uuid, mock_has_permissions, mocker, status ): - mock_get_notifications(mocker, api_user_active, noti_status=status) + notifications = create_notifications(status=status) + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) page = client_request.get( 'main.api_integration', @@ -115,14 +115,14 @@ def test_should_not_have_link_to_view_letter_for_precompiled_letters_in_virus_st ]) def test_letter_notifications_should_show_client_reference( client_request, - api_user_active, fake_uuid, mock_has_permissions, mocker, client_reference, shows_ref ): - mock_get_notifications(mocker, api_user_active, client_reference=client_reference) + notifications = create_notifications(client_reference=client_reference) + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) page = client_request.get( 'main.api_integration', diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index d1a236ea6..84c9bf726 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -11,7 +11,7 @@ from app.main.views.conversation import get_user_number from tests.conftest import ( SERVICE_ONE_ID, _template, - mock_get_notifications, + create_notifications, normalize_spaces, ) @@ -76,7 +76,6 @@ def test_get_user_phone_number_raises_if_both_api_requests_fail(mocker): def test_view_conversation( client_request, mocker, - api_user_active, mock_get_inbound_sms_by_id_with_no_messages, mock_get_notification, fake_uuid, @@ -84,14 +83,12 @@ def test_view_conversation( expected_outbound_content, mock_get_inbound_sms ): - - mock = mock_get_notifications( - mocker, - api_user_active, - template_content='Hello ((name))', + notifications = create_notifications( + content='Hello ((name))', personalisation={'name': 'Jo'}, redact_personalisation=outbound_redacted, ) + mock = mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) page = client_request.get( 'main.conversation', diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 6c4c1da76..d1cb3fa5f 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -11,7 +11,7 @@ from tests.conftest import ( SERVICE_ONE_ID, create_active_caseworking_user, create_active_user_with_permissions, - mock_get_notifications, + create_notifications, mock_get_service_letter_template, normalize_spaces, ) @@ -360,11 +360,13 @@ def test_should_show_letter_job( mock_get_job, mock_get_service_data_retention, fake_uuid, - active_user_with_permissions, mocker, ): - - get_notifications = mock_get_notifications(mocker, active_user_with_permissions, diff_template_type='letter') + notifications = create_notifications(template_type='letter', subject='template subject') + get_notifications = mocker.patch( + 'app.notification_api_client.get_notifications_for_service', + return_value=notifications, + ) page = client_request.get( 'main.view_job', @@ -742,15 +744,10 @@ def test_should_show_letter_job_with_first_class_if_notifications_are_first_clas mock_get_job, mock_get_service_data_retention, fake_uuid, - active_user_with_permissions, mocker, ): - mock_get_notifications( - mocker, - active_user_with_permissions, - diff_template_type='letter', - postage='first' - ) + notifications = create_notifications(template_type='letter', postage='first') + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) page = client_request.get( 'main.view_job', diff --git a/tests/conftest.py b/tests/conftest.py index 5e55b5817..5a78cf586 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3930,3 +3930,39 @@ def create_notification( if key_type: noti['key_type'] = key_type return noti + + +def create_notifications( + service_id=SERVICE_ONE_ID, + template_type='sms', + rows=5, + status=None, + subject='subject', + content='content', + client_reference=None, + personalisation=None, + redact_personalisation=False, + is_precompiled_letter=False, + postage=None, +): + template = template_json( + service_id, + id_=str(generate_uuid()), + type_=template_type, + subject=subject, + content=content, + redact_personalisation=redact_personalisation, + is_precompiled_letter=is_precompiled_letter + ) + + return notification_json( + service_id, + template=template, + rows=rows, + personalisation=personalisation, + template_type=template_type, + client_reference=client_reference, + status=status, + created_by_name='Firstname Lastname', + postage=postage + ) From 0ec086cd244e5676c124acc5fa3b8a7a7aab0f1f Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 8 Jan 2020 09:10:04 +0000 Subject: [PATCH 21/36] Use fixtures for templates in a Pytest 5 compatible way --- tests/app/main/views/test_jobs.py | 8 +- tests/app/main/views/test_send.py | 250 ++++++++++++++----------- tests/app/main/views/test_templates.py | 57 +++--- tests/conftest.py | 41 +++- 4 files changed, 219 insertions(+), 137 deletions(-) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index d1cb3fa5f..94c261aea 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -12,7 +12,7 @@ from tests.conftest import ( create_active_caseworking_user, create_active_user_with_permissions, create_notifications, - mock_get_service_letter_template, + create_template, normalize_spaces, ) @@ -768,8 +768,10 @@ def test_should_show_letter_job_with_first_class_if_no_notifications( mock_get_service_data_retention, mocker ): - - mock_get_service_letter_template(mocker, postage="first") + mocker.patch( + 'app.service_api_client.get_service_template', + return_value={'data': create_template(template_type='letter', postage='first')} + ) page = client_request.get( 'main.view_job', diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index bfc1a1a0f..45e7217c7 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -34,14 +34,12 @@ from tests.conftest import ( SERVICE_ONE_ID, create_active_caseworking_user, create_active_user_with_permissions, + create_multiple_email_reply_to_addresses, + create_multiple_sms_senders, + create_template, mock_get_service_email_template, mock_get_service_letter_template, mock_get_service_template, - mock_get_service_template_with_placeholders, - multiple_reply_to_email_addresses, - multiple_sms_senders, - multiple_sms_senders_no_inbound, - multiple_sms_senders_with_diff_default, normalize_spaces, ) @@ -54,72 +52,85 @@ test_spreadsheet_files = glob(path.join('tests', 'spreadsheet_files', '*')) test_non_spreadsheet_files = glob(path.join('tests', 'non_spreadsheet_files', '*')) -@pytest.mark.parametrize('template_mock, sender_data, expected_title, expected_description', [ - ( - mock_get_service_email_template, - multiple_reply_to_email_addresses, - 'Where should replies come back to?', - 'Where should replies come back to?', - ), - ( - mock_get_service_template, - multiple_sms_senders, - 'Who should the message come from?', - 'Who should the message come from?', - ) -]) -def test_show_correct_title_and_description_for_sender_type( +def test_show_correct_title_and_description_for_email_sender_type( client_request, - service_one, fake_uuid, - template_mock, - sender_data, - expected_title, - expected_description, - mocker + mock_get_service_email_template, + multiple_reply_to_email_addresses, ): - template_mock(mocker) - sender_data(mocker) - page = client_request.get( '.set_sender', - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, template_id=fake_uuid ) - assert page.select_one('h1').text == expected_title + assert page.select_one('h1').text == 'Where should replies come back to?' for element in ('legend', 'legend .govuk-visually-hidden'): - assert normalize_spaces(page.select_one(element).text) == expected_description + assert normalize_spaces(page.select_one(element).text) == 'Where should replies come back to?' -@pytest.mark.parametrize('template_mock, sender_data', [ - ( - mock_get_service_email_template, - multiple_reply_to_email_addresses, - ), - ( - mock_get_service_template, - multiple_sms_senders_with_diff_default - ), - ( - mock_get_service_template, - multiple_sms_senders_no_inbound - ) -]) -def test_default_sender_is_checked_and_has_hint( +def test_show_correct_title_and_description_for_sms_sender_type( client_request, - service_one, fake_uuid, - template_mock, - sender_data, - mocker + mock_get_service_template, + multiple_sms_senders, ): - template_mock(mocker) - sender_data(mocker) page = client_request.get( '.set_sender', - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, + template_id=fake_uuid + ) + + assert page.select_one('h1').text == 'Who should the message come from?' + + for element in ('legend', 'legend .govuk-visually-hidden'): + assert normalize_spaces(page.select_one(element).text) == 'Who should the message come from?' + + +def test_default_email_sender_is_checked_and_has_hint( + client_request, + fake_uuid, + mock_get_service_email_template, + multiple_reply_to_email_addresses, +): + page = client_request.get( + '.set_sender', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid + ) + + assert page.select('.multiple-choice input')[0].has_attr('checked') + assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "(Default)" + assert not page.select('.multiple-choice input')[1].has_attr('checked') + + +def test_default_sms_sender_is_checked_and_has_hint( + client_request, + fake_uuid, + mock_get_service_template, + multiple_sms_senders_with_diff_default, +): + page = client_request.get( + '.set_sender', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid + ) + + assert page.select('.multiple-choice input')[0].has_attr('checked') + assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "(Default)" + assert not page.select('.multiple-choice input')[1].has_attr('checked') + + +def test_default_sms_sender_is_checked_and_has_hint_when_there_are_no_inbound_numbers( + client_request, + fake_uuid, + mock_get_service_template, + multiple_sms_senders_no_inbound, +): + page = client_request.get( + '.set_sender', + service_id=SERVICE_ONE_ID, template_id=fake_uuid ) @@ -168,26 +179,32 @@ def test_sms_sender_has_receives_replies_hint( assert not page.select('.multiple-choice input')[2].has_attr('checked') -@pytest.mark.parametrize('template_mock, sender_data', [ +@pytest.mark.parametrize('template_type, sender_data', [ ( - mock_get_service_email_template, - multiple_reply_to_email_addresses, + 'email', + create_multiple_email_reply_to_addresses(), ), ( - mock_get_service_template, - multiple_sms_senders + 'sms', + create_multiple_sms_senders() ) ]) def test_sender_session_is_present_after_selected( client_request, service_one, fake_uuid, - template_mock, + template_type, sender_data, mocker ): - template_mock(mocker) - sender_data(mocker) + template_data = create_template(template_type=template_type) + mocker.patch('app.service_api_client.get_service_template', return_value={'data': template_data}) + + if template_type == 'email': + mocker.patch('app.service_api_client.get_reply_to_email_addresses', return_value=sender_data) + else: + mocker.patch('app.service_api_client.get_sms_senders', return_value=sender_data) + client_request.post( '.set_sender', service_id=service_one['id'], @@ -989,47 +1006,54 @@ def test_send_test_doesnt_show_file_contents( assert page.select_one('button[type=submit]').text.strip() == 'Send 1 text message' -@pytest.mark.parametrize('user, endpoint, template_mock, expected_recipient', [ +@pytest.mark.parametrize('user, endpoint, template_type, content_has_placeholders, expected_recipient', [ ( create_active_user_with_permissions(), 'main.send_test_step', - mock_get_service_template_with_placeholders, + 'sms', + True, '07700 900762' ), ( create_active_user_with_permissions(), 'main.send_test_step', - mock_get_service_email_template, + 'email', + False, 'test@user.gov.uk' ), ( create_active_caseworking_user(), 'main.send_test_step', - mock_get_service_email_template, + 'email', + False, 'caseworker@example.gov.uk' ), ( create_active_user_with_permissions(), 'main.send_test_step', - mock_get_service_letter_template, + 'letter', + False, None ), ( create_active_user_with_permissions(), 'main.send_one_off_step', - mock_get_service_template, + 'sms', + False, None ), ( create_active_user_with_permissions(), 'main.send_one_off_step', - mock_get_service_email_template, + 'email', + False, None ), ( create_active_user_with_permissions(), 'main.send_one_off_step', - mock_get_service_letter_template, + 'letter', + False, None ), ]) @@ -1040,13 +1064,18 @@ def test_send_test_step_redirects_if_session_not_setup( mock_get_users_by_service, mock_has_no_jobs, fake_uuid, - endpoint, - template_mock, - expected_recipient, user, + endpoint, + template_type, + content_has_placeholders, + expected_recipient, ): mocker.patch('app.user_api_client.get_user', return_value=user) - template_mock(mocker) + + template_content = 'Hi ((name))' if content_has_placeholders else 'Hi there' + template_data = create_template(template_type=template_type, content=template_content) + mocker.patch('app.service_api_client.get_service_template', return_value={'data': template_data}) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) with client_request.session_transaction() as session: @@ -1094,51 +1123,51 @@ def test_send_one_off_does_not_send_without_the_correct_permissions( create_active_user_with_permissions(), create_active_caseworking_user(), )) -@pytest.mark.parametrize('template_mock, partial_url, expected_h1, tour_shown', [ +@pytest.mark.parametrize('template_type, partial_url, expected_h1, tour_shown', [ ( - mock_get_service_template_with_placeholders, + 'sms', partial(url_for, 'main.send_test'), 'Personalise this message', False, ), ( - mock_get_service_template_with_placeholders, + 'sms', partial(url_for, 'main.send_one_off'), 'Send ‘Two week reminder’', False, ), ( - mock_get_service_template_with_placeholders, + 'sms', partial(url_for, 'main.send_test', help=1), 'Example text message', True, ), ( - mock_get_service_email_template, + 'email', partial(url_for, 'main.send_test', help=1), 'Example text message', True, ), ( - mock_get_service_email_template, + 'email', partial(url_for, 'main.send_test'), 'Personalise this message', False, ), ( - mock_get_service_email_template, + 'email', partial(url_for, 'main.send_one_off'), 'Send ‘Two week reminder’', False, ), ( - mock_get_service_letter_template, + 'letter', partial(url_for, 'main.send_test'), 'Send ‘Two week reminder’', False, ), ( - mock_get_service_letter_template, + 'letter', partial(url_for, 'main.send_one_off'), 'Send ‘Two week reminder’', False, @@ -1150,14 +1179,15 @@ def test_send_one_off_or_test_has_correct_page_titles( mock_has_no_jobs, fake_uuid, mocker, - template_mock, + template_type, partial_url, expected_h1, tour_shown, user, ): mocker.patch('app.user_api_client.get_user', return_value=user) - template_mock(mocker) + template_data = create_template(template_type=template_type, name='Two week reminder', content='Hi there ((name))') + mocker.patch('app.service_api_client.get_service_template', return_value={'data': template_data}) mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) response = logged_in_client.get( @@ -1229,27 +1259,27 @@ def test_send_one_off_or_test_shows_placeholders_in_correct_order( assert normalize_spaces(page.select_one('label').text) == expected_field_label -@pytest.mark.parametrize('user, template_mock, expected_link_text, expected_link_url', [ +@pytest.mark.parametrize('user, template_type, expected_link_text, expected_link_url', [ ( create_active_user_with_permissions(), - mock_get_service_template, + 'sms', 'Use my phone number', partial(url_for, 'main.send_test') ), ( create_active_user_with_permissions(), - mock_get_service_email_template, + 'email', 'Use my email address', partial(url_for, 'main.send_test') ), ( create_active_user_with_permissions(), - mock_get_service_letter_template, + 'letter', None, None ), ( create_active_caseworking_user(), - mock_get_service_template, + 'sms', None, None ), ]) @@ -1260,13 +1290,14 @@ def test_send_one_off_has_skip_link( mock_get_service_email_template, mock_has_no_jobs, mocker, - template_mock, + template_type, expected_link_text, expected_link_url, user, ): mocker.patch('app.user_api_client.get_user', return_value=user) - template_mock(mocker) + template_data = create_template(template_id=fake_uuid, template_type=template_type) + mocker.patch('app.service_api_client.get_service_template', return_value={'data': template_data}) mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) page = client_request.get( @@ -1289,20 +1320,21 @@ def test_send_one_off_has_skip_link( assert not skip_links -@pytest.mark.parametrize('template_mock, expected_sticky', [ - (mock_get_service_template, False), - (mock_get_service_email_template, True), - (mock_get_service_letter_template, True), +@pytest.mark.parametrize('template_type, expected_sticky', [ + ('sms', False), + ('email', True), + ('letter', True), ]) def test_send_one_off_has_sticky_header_for_email_and_letter( mocker, client_request, fake_uuid, mock_has_no_jobs, - template_mock, + template_type, expected_sticky, ): - template_mock(mocker) + template_data = create_template(template_type=template_type) + mocker.patch('app.service_api_client.get_service_template', return_value={'data': template_data}) mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) page = client_request.get( @@ -2534,25 +2566,29 @@ def test_route_permissions_sending( @pytest.mark.parametrize( - 'template_mock, extra_args, expected_url', + 'template_type, has_placeholders, extra_args, expected_url', [ ( - mock_get_service_template, + 'sms', + False, dict(), partial(url_for, '.send_messages') ), ( - mock_get_service_template_with_placeholders, + 'sms', + True, dict(), partial(url_for, '.send_messages') ), ( - mock_get_service_letter_template, # No placeholders + 'letter', + False, dict(from_test=True), partial(url_for, '.send_test') ), ( - mock_get_service_template_with_placeholders, + 'sms', + True, dict(from_test=True), partial(url_for, '.send_test') ) @@ -2570,12 +2606,14 @@ def test_check_messages_back_link( mock_s3_set_metadata, fake_uuid, mocker, - template_mock, + template_type, + has_placeholders, extra_args, expected_url ): - - template_mock(mocker) + content = 'Hi there ((name))' if has_placeholders else 'Hi there' + template_data = create_template(template_id=fake_uuid, template_type=template_type, content=content) + mocker.patch('app.service_api_client.get_service_template', return_value={'data': template_data}) mocker.patch( 'app.main.views.send.get_page_count_for_letter', diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index fd6f2cfd2..fde9b8f0b 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -29,9 +29,7 @@ from tests.conftest import ( create_active_caseworking_user, create_active_user_view_permissions, create_letter_contact_block, - mock_get_service_email_template, - mock_get_service_letter_template, - mock_get_service_template, + create_template, normalize_spaces, ) @@ -495,7 +493,11 @@ def test_view_letter_template_displays_postage( ): mocker.patch('app.main.views.templates.get_page_count_for_letter', return_value=1) client_request.login(active_user_with_permissions) - mock_get_service_letter_template(mocker, postage=template_postage) + mocker.patch( + 'app.service_api_client.get_service_template', + return_value={'data': create_template(template_type='letter', postage=template_postage)} + ) + page = client_request.get( 'main.view_template', service_id=SERVICE_ONE_ID, @@ -534,7 +536,11 @@ def test_view_letter_template_does_not_display_send_button_if_template_over_10_p ): mocker.patch('app.main.views.templates.get_page_count_for_letter', return_value=11) client_request.login(active_user_with_permissions) - mock_get_service_letter_template(mocker, postage="second") + mocker.patch( + 'app.service_api_client.get_service_template', + return_value={'data': create_template(template_type='letter', postage='second')} + ) + page = client_request.get( 'main.view_template', service_id=SERVICE_ONE_ID, @@ -1300,10 +1306,10 @@ def test_should_not_allow_creation_of_a_template_without_correct_permission( ) -@pytest.mark.parametrize('fixture, expected_status_code', [ - (mock_get_service_email_template, 200), - (mock_get_service_template, 200), - (mock_get_service_letter_template, 302), +@pytest.mark.parametrize('template_type, expected_status_code', [ + ('email', 200), + ('sms', 200), + ('letter', 302), ]) def test_should_redirect_to_one_off_if_template_type_is_letter( client_request, @@ -1311,10 +1317,13 @@ def test_should_redirect_to_one_off_if_template_type_is_letter( multiple_sms_senders, fake_uuid, mocker, - fixture, + template_type, expected_status_code ): - fixture(mocker) + mocker.patch( + 'app.service_api_client.get_service_template', + return_value={'data': create_template(template_type=template_type)} + ) client_request.get( '.set_sender', service_id=SERVICE_ONE_ID, @@ -1506,11 +1515,14 @@ def test_should_show_interstitial_when_making_breaking_change( old_content, expected_paragraphs, ): - mock_get_service_email_template( - mocker, + email_template = create_template( + template_id=fake_uuid, + template_type='email', subject="Your ((thing)) is due soon", - content=old_content, + content=old_content ) + mocker.patch('app.service_api_client.get_service_template', return_value={'data': email_template}) + data = { 'id': fake_uuid, 'name': "new name", @@ -2048,18 +2060,17 @@ def test_should_show_template_as_first_page_of_tour( ) -@pytest.mark.parametrize('template_mock', [ - mock_get_service_email_template, - mock_get_service_letter_template, -]) +@pytest.mark.parametrize('template_type', ['email', 'letter']) def test_cant_see_email_template_in_tour( client_request, fake_uuid, mocker, - template_mock, + template_type, ): - - template_mock(mocker) + mocker.patch( + 'app.service_api_client.get_service_template', + return_value={'data': create_template(template_type=template_type)} + ) client_request.get( 'main.start_tour', @@ -2124,8 +2135,8 @@ def test_should_show_hint_once_template_redacted( mock_get_template_folders, fake_uuid, ): - - mock_get_service_email_template(mocker, redact_personalisation=True) + template = create_template(template_type='email', content='hi ((name))', redact_personalisation=True) + mocker.patch('app.service_api_client.get_service_template', return_value={'data': template}) page = client_request.get( 'main.view_template', diff --git a/tests/conftest.py b/tests/conftest.py index 5a78cf586..3305e2dcb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -866,11 +866,20 @@ def mock_get_service_email_template(mocker, content=None, subject=None, redact_p @pytest.fixture(scope='function') def mock_get_service_email_template_without_placeholders(mocker): - return mock_get_service_email_template( - mocker, - content="Your vehicle tax expires soon", - subject="Your thing is due soon", - ) + def _get(service_id, template_id, version=None): + template = template_json( + service_id, + template_id, + "Two week reminder", + "email", + "Your vehicle tax expires soon", + "Your thing is due soon", + redact_personalisation=False, + ) + return {'data': template} + + return mocker.patch( + 'app.service_api_client.get_service_template', side_effect=_get) @pytest.fixture(scope='function') @@ -3966,3 +3975,25 @@ def create_notifications( created_by_name='Firstname Lastname', postage=postage ) + + +def create_template( + service_id=SERVICE_ONE_ID, + template_id=None, + template_type='sms', + name='sample template', + content='Template content', + subject='Template subject', + redact_personalisation=False, + postage=None +): + return template_json( + service_id=service_id, + id_=template_id or str(generate_uuid()), + name=name, + type_=template_type, + content=content, + subject=subject, + redact_personalisation=redact_personalisation, + postage=postage, + ) From 1c237847670f929699781fa305b20cd415d17d5b Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 8 Jan 2020 09:29:16 +0000 Subject: [PATCH 22/36] =?UTF-8?q?Pytest=205=20=F0=9F=8E=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also updates the Pytest command in `run_tests.sh` to remove junit, which we stopped using. --- requirements_for_test.txt | 4 ++-- scripts/run_tests.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 2f7e7d9e6..22260cfe2 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,10 +1,10 @@ -r requirements.txt isort==4.3.21 -pytest==3.10.1 # pyup: <4.0.0 +pytest==5.3.2 pytest-env==0.6.2 pytest-cov==2.8.1 pytest-mock==1.11.2 -pytest-xdist==1.27.0 # pyup: <1.28.0 +pytest-xdist==1.31.0 beautifulsoup4==4.8.1 freezegun==0.3.12 flake8==3.7.9 diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index b77f9b66c..4d5afe776 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -37,5 +37,5 @@ npm test display_result $? 3 "Javascript tests have" ## Code coverage -py.test -n auto --maxfail=10 --cov=app --cov-report=term-missing tests/ --junitxml=test_results.xml --strict -p no:warnings +py.test -n auto --maxfail=10 --cov=app --cov-report=term-missing tests/ --strict -p no:warnings display_result $? 4 "Code coverage" From a2220f6929bcbc94ac1e0bc5643bfc9c736b89eb Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 9 Jan 2020 15:56:14 +0000 Subject: [PATCH 23/36] Fix exception in track-error js module that broke our re-upload button --- app/assets/javascripts/errorTracking.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/errorTracking.js b/app/assets/javascripts/errorTracking.js index 3b7053841..7cabb1a26 100644 --- a/app/assets/javascripts/errorTracking.js +++ b/app/assets/javascripts/errorTracking.js @@ -5,7 +5,7 @@ this.start = function(component) { - if (!ga) return; + if (!('ga' in window)) return; ga( 'send', From 17f72dc4cd3e026432e73a115240a521b716443f Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 10 Jan 2020 10:52:40 +0000 Subject: [PATCH 24/36] Delete unused fixtures --- tests/conftest.py | 184 ---------------------------------------------- 1 file changed, 184 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 3305e2dcb..aa06e52a1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -498,15 +498,6 @@ def mock_get_service(mocker, api_user_active): return mocker.patch('app.service_api_client.get_service', side_effect=_get) -@pytest.fixture(scope='function') -def mock_get_international_service(mocker, api_user_active): - def _get(service_id): - service = service_json(service_id, users=[api_user_active['id']], permissions=['sms', 'international_sms']) - return {'data': service} - - return mocker.patch('app.service_api_client.get_service', side_effect=_get) - - @pytest.fixture(scope='function') def mock_get_service_statistics(mocker, api_user_active): def _get(service_id, today_only, limit_days=None): @@ -576,19 +567,6 @@ def mock_get_live_service(mocker, api_user_active): return mocker.patch('app.service_api_client.get_service', side_effect=_get) -@pytest.fixture(scope='function') -def mock_get_service_with_letters(mocker, api_user_active): - def _get(service_id): - return {'data': service_json( - service_id, - users=[api_user_active['id']], - restricted=False, - permissions=['email', 'sms', 'letter'] - )} - - return mocker.patch('app.service_api_client.get_service', side_effect=_get) - - @pytest.fixture(scope='function') def mock_create_service(mocker): def _create( @@ -1267,33 +1245,6 @@ def active_caseworking_user(fake_uuid): return user_data -@pytest.fixture(scope='function') -def active_user_no_mobile(fake_uuid): - user_data = {'id': fake_uuid, - 'name': 'Test User', - 'password': 'somepassword', - 'password_changed_at': str(datetime.utcnow()), - 'email_address': 'test@user.gov.uk', - 'mobile_number': None, - 'state': 'active', - 'failed_login_count': 0, - 'permissions': {SERVICE_ONE_ID: ['send_texts', - 'send_emails', - 'send_letters', - 'manage_users', - 'manage_templates', - 'manage_settings', - 'manage_api_keys', - 'view_activity']}, - 'platform_admin': False, - 'auth_type': 'email_auth', - 'organisations': [], - 'services': [SERVICE_ONE_ID], - 'current_session_id': None, - } - return user_data - - @pytest.fixture def active_user_view_permissions(fake_uuid): user_data = {'id': fake_uuid, @@ -1314,73 +1265,6 @@ def active_user_view_permissions(fake_uuid): return user_data -@pytest.fixture -def active_user_empty_permissions(fake_uuid): - user_data = {'id': fake_uuid, - 'name': 'Test User With Empty Permissions', - 'password': 'somepassword', - 'password_changed_at': str(datetime.utcnow()), - 'email_address': 'test@user.gov.uk', - 'mobile_number': '07700 900763', - 'state': 'active', - 'failed_login_count': 0, - 'permissions': {}, - 'platform_admin': False, - 'auth_type': 'sms_auth', - 'organisations': [], - 'services': [SERVICE_ONE_ID], - 'current_session_id': None, - } - return user_data - - -@pytest.fixture -def active_user_manage_template_permission(fake_uuid): - return { - 'id': fake_uuid, - 'name': 'Test User With Permissions', - 'password': 'somepassword', - 'password_changed_at': str(datetime.utcnow()), - 'email_address': 'test@user.gov.uk', - 'mobile_number': '07700 900762', - 'state': 'active', - 'failed_login_count': 0, - 'permissions': {SERVICE_ONE_ID: [ - 'manage_templates', - 'view_activity', - ]}, - 'platform_admin': False, - 'auth_type': 'sms_auth', - 'organisations': [], - 'services': [SERVICE_ONE_ID], - 'current_session_id': None, - } - - -@pytest.fixture -def active_user_no_api_key_permission(fake_uuid): - return { - 'id': fake_uuid, - 'name': 'Test User With Permissions', - 'password': 'somepassword', - 'password_changed_at': str(datetime.utcnow()), - 'email_address': 'test@user.gov.uk', - 'mobile_number': '07700 900762', - 'state': 'active', - 'failed_login_count': 0, - 'permissions': {SERVICE_ONE_ID: [ - 'manage_templates', - 'manage_settings', - 'view_activity', - ]}, - 'platform_admin': False, - 'auth_type': 'sms_auth', - 'organisations': [], - 'current_session_id': None, - 'services': [SERVICE_ONE_ID], - } - - @pytest.fixture def active_user_no_settings_permission(fake_uuid): return { @@ -1514,12 +1398,6 @@ def mock_get_locked_user(mocker, api_user_locked): 'app.user_api_client.get_user', side_effect=_get_user) -@pytest.fixture(scope='function') -def mock_get_user_locked(mocker, api_user_locked): - return mocker.patch( - 'app.user_api_client.get_user', return_value=api_user_locked) - - @pytest.fixture(scope='function') def mock_get_user_pending(mocker, api_user_pending): return mocker.patch( @@ -1546,16 +1424,6 @@ def mock_get_unknown_user_by_email(mocker, api_user_active): return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) -@pytest.fixture(scope='function') -def mock_get_user_with_permissions(mocker, api_user_active): - def _get_user(id): - api_user_active._permissions[''] = ['manage_users', 'manage_templates', 'manage_settings'] - return api_user_active - - return mocker.patch( - 'app.user_api_client.get_user', side_effect=_get_user) - - @pytest.fixture(scope='function') def mock_dont_get_user_by_email(mocker): def _get_user(email_address): @@ -1588,11 +1456,6 @@ def mock_get_user_by_email_locked(mocker, api_user_locked): 'app.user_api_client.get_user_by_email', return_value=api_user_locked) -@pytest.fixture(scope='function') -def mock_get_user_by_email_inactive(mocker, api_user_pending): - return mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_pending) - - @pytest.fixture(scope='function') def mock_get_user_by_email_pending(mocker, api_user_pending): return mocker.patch( @@ -1650,29 +1513,11 @@ def mock_activate_user(mocker, api_user_active): return mocker.patch('app.user_api_client.activate_user', side_effect=_activate) -@pytest.fixture(scope='function') -def mock_email_is_already_in_use(mocker, api_user_active): - return mocker.patch('app.user_api_client.get_user_by_email_or_none', return_value=api_user_active) - - @pytest.fixture(scope='function') def mock_email_is_not_already_in_use(mocker): return mocker.patch('app.user_api_client.get_user_by_email_or_none', return_value=None) -@pytest.fixture(scope='function') -def mock_get_all_users_from_api(mocker): - return mocker.patch('app.user_api_client.get_users', return_value={'data': []}) - - -@pytest.fixture(scope='function') -def mock_create_api_key(mocker): - def _create(service_id, key_name): - return str(generate_uuid()) - - return mocker.patch('app.api_key_api_client.create_api_key', side_effect=_create) - - @pytest.fixture(scope='function') def mock_revoke_api_key(mocker): def _revoke(service_id, key_id): @@ -2225,11 +2070,6 @@ def sample_invite(mocker, service_one, status='pending', permissions=None): id_, from_user, service_id, email_address, permissions, created_at, status, auth_type, folder_permissions) -@pytest.fixture(scope='function') -def sample_invited_user(mocker, sample_invite): - return sample_invite - - @pytest.fixture(scope='function') def mock_create_invite(mocker, sample_invite): def _create_invite(from_user, service_id, email_address, permissions, folder_permissions): @@ -3076,14 +2916,6 @@ def mock_create_service_inbound_api(mocker): return mocker.patch('app.service_api_client.create_service_inbound_api', side_effect=_create_service_inbound_api) -@pytest.fixture(scope='function') -def mock_delete_service_inbound_api(mocker): - return mocker.patch( - 'app.service_api_client.delete_service_callback_api', - side_effect=lambda service_id: None - ) - - @pytest.fixture(scope='function') def mock_update_service_inbound_api(mocker): def _update_service_inbound_api(service_id, url, bearer_token, user_id, inbound_api_id): @@ -3100,14 +2932,6 @@ def mock_create_service_callback_api(mocker): return mocker.patch('app.service_api_client.create_service_callback_api', side_effect=_create_service_callback_api) -@pytest.fixture(scope='function') -def mock_delete_service_callback_api(mocker): - return mocker.patch( - 'app.service_api_client.delete_service_callback_api', - side_effect=lambda service_id: None - ) - - @pytest.fixture(scope='function') def mock_update_service_callback_api(mocker): def _update_service_callback_api(service_id, url, bearer_token, user_id, callback_api_id): @@ -3350,14 +3174,6 @@ def mock_organisation_name_is_unique(mocker): return mocker.patch('app.organisations_client.is_organisation_name_unique', return_value=True) -@pytest.fixture(scope='function') -def mock_update_organisation_name(mocker): - def _update_org_name(organisation_id, name): - return - - return mocker.patch('app.organisations_client.update_organisation_name', side_effect=_update_org_name) - - @pytest.fixture(scope='function') def mock_update_organisation(mocker): def _update_org(organisation_id, **kwargs): From 8e9b7ae3687dce8ebde299ef9186f97fad323f5f Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 10 Jan 2020 12:17:56 +0000 Subject: [PATCH 25/36] Remove arguments from fixtures These can't be used, since we fixtures can no longer be called as if they were functions, so we can simply the code by removing them. --- tests/app/test_utils.py | 1 - tests/conftest.py | 134 ++++++++++++---------------------------- 2 files changed, 38 insertions(+), 97 deletions(-) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index cb493adeb..e08586403 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -79,7 +79,6 @@ def _get_notifications_csv( def _get_notifications_csv_mock( mocker, api_user_active, - job_id=fake_uuid ): return mocker.patch( 'app.notification_api_client.get_notifications_for_service', diff --git a/tests/conftest.py b/tests/conftest.py index aa06e52a1..295ce6380 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -825,16 +825,16 @@ def mock_get_service_template_with_placeholders_same_as_recipient(mocker): @pytest.fixture(scope='function') -def mock_get_service_email_template(mocker, content=None, subject=None, redact_personalisation=False): +def mock_get_service_email_template(mocker): def _get(service_id, template_id, version=None): template = template_json( service_id, template_id, "Two week reminder", "email", - content or "Your vehicle tax expires on ((date))", - subject or "Your ((thing)) is due soon", - redact_personalisation=redact_personalisation, + "Your vehicle tax expires on ((date))", + "Your ((thing)) is due soon", + redact_personalisation=False, ) return {'data': template} @@ -861,15 +861,15 @@ def mock_get_service_email_template_without_placeholders(mocker): @pytest.fixture(scope='function') -def mock_get_service_letter_template(mocker, content=None, subject=None, postage='second'): - def _get(service_id, template_id, version=None, postage=postage): +def mock_get_service_letter_template(mocker): + def _get(service_id, template_id, version=None, postage='second'): template = template_json( service_id, template_id, "Two week reminder", "letter", - content or "Template content with & entity", - subject or "Subject", + "Template content with & entity", + "Subject", postage=postage, ) return {'data': template} @@ -1108,11 +1108,11 @@ def api_user_active(fake_uuid): @pytest.fixture(scope='function') -def api_user_active_email_auth(fake_uuid, email_address='test@user.gov.uk'): +def api_user_active_email_auth(fake_uuid): user_data = {'id': fake_uuid, 'name': 'Test User', 'password': 'somepassword', - 'email_address': email_address, + 'email_address': 'test@user.gov.uk', 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 0, @@ -1755,14 +1755,6 @@ def mock_get_uploads(mocker, api_user_active): def mock_get_notifications( mocker, api_user_active, - template_content=None, - diff_template_type=None, - personalisation=None, - redact_personalisation=False, - is_precompiled_letter=False, - client_reference=None, - noti_status=None, - postage=None, ): def _get_notifications( service_id, @@ -1782,21 +1774,19 @@ def mock_get_notifications( job = None if job_id is not None: job = job_json(service_id, api_user_active, job_id=job_id) - if diff_template_type or template_type: + if template_type: template = template_json( service_id, id_=str(generate_uuid()), - type_=diff_template_type or template_type[0], - content=template_content, - redact_personalisation=redact_personalisation, - is_precompiled_letter=is_precompiled_letter, + type_=template_type[0], + redact_personalisation=False, + is_precompiled_letter=False, ) else: template = template_json( service_id, id_=str(generate_uuid()), - content=template_content, - redact_personalisation=redact_personalisation, + redact_personalisation=False, ) return notification_json( service_id, @@ -1804,12 +1794,7 @@ def mock_get_notifications( rows=rows, job=job, with_links=True if count_pages is None else count_pages, - personalisation=personalisation, - template_type=diff_template_type, - client_reference=client_reference, - status=noti_status, created_by_name='Firstname Lastname', - postage=postage ) return mocker.patch( @@ -2036,38 +2021,35 @@ def mock_s3_upload(mocker): @pytest.fixture(scope='function') -def mock_s3_download(mocker, content=None): - if not content: - content = """ +def mock_s3_download(mocker): + def _download(service_id, upload_id): + return """ phone number,name +447700900986,John +447700900986,Smith """ - def _download(service_id, upload_id): - return content - return mocker.patch('app.main.views.send.s3download', side_effect=_download) @pytest.fixture(scope='function') -def mock_s3_set_metadata(mocker, content=None): +def mock_s3_set_metadata(mocker): return mocker.patch('app.main.views.send.set_metadata_on_csv_upload') @pytest.fixture(scope='function') -def sample_invite(mocker, service_one, status='pending', permissions=None): +def sample_invite(mocker, service_one): id_ = USER_ONE_ID from_user = service_one['users'][0] email_address = 'invited_user@test.gov.uk' service_id = service_one['id'] - permissions = permissions or 'view_activity,send_messages,manage_service,manage_api_keys' + permissions = 'view_activity,send_messages,manage_service,manage_api_keys' created_at = str(datetime.utcnow()) auth_type = 'sms_auth' folder_permissions = [] return invite_json( - id_, from_user, service_id, email_address, permissions, created_at, status, auth_type, folder_permissions) + id_, from_user, service_id, email_address, permissions, created_at, 'pending', auth_type, folder_permissions) @pytest.fixture(scope='function') @@ -2573,18 +2555,7 @@ def mock_reset_failed_login_count(mocker): @pytest.fixture -def mock_get_notification( - mocker, - notification_id=fake_uuid, - notification_status='delivered', - redact_personalisation=False, - template_type=None, - template_name='sample template', - is_precompiled_letter=False, - key_type=None, - postage=None, - sent_one_off=True, -): +def mock_get_notification(mocker): def _get_notification( service_id, notification_id, @@ -2592,31 +2563,23 @@ def mock_get_notification( noti = notification_json( service_id, rows=1, - status=notification_status, - template_type=template_type, - postage=postage + personalisation={'name': 'Jo'} )['notifications'][0] noti['id'] = notification_id - if sent_one_off: - noti['created_by'] = { - 'id': fake_uuid, - 'name': 'Test User', - 'email_address': 'test@user.gov.uk' - } - noti['personalisation'] = {'name': 'Jo'} + noti['created_by'] = { + 'id': fake_uuid, + 'name': 'Test User', + 'email_address': 'test@user.gov.uk' + } noti['template'] = template_json( service_id, '5407f4db-51c7-4150-8758-35412d42186a', content='hello ((name))', subject='blah', - redact_personalisation=redact_personalisation, - type_=template_type, - is_precompiled_letter=is_precompiled_letter, - name=template_name + redact_personalisation=False, + name='sample template' ) - if key_type: - noti['key_type'] = key_type return noti return mocker.patch( @@ -2981,11 +2944,7 @@ def mock_get_organisations_with_unusual_domains(mocker): @pytest.fixture(scope='function') -def mock_get_organisation( - mocker, - email_branding_id=None, - letter_branding_id=None, -): +def mock_get_organisation(mocker): def _get_organisation(org_id): return organisation_json( org_id, @@ -2994,29 +2953,15 @@ def mock_get_organisation( 'o2': 'Org 2', 'o3': 'Org 3', }.get(org_id, 'Org 1'), - email_branding_id=email_branding_id, - letter_branding_id=letter_branding_id, ) return mocker.patch('app.organisations_client.get_organisation', side_effect=_get_organisation) @pytest.fixture(scope='function') -def mock_get_organisation_by_domain( - mocker, - name=False, - crown=True, - agreement_signed=False, - organisation_type='', -): +def mock_get_organisation_by_domain(mocker): def _get_organisation_by_domain(org_id): - return organisation_json( - org_id, - name, - crown=crown, - agreement_signed=agreement_signed, - organisation_type=organisation_type, - ) + return organisation_json(org_id) return mocker.patch( 'app.organisations_client.get_organisation_by_domain', @@ -3027,18 +2972,14 @@ def mock_get_organisation_by_domain( @pytest.fixture(scope='function') def mock_get_service_organisation( mocker, - name=False, - crown=True, agreement_signed=None, organisation_type=None, ): def _get_service_organisation(service_id): return organisation_json( '7aa5d4e9-4385-4488-a489-07812ba13383', - name, - crown=crown, - agreement_signed=agreement_signed, - organisation_type=organisation_type, + agreement_signed=None, + organisation_type=None, ) return mocker.patch('app.organisations_client.get_service_organisation', side_effect=_get_service_organisation) @@ -3111,12 +3052,13 @@ def mock_get_invited_users_for_organisation(mocker, sample_org_invite): @pytest.fixture(scope='function') -def sample_org_invite(mocker, organisation_one, status='pending'): +def sample_org_invite(mocker, organisation_one): id_ = str(UUID(bytes=b'sample_org_invit', version=4)) invited_by = organisation_one['users'][0] email_address = 'invited_user@test.gov.uk' organisation = organisation_one['id'] created_at = str(datetime.utcnow()) + status = 'pending' return org_invite_json(id_, invited_by, organisation, email_address, created_at, status) From 70276cb5268ea33708de479fa0027d4d70eb3f01 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 10 Jan 2020 14:42:56 +0000 Subject: [PATCH 26/36] Added a new error message when the letter is mising an address block. Before deploying a change to template-preview to return a validation error for letters that are missing the address block, we need to add the new erorr message to admin. Some content changes have been made to other messages. The format of the message has changed. --- .../invalid_precompiled_letter.html | 2 +- .../views/notifications/notification.html | 2 +- app/utils.py | 14 ++++-- tests/app/main/views/test_notifications.py | 45 ++++++++++++++++--- tests/app/test_utils.py | 19 +++++--- 5 files changed, 64 insertions(+), 18 deletions(-) diff --git a/app/templates/views/notifications/invalid_precompiled_letter.html b/app/templates/views/notifications/invalid_precompiled_letter.html index 99530d918..1c2f9af63 100644 --- a/app/templates/views/notifications/invalid_precompiled_letter.html +++ b/app/templates/views/notifications/invalid_precompiled_letter.html @@ -12,6 +12,6 @@ Provided as PDF on {{ created_at|format_datetime_short }}

    - Validation failed – Notify cannot read this PDF file + Validation failed – There’s a problem with your letter.
    Notify cannot read this PDF.

    {% endblock %} diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 12663f3fc..f9ffe85ef 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -44,7 +44,7 @@

    {% elif notification_status == 'validation-failed' %}

    - Validation failed. {{ message.detail | safe }} + Validation failed – {{ message.title | safe }}. {{ message.detail | safe }}

    {% elif notification_status == 'technical-failure' %}

    diff --git a/app/utils.py b/app/utils.py index 3bd002238..b7ce5b69e 100644 --- a/app/utils.py +++ b/app/utils.py @@ -566,14 +566,14 @@ def get_letter_printing_statement(status, created_at): LETTER_VALIDATION_MESSAGES = { 'letter-not-a4-portrait-oriented': { - 'title': 'We cannot print your letter', - 'detail': 'Your letter is not A4 portrait size on {invalid_pages}
    ' + 'title': 'Your letter is not A4 portrait size', + 'detail': 'You need to change the size or orientation of {invalid_pages}.
    ' 'Files must meet our letter specification.' }, 'content-outside-printable-area': { - 'title': 'We cannot print your letter', - 'detail': 'The content appears outside the printable area on {invalid_pages}.
    ' + 'title': 'Your content is outside the printable area', + 'detail': 'You need to edit {invalid_pages}.
    ' 'Files must meet our letter specification.' }, @@ -587,6 +587,12 @@ LETTER_VALIDATION_MESSAGES = { 'unable-to-read-the-file': { 'title': 'There’s a problem with your file', 'detail': 'Notify cannot read this PDF.
    Save a new copy of your file and try again.' + }, + 'address-is-empty': { + 'title': 'The address block is empty', + 'detail': 'You need to add a recipient address.
    ' + 'Files must meet our letter specification.' } } diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 98a634897..9e1c1bcdf 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -304,6 +304,42 @@ def test_notification_page_shows_page_for_letter_sent_with_test_key( assert page.select('p.notification-status') == [] +def test_notification_page_shows_validation_failed_precompiled_letter( + client_request, + mocker, + fake_uuid, +): + notification = create_notification(template_type='letter', + notification_status='validation-failed', + is_precompiled_letter=True + ) + mocker.patch('app.notification_api_client.get_notification', return_value=notification) + metadata = {"page_count": "1", "status": "validation-failed", + "invalid_pages": "[1]", + "message": "content-outside-printable-area"} + mocker.patch('app.main.views.notifications.view_letter_notification_as_preview', + return_value=("some letter content", metadata)) + mocker.patch( + 'app.main.views.notifications.get_page_count_for_letter', + return_value=1, + ) + + page = client_request.get( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + error_message = page.find('p', class_='notification-status-cancelled').text + assert normalize_spaces(error_message) == \ + "Validation failed – Your content is outside the printable area. " \ + "You need to edit page 1.Files must meet our letter specification." + + assert not page.select('p.notification-status') + + assert page.select_one('main img')['src'].endswith('.png?page=1') + + @pytest.mark.parametrize('notification_status, expected_message', ( ( 'permanent-failure', @@ -313,10 +349,6 @@ def test_notification_page_shows_page_for_letter_sent_with_test_key( 'cancelled', 'Cancelled 1 January at 1:02am', ), - ( - 'validation-failed', - 'Validation failed.', - ), ( 'technical-failure', 'Technical failure – Notify will resend once the team have fixed the problem', @@ -538,7 +570,7 @@ def test_should_show_preview_error_image_letter_notification_on_preview_error( assert response.get_data(as_text=True) == 'preview error image' -def test_notifification_page_shows_error_message_if_precompiled_letter_cannot_be_opened( +def test_notification_page_shows_error_message_if_precompiled_letter_cannot_be_opened( client_request, mocker, fake_uuid, @@ -563,7 +595,8 @@ def test_notifification_page_shows_error_message_if_precompiled_letter_cannot_be ) error_message = page.find('p', class_='notification-status-cancelled').text - assert normalize_spaces(error_message) == "Validation failed – Notify cannot read this PDF file" + assert normalize_spaces(error_message) == \ + "Validation failed – There’s a problem with your letter. Notify cannot read this PDF." def test_should_404_for_unknown_extension( diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index cb493adeb..a81032b5b 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -414,14 +414,21 @@ def test_get_letter_validation_error_for_unknown_error(): @pytest.mark.parametrize('error_message, expected_title, expected_content', [ - ('letter-not-a4-portrait-oriented', 'We cannot print your letter', 'A4 portrait size on page 2'), - ('content-outside-printable-area', 'We cannot print your letter', 'outside the printable area on page 2'), - ('letter-too-long', 'Your letter is too long', 'letter is 13 pages long.') + ('letter-not-a4-portrait-oriented', 'Your letter is not A4 portrait size', + 'You need to change the size or orientation of page 2.
    Files must meet our ' + 'letter specification.'), + ('content-outside-printable-area', 'Your content is outside the printable area', + 'You need to edit page 2.
    Files must meet our ' + 'letter specification.'), + ('letter-too-long', 'Your letter is too long', + 'Letters must be 10 pages or less.
    Your letter is 13 pages long.') ]) def test_get_letter_validation_error_for_known_errors( - error_message, - expected_title, - expected_content, + error_message, + expected_title, + expected_content, ): error = get_letter_validation_error(error_message, invalid_pages=[2], page_count=13) From f8e7635a1db1719f7a92d70b3b8390bb18e70f65 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Jan 2020 16:35:54 +0000 Subject: [PATCH 27/36] Show the first line of the address from the to field. Now persisting the address to the "to" field of the Notification, after the notification has been validated. If the letter is pending validation, then "Checking..." will appear as the identifier for the letter. If the letter has passed validation, then the first line of the address (now persisted in the "to" field) will be displayed, with the client reference underneath. If the letter has failed validation the "Provided as PDF" will show be displayed, which is now the initial value of the "to" field. --- app/main/views/jobs.py | 2 +- app/main/views/uploads.py | 25 +++++++++++++++-- app/notify_client/notification_api_client.py | 3 +- app/s3_client/s3_letter_upload_client.py | 21 ++------------ .../views/activity/notifications.html | 4 +-- tests/app/main/views/test_activity.py | 11 ++++++-- tests/app/main/views/test_uploads.py | 22 +++++++++++++-- .../notify_client/test_notification_client.py | 4 ++- .../s3_client/test_s3_letter_upload_client.py | 28 ++++++------------- 9 files changed, 69 insertions(+), 51 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 147078e5c..5b457c3f8 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -513,7 +513,7 @@ def add_preview_of_content_to_notifications(notifications): ) else: if notification['template']['is_precompiled_letter']: - notification['template']['subject'] = 'Provided as PDF' + notification['template']['subject'] = notification['client_reference'] yield dict( preview_of_content=( WithSubjectTemplate( diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 84cac95ad..8be0aa4dc 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -1,5 +1,6 @@ import base64 import json +import urllib import uuid from io import BytesIO @@ -168,6 +169,25 @@ def _get_error_from_upload_form(form_errors): return error +def format_recipient(address): + ''' + To format the recipient we need to: + - decode, address is url encoded + - remove new line characters + - remove whitespace around the lines + - join the address lines, separated by a comma + ''' + if not address: + return address + address = urllib.parse.unquote(address) + stripped_address_lines_no_trailing_commas = [ + line.lstrip().rstrip(' ,') + for line in address.splitlines() if line + ] + one_line_address = ', '.join(stripped_address_lines_no_trailing_commas) + return one_line_address + + @main.route("/services//preview-letter/") @user_has_permissions('send_messages') def uploaded_letter_preview(service_id, file_id): @@ -179,7 +199,7 @@ def uploaded_letter_preview(service_id, file_id): status = metadata.get('status') error_shortcode = metadata.get('message') invalid_pages = metadata.get('invalid_pages') - recipient = metadata.get('recipient') + recipient = format_recipient(metadata.get('recipient')) if invalid_pages: invalid_pages = json.loads(invalid_pages) @@ -246,11 +266,12 @@ def send_uploaded_letter(service_id): postage = form.postage.data metadata = get_letter_metadata(service_id, file_id) filename = metadata.get('filename') + recipient_address = metadata.get('recipient') if metadata.get('status') != 'valid': abort(403) - notification_api_client.send_precompiled_letter(service_id, filename, file_id, postage) + notification_api_client.send_precompiled_letter(service_id, filename, file_id, postage, recipient_address) return redirect(url_for( '.view_notification', diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index a6982b553..87898fe55 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -59,11 +59,12 @@ class NotificationApiClient(NotifyAdminAPIClient): data = _attach_current_user(data) return self.post(url='/service/{}/send-notification'.format(service_id), data=data) - def send_precompiled_letter(self, service_id, filename, file_id, postage): + def send_precompiled_letter(self, service_id, filename, file_id, postage, recipient_address): data = { 'filename': filename, 'file_id': file_id, 'postage': postage, + 'recipient_address': recipient_address } data = _attach_current_user(data) return self.post(url='/service/{}/send-pdf-letter'.format(service_id), data=data) diff --git a/app/s3_client/s3_letter_upload_client.py b/app/s3_client/s3_letter_upload_client.py index adff787e9..bf263fb6f 100644 --- a/app/s3_client/s3_letter_upload_client.py +++ b/app/s3_client/s3_letter_upload_client.py @@ -1,9 +1,9 @@ import json +import urllib from boto3 import resource from flask import current_app from notifications_utils.s3 import s3upload as utils_s3upload -from notifications_utils.sanitise_text import SanitiseASCII def get_transient_letter_file_location(service_id, upload_id): @@ -31,7 +31,7 @@ def upload_letter_to_s3( if invalid_pages: metadata['invalid_pages'] = json.dumps(invalid_pages) if recipient: - metadata['recipient'] = format_recipient(recipient) + metadata['recipient'] = urllib.parse.quote(recipient) utils_s3upload( filedata=data, @@ -59,20 +59,3 @@ def get_letter_metadata(service_id, file_id): s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() return s3_object['Metadata'] - - -def format_recipient(address): - ''' - To format the recipient we need to: - - remove new line characters - - remove whitespace around the lines - - join the address lines, separated by a comma - - convert the string to ASCII (S3 metadata must be stored as ASCII) - ''' - stripped_address_lines_no_trailing_commas = [ - line.lstrip().rstrip(' ,') - for line in address.splitlines() if line - ] - one_line_address = ', '.join(stripped_address_lines_no_trailing_commas) - - return SanitiseASCII.encode(one_line_address) diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index d2b8e3465..09ab70657 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -17,9 +17,9 @@ ) %} {% call row_heading() %} {% if item.status in ('pending-virus-check', 'virus-scan-failed') %} - {{ item.to }} + Checking {% else %} - {{ item.to }} + {{ item.to.split('\n')[0] }} {% endif %}

    {{ item.preview_of_content }} diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index b2c7fe550..5781148db 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -265,15 +265,19 @@ def test_download_not_available_to_users_without_dashboard( def test_letters_with_status_virus_scan_failed_shows_a_failure_description( mocker, + active_user_with_permissions, client_request, service_one, mock_get_service_statistics, mock_get_service_data_retention, mock_get_api_keys, ): - notifications = create_notifications(template_type='letter', status='virus-scan-failed', is_precompiled_letter=True) - mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) - + mock_get_notifications( + mocker, + active_user_with_permissions, + is_precompiled_letter=True, + noti_status='virus-scan-failed' + ) page = client_request.get( 'main.view_notifications', service_id=service_one['id'], @@ -563,6 +567,7 @@ def test_html_contains_notification_id( def test_html_contains_links_for_failed_notifications( client_request, + active_user_with_permissions, mock_get_service_statistics, mock_get_service_data_retention, mock_get_no_api_keys, diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index da54ac3dd..490adb1b3 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -1,9 +1,11 @@ +import urllib from unittest.mock import Mock import pytest from flask import make_response, url_for from requests import RequestException +from app.main.views.uploads import format_recipient from app.utils import normalize_spaces from tests.conftest import SERVICE_ONE_ID @@ -350,8 +352,11 @@ def test_uploaded_letter_preview( fake_uuid, ): mocker.patch('app.main.views.uploads.service_api_client') + recipient = 'Bugs Bunny\n123 Big Hole\rLooney Town' mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ - 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid'}) + 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid', + 'recipient': urllib.parse.quote(recipient) + }) service_one['restricted'] = False client_request.login(active_user_with_permissions, service=service_one) @@ -455,7 +460,7 @@ def test_uploaded_letter_preview_image_does_not_show_overlay_if_no_content_outsi def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mocker, service_one, client_request): - metadata = {'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid'} + metadata = {'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'address'} mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', metadata)) mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter') @@ -475,7 +480,7 @@ def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mo _external=True ) ) - mock_send.assert_called_once_with(SERVICE_ONE_ID, 'my_file.pdf', file_id, 'first') + mock_send.assert_called_once_with(SERVICE_ONE_ID, 'my_file.pdf', file_id, 'first', 'address') @pytest.mark.parametrize('permissions', [ @@ -522,3 +527,14 @@ def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, servic _expected_status=403 ) assert not mock_send.called + + +@pytest.mark.parametrize('original_address,expected_address', [ + ('The Queen, Buckingham Palace, SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), + ('The Queen Buckingham Palace SW1 1AA', 'The Queen Buckingham Palace SW1 1AA'), + ('The Queen,\nBuckingham Palace,\r\nSW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), + ('The Queen ,,\nBuckingham Palace,\rSW1 1AA,', 'The Queen, Buckingham Palace, SW1 1AA'), + (' The Queen\n Buckingham Palace\n SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), +]) +def test_format_recipient(original_address, expected_address): + assert format_recipient(urllib.parse.quote(original_address)) == expected_address diff --git a/tests/app/notify_client/test_notification_client.py b/tests/app/notify_client/test_notification_client.py index 5df1a97b1..5741fe219 100644 --- a/tests/app/notify_client/test_notification_client.py +++ b/tests/app/notify_client/test_notification_client.py @@ -65,7 +65,8 @@ def test_send_precompiled_letter(mocker, logged_in_client, active_user_with_perm 'abcd-1234', 'my_file.pdf', 'file-ID', - 'second' + 'second', + 'Bugs Bunny, 12 Hole Avenue, Looney Town' ) mock_post.assert_called_once_with( url='/service/abcd-1234/send-pdf-letter', @@ -74,6 +75,7 @@ def test_send_precompiled_letter(mocker, logged_in_client, active_user_with_perm 'file_id': 'file-ID', 'created_by': active_user_with_permissions['id'], 'postage': 'second', + 'recipient_address': 'Bugs Bunny, 12 Hole Avenue, Looney Town', } ) diff --git a/tests/app/s3_client/test_s3_letter_upload_client.py b/tests/app/s3_client/test_s3_letter_upload_client.py index 0c65dcc77..e03aae794 100644 --- a/tests/app/s3_client/test_s3_letter_upload_client.py +++ b/tests/app/s3_client/test_s3_letter_upload_client.py @@ -1,27 +1,29 @@ -import pytest +import urllib + from flask import current_app -from app.s3_client.s3_letter_upload_client import ( - format_recipient, - upload_letter_to_s3, -) +from app.s3_client.s3_letter_upload_client import upload_letter_to_s3 def test_upload_letter_to_s3(mocker): s3_mock = mocker.patch('app.s3_client.s3_letter_upload_client.utils_s3upload') + recipient = 'Bugs Bunny\n123 Big Hole\nLooney Town' upload_letter_to_s3( 'pdf_data', file_location='service_id/upload_id.pdf', status='valid', page_count=3, - filename='my_doc') + filename='my_doc', + recipient=recipient + ) s3_mock.assert_called_once_with( bucket_name=current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location='service_id/upload_id.pdf', filedata='pdf_data', - metadata={'status': 'valid', 'page_count': '3', 'filename': 'my_doc'}, + metadata={'status': 'valid', 'page_count': '3', 'filename': 'my_doc', + 'recipient': urllib.parse.quote(recipient)}, region=current_app.config['AWS_REGION'] ) @@ -51,15 +53,3 @@ def test_upload_letter_to_s3_with_message_and_invalid_pages(mocker): }, region=current_app.config['AWS_REGION'] ) - - -@pytest.mark.parametrize('original_address,expected_address', [ - ('The Queen, Buckingham Palace, SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), - ('The Queen Buckingham Palace SW1 1AA', 'The Queen Buckingham Palace SW1 1AA'), - ('The Queen,\nBuckingham Palace,\r\nSW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), - ('The Queen ,,\nBuckingham Palace,\rSW1 1AA,', 'The Queen, Buckingham Palace, SW1 1AA'), - (' The Queen\n Buckingham Palace\n SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), - ("The ’Queen\n Buckingham Palace\n SW1 1AA", "The 'Queen, Buckingham Palace, SW1 1AA"), -]) -def test_format_recipient(original_address, expected_address): - assert format_recipient(original_address) == expected_address From 57b4fa039745825d6a26a6fb3ef6850518afd63e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 6 Jan 2020 15:37:53 +0000 Subject: [PATCH 28/36] Use splitlines instead of split --- app/templates/views/activity/notifications.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index 09ab70657..0107ff815 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -19,7 +19,7 @@ {% if item.status in ('pending-virus-check', 'virus-scan-failed') %} Checking {% else %} - {{ item.to.split('\n')[0] }} + {{ item.to.splitlines()[0] }} {% endif %}

    {{ item.preview_of_content }} From f7b3f9b06a19fd7e4c7bc97fd8009e071a6e00f9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 7 Jan 2020 15:40:17 +0000 Subject: [PATCH 29/36] - Remove the trailing comma from the first line of the "to" field - Add client_reference to the activity CSV report - Show the recipient on the CSV report the same way as the view activity page. --- app/templates/views/activity/notifications.html | 2 +- app/utils.py | 6 ++++-- tests/app/test_utils.py | 11 ++++++----- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index 0107ff815..cb780207e 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -19,7 +19,7 @@ {% if item.status in ('pending-virus-check', 'virus-scan-failed') %} Checking {% else %} - {{ item.to.splitlines()[0] }} + {{ item.to.splitlines()[0].lstrip().rstrip(' ,') }} {% endif %}

    {{ item.preview_of_content }} diff --git a/app/utils.py b/app/utils.py index b7ce5b69e..af2537a05 100644 --- a/app/utils.py +++ b/app/utils.py @@ -165,7 +165,7 @@ def generate_notifications_csv(**kwargs): original_column_headers = original_upload.column_headers fieldnames = ['Row number'] + original_column_headers + ['Template', 'Type', 'Job', 'Status', 'Time'] else: - fieldnames = ['Recipient', 'Template', 'Type', 'Sent by', 'Sent by email', 'Job', 'Status', 'Time'] + fieldnames = ['Recipient', 'Reference', 'Template', 'Type', 'Sent by', 'Sent by email', 'Job', 'Status', 'Time'] yield ','.join(fieldnames) + '\n' @@ -187,7 +187,9 @@ def generate_notifications_csv(**kwargs): ] else: values = [ - notification['recipient'], + # the recipient for precompiled letters is the full address block + notification['recipient'].splitlines()[0].lstrip().rstrip(' ,'), + notification['client_reference'], notification['template_name'], notification['template_type'], notification['created_by_name'] or '', diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index fea2d3121..8a62252d4 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -55,6 +55,7 @@ def _get_notifications_csv( "row_number": row_number + i, "to": recipient, "recipient": recipient, + "client_reference": 'ref 1234', "template_name": template_name, "template_type": template_type, "template": {"name": template_name, "template_type": template_type}, @@ -160,14 +161,14 @@ def test_spreadsheet_checks_for_bad_arguments(args, kwargs): @pytest.mark.parametrize('created_by_name, expected_content', [ ( None, [ - 'Recipient,Template,Type,Sent by,Sent by email,Job,Status,Time\n', - 'foo@bar.com,foo,sms,,sender@email.gov.uk,,Delivered,1943-04-19 12:00:00\r\n', + 'Recipient,Reference,Template,Type,Sent by,Sent by email,Job,Status,Time\n', + 'foo@bar.com,ref 1234,foo,sms,,sender@email.gov.uk,,Delivered,1943-04-19 12:00:00\r\n', ] ), ( 'Anne Example', [ - 'Recipient,Template,Type,Sent by,Sent by email,Job,Status,Time\n', - 'foo@bar.com,foo,sms,Anne Example,sender@email.gov.uk,,Delivered,1943-04-19 12:00:00\r\n', + 'Recipient,Reference,Template,Type,Sent by,Sent by email,Job,Status,Time\n', + 'foo@bar.com,ref 1234,foo,sms,Anne Example,sender@email.gov.uk,,Delivered,1943-04-19 12:00:00\r\n', ] ), ]) @@ -183,7 +184,7 @@ def test_generate_notifications_csv_without_job( created_by_name=created_by_name, created_by_email_address="sender@email.gov.uk", job_id=None, - job_name=None, + job_name=None ) ) assert list(generate_notifications_csv(service_id=fake_uuid)) == expected_content From 9d1f92a75210f44cdd7b08b06a9ccf5930800079 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 8 Jan 2020 11:22:14 +0000 Subject: [PATCH 30/36] Strip the trailing comma on the first line of an address block. --- app/main/views/uploads.py | 2 +- .../views/activity/notifications.html | 2 +- tests/app/main/views/test_activity.py | 91 +++++++++++-------- tests/app/main/views/test_uploads.py | 1 + tests/conftest.py | 9 ++ 5 files changed, 65 insertions(+), 40 deletions(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 8be0aa4dc..fcbd64ef0 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -199,7 +199,7 @@ def uploaded_letter_preview(service_id, file_id): status = metadata.get('status') error_shortcode = metadata.get('message') invalid_pages = metadata.get('invalid_pages') - recipient = format_recipient(metadata.get('recipient')) + recipient = format_recipient(metadata.get('recipient', '')) if invalid_pages: invalid_pages = json.loads(invalid_pages) diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index cb780207e..97e24a301 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -19,7 +19,7 @@ {% if item.status in ('pending-virus-check', 'virus-scan-failed') %} Checking {% else %} - {{ item.to.splitlines()[0].lstrip().rstrip(' ,') }} + {{ item.to.splitlines()[0].lstrip().rstrip(' ,') if item.to else '' }} {% endif %}

    {{ item.preview_of_content }} diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index 5781148db..512c9f986 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -276,7 +276,8 @@ def test_letters_with_status_virus_scan_failed_shows_a_failure_description( mocker, active_user_with_permissions, is_precompiled_letter=True, - noti_status='virus-scan-failed' + noti_status='virus-scan-failed', + client_reference='client reference' ) page = client_request.get( 'main.view_notifications', @@ -294,6 +295,7 @@ def test_letters_with_status_virus_scan_failed_shows_a_failure_description( ]) def test_should_not_show_preview_link_for_precompiled_letters_in_virus_states( mocker, + active_user_with_permissions, client_request, service_one, mock_get_service_statistics, @@ -301,9 +303,13 @@ def test_should_not_show_preview_link_for_precompiled_letters_in_virus_states( mock_get_no_api_keys, letter_status, ): - notifications = create_notifications(template_type='letter', status=letter_status) - mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) - + mock_get_notifications( + mocker, + active_user_with_permissions, + is_precompiled_letter=True, + noti_status=letter_status, + client_reference='ref' + ) page = client_request.get( 'main.view_notifications', service_id=service_one['id'], @@ -414,12 +420,43 @@ def test_search_recipient_form( assert field['value'] == expected_search_box_contents -@pytest.mark.parametrize('message_type, expected_search_box_label', [ - (None, 'Search by email address, phone number or reference'), - ('sms', 'Search by phone number or reference'), - ('email', 'Search by email address or reference'), +@pytest.mark.parametrize(( + 'message_type,' + 'api_keys_mock,' + 'expected_search_box_label,' +), [ + ( + None, + mock_get_no_api_keys, + 'Search by email address or phone number', + ), + ( + None, + mock_get_api_keys, + 'Search by email address, phone number or reference', + ), + ( + 'sms', + mock_get_no_api_keys, + 'Search by phone number', + ), + ( + 'sms', + mock_get_api_keys, + 'Search by phone number or reference', + ), + ( + 'email', + mock_get_no_api_keys, + 'Search by email address', + ), + ( + 'email', + mock_get_api_keys, + 'Search by email address or reference', + ), ]) -def test_api_users_are_told_they_can_search_by_reference_when_service_has_api_keys( +def test_api_users_are_told_they_can_search_by_reference( client_request, mocker, fake_uuid, @@ -428,32 +465,9 @@ def test_api_users_are_told_they_can_search_by_reference_when_service_has_api_ke mock_get_service_data_retention, message_type, expected_search_box_label, - mock_get_api_keys, -): - page = client_request.get( - 'main.view_notifications', - service_id=SERVICE_ONE_ID, - message_type=message_type, - ) - assert page.select_one('label[for=to]').text.strip() == expected_search_box_label - - -@pytest.mark.parametrize('message_type, expected_search_box_label', [ - (None, 'Search by email address or phone number'), - ('sms', 'Search by phone number'), - ('email', 'Search by email address'), -]) -def test_api_users_are_not_told_they_can_search_by_reference_when_service_has_no_api_keys( - client_request, - mocker, - fake_uuid, - mock_get_notifications, - mock_get_service_statistics, - mock_get_service_data_retention, - message_type, - expected_search_box_label, - mock_get_no_api_keys, + api_keys_mock, ): + api_keys_mock(mocker, fake_uuid) page = client_request.get( 'main.view_notifications', service_id=SERVICE_ONE_ID, @@ -697,11 +711,11 @@ def test_sending_status_hint_displays_correctly_on_notifications_page( assert bool(page.select('.align-with-message-body')) is single_line -@pytest.mark.parametrize("is_precompiled_letter,expected_hint", [ - (True, "Provided as PDF"), - (False, "template subject") +@pytest.mark.parametrize("is_precompiled_letter,expected_address,expected_hint", [ + (True, "Full Name,\nFirst address line\npostcode", "ref"), + (False, "Full Name,\nFirst address line\npostcode", "template subject") ]) -def test_should_expected_hint_for_letters( +def test_should_show_address_and_hint_for_letters( client_request, service_one, mock_get_service_statistics, @@ -724,4 +738,5 @@ def test_should_expected_hint_for_letters( message_type='letter', ) + assert page.select_one('a.file-list-filename').text == 'Full Name' assert page.find('p', {'class': 'file-list-hint'}).text.strip() == expected_hint diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index 490adb1b3..59f84f829 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -535,6 +535,7 @@ def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, servic ('The Queen,\nBuckingham Palace,\r\nSW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), ('The Queen ,,\nBuckingham Palace,\rSW1 1AA,', 'The Queen, Buckingham Palace, SW1 1AA'), (' The Queen\n Buckingham Palace\n SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), + ('', ''), ]) def test_format_recipient(original_address, expected_address): assert format_recipient(urllib.parse.quote(original_address)) == expected_address diff --git a/tests/conftest.py b/tests/conftest.py index 295ce6380..25a9dc821 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1791,6 +1791,7 @@ def mock_get_notifications( return notification_json( service_id, template=template, + to=to, rows=rows, job=job, with_links=True if count_pages is None else count_pages, @@ -2879,6 +2880,14 @@ def mock_create_service_inbound_api(mocker): return mocker.patch('app.service_api_client.create_service_inbound_api', side_effect=_create_service_inbound_api) +@pytest.fixture(scope='function') +def mock_delete_service_inbound_api(mocker): + return mocker.patch( + 'app.service_api_client.delete_service_callback_api', + side_effect=lambda service_id: None + ) + + @pytest.fixture(scope='function') def mock_update_service_inbound_api(mocker): def _update_service_inbound_api(service_id, url, bearer_token, user_id, inbound_api_id): From 40575bbb7569762ffbf35a45971648b1075364e0 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 10 Jan 2020 15:42:46 +0000 Subject: [PATCH 31/36] Rebase pytest 5 changes Add to field in creation of notification --- tests/app/main/views/test_activity.py | 96 ++++++++++++--------------- tests/conftest.py | 4 +- 2 files changed, 44 insertions(+), 56 deletions(-) diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index 512c9f986..8ed04e75c 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -265,20 +265,16 @@ def test_download_not_available_to_users_without_dashboard( def test_letters_with_status_virus_scan_failed_shows_a_failure_description( mocker, - active_user_with_permissions, client_request, service_one, mock_get_service_statistics, mock_get_service_data_retention, mock_get_api_keys, ): - mock_get_notifications( - mocker, - active_user_with_permissions, - is_precompiled_letter=True, - noti_status='virus-scan-failed', - client_reference='client reference' - ) + notifications = create_notifications(template_type='letter', status='virus-scan-failed', is_precompiled_letter=True, + client_reference='client reference') + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) + page = client_request.get( 'main.view_notifications', service_id=service_one['id'], @@ -295,7 +291,6 @@ def test_letters_with_status_virus_scan_failed_shows_a_failure_description( ]) def test_should_not_show_preview_link_for_precompiled_letters_in_virus_states( mocker, - active_user_with_permissions, client_request, service_one, mock_get_service_statistics, @@ -303,13 +298,10 @@ def test_should_not_show_preview_link_for_precompiled_letters_in_virus_states( mock_get_no_api_keys, letter_status, ): - mock_get_notifications( - mocker, - active_user_with_permissions, - is_precompiled_letter=True, - noti_status=letter_status, - client_reference='ref' - ) + notifications = create_notifications(template_type='letter', status=letter_status, + is_precompiled_letter=True, client_reference='ref') + mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) + page = client_request.get( 'main.view_notifications', service_id=service_one['id'], @@ -420,43 +412,12 @@ def test_search_recipient_form( assert field['value'] == expected_search_box_contents -@pytest.mark.parametrize(( - 'message_type,' - 'api_keys_mock,' - 'expected_search_box_label,' -), [ - ( - None, - mock_get_no_api_keys, - 'Search by email address or phone number', - ), - ( - None, - mock_get_api_keys, - 'Search by email address, phone number or reference', - ), - ( - 'sms', - mock_get_no_api_keys, - 'Search by phone number', - ), - ( - 'sms', - mock_get_api_keys, - 'Search by phone number or reference', - ), - ( - 'email', - mock_get_no_api_keys, - 'Search by email address', - ), - ( - 'email', - mock_get_api_keys, - 'Search by email address or reference', - ), +@pytest.mark.parametrize('message_type, expected_search_box_label', [ + (None, 'Search by email address, phone number or reference'), + ('sms', 'Search by phone number or reference'), + ('email', 'Search by email address or reference'), ]) -def test_api_users_are_told_they_can_search_by_reference( +def test_api_users_are_told_they_can_search_by_reference_when_service_has_api_keys( client_request, mocker, fake_uuid, @@ -465,9 +426,32 @@ def test_api_users_are_told_they_can_search_by_reference( mock_get_service_data_retention, message_type, expected_search_box_label, - api_keys_mock, + mock_get_api_keys, +): + page = client_request.get( + 'main.view_notifications', + service_id=SERVICE_ONE_ID, + message_type=message_type, + ) + assert page.select_one('label[for=to]').text.strip() == expected_search_box_label + + +@pytest.mark.parametrize('message_type, expected_search_box_label', [ + (None, 'Search by email address or phone number'), + ('sms', 'Search by phone number'), + ('email', 'Search by email address'), +]) +def test_api_users_are_not_told_they_can_search_by_reference_when_service_has_no_api_keys( + client_request, + mocker, + fake_uuid, + mock_get_notifications, + mock_get_service_statistics, + mock_get_service_data_retention, + message_type, + expected_search_box_label, + mock_get_no_api_keys, ): - api_keys_mock(mocker, fake_uuid) page = client_request.get( 'main.view_notifications', service_id=SERVICE_ONE_ID, @@ -581,7 +565,6 @@ def test_html_contains_notification_id( def test_html_contains_links_for_failed_notifications( client_request, - active_user_with_permissions, mock_get_service_statistics, mock_get_service_data_retention, mock_get_no_api_keys, @@ -723,12 +706,15 @@ def test_should_show_address_and_hint_for_letters( mock_get_no_api_keys, mocker, is_precompiled_letter, + expected_address, expected_hint ): notifications = create_notifications( template_type='letter', subject=expected_hint, is_precompiled_letter=is_precompiled_letter, + client_reference=expected_hint, + to=expected_address ) mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) diff --git a/tests/conftest.py b/tests/conftest.py index 25a9dc821..cc5e1b3f8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3720,6 +3720,7 @@ def create_notifications( redact_personalisation=False, is_precompiled_letter=False, postage=None, + to=None ): template = template_json( service_id, @@ -3740,7 +3741,8 @@ def create_notifications( client_reference=client_reference, status=status, created_by_name='Firstname Lastname', - postage=postage + postage=postage, + to=to ) From 8067634447b54981fdad6140737e9f3c220b7b2f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 10 Jan 2020 16:16:11 +0000 Subject: [PATCH 32/36] Fix merge conflict --- tests/conftest.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index cc5e1b3f8..0d31001ac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1791,7 +1791,6 @@ def mock_get_notifications( return notification_json( service_id, template=template, - to=to, rows=rows, job=job, with_links=True if count_pages is None else count_pages, From 0fc1ae63e9bd2003abb174664b757d2aad05af9e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 13 Jan 2020 10:26:11 +0000 Subject: [PATCH 33/36] Add a command to fix import order We check import order as part of our automated tests. But fixing them means: - manually editing them and rechecking - remembering the parameters to `isort` - looking up the `isort` command from the last time you ran it Putting it in the Makefile should make life a bit easier. --- Makefile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Makefile b/Makefile index 0ef85bbdc..17cfe64fa 100644 --- a/Makefile +++ b/Makefile @@ -96,6 +96,10 @@ upload-static: test: ## Run tests ./scripts/run_tests.sh +.PHONY: fix-imports +fix-imports: + isort -rc ./app ./tests + .PHONY: freeze-requirements freeze-requirements: rm -rf venv-freeze From 68bf013a0c92b39836088e387dd8253fc1082e7c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 13 Jan 2020 14:19:20 +0000 Subject: [PATCH 34/36] Removed method from conftest, that's not being used. --- tests/conftest.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 0d31001ac..67b1170c5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2879,14 +2879,6 @@ def mock_create_service_inbound_api(mocker): return mocker.patch('app.service_api_client.create_service_inbound_api', side_effect=_create_service_inbound_api) -@pytest.fixture(scope='function') -def mock_delete_service_inbound_api(mocker): - return mocker.patch( - 'app.service_api_client.delete_service_callback_api', - side_effect=lambda service_id: None - ) - - @pytest.fixture(scope='function') def mock_update_service_inbound_api(mocker): def _update_service_inbound_api(service_id, url, bearer_token, user_id, inbound_api_id): From 3dab5ca73e858d35aebe6b20a39051bfddb0655d Mon Sep 17 00:00:00 2001 From: karlchillmaid Date: Tue, 14 Jan 2020 10:38:34 +0000 Subject: [PATCH 35/36] Update list of organisations that can use Notify Update list of organisations that can use Notify to remove 'housing associations' and rephrase 'companies owned by local or central government that deliver services on their behalf'. --- app/templates/views/get-started.html | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/templates/views/get-started.html b/app/templates/views/get-started.html index 0d20b17b9..415fdb700 100644 --- a/app/templates/views/get-started.html +++ b/app/templates/views/get-started.html @@ -24,9 +24,8 @@

  • central government departments
  • local authorities
  • state-funded schools
    • -
  • housing associations
  • the NHS
    • -
  • companies owned by local or central government that deliver services on their behalf
    • +
  • companies running a service on behalf of a public sector organisation

Notify is not currently available to charities.

''' From d0ee3c45b710b453d644d1f03996297267d50888 Mon Sep 17 00:00:00 2001 From: karlchillmaid Date: Tue, 14 Jan 2020 10:45:10 +0000 Subject: [PATCH 36/36] Move 'performance' to the Support column --- app/templates/admin_template.html | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/templates/admin_template.html b/app/templates/admin_template.html index 896dc6efd..59fcc33a1 100644 --- a/app/templates/admin_template.html +++ b/app/templates/admin_template.html @@ -163,6 +163,10 @@ "href": "https://status.notifications.service.gov.uk", "text": "System status" }, + { + "href": "https://www.gov.uk/performance/govuk-notify", + "text": "Performance data" + }, { "href": "https://ukgovernmentdigital.slack.com/messages/C0E1ADVPC", "text": "Chat to us on Slack" @@ -189,10 +193,6 @@ "href": url_for("main.terms"), "text": "Terms of use" }, - { - "href": "https://www.gov.uk/performance/govuk-notify", - "text": "Performance" - }, { "href": "https://gds.blog.gov.uk/category/notify/", "text": "Blog"