From 47a1403591a5b42bf1234ea5500a97b82f9ff686 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 10 Aug 2017 17:51:47 +0100 Subject: [PATCH 01/26] Refactor code to add updated_at --- app/dao/inbound_numbers_dao.py | 4 ++++ tests/app/dao/test_inbound_numbers_dao.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/dao/inbound_numbers_dao.py b/app/dao/inbound_numbers_dao.py index 1c184b7f6..4a4143ffa 100644 --- a/app/dao/inbound_numbers_dao.py +++ b/app/dao/inbound_numbers_dao.py @@ -15,6 +15,10 @@ def dao_get_inbound_number_for_service(service_id): return InboundNumber.query.filter(InboundNumber.service_id == service_id).first() +def dao_get_inbound_number(inbound_number_id): + return InboundNumber.query.filter(InboundNumber.id == inbound_number_id).first() + + @transactional def dao_set_inbound_number_to_service(service_id, inbound_number): inbound_number.service_id = service_id diff --git a/tests/app/dao/test_inbound_numbers_dao.py b/tests/app/dao/test_inbound_numbers_dao.py index 9d59a8110..d38622d86 100644 --- a/tests/app/dao/test_inbound_numbers_dao.py +++ b/tests/app/dao/test_inbound_numbers_dao.py @@ -76,6 +76,6 @@ def test_set_inbound_number_active_flag(notify_db, notify_db_session, sample_ser dao_set_inbound_number_active_flag(inbound_number.id, active=active) - inbound_number = dao_get_inbound_number_for_service(sample_service.id) + inbound_number = dao_get_inbound_number(inbound_number.id) assert inbound_number.active is active From 6908ec482168d52740328dd2fa0c0fc323ed9a90 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 4 Aug 2017 19:26:51 +0100 Subject: [PATCH 02/26] Add inbound_number rest and tests --- app/__init__.py | 4 ++ app/inbound_number/__init__.py | 0 app/inbound_number/rest.py | 45 +++++++++++++++++ tests/app/inbound_number/test_rest.py | 70 +++++++++++++++++++++++++++ 4 files changed, 119 insertions(+) create mode 100644 app/inbound_number/__init__.py create mode 100644 app/inbound_number/rest.py create mode 100644 tests/app/inbound_number/test_rest.py diff --git a/app/__init__.py b/app/__init__.py index e0ec02723..cd293ad8a 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -93,6 +93,7 @@ def register_blueprint(application): from app.organisation.rest import organisation_blueprint from app.dvla_organisation.rest import dvla_organisation_blueprint from app.delivery.rest import delivery_blueprint + from app.inbound_number.rest import inbound_number_blueprint from app.inbound_sms.rest import inbound_sms as inbound_sms_blueprint from app.notifications.receive_notifications import receive_notifications_blueprint from app.notifications.notifications_ses_callback import ses_callback_blueprint @@ -134,6 +135,9 @@ def register_blueprint(application): delivery_blueprint.before_request(requires_admin_auth) application.register_blueprint(delivery_blueprint) + inbound_number_blueprint.before_request(requires_admin_auth) + application.register_blueprint(inbound_number_blueprint, url_prefix='/inbound_number') + inbound_sms_blueprint.before_request(requires_admin_auth) application.register_blueprint(inbound_sms_blueprint) diff --git a/app/inbound_number/__init__.py b/app/inbound_number/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py new file mode 100644 index 000000000..b13bed3b1 --- /dev/null +++ b/app/inbound_number/rest.py @@ -0,0 +1,45 @@ +from flask import Blueprint, jsonify, request + +from app.dao.inbound_numbers_dao import ( + dao_get_inbound_numbers, + dao_get_inbound_number_for_service, + dao_get_available_inbound_numbers, + dao_set_inbound_number_to_service, + dao_set_inbound_number_active_flag_for_service +) +from app.errors import register_errors +from app.models import InboundNumber +from app.schema_validation import validate + +inbound_number_blueprint = Blueprint('inbound_number', __name__) +register_errors(inbound_number_blueprint) + + +@inbound_number_blueprint.route('', methods=['GET']) +def get_inbound_numbers(): + inbound_numbers = [i.serialize() for i in dao_get_inbound_numbers()] + + return jsonify(data=inbound_numbers) + + +@inbound_number_blueprint.route('/', methods=['POST']) +def post_allocate_or_reactivate_inbound_number(service_id): + inbound_number = dao_get_inbound_number_for_service(service_id) + + if not inbound_number: + available_numbers = dao_get_available_inbound_numbers() + + if len(available_numbers) > 0: + dao_set_inbound_number_to_service(service_id, available_numbers[0]) + return '', 204 + else: + return '', 409 + else: + dao_set_inbound_number_active_flag_for_service(service_id, active=True) + return '', 204 + + +@inbound_number_blueprint.route('//off', methods=['POST']) +def post_deactivate_inbound_number(service_id): + dao_set_inbound_number_active_flag_for_service(service_id, active=False) + return '', 204 diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py new file mode 100644 index 000000000..ee3115e0c --- /dev/null +++ b/tests/app/inbound_number/test_rest.py @@ -0,0 +1,70 @@ +from flask import url_for +import json + +from app.models import InboundNumber +from app.dao.inbound_numbers_dao import ( + dao_get_inbound_numbers, + dao_get_available_inbound_numbers, + dao_get_inbound_number_for_service, + dao_set_inbound_number_to_service +) + +from tests.app.db import create_service, create_inbound_number + + +def test_rest_get_inbound_numbers(admin_request, sample_inbound_numbers): + result = admin_request.get('inbound_number.get_inbound_numbers') + + assert len(result['data']) == len(sample_inbound_numbers) + assert result['data'] == [i.serialize() for i in sample_inbound_numbers] + + +def test_rest_allocate_inbound_number(admin_request, notify_db_session, sample_inbound_numbers): + service = create_service(service_name='test service') + admin_request.post( + 'inbound_number.post_allocate_or_reactivate_inbound_number', + _expected_status=204, + service_id=service.id + ) + + +def test_rest_allocate_inbound_number_when_no_inbound_available_returns_409( + admin_request, notify_db_session, sample_service): + service_1 = create_service(service_name='test service 1') + create_inbound_number(number='4', provider='mmg', active=False, service_id=sample_service.id) + create_inbound_number(number='5', provider='mmg', active=True, service_id=service_1.id) + service_2 = create_service(service_name='test service 2') + + admin_request.post( + 'inbound_number.post_allocate_or_reactivate_inbound_number', + _expected_status=409, + service_id=service_2.id + ) + + +def test_rest_deactivate_inbound_number_for_service(admin_request, notify_db_session, sample_inbound_numbers): + service = create_service(service_name='test service 1') + create_inbound_number(number='4', provider='mmg', active=True, service_id=service.id) + + admin_request.post( + 'inbound_number.post_deactivate_inbound_number', + _expected_status=204, + service_id=service.id + ) + + inbound_number_deactivated = dao_get_inbound_number_for_service(service.id) + assert not inbound_number_deactivated.active + + +def test_rest_reactivate_inbound_number_for_service(admin_request, notify_db_session, sample_inbound_numbers): + service = create_service(service_name='test service 1') + create_inbound_number(number='4', provider='mmg', active=False, service_id=service.id) + + admin_request.post( + 'inbound_number.post_allocate_or_reactivate_inbound_number', + _expected_status=204, + service_id=service.id + ) + + inbound_number_reactivated = dao_get_inbound_number_for_service(service.id) + assert inbound_number_reactivated.active From 468048797adada98b86a334b45c1bbe1443e53bd Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 4 Aug 2017 21:27:09 +0100 Subject: [PATCH 03/26] Refactor conftest --- tests/app/conftest.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d111d8d3c..fc50cb623 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1051,8 +1051,16 @@ def admin_request(client): data=json.dumps(_data), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) +<<<<<<< HEAD json_resp = json.loads(resp.get_data(as_text=True)) assert resp.status_code == _expected_status, json_resp +======= + if resp.get_data: + json_resp = json.loads(resp.get_data(as_text=True)) + else: + json_resp = None + assert resp.status_code == _expected_status +>>>>>>> Refactor conftest return json_resp @staticmethod From d5b91f99115c4772950e03966139d522e6aebe81 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 10 Aug 2017 12:47:57 +0100 Subject: [PATCH 04/26] Fixed admin_request bug --- tests/app/conftest.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index fc50cb623..5227b5e5c 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1051,11 +1051,15 @@ def admin_request(client): data=json.dumps(_data), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) +<<<<<<< HEAD <<<<<<< HEAD json_resp = json.loads(resp.get_data(as_text=True)) assert resp.status_code == _expected_status, json_resp ======= if resp.get_data: +======= + if resp.get_data(): +>>>>>>> Fixed admin_request bug json_resp = json.loads(resp.get_data(as_text=True)) else: json_resp = None From 104fc9350343229ada93effb560eb5259d1e1307 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 10 Aug 2017 18:03:20 +0100 Subject: [PATCH 05/26] Refactor code --- app/inbound_number/rest.py | 6 +++--- tests/app/conftest.py | 9 --------- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index b13bed3b1..7d87f68b4 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -5,7 +5,7 @@ from app.dao.inbound_numbers_dao import ( dao_get_inbound_number_for_service, dao_get_available_inbound_numbers, dao_set_inbound_number_to_service, - dao_set_inbound_number_active_flag_for_service + dao_set_inbound_number_active_flag ) from app.errors import register_errors from app.models import InboundNumber @@ -35,11 +35,11 @@ def post_allocate_or_reactivate_inbound_number(service_id): else: return '', 409 else: - dao_set_inbound_number_active_flag_for_service(service_id, active=True) + dao_set_inbound_number_active_flag(service_id, active=True) return '', 204 @inbound_number_blueprint.route('//off', methods=['POST']) def post_deactivate_inbound_number(service_id): - dao_set_inbound_number_active_flag_for_service(service_id, active=False) + dao_set_inbound_number_active_flag(service_id, active=False) return '', 204 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 5227b5e5c..5b38fa739 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1051,20 +1051,11 @@ def admin_request(client): data=json.dumps(_data), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) -<<<<<<< HEAD -<<<<<<< HEAD - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == _expected_status, json_resp -======= - if resp.get_data: -======= if resp.get_data(): ->>>>>>> Fixed admin_request bug json_resp = json.loads(resp.get_data(as_text=True)) else: json_resp = None assert resp.status_code == _expected_status ->>>>>>> Refactor conftest return json_resp @staticmethod From 838401ebb39285f9113ba42adc77c340065ccfed Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 10 Aug 2017 18:35:57 +0100 Subject: [PATCH 06/26] Refactored endpoints --- app/dao/inbound_numbers_dao.py | 1 - app/inbound_number/rest.py | 36 ++++++++++++-- tests/app/inbound_number/test_rest.py | 68 +++++++++++++++++++-------- 3 files changed, 80 insertions(+), 25 deletions(-) diff --git a/app/dao/inbound_numbers_dao.py b/app/dao/inbound_numbers_dao.py index 4a4143ffa..085ed3f22 100644 --- a/app/dao/inbound_numbers_dao.py +++ b/app/dao/inbound_numbers_dao.py @@ -22,7 +22,6 @@ def dao_get_inbound_number(inbound_number_id): @transactional def dao_set_inbound_number_to_service(service_id, inbound_number): inbound_number.service_id = service_id - db.session.add(inbound_number) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index 7d87f68b4..f2d347c01 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -2,6 +2,7 @@ from flask import Blueprint, jsonify, request from app.dao.inbound_numbers_dao import ( dao_get_inbound_numbers, + dao_get_inbound_number, dao_get_inbound_number_for_service, dao_get_available_inbound_numbers, dao_set_inbound_number_to_service, @@ -22,8 +23,15 @@ def get_inbound_numbers(): return jsonify(data=inbound_numbers) -@inbound_number_blueprint.route('/', methods=['POST']) -def post_allocate_or_reactivate_inbound_number(service_id): +@inbound_number_blueprint.route('/available', methods=['GET']) +def get_inbound_numbers_available(): + inbound_numbers = [i.serialize() for i in dao_get_available_inbound_numbers()] + + return jsonify(data=inbound_numbers) + + +@inbound_number_blueprint.route('/service/', methods=['POST']) +def post_allocate_inbound_number(service_id): inbound_number = dao_get_inbound_number_for_service(service_id) if not inbound_number: @@ -39,7 +47,25 @@ def post_allocate_or_reactivate_inbound_number(service_id): return '', 204 -@inbound_number_blueprint.route('//off', methods=['POST']) -def post_deactivate_inbound_number(service_id): - dao_set_inbound_number_active_flag(service_id, active=False) +@inbound_number_blueprint.route('//service/', methods=['POST']) +def post_set_inbound_number_for_service(inbound_number_id, service_id): + try: + dao_set_inbound_number_to_service(service_id, inbound_number_id) + except TypeError as e: + if str(e) == 'UUID objects are immutable': + return '', 409 + else: + raise e + return '', 204 + + +@inbound_number_blueprint.route('/service//on', methods=['POST']) +def post_set_inbound_number_on(inbound_number_id): + dao_set_inbound_number_active_flag(inbound_number_id, active=True) + return '', 204 + + +@inbound_number_blueprint.route('//off', methods=['POST']) +def post_set_inbound_number_off(inbound_number_id): + dao_set_inbound_number_active_flag(inbound_number_id, active=False) return '', 204 diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py index ee3115e0c..88b2b5661 100644 --- a/tests/app/inbound_number/test_rest.py +++ b/tests/app/inbound_number/test_rest.py @@ -4,6 +4,7 @@ import json from app.models import InboundNumber from app.dao.inbound_numbers_dao import ( dao_get_inbound_numbers, + dao_get_inbound_number, dao_get_available_inbound_numbers, dao_get_inbound_number_for_service, dao_set_inbound_number_to_service @@ -22,7 +23,7 @@ def test_rest_get_inbound_numbers(admin_request, sample_inbound_numbers): def test_rest_allocate_inbound_number(admin_request, notify_db_session, sample_inbound_numbers): service = create_service(service_name='test service') admin_request.post( - 'inbound_number.post_allocate_or_reactivate_inbound_number', + 'inbound_number.post_allocate_inbound_number', _expected_status=204, service_id=service.id ) @@ -36,35 +37,64 @@ def test_rest_allocate_inbound_number_when_no_inbound_available_returns_409( service_2 = create_service(service_name='test service 2') admin_request.post( - 'inbound_number.post_allocate_or_reactivate_inbound_number', + 'inbound_number.post_allocate_inbound_number', _expected_status=409, service_id=service_2.id ) -def test_rest_deactivate_inbound_number_for_service(admin_request, notify_db_session, sample_inbound_numbers): - service = create_service(service_name='test service 1') - create_inbound_number(number='4', provider='mmg', active=True, service_id=service.id) +def test_rest_set_service_to_several_inbound_numbers_returns_409( + admin_request, notify_db_session, sample_service): + service_1 = create_service(service_name='test service 1') + create_inbound_number(number='4', provider='mmg', active=False, service_id=sample_service.id) + inbound_number = create_inbound_number(number='5', provider='mmg', active=True, service_id=service_1.id) + service_2 = create_service(service_name='test service 2') admin_request.post( - 'inbound_number.post_deactivate_inbound_number', - _expected_status=204, - service_id=service.id + 'inbound_number.post_set_inbound_number_for_service', + _expected_status=409, + inbound_number_id=inbound_number.id, + service_id=service_2.id ) - inbound_number_deactivated = dao_get_inbound_number_for_service(service.id) - assert not inbound_number_deactivated.active - -def test_rest_reactivate_inbound_number_for_service(admin_request, notify_db_session, sample_inbound_numbers): - service = create_service(service_name='test service 1') - create_inbound_number(number='4', provider='mmg', active=False, service_id=service.id) +def test_rest_set_number_to_several_services_returns_409( + admin_request, notify_db_session, sample_service): + service_1 = create_service(service_name='test service 1') + inbound_number = create_inbound_number(number='4', provider='mmg', active=False, service_id=sample_service.id) + service_2 = create_service(service_name='test service 2') admin_request.post( - 'inbound_number.post_allocate_or_reactivate_inbound_number', - _expected_status=204, - service_id=service.id + 'inbound_number.post_set_inbound_number_for_service', + _expected_status=409, + inbound_number_id=inbound_number.id, + service_id=service_2.id ) - inbound_number_reactivated = dao_get_inbound_number_for_service(service.id) - assert inbound_number_reactivated.active + +def test_rest_set_inbound_number_active_flag_off(admin_request, notify_db_session): + service = create_service(service_name='test service 1') + inbound_number = create_inbound_number(number='1', provider='mmg', active=True, service_id=service.id) + + admin_request.post( + 'inbound_number.post_set_inbound_number_off', + _expected_status=204, + inbound_number_id=inbound_number.id + ) + + inbound_number_off = dao_get_inbound_number(inbound_number.id) + assert not inbound_number_off.active + + +def test_rest_set_inbound_number_active_flag_on(admin_request, notify_db_session): + service = create_service(service_name='test service 1') + inbound_number = create_inbound_number(number='1', provider='mmg', active=True, service_id=service.id) + + admin_request.post( + 'inbound_number.post_set_inbound_number_on', + _expected_status=204, + inbound_number_id=inbound_number.id + ) + + inbound_number_on = dao_get_inbound_number(inbound_number.id) + assert inbound_number_on.active From c81b1aa6a09f973bc9f21d5e1dd1a0125abfdec3 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 11 Aug 2017 10:32:22 +0100 Subject: [PATCH 07/26] Correct test for inbound_number --- tests/app/inbound_number/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py index 88b2b5661..2ece65883 100644 --- a/tests/app/inbound_number/test_rest.py +++ b/tests/app/inbound_number/test_rest.py @@ -88,7 +88,7 @@ def test_rest_set_inbound_number_active_flag_off(admin_request, notify_db_sessio def test_rest_set_inbound_number_active_flag_on(admin_request, notify_db_session): service = create_service(service_name='test service 1') - inbound_number = create_inbound_number(number='1', provider='mmg', active=True, service_id=service.id) + inbound_number = create_inbound_number(number='1', provider='mmg', active=False, service_id=service.id) admin_request.post( 'inbound_number.post_set_inbound_number_on', From 0c4c467cacac7ef62c319cd26a787f5fe6b57039 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 11 Aug 2017 12:51:52 +0100 Subject: [PATCH 08/26] Added extra tests after refactoring --- app/inbound_number/rest.py | 38 ++++++------- tests/app/dao/test_inbound_numbers_dao.py | 2 +- tests/app/inbound_number/test_rest.py | 67 ++++++++++++----------- 3 files changed, 53 insertions(+), 54 deletions(-) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index f2d347c01..f95fce48b 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -8,7 +8,7 @@ from app.dao.inbound_numbers_dao import ( dao_set_inbound_number_to_service, dao_set_inbound_number_active_flag ) -from app.errors import register_errors +from app.errors import InvalidRequest, register_errors from app.models import InboundNumber from app.schema_validation import validate @@ -30,32 +30,28 @@ def get_inbound_numbers_available(): return jsonify(data=inbound_numbers) -@inbound_number_blueprint.route('/service/', methods=['POST']) -def post_allocate_inbound_number(service_id): +@inbound_number_blueprint.route('/service/', methods=['GET']) +def get_inbound_number_for_service(service_id): inbound_number = dao_get_inbound_number_for_service(service_id) - if not inbound_number: - available_numbers = dao_get_available_inbound_numbers() - - if len(available_numbers) > 0: - dao_set_inbound_number_to_service(service_id, available_numbers[0]) - return '', 204 - else: - return '', 409 - else: - dao_set_inbound_number_active_flag(service_id, active=True) - return '', 204 + return jsonify(data=inbound_number.serialize()) @inbound_number_blueprint.route('//service/', methods=['POST']) def post_set_inbound_number_for_service(inbound_number_id, service_id): - try: - dao_set_inbound_number_to_service(service_id, inbound_number_id) - except TypeError as e: - if str(e) == 'UUID objects are immutable': - return '', 409 - else: - raise e + if len(dao_get_available_inbound_numbers()) == 0: + raise InvalidRequest('No inbound numbers available', status_code=400) + + inbound_number = dao_get_inbound_number_for_service(service_id) + if inbound_number: + raise InvalidRequest('Service already has an inbound number', status_code=400) + + inbound_number = dao_get_inbound_number(inbound_number_id) + if inbound_number.service_id: + raise InvalidRequest('Inbound number already assigned', status_code=400) + + dao_set_inbound_number_to_service(service_id, inbound_number_id) + return '', 204 diff --git a/tests/app/dao/test_inbound_numbers_dao.py b/tests/app/dao/test_inbound_numbers_dao.py index d38622d86..9d59a8110 100644 --- a/tests/app/dao/test_inbound_numbers_dao.py +++ b/tests/app/dao/test_inbound_numbers_dao.py @@ -76,6 +76,6 @@ def test_set_inbound_number_active_flag(notify_db, notify_db_session, sample_ser dao_set_inbound_number_active_flag(inbound_number.id, active=active) - inbound_number = dao_get_inbound_number(inbound_number.id) + inbound_number = dao_get_inbound_number_for_service(sample_service.id) assert inbound_number.active is active diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py index 2ece65883..aca7c4c3a 100644 --- a/tests/app/inbound_number/test_rest.py +++ b/tests/app/inbound_number/test_rest.py @@ -4,7 +4,6 @@ import json from app.models import InboundNumber from app.dao.inbound_numbers_dao import ( dao_get_inbound_numbers, - dao_get_inbound_number, dao_get_available_inbound_numbers, dao_get_inbound_number_for_service, dao_set_inbound_number_to_service @@ -20,56 +19,60 @@ def test_rest_get_inbound_numbers(admin_request, sample_inbound_numbers): assert result['data'] == [i.serialize() for i in sample_inbound_numbers] -def test_rest_allocate_inbound_number(admin_request, notify_db_session, sample_inbound_numbers): - service = create_service(service_name='test service') - admin_request.post( - 'inbound_number.post_allocate_inbound_number', - _expected_status=204, - service_id=service.id +def test_rest_get_inbound_number(admin_request, notify_db_session, sample_service): + inbound_number = create_inbound_number(number='1', provider='mmg', active=False, service_id=sample_service.id) + + result = admin_request.get( + 'inbound_number.get_inbound_number_for_service', + service_id=sample_service.id ) + assert result['data'] == inbound_number.serialize() -def test_rest_allocate_inbound_number_when_no_inbound_available_returns_409( +def test_rest_set_service_to_several_inbound_numbers_returns_400( admin_request, notify_db_session, sample_service): service_1 = create_service(service_name='test service 1') - create_inbound_number(number='4', provider='mmg', active=False, service_id=sample_service.id) - create_inbound_number(number='5', provider='mmg', active=True, service_id=service_1.id) + create_inbound_number(number='1', provider='mmg', active=False, service_id=sample_service.id) + inbound_number = create_inbound_number(number='2', provider='mmg', active=True, service_id=service_1.id) service_2 = create_service(service_name='test service 2') - admin_request.post( - 'inbound_number.post_allocate_inbound_number', - _expected_status=409, - service_id=service_2.id - ) - - -def test_rest_set_service_to_several_inbound_numbers_returns_409( - admin_request, notify_db_session, sample_service): - service_1 = create_service(service_name='test service 1') - create_inbound_number(number='4', provider='mmg', active=False, service_id=sample_service.id) - inbound_number = create_inbound_number(number='5', provider='mmg', active=True, service_id=service_1.id) - service_2 = create_service(service_name='test service 2') - - admin_request.post( + result = admin_request.post( 'inbound_number.post_set_inbound_number_for_service', - _expected_status=409, + _expected_status=400, inbound_number_id=inbound_number.id, service_id=service_2.id ) + assert result['message'] == 'No inbound numbers available' -def test_rest_set_number_to_several_services_returns_409( +def test_rest_set_number_to_several_services_returns_400( admin_request, notify_db_session, sample_service): service_1 = create_service(service_name='test service 1') - inbound_number = create_inbound_number(number='4', provider='mmg', active=False, service_id=sample_service.id) + inbound_number = create_inbound_number(number='1', provider='mmg', active=True, service_id=sample_service.id) + create_inbound_number(number='2', provider='mmg', active=True, service_id=None) service_2 = create_service(service_name='test service 2') - admin_request.post( + result = admin_request.post( 'inbound_number.post_set_inbound_number_for_service', - _expected_status=409, + _expected_status=400, inbound_number_id=inbound_number.id, service_id=service_2.id ) + assert result['message'] == 'Inbound number already assigned' + + +def test_rest_set_multiple_number_to_a_service_returns_400( + admin_request, notify_db_session, sample_service): + create_inbound_number(number='1', provider='mmg', active=True, service_id=sample_service.id) + inbound_number = create_inbound_number(number='2', provider='mmg', active=True, service_id=None) + + result = admin_request.post( + 'inbound_number.post_set_inbound_number_for_service', + _expected_status=400, + inbound_number_id=inbound_number.id, + service_id=sample_service.id + ) + assert result['message'] == 'Service already has an inbound number' def test_rest_set_inbound_number_active_flag_off(admin_request, notify_db_session): @@ -82,7 +85,7 @@ def test_rest_set_inbound_number_active_flag_off(admin_request, notify_db_sessio inbound_number_id=inbound_number.id ) - inbound_number_off = dao_get_inbound_number(inbound_number.id) + inbound_number_off = dao_get_inbound_number_for_service(service.id) assert not inbound_number_off.active @@ -96,5 +99,5 @@ def test_rest_set_inbound_number_active_flag_on(admin_request, notify_db_session inbound_number_id=inbound_number.id ) - inbound_number_on = dao_get_inbound_number(inbound_number.id) + inbound_number_on = dao_get_inbound_number_for_service(service.id) assert inbound_number_on.active From d2a618cf0a0daf4f1df531f9ec1a4c5ddbc1bd81 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 11 Aug 2017 16:10:52 +0100 Subject: [PATCH 09/26] Refactor set_inbound_number_for_service As don't need the check for available inbound numbers --- app/inbound_number/rest.py | 3 -- tests/app/inbound_number/test_rest.py | 47 +++++++-------------------- 2 files changed, 11 insertions(+), 39 deletions(-) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index f95fce48b..c43522c49 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -39,9 +39,6 @@ def get_inbound_number_for_service(service_id): @inbound_number_blueprint.route('//service/', methods=['POST']) def post_set_inbound_number_for_service(inbound_number_id, service_id): - if len(dao_get_available_inbound_numbers()) == 0: - raise InvalidRequest('No inbound numbers available', status_code=400) - inbound_number = dao_get_inbound_number_for_service(service_id) if inbound_number: raise InvalidRequest('Service already has an inbound number', status_code=400) diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py index aca7c4c3a..94cd4d249 100644 --- a/tests/app/inbound_number/test_rest.py +++ b/tests/app/inbound_number/test_rest.py @@ -1,3 +1,5 @@ +import pytest + from flask import url_for import json @@ -29,22 +31,6 @@ def test_rest_get_inbound_number(admin_request, notify_db_session, sample_servic assert result['data'] == inbound_number.serialize() -def test_rest_set_service_to_several_inbound_numbers_returns_400( - admin_request, notify_db_session, sample_service): - service_1 = create_service(service_name='test service 1') - create_inbound_number(number='1', provider='mmg', active=False, service_id=sample_service.id) - inbound_number = create_inbound_number(number='2', provider='mmg', active=True, service_id=service_1.id) - service_2 = create_service(service_name='test service 2') - - result = admin_request.post( - 'inbound_number.post_set_inbound_number_for_service', - _expected_status=400, - inbound_number_id=inbound_number.id, - service_id=service_2.id - ) - assert result['message'] == 'No inbound numbers available' - - def test_rest_set_number_to_several_services_returns_400( admin_request, notify_db_session, sample_service): service_1 = create_service(service_name='test service 1') @@ -61,7 +47,7 @@ def test_rest_set_number_to_several_services_returns_400( assert result['message'] == 'Inbound number already assigned' -def test_rest_set_multiple_number_to_a_service_returns_400( +def test_rest_set_multiple_numbers_to_a_service_returns_400( admin_request, notify_db_session, sample_service): create_inbound_number(number='1', provider='mmg', active=True, service_id=sample_service.id) inbound_number = create_inbound_number(number='2', provider='mmg', active=True, service_id=None) @@ -75,29 +61,18 @@ def test_rest_set_multiple_number_to_a_service_returns_400( assert result['message'] == 'Service already has an inbound number' -def test_rest_set_inbound_number_active_flag_off(admin_request, notify_db_session): +@pytest.mark.parametrize("active_flag,expected_flag_state", [("on", True), ("off", False)]) +def test_rest_set_inbound_number_active_flag( + admin_request, notify_db_session, active_flag, expected_flag_state): service = create_service(service_name='test service 1') - inbound_number = create_inbound_number(number='1', provider='mmg', active=True, service_id=service.id) + inbound_number = create_inbound_number( + number='1', provider='mmg', active=not expected_flag_state, service_id=service.id) admin_request.post( - 'inbound_number.post_set_inbound_number_off', + 'inbound_number.post_set_inbound_number_{}'.format(active_flag), _expected_status=204, inbound_number_id=inbound_number.id ) - inbound_number_off = dao_get_inbound_number_for_service(service.id) - assert not inbound_number_off.active - - -def test_rest_set_inbound_number_active_flag_on(admin_request, notify_db_session): - service = create_service(service_name='test service 1') - inbound_number = create_inbound_number(number='1', provider='mmg', active=False, service_id=service.id) - - admin_request.post( - 'inbound_number.post_set_inbound_number_on', - _expected_status=204, - inbound_number_id=inbound_number.id - ) - - inbound_number_on = dao_get_inbound_number_for_service(service.id) - assert inbound_number_on.active + inbound_number_from_db = dao_get_inbound_number_for_service(service.id) + assert inbound_number_from_db.active == expected_flag_state From 22956b74275623550f2d3b20ff3a6efe0cc5abee Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 11 Aug 2017 17:28:16 +0100 Subject: [PATCH 10/26] Refactor code --- app/dao/inbound_numbers_dao.py | 1 + app/inbound_number/rest.py | 12 +++++------ tests/app/inbound_number/test_rest.py | 29 +++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/app/dao/inbound_numbers_dao.py b/app/dao/inbound_numbers_dao.py index 085ed3f22..1c2bcacbf 100644 --- a/app/dao/inbound_numbers_dao.py +++ b/app/dao/inbound_numbers_dao.py @@ -21,6 +21,7 @@ def dao_get_inbound_number(inbound_number_id): @transactional def dao_set_inbound_number_to_service(service_id, inbound_number): + print('set: {}'.format(inbound_number.id)) inbound_number.service_id = service_id db.session.add(inbound_number) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index c43522c49..336ec57ae 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -20,21 +20,21 @@ register_errors(inbound_number_blueprint) def get_inbound_numbers(): inbound_numbers = [i.serialize() for i in dao_get_inbound_numbers()] - return jsonify(data=inbound_numbers) + return jsonify(data=inbound_numbers if inbound_numbers else None) @inbound_number_blueprint.route('/available', methods=['GET']) -def get_inbound_numbers_available(): +def get_next_available_inbound_numbers(): inbound_numbers = [i.serialize() for i in dao_get_available_inbound_numbers()] - return jsonify(data=inbound_numbers) + return jsonify(data=inbound_numbers[0] if len(inbound_numbers) else []) @inbound_number_blueprint.route('/service/', methods=['GET']) def get_inbound_number_for_service(service_id): inbound_number = dao_get_inbound_number_for_service(service_id) - return jsonify(data=inbound_number.serialize()) + return jsonify(data=inbound_number.serialize() if inbound_number else None) @inbound_number_blueprint.route('//service/', methods=['POST']) @@ -47,12 +47,12 @@ def post_set_inbound_number_for_service(inbound_number_id, service_id): if inbound_number.service_id: raise InvalidRequest('Inbound number already assigned', status_code=400) - dao_set_inbound_number_to_service(service_id, inbound_number_id) + dao_set_inbound_number_to_service(service_id, inbound_number) return '', 204 -@inbound_number_blueprint.route('/service//on', methods=['POST']) +@inbound_number_blueprint.route('//on', methods=['POST']) def post_set_inbound_number_on(inbound_number_id): dao_set_inbound_number_active_flag(inbound_number_id, active=True) return '', 204 diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py index 94cd4d249..8b55fd96b 100644 --- a/tests/app/inbound_number/test_rest.py +++ b/tests/app/inbound_number/test_rest.py @@ -21,6 +21,16 @@ def test_rest_get_inbound_numbers(admin_request, sample_inbound_numbers): assert result['data'] == [i.serialize() for i in sample_inbound_numbers] +def test_rest_get_next_available_inbound_numbers(admin_request, sample_service): + create_inbound_number(number='1', provider='mmg', active=False, service_id=sample_service.id) + next_available_inbound_number = create_inbound_number(number='2', provider='mmg', active=True) + create_inbound_number(number='3', provider='firetext', active=True) + + result = admin_request.get('inbound_number.get_next_available_inbound_numbers') + + assert result['data'] == next_available_inbound_number.serialize() + + def test_rest_get_inbound_number(admin_request, notify_db_session, sample_service): inbound_number = create_inbound_number(number='1', provider='mmg', active=False, service_id=sample_service.id) @@ -31,6 +41,25 @@ def test_rest_get_inbound_number(admin_request, notify_db_session, sample_servic assert result['data'] == inbound_number.serialize() +def test_rest_set_number_to_service( + admin_request, notify_db_session, sample_service): + service = create_service(service_name='test service 1') + inbound_number = create_inbound_number(number='1', provider='mmg', active=True) + + result = admin_request.post( + 'inbound_number.post_set_inbound_number_for_service', + _expected_status=204, + inbound_number_id=inbound_number.id, + service_id=service.id + ) + + inbound_number_from_db = dao_get_inbound_number_for_service(service.id) + + assert inbound_number_from_db.active + assert inbound_number_from_db.id == inbound_number.id + assert inbound_number_from_db.number == inbound_number.number + + def test_rest_set_number_to_several_services_returns_400( admin_request, notify_db_session, sample_service): service_1 = create_service(service_name='test service 1') From faae6e35378c809d4c32dcb51d439d69291d4e73 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 11 Aug 2017 17:52:41 +0100 Subject: [PATCH 11/26] Update inbound_number number col size --- app/models.py | 2 +- migrations/env.py | 3 +- .../versions/0116_alter_number_col_size.py | 28 +++++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/0116_alter_number_col_size.py diff --git a/app/models.py b/app/models.py index 9cd0b12b6..b93f23bc6 100644 --- a/app/models.py +++ b/app/models.py @@ -246,7 +246,7 @@ class InboundNumber(db.Model): __tablename__ = "inbound_numbers" id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - number = db.Column(db.String(11), unique=True, nullable=False) + number = db.Column(db.String(12), unique=True, nullable=False) provider = db.Column(db.String(), nullable=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=True, index=True, nullable=True) service = db.relationship(Service, backref=db.backref("inbound_number", uselist=False)) diff --git a/migrations/env.py b/migrations/env.py index 70961ce2c..e69a205ac 100644 --- a/migrations/env.py +++ b/migrations/env.py @@ -57,7 +57,8 @@ def run_migrations_online(): connection = engine.connect() context.configure( connection=connection, - target_metadata=target_metadata + target_metadata=target_metadata, + compare_type=True ) try: diff --git a/migrations/versions/0116_alter_number_col_size.py b/migrations/versions/0116_alter_number_col_size.py new file mode 100644 index 000000000..d5d07c9e9 --- /dev/null +++ b/migrations/versions/0116_alter_number_col_size.py @@ -0,0 +1,28 @@ +"""empty message + +Revision ID: 0116_alter_number_col_size +Revises: 0115_add_inbound_numbers +Create Date: 2017-08-11 17:48:30.358584 + +""" + +# revision identifiers, used by Alembic. +revision = '0116_alter_number_col_size' +down_revision = '0115_add_inbound_numbers' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.alter_column('inbound_numbers', 'number', + existing_type=sa.VARCHAR(length=11), + type_=sa.String(length=12), + existing_nullable=False) + + +def downgrade(): + op.alter_column('inbound_numbers', 'number', + existing_type=sa.String(length=12), + type_=sa.VARCHAR(length=11), + existing_nullable=False) From 6f12b760f3b113f1639c831a0a7cb212c1be805a Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 14 Aug 2017 12:13:45 +0100 Subject: [PATCH 12/26] Removed print --- app/dao/inbound_numbers_dao.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/dao/inbound_numbers_dao.py b/app/dao/inbound_numbers_dao.py index 1c2bcacbf..085ed3f22 100644 --- a/app/dao/inbound_numbers_dao.py +++ b/app/dao/inbound_numbers_dao.py @@ -21,7 +21,6 @@ def dao_get_inbound_number(inbound_number_id): @transactional def dao_set_inbound_number_to_service(service_id, inbound_number): - print('set: {}'.format(inbound_number.id)) inbound_number.service_id = service_id db.session.add(inbound_number) From f0e9b931360276d34062701f3f9695f3c7fb2ee0 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 14 Aug 2017 12:14:01 +0100 Subject: [PATCH 13/26] Update path to use dash --- app/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/__init__.py b/app/__init__.py index cd293ad8a..c2b53d404 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -136,7 +136,7 @@ def register_blueprint(application): application.register_blueprint(delivery_blueprint) inbound_number_blueprint.before_request(requires_admin_auth) - application.register_blueprint(inbound_number_blueprint, url_prefix='/inbound_number') + application.register_blueprint(inbound_number_blueprint, url_prefix='/inbound-number') inbound_sms_blueprint.before_request(requires_admin_auth) application.register_blueprint(inbound_sms_blueprint) From 7f1de195928110e93a7ea5a77d1ac34d027f4091 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 14 Aug 2017 12:17:14 +0100 Subject: [PATCH 14/26] Refactored to put logic into API --- app/inbound_number/rest.py | 45 +++++-------- tests/app/inbound_number/test_rest.py | 94 +++++++++++++++------------ 2 files changed, 69 insertions(+), 70 deletions(-) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index 336ec57ae..b064f74e6 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -1,16 +1,13 @@ -from flask import Blueprint, jsonify, request +from flask import Blueprint, jsonify from app.dao.inbound_numbers_dao import ( dao_get_inbound_numbers, - dao_get_inbound_number, dao_get_inbound_number_for_service, dao_get_available_inbound_numbers, dao_set_inbound_number_to_service, dao_set_inbound_number_active_flag ) from app.errors import InvalidRequest, register_errors -from app.models import InboundNumber -from app.schema_validation import validate inbound_number_blueprint = Blueprint('inbound_number', __name__) register_errors(inbound_number_blueprint) @@ -20,42 +17,34 @@ register_errors(inbound_number_blueprint) def get_inbound_numbers(): inbound_numbers = [i.serialize() for i in dao_get_inbound_numbers()] - return jsonify(data=inbound_numbers if inbound_numbers else None) - - -@inbound_number_blueprint.route('/available', methods=['GET']) -def get_next_available_inbound_numbers(): - inbound_numbers = [i.serialize() for i in dao_get_available_inbound_numbers()] - - return jsonify(data=inbound_numbers[0] if len(inbound_numbers) else []) + return jsonify(data=inbound_numbers if inbound_numbers else []) @inbound_number_blueprint.route('/service/', methods=['GET']) def get_inbound_number_for_service(service_id): inbound_number = dao_get_inbound_number_for_service(service_id) - return jsonify(data=inbound_number.serialize() if inbound_number else None) + return jsonify(data=inbound_number.serialize() if inbound_number else {}) -@inbound_number_blueprint.route('//service/', methods=['POST']) -def post_set_inbound_number_for_service(inbound_number_id, service_id): +@inbound_number_blueprint.route('/service/', methods=['POST']) +def post_allocate_inbound_number(service_id): inbound_number = dao_get_inbound_number_for_service(service_id) + if inbound_number: - raise InvalidRequest('Service already has an inbound number', status_code=400) + if not inbound_number.active: + dao_set_inbound_number_active_flag(inbound_number.id, active=True) + return '', 204 + else: + return '', 200 - inbound_number = dao_get_inbound_number(inbound_number_id) - if inbound_number.service_id: - raise InvalidRequest('Inbound number already assigned', status_code=400) + available_numbers = dao_get_available_inbound_numbers() - dao_set_inbound_number_to_service(service_id, inbound_number) - - return '', 204 - - -@inbound_number_blueprint.route('//on', methods=['POST']) -def post_set_inbound_number_on(inbound_number_id): - dao_set_inbound_number_active_flag(inbound_number_id, active=True) - return '', 204 + if len(available_numbers) > 0: + dao_set_inbound_number_to_service(service_id, available_numbers[0]) + return '', 204 + else: + raise InvalidRequest('No available inbound numbers', status_code=400) @inbound_number_blueprint.route('//off', methods=['POST']) diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py index 8b55fd96b..3baba5e3b 100644 --- a/tests/app/inbound_number/test_rest.py +++ b/tests/app/inbound_number/test_rest.py @@ -4,16 +4,17 @@ from flask import url_for import json from app.models import InboundNumber -from app.dao.inbound_numbers_dao import ( - dao_get_inbound_numbers, - dao_get_available_inbound_numbers, - dao_get_inbound_number_for_service, - dao_set_inbound_number_to_service -) +from app.dao.inbound_numbers_dao import dao_get_inbound_number_for_service from tests.app.db import create_service, create_inbound_number +def test_rest_get_inbound_numbers_when_none_set_returns_empty_list(admin_request): + result = admin_request.get('inbound_number.get_inbound_numbers') + + assert result['data'] == [] + + def test_rest_get_inbound_numbers(admin_request, sample_inbound_numbers): result = admin_request.get('inbound_number.get_inbound_numbers') @@ -21,16 +22,6 @@ def test_rest_get_inbound_numbers(admin_request, sample_inbound_numbers): assert result['data'] == [i.serialize() for i in sample_inbound_numbers] -def test_rest_get_next_available_inbound_numbers(admin_request, sample_service): - create_inbound_number(number='1', provider='mmg', active=False, service_id=sample_service.id) - next_available_inbound_number = create_inbound_number(number='2', provider='mmg', active=True) - create_inbound_number(number='3', provider='firetext', active=True) - - result = admin_request.get('inbound_number.get_next_available_inbound_numbers') - - assert result['data'] == next_available_inbound_number.serialize() - - def test_rest_get_inbound_number(admin_request, notify_db_session, sample_service): inbound_number = create_inbound_number(number='1', provider='mmg', active=False, service_id=sample_service.id) @@ -41,15 +32,23 @@ def test_rest_get_inbound_number(admin_request, notify_db_session, sample_servic assert result['data'] == inbound_number.serialize() -def test_rest_set_number_to_service( +def test_rest_get_inbound_number_when_service_is_not_assigned_returns_empty_dict( + admin_request, notify_db_session, sample_service): + result = admin_request.get( + 'inbound_number.get_inbound_number_for_service', + service_id=sample_service.id + ) + assert result['data'] == {} + + +def test_rest_allocate_inbound_number_to_service( admin_request, notify_db_session, sample_service): service = create_service(service_name='test service 1') inbound_number = create_inbound_number(number='1', provider='mmg', active=True) result = admin_request.post( - 'inbound_number.post_set_inbound_number_for_service', + 'inbound_number.post_allocate_inbound_number', _expected_status=204, - inbound_number_id=inbound_number.id, service_id=service.id ) @@ -60,48 +59,59 @@ def test_rest_set_number_to_service( assert inbound_number_from_db.number == inbound_number.number -def test_rest_set_number_to_several_services_returns_400( +def test_rest_allocate_inbound_number_to_service_raises_400_when_no_available_numbers( admin_request, notify_db_session, sample_service): - service_1 = create_service(service_name='test service 1') - inbound_number = create_inbound_number(number='1', provider='mmg', active=True, service_id=sample_service.id) - create_inbound_number(number='2', provider='mmg', active=True, service_id=None) - service_2 = create_service(service_name='test service 2') + service = create_service(service_name='test service 1') + create_inbound_number(number='1', provider='mmg', active=False) result = admin_request.post( - 'inbound_number.post_set_inbound_number_for_service', + 'inbound_number.post_allocate_inbound_number', _expected_status=400, - inbound_number_id=inbound_number.id, - service_id=service_2.id + service_id=service.id ) - assert result['message'] == 'Inbound number already assigned' + + assert result['message'] == 'No available inbound numbers' -def test_rest_set_multiple_numbers_to_a_service_returns_400( +def test_rest_allocate_inbound_number_to_service_sets_active_flag_true_when_flag_is_false( admin_request, notify_db_session, sample_service): - create_inbound_number(number='1', provider='mmg', active=True, service_id=sample_service.id) - inbound_number = create_inbound_number(number='2', provider='mmg', active=True, service_id=None) + service = create_service(service_name='test service 1') + create_inbound_number(number='1', provider='mmg', active=False, service_id=service.id) result = admin_request.post( - 'inbound_number.post_set_inbound_number_for_service', - _expected_status=400, - inbound_number_id=inbound_number.id, - service_id=sample_service.id + 'inbound_number.post_allocate_inbound_number', + _expected_status=204, + service_id=service.id ) - assert result['message'] == 'Service already has an inbound number' + + inbound_number = dao_get_inbound_number_for_service(service.id) + + assert inbound_number.active -@pytest.mark.parametrize("active_flag,expected_flag_state", [("on", True), ("off", False)]) -def test_rest_set_inbound_number_active_flag( - admin_request, notify_db_session, active_flag, expected_flag_state): +def test_rest_allocate_inbound_number_to_service_sets_active_flag_true_when_flag_is_true( + admin_request, notify_db_session, sample_service): + service = create_service(service_name='test service 1') + create_inbound_number(number='1', provider='mmg', active=True, service_id=service.id) + + result = admin_request.post( + 'inbound_number.post_allocate_inbound_number', + _expected_status=200, + service_id=service.id + ) + + +def test_rest_set_inbound_number_active_flag_off( + admin_request, notify_db_session): service = create_service(service_name='test service 1') inbound_number = create_inbound_number( - number='1', provider='mmg', active=not expected_flag_state, service_id=service.id) + number='1', provider='mmg', active=True, service_id=service.id) admin_request.post( - 'inbound_number.post_set_inbound_number_{}'.format(active_flag), + 'inbound_number.post_set_inbound_number_off', _expected_status=204, inbound_number_id=inbound_number.id ) inbound_number_from_db = dao_get_inbound_number_for_service(service.id) - assert inbound_number_from_db.active == expected_flag_state + assert not inbound_number_from_db.active From 81754712ce6d7625e1f377b7f9620f987d7291d6 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 14 Aug 2017 12:39:50 +0100 Subject: [PATCH 15/26] Removed number col size migration --- app/models.py | 2 +- .../versions/0116_alter_number_col_size.py | 28 ------------------- 2 files changed, 1 insertion(+), 29 deletions(-) delete mode 100644 migrations/versions/0116_alter_number_col_size.py diff --git a/app/models.py b/app/models.py index b93f23bc6..9cd0b12b6 100644 --- a/app/models.py +++ b/app/models.py @@ -246,7 +246,7 @@ class InboundNumber(db.Model): __tablename__ = "inbound_numbers" id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - number = db.Column(db.String(12), unique=True, nullable=False) + number = db.Column(db.String(11), unique=True, nullable=False) provider = db.Column(db.String(), nullable=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=True, index=True, nullable=True) service = db.relationship(Service, backref=db.backref("inbound_number", uselist=False)) diff --git a/migrations/versions/0116_alter_number_col_size.py b/migrations/versions/0116_alter_number_col_size.py deleted file mode 100644 index d5d07c9e9..000000000 --- a/migrations/versions/0116_alter_number_col_size.py +++ /dev/null @@ -1,28 +0,0 @@ -"""empty message - -Revision ID: 0116_alter_number_col_size -Revises: 0115_add_inbound_numbers -Create Date: 2017-08-11 17:48:30.358584 - -""" - -# revision identifiers, used by Alembic. -revision = '0116_alter_number_col_size' -down_revision = '0115_add_inbound_numbers' - -from alembic import op -import sqlalchemy as sa -from sqlalchemy.dialects import postgresql - -def upgrade(): - op.alter_column('inbound_numbers', 'number', - existing_type=sa.VARCHAR(length=11), - type_=sa.String(length=12), - existing_nullable=False) - - -def downgrade(): - op.alter_column('inbound_numbers', 'number', - existing_type=sa.String(length=12), - type_=sa.VARCHAR(length=11), - existing_nullable=False) From 4eba6335d6111d5a737e4f979512224313e43e8d Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 14 Aug 2017 12:46:04 +0100 Subject: [PATCH 16/26] Moved url_prefix to inbound_number rest.py --- app/__init__.py | 2 +- app/inbound_number/rest.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index c2b53d404..a1bdf8c16 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -136,7 +136,7 @@ def register_blueprint(application): application.register_blueprint(delivery_blueprint) inbound_number_blueprint.before_request(requires_admin_auth) - application.register_blueprint(inbound_number_blueprint, url_prefix='/inbound-number') + application.register_blueprint(inbound_number_blueprint) inbound_sms_blueprint.before_request(requires_admin_auth) application.register_blueprint(inbound_sms_blueprint) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index b064f74e6..3f21d9551 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -9,7 +9,7 @@ from app.dao.inbound_numbers_dao import ( ) from app.errors import InvalidRequest, register_errors -inbound_number_blueprint = Blueprint('inbound_number', __name__) +inbound_number_blueprint = Blueprint('inbound_number', __name__, url_prefix='/inbound-number') register_errors(inbound_number_blueprint) From e14fa2d87e0243ef4436bcc461d6b5bdb9bc185a Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 14 Aug 2017 18:20:25 +0100 Subject: [PATCH 17/26] Refactored to use service_id to set flag --- app/dao/inbound_numbers_dao.py | 4 ++-- app/inbound_number/rest.py | 8 ++++---- tests/app/dao/test_inbound_numbers_dao.py | 2 +- tests/app/inbound_number/test_rest.py | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/dao/inbound_numbers_dao.py b/app/dao/inbound_numbers_dao.py index 085ed3f22..fa30258c2 100644 --- a/app/dao/inbound_numbers_dao.py +++ b/app/dao/inbound_numbers_dao.py @@ -26,8 +26,8 @@ def dao_set_inbound_number_to_service(service_id, inbound_number): @transactional -def dao_set_inbound_number_active_flag(inbound_number_id, active): - inbound_number = InboundNumber.query.filter(InboundNumber.id == inbound_number_id).first() +def dao_set_inbound_number_active_flag(service_id, active): + inbound_number = InboundNumber.query.filter(InboundNumber.service_id == service_id).first() inbound_number.active = active db.session.add(inbound_number) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index 3f21d9551..3fba96774 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -33,7 +33,7 @@ def post_allocate_inbound_number(service_id): if inbound_number: if not inbound_number.active: - dao_set_inbound_number_active_flag(inbound_number.id, active=True) + dao_set_inbound_number_active_flag(service_id, active=True) return '', 204 else: return '', 200 @@ -47,7 +47,7 @@ def post_allocate_inbound_number(service_id): raise InvalidRequest('No available inbound numbers', status_code=400) -@inbound_number_blueprint.route('//off', methods=['POST']) -def post_set_inbound_number_off(inbound_number_id): - dao_set_inbound_number_active_flag(inbound_number_id, active=False) +@inbound_number_blueprint.route('/service//off', methods=['POST']) +def post_set_inbound_number_off(service_id): + dao_set_inbound_number_active_flag(service_id, active=False) return '', 204 diff --git a/tests/app/dao/test_inbound_numbers_dao.py b/tests/app/dao/test_inbound_numbers_dao.py index 9d59a8110..f55dda956 100644 --- a/tests/app/dao/test_inbound_numbers_dao.py +++ b/tests/app/dao/test_inbound_numbers_dao.py @@ -74,7 +74,7 @@ def test_set_inbound_number_active_flag(notify_db, notify_db_session, sample_ser inbound_number = create_inbound_number(number='1') dao_set_inbound_number_to_service(sample_service.id, inbound_number) - dao_set_inbound_number_active_flag(inbound_number.id, active=active) + dao_set_inbound_number_active_flag(sample_service.id, active=active) inbound_number = dao_get_inbound_number_for_service(sample_service.id) diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py index 3baba5e3b..a5268f283 100644 --- a/tests/app/inbound_number/test_rest.py +++ b/tests/app/inbound_number/test_rest.py @@ -110,7 +110,7 @@ def test_rest_set_inbound_number_active_flag_off( admin_request.post( 'inbound_number.post_set_inbound_number_off', _expected_status=204, - inbound_number_id=inbound_number.id + service_id=service.id ) inbound_number_from_db = dao_get_inbound_number_for_service(service.id) From 667ee57a35a20811658870db6965ac1c1140976f Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 14 Aug 2017 19:47:09 +0100 Subject: [PATCH 18/26] Refactor code to use inbound_numbers if set --- app/delivery/send_to_providers.py | 2 +- app/models.py | 6 +++++ app/notifications/receive_notifications.py | 2 +- app/v2/notifications/post_notifications.py | 3 +-- tests/app/delivery/test_send_to_providers.py | 24 ++++++++++++++++- .../test_receive_notification.py | 27 ++++++++++++++++++- tests/app/test_model.py | 21 +++++++++++++++ .../notifications/test_post_notifications.py | 25 ++++++++++++++++- 8 files changed, 103 insertions(+), 7 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index fcf4348d8..c9aa84a20 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -51,7 +51,7 @@ def send_sms_to_provider(notification): to=validate_and_format_phone_number(notification.to, international=notification.international), content=str(template), reference=str(notification.id), - sender=service.sms_sender or current_app.config['FROM_NUMBER'] + sender=service.get_inbound_number() ) except Exception as e: dao_toggle_sms_provider(provider.name) diff --git a/app/models.py b/app/models.py index 9cd0b12b6..e0a0ad474 100644 --- a/app/models.py +++ b/app/models.py @@ -241,6 +241,12 @@ class Service(db.Model, Versioned): return cls(**fields) + def get_inbound_number(self): + if self.inbound_number and self.inbound_number.active: + return self.inbound_number.number + else: + return self.sms_sender or current_app.config['FROM_NUMBER'] + class InboundNumber(db.Model): __tablename__ = "inbound_numbers" diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index 34e726cf1..4fd3d33c2 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -107,7 +107,7 @@ def create_inbound_sms_object(service, content, from_number, provider_ref, date_ inbound = InboundSms( service=service, - notify_number=service.sms_sender, + notify_number=service.get_inbound_number(), user_number=user_number, provider_date=provider_date, provider_reference=provider_ref, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index d18bb3b4d..c27ca8dc5 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -80,10 +80,9 @@ def post_notification(notification_type): ) if notification_type == SMS_TYPE: - sms_sender = authenticated_service.sms_sender or current_app.config.get('FROM_NUMBER') create_resp_partial = functools.partial( create_post_sms_response_from_notification, - from_number=sms_sender + from_number=authenticated_service.get_inbound_number() ) elif notification_type == EMAIL_TYPE: create_resp_partial = functools.partial( diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index d14fee6d8..35368becf 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -21,7 +21,7 @@ from app.models import ( BRANDING_ORG, BRANDING_BOTH) -from tests.app.db import create_service, create_template, create_notification +from tests.app.db import create_service, create_template, create_notification, create_inbound_number def test_should_return_highest_priority_active_provider(restore_provider_details): @@ -643,3 +643,25 @@ def test_should_handle_sms_sender_and_prefix_message( to=ANY, reference=ANY, ) + + +def test_should_use_inbound_number_as_sender_if_set( + sample_service, + mocker +): + sample_service.sms_sender = 'test sender' + template = create_template(sample_service, content='bar') + notification = create_notification(template) + inbound_number = create_inbound_number('1', service_id=sample_service.id) + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') + + send_to_providers.send_sms_to_provider(notification) + + mmg_client.send_sms.assert_called_once_with( + to=ANY, + content=ANY, + reference=str(notification.id), + sender=inbound_number.number + ) diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 36392689f..0b99d7cbb 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -15,7 +15,7 @@ from app.notifications.receive_notifications import ( ) from app.models import InboundSms, EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE -from tests.app.db import create_service +from tests.app.db import create_inbound_number, create_service from tests.app.conftest import sample_service @@ -145,6 +145,31 @@ def test_create_inbound_mmg_sms_object(sample_service_full_permissions): assert inbound_sms.provider == 'mmg' +def test_create_inbound_mmg_sms_object_uses_inbound_number_if_set(sample_service_full_permissions): + sample_service_full_permissions.sms_sender = 'foo' + inbound_number = create_inbound_number('1', service_id=sample_service_full_permissions.id) + + data = { + 'Message': 'hello+there+%F0%9F%93%A9', + 'Number': 'foo', + 'MSISDN': '07700 900 001', + 'DateRecieved': '2017-01-02+03%3A04%3A05', + 'ID': 'bar', + } + + inbound_sms = create_inbound_sms_object( + sample_service_full_permissions, + format_mmg_message(data["Message"]), + data["MSISDN"], + data["ID"], + data["DateRecieved"], + "mmg" + ) + + assert inbound_sms.service_id == sample_service_full_permissions.id + assert inbound_sms.notify_number == inbound_number.number + + @pytest.mark.parametrize('notify_number', ['foo', 'baz'], ids=['two_matching_services', 'no_matching_services']) def test_receive_notification_error_if_not_single_matching_service(client, notify_db_session, notify_number): create_service(service_name='a', sms_sender='foo', service_permissions=[EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE]) diff --git a/tests/app/test_model.py b/tests/app/test_model.py index b8ffcf9b4..56a121e01 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -20,6 +20,7 @@ from tests.app.conftest import ( sample_notification_with_job as create_sample_notification_with_job ) from tests.app.db import create_notification, create_service, create_inbound_number +from tests.conftest import set_config @pytest.mark.parametrize('mobile_number', [ @@ -227,3 +228,23 @@ def test_inbound_number_serializes_with_service(client, notify_db_session): assert serialized_inbound_number.get('id') == str(inbound_number.id) assert serialized_inbound_number.get('service').get('id') == str(inbound_number.service.id) assert serialized_inbound_number.get('service').get('name') == inbound_number.service.name + + +def test_inbound_number_returns_inbound_number(client, notify_db_session): + service = create_service() + inbound_number = create_inbound_number(number='1', service_id=service.id) + + assert service.get_inbound_number() == inbound_number.number + + +def test_inbound_number_returns_sms_sender(client, notify_db_session): + service = create_service(sms_sender='testing') + + assert service.get_inbound_number() == service.sms_sender + + +def test_inbound_number_returns_from_number_config(client, notify_db_session): + with set_config(client.application, 'FROM_NUMBER', 'test'): + service = create_service(sms_sender=None) + + assert service.get_inbound_number() == 'test' diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index c108faacf..bd5b4f711 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -17,7 +17,7 @@ from tests.app.conftest import ( sample_template_without_sms_permission, sample_template_without_email_permission ) -from tests.app.db import create_service, create_template +from tests.app.db import create_inbound_number, create_service, create_template @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -55,6 +55,29 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol assert mocked.called +def test_post_sms_notification_uses_inbound_number_as_sender(client, sample_template_with_placeholders, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template_with_placeholders.id), + 'personalisation': {' Name': 'Jo'} + } + inbound_number = create_inbound_number('1', service_id=sample_template_with_placeholders.service_id) + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + notifications = Notification.query.all() + assert len(notifications) == 1 + notification_id = notifications[0].id + assert resp_json['id'] == str(notification_id) + assert resp_json['content']['from_number'] == inbound_number.number + + @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "+447700900855"), ("email", "email_address", "sample@email.com")]) From ce962380e3e5b97c2a15deb88ba499aac9d7d237 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 15 Aug 2017 14:11:00 +0100 Subject: [PATCH 19/26] Update to return empty json block not '' --- app/inbound_number/rest.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index 3fba96774..0127e4c8c 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -34,15 +34,15 @@ def post_allocate_inbound_number(service_id): if inbound_number: if not inbound_number.active: dao_set_inbound_number_active_flag(service_id, active=True) - return '', 204 + return jsonify(), 204 else: - return '', 200 + return jsonify(), 200 available_numbers = dao_get_available_inbound_numbers() if len(available_numbers) > 0: dao_set_inbound_number_to_service(service_id, available_numbers[0]) - return '', 204 + return jsonify(), 204 else: raise InvalidRequest('No available inbound numbers', status_code=400) @@ -50,4 +50,4 @@ def post_allocate_inbound_number(service_id): @inbound_number_blueprint.route('/service//off', methods=['POST']) def post_set_inbound_number_off(service_id): dao_set_inbound_number_active_flag(service_id, active=False) - return '', 204 + return jsonify(), 204 From c36423aac610122bfe066595a5c8367aba46eb7b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 16 Aug 2017 12:27:42 +0100 Subject: [PATCH 20/26] Refactor code for `dao_fetch_servies_by_sms_sender` to use inbound_numbers This will need to be refactored after the deployment of api and admin and after the update script for existing services using inbound numbers has been executed. --- app/dao/services_dao.py | 18 ++++- tests/app/conftest.py | 11 ++- tests/app/dao/test_services_dao.py | 67 +++++++++++++++++-- tests/app/db.py | 12 +++- .../test_receive_notification.py | 27 +++++--- 5 files changed, 113 insertions(+), 22 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 20099c009..b4cd2e649 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 +from sqlalchemy import asc, func, or_, and_ from sqlalchemy.orm import joinedload from flask import current_app @@ -19,6 +19,7 @@ from app.models import ( Template, TemplateHistory, TemplateRedacted, + InboundNumber, Job, NotificationHistory, Notification, @@ -65,9 +66,22 @@ 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): + inbound_number = InboundNumber.query.filter( + InboundNumber.number == sms_sender + ).first() + + if not inbound_number: + return [] + return Service.query.filter( - Service.sms_sender == sms_sender + or_( + Service.sms_sender == sms_sender, + Service.id == inbound_number.service_id + ) ).all() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 5b38fa739..74ea2bbd7 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -25,9 +25,10 @@ from app.models import ( ProviderDetailsHistory, ProviderRates, NotificationStatistics, + ScheduledNotification, ServiceWhitelist, KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM, - MOBILE_TYPE, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, ScheduledNotification, + MOBILE_TYPE, EMAIL_TYPE, INBOUND_SMS_TYPE, SMS_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, SERVICE_PERMISSION_TYPES) from app.dao.users_dao import (create_user_code, create_secret_code) from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) @@ -156,12 +157,18 @@ def sample_service( else: if user not in service.users: dao_add_user_to_service(service, user) + + if INBOUND_SMS_TYPE in permissions: + create_inbound_number('12345', service_id=service.id) + return service @pytest.fixture(scope='function') def sample_service_full_permissions(notify_db, notify_db_session): - return sample_service(notify_db, notify_db_session, permissions=SERVICE_PERMISSION_TYPES) + service = sample_service(notify_db, notify_db_session, permissions=SERVICE_PERMISSION_TYPES) + create_inbound_number('12345', service_id=service.id) + return service @pytest.fixture(scope='function') diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index b60767289..35fa803ca 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -61,7 +61,7 @@ from app.models import ( SERVICE_PERMISSION_TYPES ) -from tests.app.db import create_user, create_service +from tests.app.db import create_inbound_number, create_user, create_service from tests.app.conftest import ( sample_notification as create_notification, sample_notification_history as create_notification_history, @@ -892,14 +892,67 @@ 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(notify_db_session): - foo1 = create_service(service_name='a', sms_sender='foo') - foo2 = create_service(service_name='b', sms_sender='foo') - bar = create_service(service_name='c', sms_sender='bar') +def test_dao_fetch_services_by_sms_sender_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('2') + create_inbound_number('3') - services = dao_fetch_services_by_sms_sender('foo') + services = dao_fetch_services_by_sms_sender('1') - assert {foo1.id, foo2.id} == {x.id for x in services} + assert len(services) == 1 + assert foo1.id == services[0].id + + +def test_dao_fetch_services_by_sms_sender_with_inbound_number_not_set(notify_db_session): + create_inbound_number('1') + + services = dao_fetch_services_by_sms_sender('1') + + assert services == [] + + +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) + + 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_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): + 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') + + assert services == [] def test_dao_allocating_inbound_number_shows_on_service(notify_db_session, sample_inbound_numbers): diff --git a/tests/app/db.py b/tests/app/db.py index 676487135..9f7539861 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -21,8 +21,11 @@ from app.models import ( InboundSms, InboundNumber, Organisation, - ServiceInboundApi -) + EMAIL_TYPE, + SMS_TYPE, + INBOUND_SMS_TYPE, + KEY_TYPE_NORMAL, + ServiceInboundApi) from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification, dao_created_scheduled_notification from app.dao.templates_dao import dao_create_template @@ -57,6 +60,7 @@ def create_service( sms_sender='testing', research_mode=False, active=True, + do_create_inbound_number=True, ): service = Service( name=service_name, @@ -66,6 +70,10 @@ def create_service( created_by=user or create_user(), sms_sender=sms_sender, ) + + 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 diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 0b99d7cbb..8dab45320 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -26,7 +26,7 @@ def test_receive_notification_returns_received_to_mmg(client, mocker, sample_ser "MSISDN": "447700900855", "Message": "Some message to notify", "Trigger": "Trigger?", - "Number": "testing", + "Number": sample_service_full_permissions.get_inbound_number(), "Channel": "SMS", "DateRecieved": "2012-06-27 12:33:00" } @@ -123,10 +123,9 @@ def test_format_mmg_datetime(provider_date, expected_output): def test_create_inbound_mmg_sms_object(sample_service_full_permissions): - sample_service_full_permissions.sms_sender = 'foo' data = { 'Message': 'hello+there+%F0%9F%93%A9', - 'Number': 'foo', + 'Number': sample_service_full_permissions.get_inbound_number(), 'MSISDN': '07700 900 001', 'DateRecieved': '2017-01-02+03%3A04%3A05', 'ID': 'bar', @@ -136,7 +135,7 @@ def test_create_inbound_mmg_sms_object(sample_service_full_permissions): data["MSISDN"], data["ID"], data["DateRecieved"], "mmg") assert inbound_sms.service_id == sample_service_full_permissions.id - assert inbound_sms.notify_number == 'foo' + assert inbound_sms.notify_number == sample_service_full_permissions.get_inbound_number() assert inbound_sms.user_number == '447700900001' assert inbound_sms.provider_date == datetime(2017, 1, 2, 3, 4, 5) assert inbound_sms.provider_reference == 'bar' @@ -147,11 +146,11 @@ def test_create_inbound_mmg_sms_object(sample_service_full_permissions): def test_create_inbound_mmg_sms_object_uses_inbound_number_if_set(sample_service_full_permissions): sample_service_full_permissions.sms_sender = 'foo' - inbound_number = create_inbound_number('1', service_id=sample_service_full_permissions.id) + inbound_number = sample_service_full_permissions.get_inbound_number() data = { 'Message': 'hello+there+%F0%9F%93%A9', - 'Number': 'foo', + 'Number': sample_service_full_permissions.get_inbound_number(), 'MSISDN': '07700 900 001', 'DateRecieved': '2017-01-02+03%3A04%3A05', 'ID': 'bar', @@ -167,13 +166,23 @@ def test_create_inbound_mmg_sms_object_uses_inbound_number_if_set(sample_service ) assert inbound_sms.service_id == sample_service_full_permissions.id - assert inbound_sms.notify_number == inbound_number.number + assert inbound_sms.notify_number == inbound_number @pytest.mark.parametrize('notify_number', ['foo', 'baz'], ids=['two_matching_services', 'no_matching_services']) def test_receive_notification_error_if_not_single_matching_service(client, notify_db_session, notify_number): - create_service(service_name='a', sms_sender='foo', service_permissions=[EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE]) - create_service(service_name='b', sms_sender='foo', service_permissions=[EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE]) + create_service( + service_name='a', + sms_sender='foo', + service_permissions=[EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE], + do_create_inbound_number=False + ) + create_service( + service_name='b', + sms_sender='foo', + service_permissions=[EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE], + do_create_inbound_number=False + ) data = { 'Message': 'hello', From 084eac5735404edeed62cee4e2b429c8f4f2a7a5 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 16 Aug 2017 14:02:47 +0100 Subject: [PATCH 21/26] Update dao to order by updated_at, number --- app/dao/inbound_numbers_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/dao/inbound_numbers_dao.py b/app/dao/inbound_numbers_dao.py index fa30258c2..c4fe83fd8 100644 --- a/app/dao/inbound_numbers_dao.py +++ b/app/dao/inbound_numbers_dao.py @@ -4,7 +4,7 @@ from app.models import InboundNumber def dao_get_inbound_numbers(): - return InboundNumber.query.all() + return InboundNumber.query.order_by(InboundNumber.updated_at, InboundNumber.number).all() def dao_get_available_inbound_numbers(): From fbe1a143044181f4e5ff559d7b9268f97378a31c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 16 Aug 2017 14:23:32 +0100 Subject: [PATCH 22/26] Removed create_inbound_numberfrom fixture --- tests/app/conftest.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 74ea2bbd7..86437e7a1 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -166,9 +166,7 @@ def sample_service( @pytest.fixture(scope='function') def sample_service_full_permissions(notify_db, notify_db_session): - service = sample_service(notify_db, notify_db_session, permissions=SERVICE_PERMISSION_TYPES) - create_inbound_number('12345', service_id=service.id) - return service + return sample_service(notify_db, notify_db_session, permissions=SERVICE_PERMISSION_TYPES) @pytest.fixture(scope='function') From 902b28e00f8ebd84d8ad6ec7190da6c04abcabb7 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 16 Aug 2017 16:27:42 +0100 Subject: [PATCH 23/26] Refactor to check active flag --- app/dao/services_dao.py | 3 ++- tests/app/dao/test_services_dao.py | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index b4cd2e649..9898d2855 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -71,7 +71,8 @@ def dao_fetch_service_by_id(service_id, only_active=False): ############ def dao_fetch_services_by_sms_sender(sms_sender): inbound_number = InboundNumber.query.filter( - InboundNumber.number == sms_sender + InboundNumber.number == sms_sender, + InboundNumber.active ).first() if not inbound_number: diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 35fa803ca..b7b92a0c0 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -955,6 +955,15 @@ def test_dao_fetch_services_by_sms_sender_with_unknown_number(notify_db_session) assert services == [] +def test_dao_fetch_services_by_sms_sender_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') + + assert services == [] + + def test_dao_allocating_inbound_number_shows_on_service(notify_db_session, sample_inbound_numbers): inbound_numbers = dao_get_available_inbound_numbers() From 9ab1dfd6d073bb5939ef149d11b58613ae79d408 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 16 Aug 2017 16:28:21 +0100 Subject: [PATCH 24/26] Just order by updated_at for inbound_numbers --- app/dao/inbound_numbers_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/dao/inbound_numbers_dao.py b/app/dao/inbound_numbers_dao.py index c4fe83fd8..4d2604abc 100644 --- a/app/dao/inbound_numbers_dao.py +++ b/app/dao/inbound_numbers_dao.py @@ -4,7 +4,7 @@ from app.models import InboundNumber def dao_get_inbound_numbers(): - return InboundNumber.query.order_by(InboundNumber.updated_at, InboundNumber.number).all() + return InboundNumber.query.order_by(InboundNumber.updated_at).all() def dao_get_available_inbound_numbers(): From 3a70d63a7ca229575379643d1913538a5707da5d Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 16 Aug 2017 16:28:56 +0100 Subject: [PATCH 25/26] Test for inbound_number when no inbound_sms permissions --- .../test_receive_notification.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 8dab45320..d1f14cc13 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -93,6 +93,36 @@ def test_receive_notification_without_permissions_does_not_create_inbound( mocked_send_inbound_sms.assert_not_called() +def test_receive_notification_without_permissions_does_not_create_inbound_even_with_inbound_number_set( + client, mocker, notify_db, notify_db_session): + service = sample_service(notify_db, notify_db_session, permissions=[SMS_TYPE]) + inbound_number = create_inbound_number('1', service_id=service.id, active=True) + + 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 = json.dumps({ + "ID": "1234", + "MSISDN": "447700900855", + "Message": "Some message to notify", + "Trigger": "Trigger?", + "Number": inbound_number.number, + "Channel": "SMS", + "DateRecieved": "2012-06-27 12:33:00" + }) + + response = client.post(path='/notifications/sms/receive/mmg', + data=data, + headers=[('Content-Type', 'application/json')]) + + assert response.status_code == 200 + assert len(InboundSms.query.all()) == 0 + assert mocked_has_permissions.called + mocked_send_inbound_sms.assert_not_called() + + @pytest.mark.parametrize('permissions,expected_response', [ ([SMS_TYPE, INBOUND_SMS_TYPE], True), ([INBOUND_SMS_TYPE], False), From 5bb20e41b31ffaa418e05dad4c895f3c7fe96084 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 17 Aug 2017 18:15:22 +0100 Subject: [PATCH 26/26] Update dao_fetch_services_by_ss_sender --- app/dao/services_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 9898d2855..d75117308 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_, and_ +from sqlalchemy import asc, func, or_ from sqlalchemy.orm import joinedload from flask import current_app