From 7897ae70ceedd5853f60fa4d67f5241ae31a7ce1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 21 Apr 2020 14:19:41 +0100 Subject: [PATCH 1/4] Let users search for letters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Like we have search by email address or phone number, finding an individual letter is a common task. At the moment users are having to click through pages and pages of letters to find the one they’re looking for. We have to search in the `to` and `normalised_to` fields for now because we’re not populating the `normalised_to` column for letters at the moment. --- app/dao/notifications_dao.py | 7 ++-- .../notification_dao/test_notification_dao.py | 35 +++++++++++++++++-- tests/app/service/test_rest.py | 24 +++++++++---- 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d959ecb13..a06174f49 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -26,7 +26,6 @@ from werkzeug.datastructures import MultiDict from app import db, create_uuid from app.aws.s3 import remove_s3_object, get_s3_bucket_objects from app.dao.dao_utils import transactional -from app.errors import InvalidRequest from app.letters.utils import get_letter_pdf_filename from app.models import ( FactNotificationStatus, @@ -600,11 +599,8 @@ def dao_get_notifications_by_recipient_or_reference(service_id, search_term, not except InvalidEmailError: normalised = search_term.lower() - elif notification_type == LETTER_TYPE: - raise InvalidRequest("Only email and SMS can use search by recipient", 400) - else: - normalised = search_term.lower() + normalised = ''.join(search_term.split()).lower() normalised = escape_special_characters(normalised) search_term = escape_special_characters(search_term) @@ -612,6 +608,7 @@ def dao_get_notifications_by_recipient_or_reference(service_id, search_term, not filters = [ Notification.service_id == service_id, or_( + Notification.to.ilike("%{}%".format(search_term)), Notification.normalised_to.like("%{}%".format(normalised)), Notification.client_reference.ilike("%{}%".format(search_term)), ), diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 3a9ddf75f..1f3290913 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1293,6 +1293,7 @@ def test_dao_get_notifications_by_reference( service = create_service() sms_template = create_template(service=service) email_template = create_template(service=service, template_type='email') + letter_template = create_template(service=service, template_type='letter') sms = create_notification( template=sms_template, to_field='07711111111', @@ -1305,15 +1306,28 @@ def test_dao_get_notifications_by_reference( normalised_to='077@example.com', client_reference='77bB', ) + letter = create_notification( + template=letter_template, + to_field='123 Example Street\nXX1X 1XX', + normalised_to='123examplestreetxx1x1xx', + client_reference='77bB', + ) results = dao_get_notifications_by_recipient_or_reference(service.id, '77') + assert len(results) == 3 + assert results[0].id == letter.id + assert results[1].id == email.id + assert results[2].id == sms.id + + # If notification_type isn’t specified then we can’t normalise the phone number + # to 4477… but this query will still find the 077… variant in the `to` field, + # as well as the email + results = dao_get_notifications_by_recipient_or_reference(service.id, '077') assert len(results) == 2 assert results[0].id == email.id assert results[1].id == sms.id - # If notification_type isn’t specified then we can’t normalise the phone number - # to 4477… so this query will only find the email sent to 077@example.com - results = dao_get_notifications_by_recipient_or_reference(service.id, '077') + results = dao_get_notifications_by_recipient_or_reference(service.id, '077@') assert len(results) == 1 assert results[0].id == email.id @@ -1343,6 +1357,21 @@ def test_dao_get_notifications_by_reference( results = dao_get_notifications_by_recipient_or_reference(service.id, 'aA', notification_type='email') assert len(results) == 0 + results = dao_get_notifications_by_recipient_or_reference(service.id, 'aA', notification_type='letter') + assert len(results) == 0 + + results = dao_get_notifications_by_recipient_or_reference(service.id, '123') + assert len(results) == 1 + assert results[0].id == letter.id + + results = dao_get_notifications_by_recipient_or_reference(service.id, 'xX 1x1 Xx') + assert len(results) == 1 + assert results[0].id == letter.id + + results = dao_get_notifications_by_recipient_or_reference(service.id, '77', notification_type='letter') + assert len(results) == 1 + assert results[0].id == letter.id + def test_dao_created_scheduled_notification(sample_notification): diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 403d5d6fd..eb5788c03 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2103,16 +2103,28 @@ def test_search_for_notification_by_to_field_return_multiple_matches(client, sam assert str(notification4.id) not in notification_ids -def test_search_for_notification_by_to_field_return_400_for_letter_type( - client, notify_db, notify_db_session, sample_service +def test_search_for_notification_by_to_field_for_letter( + client, + notify_db, + notify_db_session, + sample_letter_template, + sample_email_template, + sample_template, ): + letter_notification = create_notification(sample_letter_template, to_field='A. Name', normalised_to='a.name') + create_notification(sample_email_template, to_field='A.Name@example.com', normalised_to='a.name@example.com') + create_notification(sample_template, to_field='44770900123', normalised_to='44770900123') response = client.get( - '/service/{}/notifications?to={}&template_type={}'.format(sample_service.id, 'A. Name', 'letter'), + '/service/{}/notifications?to={}&template_type={}'.format( + sample_letter_template.service_id, 'A. Name', 'letter', + ), headers=[create_authorization_header()] ) - response.status_code = 400 - error_message = json.loads(response.get_data(as_text=True)) - assert error_message['message'] == 'Only email and SMS can use search by recipient' + notifications = json.loads(response.get_data(as_text=True))['notifications'] + + assert response.status_code == 200 + assert len(notifications) == 1 + assert notifications[0]['id'] == str(letter_notification.id) def test_update_service_calls_send_notification_as_service_becomes_live(notify_db, notify_db_session, client, mocker): From 9047b273da5fbe9139c74b58197a1549b9a047e1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 22 Apr 2020 10:06:25 +0100 Subject: [PATCH 2/4] Save whole letter address into the `to` field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment we’re not consistent: Precompiled (API and one-off): `to` has the whole address `normalised_to` has nothing Templated (API, CSV and one off): `to` has the first line of the address `normalised_to` has nothing This commit makes us consistently store the whole address in the `to` field. We think that people might want to search by postcode, not just first line of the address. This commit also starts to populate the normalised_to field with the address lowercased and with all spaces removed, to make it easier to search on. --- app/celery/tasks.py | 2 +- .../process_letter_notifications.py | 4 +- app/notifications/process_notifications.py | 1 + tests/app/celery/test_tasks.py | 51 +++++++++++++++---- 4 files changed, 46 insertions(+), 12 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 344695f8f..0d5f9ae85 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -345,7 +345,7 @@ def save_letter( # we store the recipient as just the first item of the person's address recipient = PostalAddress.from_personalisation( Columns(notification['personalisation']) - ).normalised_lines[0] + ).normalised service = dao_fetch_service_by_id(service_id) template = dao_get_template_by_id(notification['template'], version=notification['template_version']) diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index 06d1127bf..5ba9c5a19 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -1,3 +1,5 @@ +from notifications_utils.postal_address import PostalAddress + from app import create_random_identifier from app.models import LETTER_TYPE from app.notifications.process_notifications import persist_notification @@ -9,7 +11,7 @@ def create_letter_notification(letter_data, template, api_key, status, reply_to_ template_version=template.version, template_postage=template.postage, # we only accept addresses_with_underscores from the API (from CSV we also accept dashes, spaces etc) - recipient=letter_data['personalisation']['address_line_1'], + recipient=PostalAddress.from_personalisation(letter_data['personalisation']).normalised, service=template.service, personalisation=letter_data['personalisation'], notification_type=LETTER_TYPE, diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 9c698e693..d67098aa8 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -109,6 +109,7 @@ def persist_notification( notification.normalised_to = format_email_address(notification.to) elif notification_type == LETTER_TYPE: notification.postage = postage or template_postage + notification.normalised_to = ''.join(notification.to.split()).lower() # if simulated create a Notification model to return but do not persist the Notification to the dB if not simulated: diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 254ef7488..94baf03be 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -950,8 +950,8 @@ def test_save_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample assert not retry.called -@pytest.mark.parametrize('personalisation', ( - { +@pytest.mark.parametrize('personalisation, expected_to, expected_normalised', ( + ({ 'addressline1': 'Foo', 'addressline2': 'Bar', 'addressline3': 'Baz', @@ -959,17 +959,39 @@ def test_save_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample 'addressline5': 'Wobble', 'addressline6': 'Wubble', 'postcode': 'SE1 2SA', - }, - { + }, ( + 'Foo\n' + 'Bar\n' + 'Baz\n' + 'Wibble\n' + 'Wobble\n' + 'Wubble\n' + 'SE1 2SA' + ), ( + 'foobarbazwibblewobblewubblese12sa' + )), + ({ # The address isn’t normalised when we store it in the - # `personalisation` column, but the first line is normalised for - # Storing in the `to` column + # `personalisation` column, but is normalised for storing in the + # `to` column 'addressline2': ' Foo ', 'addressline4': 'Bar', 'addressline6': 'se12sa', - }, + }, ( + 'Foo\n' + 'Bar\n' + 'SE1 2SA' + ), ( + 'foobarse12sa' + )), )) -def test_save_letter_saves_letter_to_database(mocker, notify_db_session, personalisation): +def test_save_letter_saves_letter_to_database( + mocker, + notify_db_session, + personalisation, + expected_to, + expected_normalised, +): service = create_service() contact_block = create_letter_contact(service=service, contact_block="Address contact", is_default=True) template = create_template(service=service, template_type=LETTER_TYPE, reply_to=contact_block.id) @@ -996,7 +1018,8 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session, persona notification_db = Notification.query.one() assert notification_db.id == notification_id - assert notification_db.to == 'Foo' + assert notification_db.to == expected_to + assert notification_db.normalised_to == expected_normalised assert notification_db.job_id == job.id assert notification_db.template_id == job.template.id assert notification_db.template_version == job.template.version @@ -1097,7 +1120,15 @@ def test_save_letter_saves_letter_to_database_right_reply_to(mocker, notify_db_s notification_db = Notification.query.one() assert notification_db.id == notification_id - assert notification_db.to == 'Foo' + assert notification_db.to == ( + 'Foo\n' + 'Bar\n' + 'Baz\n' + 'Wibble\n' + 'Wobble\n' + 'Wubble\n' + 'SE1 3WS' + ) assert notification_db.job_id == job.id assert notification_db.template_id == job.template.id assert notification_db.template_version == job.template.version From 11008af96a1c83637048ba54f2f26e48293119aa Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 23 Apr 2020 15:35:25 +0100 Subject: [PATCH 3/4] Remove outdated comment --- app/celery/tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 0d5f9ae85..4949b39b0 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -342,7 +342,6 @@ def save_letter( ): notification = encryption.decrypt(encrypted_notification) - # we store the recipient as just the first item of the person's address recipient = PostalAddress.from_personalisation( Columns(notification['personalisation']) ).normalised From 64674be817065f8ab5db2bd7e355a374ddfb3e1a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 23 Apr 2020 16:06:34 +0100 Subject: [PATCH 4/4] Make allowed notification types explicit in search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By not having a catch-all else, it makes it clearer what we’re expecting. And then we think it’s worth adding a comment explaining why we normalise as we do for letters and the `None` case. --- app/dao/notifications_dao.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index a06174f49..d348660df 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -599,9 +599,20 @@ def dao_get_notifications_by_recipient_or_reference(service_id, search_term, not except InvalidEmailError: normalised = search_term.lower() - else: + elif notification_type in {LETTER_TYPE, None}: + # For letters, we store the address without spaces, so we need + # to removes spaces from the search term to match. We also do + # this when a notification type isn’t provided (this will + # happen if a user doesn’t have permission to see the dashboard) + # because email addresses and phone numbers will never be stored + # with spaces either. normalised = ''.join(search_term.split()).lower() + else: + raise TypeError( + f'Notification type must be {EMAIL_TYPE}, {SMS_TYPE}, {LETTER_TYPE} or None' + ) + normalised = escape_special_characters(normalised) search_term = escape_special_characters(search_term)