From 8cb6907828342008b521cc0c958fe6b3382dba80 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 12 Dec 2019 16:01:22 +0000 Subject: [PATCH] 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')