Refactored code to use inbound_number.number

- Removed filter on sms_sender for `dao_fetch_services_by_inbound_number`

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()
 
 
-############
+def dao_fetch_service_by_inbound_number(number):
-# refactor this when API only uses inbound_numbers and not sms_sender
-############
-def dao_fetch_services_by_sms_sender(sms_sender):
     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.id == inbound_number.service_id
-            Service.sms_sender == sms_sender,
+    ).first()
-            Service.id == inbound_number.service_id
-        )
-    ).all()
 
 
 def dao_fetch_service_by_id_with_api_keys(service_id, only_active=False):

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')
+    service = fetch_potential_service(inbound_number, 'mmg')
-    if not potential_services:
+    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')
+    service = fetch_potential_service(inbound_number, 'firetext')
-    if not potential_services:
+    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):
+def fetch_potential_service(inbound_number, provider_name):
-    potential_services = dao_fetch_services_by_sms_sender(inbound_number)
+    service = dao_fetch_service_by_inbound_number(inbound_number)
 
-    if len(potential_services) != 1:
+    if not service:
-        current_app.logger.error('Inbound number "{}" from {} not associated with exactly one service'.format(
+        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):

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 == service.id
-    assert foo1.id == services[0].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):
+def test_dao_fetch_service_by_inbound_number_when_inbound_number_set(notify_db_session):
-    service = create_service(service_name='a', sms_sender=None)
+    service_1 = create_service(service_name='a', sms_sender=None)
-    service = create_service(service_name='b')
+    service_2 = create_service(service_name='b')
-    inbound_number = create_inbound_number('1', service_id=service.id)
+    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 service.id == service_1.id
-    assert services[0].id == service.id
 
 
-def test_dao_fetch_services_by_sms_sender_when_inbound_number_set_and_sms_sender_same(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)
-    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):
     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):

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(