From fb2623962f9f227189b23e8214d21551b1898e80 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 23 Aug 2017 11:09:54 +0100 Subject: [PATCH 1/2] Moved create_inbound after dao_create_service - Need to do this otherwise no service.id is available to link the servce to the inbound number --- tests/app/db.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/db.py b/tests/app/db.py index 9f7539861..8283428c9 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -71,11 +71,11 @@ def create_service( sms_sender=sms_sender, ) + dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) + if do_create_inbound_number and INBOUND_SMS_TYPE in service_permissions: create_inbound_number(number=sms_sender, service_id=service.id) - dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) - service.active = active service.research_mode = research_mode From d99ab329eb5937c87c50ac51332beadcd26b53f5 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 23 Aug 2017 13:03:52 +0100 Subject: [PATCH 2/2] Refactored code to use inbound_number.number - Removed filter on sms_sender for `dao_fetch_services_by_inbound_number` --- app/dao/services_dao.py | 18 ++---- app/notifications/receive_notifications.py | 27 ++++---- tests/app/dao/test_services_dao.py | 63 ++++++------------- .../test_receive_notification.py | 11 +++- 4 files changed, 46 insertions(+), 73 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index d75117308..ad5ebf80a 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -1,7 +1,7 @@ import uuid from datetime import date, datetime, timedelta -from sqlalchemy import asc, func, or_ +from sqlalchemy import asc, func from sqlalchemy.orm import joinedload from flask import current_app @@ -66,24 +66,18 @@ def dao_fetch_service_by_id(service_id, only_active=False): return query.one() -############ -# refactor this when API only uses inbound_numbers and not sms_sender -############ -def dao_fetch_services_by_sms_sender(sms_sender): +def dao_fetch_service_by_inbound_number(number): inbound_number = InboundNumber.query.filter( - InboundNumber.number == sms_sender, + InboundNumber.number == number, InboundNumber.active ).first() if not inbound_number: - return [] + return None return Service.query.filter( - or_( - Service.sms_sender == sms_sender, - Service.id == inbound_number.service_id - ) - ).all() + Service.id == inbound_number.service_id + ).first() def dao_fetch_service_by_id_with_api_keys(service_id, only_active=False): diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index 4fd3d33c2..ac116b28a 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -7,7 +7,7 @@ from notifications_utils.recipients import validate_and_format_phone_number from app import statsd_client, firetext_client, mmg_client from app.celery import tasks from app.config import QueueNames -from app.dao.services_dao import dao_fetch_services_by_sms_sender +from app.dao.services_dao import dao_fetch_service_by_inbound_number from app.dao.inbound_sms_dao import dao_create_inbound_sms from app.models import InboundSms, INBOUND_SMS_TYPE, SMS_TYPE from app.errors import register_errors @@ -32,16 +32,14 @@ def receive_mmg_sms(): inbound_number = strip_leading_forty_four(post_data['Number']) - potential_services = fetch_potential_services(inbound_number, 'mmg') - if not potential_services: + service = fetch_potential_service(inbound_number, 'mmg') + if not service: # since this is an issue with our service <-> number mapping, or no inbound_sms service permission # we should still tell MMG that we received it successfully return 'RECEIVED', 200 statsd_client.incr('inbound.mmg.successful') - service = potential_services[0] - inbound = create_inbound_sms_object(service, content=format_mmg_message(post_data["Message"]), from_number=post_data['MSISDN'], @@ -60,13 +58,12 @@ def receive_firetext_sms(): inbound_number = strip_leading_forty_four(post_data['destination']) - potential_services = fetch_potential_services(inbound_number, 'firetext') - if not potential_services: + service = fetch_potential_service(inbound_number, 'firetext') + if not service: return jsonify({ "status": "ok" }), 200 - service = potential_services[0] inbound = create_inbound_sms_object(service=service, content=post_data["message"], from_number=post_data['source'], @@ -118,22 +115,22 @@ def create_inbound_sms_object(service, content, from_number, provider_ref, date_ return inbound -def fetch_potential_services(inbound_number, provider_name): - potential_services = dao_fetch_services_by_sms_sender(inbound_number) +def fetch_potential_service(inbound_number, provider_name): + service = dao_fetch_service_by_inbound_number(inbound_number) - if len(potential_services) != 1: - current_app.logger.error('Inbound number "{}" from {} not associated with exactly one service'.format( + if not service: + current_app.logger.error('Inbound number "{}" from {} not associated with a service'.format( inbound_number, provider_name )) statsd_client.incr('inbound.{}.failed'.format(provider_name)) return False - if not has_inbound_sms_permissions(potential_services[0].permissions): + if not has_inbound_sms_permissions(service.permissions): current_app.logger.error( - 'Service "{}" does not allow inbound SMS'.format(potential_services[0].id)) + 'Service "{}" does not allow inbound SMS'.format(service.id)) return False - return potential_services + return service def has_inbound_sms_permissions(permissions): diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index b7b92a0c0..1d01c4a3e 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -30,7 +30,7 @@ from app.dao.services_dao import ( dao_suspend_service, dao_resume_service, dao_fetch_active_users_for_service, - dao_fetch_services_by_sms_sender + dao_fetch_service_by_inbound_number ) from app.dao.service_permissions_dao import dao_add_service_permission, dao_remove_service_permission from app.dao.users_dao import save_model_user @@ -892,76 +892,53 @@ def test_dao_fetch_active_users_for_service_returns_active_only(notify_db, notif assert len(users) == 1 -def test_dao_fetch_services_by_sms_sender_with_inbound_number(notify_db_session): +def test_dao_fetch_service_by_inbound_number_with_inbound_number(notify_db_session): foo1 = create_service(service_name='a', sms_sender='1') foo2 = create_service(service_name='b', sms_sender='2') bar = create_service(service_name='c', sms_sender='3') - create_inbound_number('1') + create_inbound_number('1', service_id=foo1.id) create_inbound_number('2') create_inbound_number('3') - services = dao_fetch_services_by_sms_sender('1') + service = dao_fetch_service_by_inbound_number('1') - assert len(services) == 1 - assert foo1.id == services[0].id + assert foo1.id == service.id -def test_dao_fetch_services_by_sms_sender_with_inbound_number_not_set(notify_db_session): +def test_dao_fetch_service_by_inbound_number_with_inbound_number_not_set(notify_db_session): create_inbound_number('1') - services = dao_fetch_services_by_sms_sender('1') + service = dao_fetch_service_by_inbound_number('1') - assert services == [] + assert service is None -def test_dao_fetch_services_by_sms_sender_when_inbound_number_set(notify_db_session): - service = create_service(service_name='a', sms_sender=None) - service = create_service(service_name='b') - inbound_number = create_inbound_number('1', service_id=service.id) +def test_dao_fetch_service_by_inbound_number_when_inbound_number_set(notify_db_session): + service_1 = create_service(service_name='a', sms_sender=None) + service_2 = create_service(service_name='b') + inbound_number = create_inbound_number('1', service_id=service_1.id) - services = dao_fetch_services_by_sms_sender('1') + service = dao_fetch_service_by_inbound_number('1') - assert len(services) == 1 - assert services[0].id == service.id + assert service.id == service_1.id -def test_dao_fetch_services_by_sms_sender_when_inbound_number_set_and_sms_sender_same(notify_db_session): - service = create_service(service_name='a', sms_sender=None) - service = create_service(service_name='b', sms_sender='1') - inbound_number = create_inbound_number('1', service_id=service.id) - - services = dao_fetch_services_by_sms_sender('1') - - assert len(services) == 1 - assert services[0].id == service.id - - -def test_dao_fetch_services_by_sms_sender_when_inbound_number_not_set_gets_sms_sender(notify_db_session): - service = create_service(service_name='a', sms_sender=None) - service = create_service(service_name='b', sms_sender='testing_gov') - inbound_number = create_inbound_number('1', service_id=service.id) - - services = dao_fetch_services_by_sms_sender('testing_gov') - - assert services == [] - - -def test_dao_fetch_services_by_sms_sender_with_unknown_number(notify_db_session): +def test_dao_fetch_service_by_inbound_number_with_unknown_number(notify_db_session): service = create_service(service_name='a', sms_sender=None) inbound_number = create_inbound_number('1', service_id=service.id) - services = dao_fetch_services_by_sms_sender('9') + service = dao_fetch_service_by_inbound_number('9') - assert services == [] + assert service is None -def test_dao_fetch_services_by_sms_sender_with_inactive_number_returns_empty(notify_db_session): +def test_dao_fetch_service_by_inbound_number_with_inactive_number_returns_empty(notify_db_session): service = create_service(service_name='a', sms_sender=None) inbound_number = create_inbound_number('1', service_id=service.id, active=False) - services = dao_fetch_services_by_sms_sender('1') + service = dao_fetch_service_by_inbound_number('1') - assert services == [] + assert service is None def test_dao_allocating_inbound_number_shows_on_service(notify_db_session, sample_inbound_numbers): diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index b8bf2da9b..0734705ef 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -5,7 +5,7 @@ from unittest.mock import call import pytest from flask import json -from app.dao.services_dao import dao_fetch_services_by_sms_sender +from app.dao.services_dao import dao_fetch_service_by_inbound_number from app.notifications.receive_notifications import ( format_mmg_message, format_mmg_datetime, @@ -85,7 +85,12 @@ def test_receive_notification_from_firetext_without_permissions_does_not_persist permissions ): service = create_service(sms_sender='07111111111', service_permissions=permissions) - mocked = mocker.patch("app.notifications.receive_notifications.tasks.send_inbound_sms_to_service.apply_async") + mocker.patch("app.notifications.receive_notifications.dao_fetch_service_by_inbound_number", + return_value=service) + mocked_send_inbound_sms = mocker.patch( + "app.notifications.receive_notifications.tasks.send_inbound_sms_to_service.apply_async") + mocked_has_permissions = mocker.patch( + "app.notifications.receive_notifications.has_inbound_sms_permissions", return_value=False) data = "source=07999999999&destination=07111111111&message=this is a message&time=2017-01-01 12:00:00" response = client.post( @@ -99,7 +104,7 @@ def test_receive_notification_from_firetext_without_permissions_does_not_persist assert result['status'] == 'ok' assert InboundSms.query.count() == 0 - assert mocked.called is False + assert not mocked_send_inbound_sms.called def test_receive_notification_without_permissions_does_not_create_inbound_even_with_inbound_number_set(