From 8cb6907828342008b521cc0c958fe6b3382dba80 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 12 Dec 2019 16:01:22 +0000 Subject: [PATCH 1/2] Allow searching by reference as well as recipient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have a team who want to find emails that might have been sent to an incorrect address. Therefore they can’t search by the correct address, because it won’t match. What they do have is the reference number of the user’s application, which is also stored in the `client_reference` field on the notification. So when a user is searching we should also look at the client reference, as well as the recipient, allowing the user to enter either in the search box. --- app/dao/notifications_dao.py | 10 +- .../notification_dao/test_notification_dao.py | 159 +++++++++++++++--- 2 files changed, 139 insertions(+), 30 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index fb195cab8..3eb999d3b 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -17,7 +17,7 @@ from notifications_utils.recipients import ( ) from notifications_utils.statsd_decorators import statsd from notifications_utils.timezones import convert_bst_to_utc, convert_utc_to_bst -from sqlalchemy import (desc, func, asc, and_) +from sqlalchemy import (desc, func, asc, and_, or_) from sqlalchemy.orm import joinedload from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.sql import functions @@ -569,7 +569,7 @@ def dao_update_notifications_by_reference(references, update_dict): @statsd(namespace="dao") -def dao_get_notifications_by_to_field(service_id, search_term, notification_type=None, statuses=None): +def dao_get_notifications_by_recipient_or_reference(service_id, search_term, notification_type=None, statuses=None): if notification_type is None: notification_type = guess_notification_type(search_term) @@ -591,10 +591,14 @@ def dao_get_notifications_by_to_field(service_id, search_term, notification_type raise InvalidRequest("Only email and SMS can use search by recipient", 400) normalised = escape_special_characters(normalised) + search_term = escape_special_characters(search_term) filters = [ Notification.service_id == service_id, - Notification.normalised_to.like("%{}%".format(normalised)), + or_( + Notification.normalised_to.like("%{}%".format(normalised)), + Notification.client_reference.ilike("%{}%".format(search_term)), + ), Notification.key_type != KEY_TYPE_TEST, ] diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 8f56d2c56..546288c2c 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -14,7 +14,7 @@ from app.dao.notifications_dao import ( dao_delete_notifications_by_id, dao_get_last_notification_added_for_job_id, dao_get_last_template_usage, - dao_get_notifications_by_to_field, + dao_get_notifications_by_recipient_or_reference, dao_get_notification_count_for_job_id, dao_get_scheduled_notifications, dao_timeout_notifications, @@ -1031,7 +1031,7 @@ def test_delivery_is_delivery_slow_for_providers_filters_out_notifications_it_sh assert result['mmg'] == expected_result -def test_dao_get_notifications_by_to_field(sample_template): +def test_dao_get_notifications_by_recipient(sample_template): recipient_to_search_for = { 'to_field': '+447700900855', @@ -1051,7 +1051,7 @@ def test_dao_get_notifications_by_to_field(sample_template): template=sample_template, to_field='jane@gmail.com', normalised_to='jane@gmail.com' ) - results = dao_get_notifications_by_to_field( + results = dao_get_notifications_by_recipient_or_reference( notification1.service_id, recipient_to_search_for["to_field"], notification_type='sms' @@ -1063,25 +1063,29 @@ def test_dao_get_notifications_by_to_field(sample_template): @pytest.mark.parametrize("search_term", ["JACK", "JACK@gmail.com", "jack@gmail.com"]) -def test_dao_get_notifications_by_to_field_search_is_not_case_sensitive(sample_email_template, search_term): +def test_dao_get_notifications_by_recipient_is_not_case_sensitive(sample_email_template, search_term): notification = create_notification( template=sample_email_template, to_field='jack@gmail.com', normalised_to='jack@gmail.com' ) - results = dao_get_notifications_by_to_field(notification.service_id, search_term, notification_type='email') + results = dao_get_notifications_by_recipient_or_reference( + notification.service_id, search_term, notification_type='email' + ) notification_ids = [notification.id for notification in results] assert len(results) == 1 assert notification.id in notification_ids -def test_dao_get_notifications_by_to_field_matches_partial_emails(sample_email_template): +def test_dao_get_notifications_by_recipient_matches_partial_emails(sample_email_template): notification_1 = create_notification( template=sample_email_template, to_field='jack@gmail.com', normalised_to='jack@gmail.com' ) notification_2 = create_notification( template=sample_email_template, to_field='jacque@gmail.com', normalised_to='jacque@gmail.com' ) - results = dao_get_notifications_by_to_field(notification_1.service_id, 'ack', notification_type='email') + results = dao_get_notifications_by_recipient_or_reference( + notification_1.service_id, 'ack', notification_type='email' + ) notification_ids = [notification.id for notification in results] assert len(results) == 1 @@ -1105,7 +1109,7 @@ def test_dao_get_notifications_by_to_field_matches_partial_emails(sample_email_t ('%_%', 0), ('example.com', 5), ]) -def test_dao_get_notifications_by_to_field_escapes( +def test_dao_get_notifications_by_recipient_escapes( sample_email_template, search_term, expected_result_count, @@ -1124,7 +1128,50 @@ def test_dao_get_notifications_by_to_field_escapes( normalised_to=email_address, ) - assert len(dao_get_notifications_by_to_field( + assert len(dao_get_notifications_by_recipient_or_reference( + sample_email_template.service_id, + search_term, + notification_type='email', + )) == expected_result_count + + +@pytest.mark.parametrize('search_term, expected_result_count', [ + ('foobar', 1), + ('foo', 2), + ('bar', 2), + ('foo%', 1), + ('%%bar', 1), + ('%_', 1), + ('%', 2), + ('_', 1), + ('/', 1), + ('\\', 1), + ('baz\\baz', 1), + ('%foo', 0), + ('%_%', 0), + ('test@example.com', 5), +]) +def test_dao_get_notifications_by_reference_escapes_special_character( + sample_email_template, + search_term, + expected_result_count, +): + + for reference in { + 'foo%_', + '%%bar', + 'foobar', + '/', + 'baz\\baz', + }: + create_notification( + template=sample_email_template, + to_field='test@example.com', + normalised_to='test@example.com', + client_reference=reference, + ) + + assert len(dao_get_notifications_by_recipient_or_reference( sample_email_template.service_id, search_term, notification_type='email', @@ -1143,7 +1190,7 @@ def test_dao_get_notifications_by_to_field_escapes( pytest.param('+44077009001', marks=pytest.mark.skip(reason='No easy way to normalise this')), pytest.param('+44(0)77009001', marks=pytest.mark.skip(reason='No easy way to normalise this')), ]) -def test_dao_get_notifications_by_to_field_matches_partial_phone_numbers( +def test_dao_get_notifications_by_recipient_matches_partial_phone_numbers( sample_template, search_term, ): @@ -1158,7 +1205,9 @@ def test_dao_get_notifications_by_to_field_matches_partial_phone_numbers( to_field='+447700900200', normalised_to='447700900200', ) - results = dao_get_notifications_by_to_field(notification_1.service_id, search_term, notification_type='sms') + results = dao_get_notifications_by_recipient_or_reference( + notification_1.service_id, search_term, notification_type='sms' + ) notification_ids = [notification.id for notification in results] assert len(results) == 1 @@ -1169,18 +1218,18 @@ def test_dao_get_notifications_by_to_field_matches_partial_phone_numbers( @pytest.mark.parametrize('to', [ 'not@email', '123' ]) -def test_dao_get_notifications_by_to_field_accepts_invalid_phone_numbers_and_email_addresses( +def test_dao_get_notifications_by_recipient_accepts_invalid_phone_numbers_and_email_addresses( sample_template, to, ): notification = create_notification( template=sample_template, to_field='test@example.com', normalised_to='test@example.com' ) - results = dao_get_notifications_by_to_field(notification.service_id, to, notification_type='email') + results = dao_get_notifications_by_recipient_or_reference(notification.service_id, to, notification_type='email') assert len(results) == 0 -def test_dao_get_notifications_by_to_field_search_ignores_spaces(sample_template): +def test_dao_get_notifications_by_recipient_ignores_spaces(sample_template): notification1 = create_notification( template=sample_template, to_field='+447700900855', normalised_to='447700900855' ) @@ -1194,7 +1243,9 @@ def test_dao_get_notifications_by_to_field_search_ignores_spaces(sample_template template=sample_template, to_field='jaCK@gmail.com', normalised_to='jack@gmail.com' ) - results = dao_get_notifications_by_to_field(notification1.service_id, '+447700900855', notification_type='sms') + results = dao_get_notifications_by_recipient_or_reference( + notification1.service_id, '+447700900855', notification_type='sms' + ) notification_ids = [notification.id for notification in results] assert len(results) == 3 @@ -1209,7 +1260,7 @@ def test_dao_get_notifications_by_to_field_search_ignores_spaces(sample_template @pytest.mark.parametrize('email_search', ( 'example', 'eXaMpLe', )) -def test_dao_get_notifications_by_to_field_only_searches_one_notification_type( +def test_dao_get_notifications_by_recipient_searches_across_notification_types( notify_db_session, phone_search, email_search, @@ -1221,20 +1272,71 @@ def test_dao_get_notifications_by_to_field_only_searches_one_notification_type( email = create_notification( template=email_template, to_field='077@example.com', normalised_to='077@example.com' ) - results = dao_get_notifications_by_to_field(service.id, phone_search, notification_type='sms') + results = dao_get_notifications_by_recipient_or_reference(service.id, phone_search, notification_type='sms') assert len(results) == 1 assert results[0].id == sms.id - results = dao_get_notifications_by_to_field(service.id, phone_search) # should assume SMS + results = dao_get_notifications_by_recipient_or_reference(service.id, phone_search) # should assume SMS assert len(results) == 1 assert results[0].id == sms.id - results = dao_get_notifications_by_to_field(service.id, '077', notification_type='email') + results = dao_get_notifications_by_recipient_or_reference(service.id, '077', notification_type='email') assert len(results) == 1 assert results[0].id == email.id - results = dao_get_notifications_by_to_field(service.id, email_search) # should assume email + results = dao_get_notifications_by_recipient_or_reference(service.id, email_search) # should assume email assert len(results) == 1 assert results[0].id == email.id +def test_dao_get_notifications_by_reference( + notify_db_session +): + service = create_service() + sms_template = create_template(service=service) + email_template = create_template(service=service, template_type='email') + sms = create_notification( + template=sms_template, + to_field='07711111111', + normalised_to='447711111111', + client_reference='77aA', + ) + email = create_notification( + template=email_template, + to_field='077@example.com', + normalised_to='077@example.com', + client_reference='77bB', + ) + + results = dao_get_notifications_by_recipient_or_reference(service.id, '77') + assert len(results) == 2 + assert results[0].id == sms.id + assert results[0].id == email.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 + results = dao_get_notifications_by_recipient_or_reference(service.id, '077') + assert len(results) == 1 + assert results[0].id == email.id + + results = dao_get_notifications_by_recipient_or_reference(service.id, '077', notification_type='sms') + assert len(results) == 1 + assert results[0].id == sms.id + results = dao_get_notifications_by_recipient_or_reference(service.id, '77', notification_type='sms') + assert len(results) == 1 + assert results[0].id == sms.id + results = dao_get_notifications_by_recipient_or_reference(service.id, 'Aa', notification_type='sms') + assert len(results) == 1 + assert results[0].id == sms.id + results = dao_get_notifications_by_recipient_or_reference(service.id, 'bB', notification_type='sms') + assert len(results) == 0 + + results = dao_get_notifications_by_recipient_or_reference(service.id, '77', notification_type='email') + assert len(results) == 1 + assert results[0].id == email.id + results = dao_get_notifications_by_recipient_or_reference(service.id, 'Bb', notification_type='email') + assert len(results) == 1 + assert results[0].id == email.id + results = dao_get_notifications_by_recipient_or_reference(service.id, 'aA', notification_type='email') + assert len(results) == 0 + + def test_dao_created_scheduled_notification(sample_notification): scheduled_notification = ScheduledNotification(notification_id=sample_notification.id, @@ -1281,9 +1383,12 @@ def test_dao_get_notifications_by_to_field_filters_status(sample_template): normalised_to='447700900855', status='temporary-failure' ) - notifications = dao_get_notifications_by_to_field(notification.service_id, "+447700900855", - statuses=['delivered'], - notification_type='sms') + notifications = dao_get_notifications_by_recipient_or_reference( + notification.service_id, + "+447700900855", + statuses=['delivered'], + notification_type='sms', + ) assert len(notifications) == 1 assert notification.id == notifications[0].id @@ -1299,7 +1404,7 @@ def test_dao_get_notifications_by_to_field_filters_multiple_statuses(sample_temp normalised_to='447700900855', status='sending' ) - notifications = dao_get_notifications_by_to_field( + notifications = dao_get_notifications_by_recipient_or_reference( notification1.service_id, "+447700900855", statuses=['delivered', 'sending'], notification_type='sms' ) notification_ids = [notification.id for notification in notifications] @@ -1319,7 +1424,7 @@ def test_dao_get_notifications_by_to_field_returns_all_if_no_status_filter(sampl normalised_to='447700900855', status='temporary-failure' ) - notifications = dao_get_notifications_by_to_field( + notifications = dao_get_notifications_by_recipient_or_reference( notification1.service_id, "+447700900855", notification_type='sms' ) notification_ids = [notification.id for notification in notifications] @@ -1341,7 +1446,7 @@ def test_dao_get_notifications_by_to_field_orders_by_created_at_desc(sample_temp notification_a_minute_ago = notification(created_at=datetime.utcnow() - timedelta(minutes=1)) notification = notification(created_at=datetime.utcnow()) - notifications = dao_get_notifications_by_to_field( + notifications = dao_get_notifications_by_recipient_or_reference( sample_template.service_id, '+447700900855', notification_type='sms' ) @@ -1493,7 +1598,7 @@ def test_dao_get_notification_by_reference_with_no_matches_raises_error(notify_d dao_get_notification_by_reference('REF1') -def test_dao_get_notifications_by_reference(sample_template): +def test_dao_get_notifications_by_references(sample_template): create_notification(template=sample_template, reference='noref') notification_1 = create_notification(template=sample_template, reference='ref') notification_2 = create_notification(template=sample_template, reference='ref') From c573209d7e6e1771aa1b9f6c10a8988bf631c950 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 16 Dec 2019 10:27:55 +0000 Subject: [PATCH 2/2] Stop guessing notification type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before the search term was either: - an email address (or partial email address) - a phone number (or partial phone number) Now it can also be: - a reference (or partial reference) We can take a pretty good guess, by looking at the search term, whether the thing the user is searching by email address or phone number. This helps us: - only show relevant notifications - normalise the search term to give the best chance of matching what we store in the `normalised_to` field However we can’t look at a search term and guess whether it’s a reference, because a reference could take any format. Therefore if the user hasn’t told us what kind of thing their search term is, we should stop trying to guess. --- app/dao/notifications_dao.py | 15 +++------- app/service/rest.py | 2 +- .../notification_dao/test_notification_dao.py | 29 +++++++++++++------ 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 3eb999d3b..3f46577b3 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,5 +1,4 @@ import functools -import string from itertools import groupby from operator import attrgetter from datetime import ( @@ -570,8 +569,6 @@ def dao_update_notifications_by_reference(references, update_dict): @statsd(namespace="dao") def dao_get_notifications_by_recipient_or_reference(service_id, search_term, notification_type=None, statuses=None): - if notification_type is None: - notification_type = guess_notification_type(search_term) if notification_type == SMS_TYPE: normalised = try_validate_and_format_phone_number(search_term) @@ -587,9 +584,12 @@ def dao_get_notifications_by_recipient_or_reference(service_id, search_term, not except InvalidEmailError: normalised = search_term.lower() - else: + elif notification_type == LETTER_TYPE: raise InvalidRequest("Only email and SMS can use search by recipient", 400) + else: + normalised = search_term.lower() + normalised = escape_special_characters(normalised) search_term = escape_special_characters(search_term) @@ -765,13 +765,6 @@ def dao_precompiled_letters_still_pending_virus_check(): return notifications -def guess_notification_type(search_term): - if set(search_term) & set(string.ascii_letters + '@'): - return EMAIL_TYPE - else: - return SMS_TYPE - - def _duplicate_update_warning(notification, status): current_app.logger.info( ( diff --git a/app/service/rest.py b/app/service/rest.py index 60141e87e..7cdd259d5 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -455,7 +455,7 @@ def cancel_notification_for_service(service_id, notification_id): def search_for_notification_by_to_field(service_id, search_term, statuses, notification_type): - results = notifications_dao.dao_get_notifications_by_to_field( + results = notifications_dao.dao_get_notifications_by_recipient_or_reference( service_id=service_id, search_term=search_term, statuses=statuses, diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 546288c2c..7cfa9e76e 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1272,18 +1272,23 @@ def test_dao_get_notifications_by_recipient_searches_across_notification_types( email = create_notification( template=email_template, to_field='077@example.com', normalised_to='077@example.com' ) - results = dao_get_notifications_by_recipient_or_reference(service.id, phone_search, notification_type='sms') + + results = dao_get_notifications_by_recipient_or_reference( + service.id, phone_search, notification_type='sms' + ) assert len(results) == 1 assert results[0].id == sms.id - results = dao_get_notifications_by_recipient_or_reference(service.id, phone_search) # should assume SMS - assert len(results) == 1 - assert results[0].id == sms.id - results = dao_get_notifications_by_recipient_or_reference(service.id, '077', notification_type='email') + + results = dao_get_notifications_by_recipient_or_reference( + service.id, email_search, notification_type='email' + ) assert len(results) == 1 assert results[0].id == email.id - results = dao_get_notifications_by_recipient_or_reference(service.id, email_search) # should assume email - assert len(results) == 1 + + results = dao_get_notifications_by_recipient_or_reference(service.id, '77') + assert len(results) == 2 assert results[0].id == email.id + assert results[1].id == sms.id def test_dao_get_notifications_by_reference( @@ -1307,10 +1312,11 @@ def test_dao_get_notifications_by_reference( results = dao_get_notifications_by_recipient_or_reference(service.id, '77') assert len(results) == 2 - assert results[0].id == sms.id 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 + # 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') assert len(results) == 1 assert results[0].id == email.id @@ -1318,21 +1324,26 @@ def test_dao_get_notifications_by_reference( results = dao_get_notifications_by_recipient_or_reference(service.id, '077', notification_type='sms') assert len(results) == 1 assert results[0].id == sms.id + results = dao_get_notifications_by_recipient_or_reference(service.id, '77', notification_type='sms') assert len(results) == 1 assert results[0].id == sms.id + results = dao_get_notifications_by_recipient_or_reference(service.id, 'Aa', notification_type='sms') assert len(results) == 1 assert results[0].id == sms.id + results = dao_get_notifications_by_recipient_or_reference(service.id, 'bB', notification_type='sms') assert len(results) == 0 results = dao_get_notifications_by_recipient_or_reference(service.id, '77', notification_type='email') assert len(results) == 1 assert results[0].id == email.id + results = dao_get_notifications_by_recipient_or_reference(service.id, 'Bb', notification_type='email') assert len(results) == 1 assert results[0].id == email.id + results = dao_get_notifications_by_recipient_or_reference(service.id, 'aA', notification_type='email') assert len(results) == 0