From ba7cd79581b06e82413aae9e4af7e72965460f21 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 21 Sep 2017 16:41:10 +0100 Subject: [PATCH 1/7] Created a get_default_sms_sender method, which returns the default sms_sender from the service_sms_sender table rather than service.sms_sender. One step closer to removing services.sms_sender. fix the unit tests --- app/models.py | 12 ++++++--- tests/app/db.py | 26 +++++++++++++++----- tests/app/delivery/test_send_to_providers.py | 22 ++++++++--------- tests/app/test_model.py | 6 +++++ 4 files changed, 45 insertions(+), 21 deletions(-) diff --git a/app/models.py b/app/models.py index be6d54cf1..b35d86ab8 100644 --- a/app/models.py +++ b/app/models.py @@ -249,7 +249,13 @@ class Service(db.Model, Versioned): if self.inbound_number and self.inbound_number.active: return self.inbound_number.number else: - return self.sms_sender + return self.get_default_sms_sender() + + def get_default_sms_sender(self): + default_sms_sender = [x for x in self.service_sms_senders if x.is_default] + if len(default_sms_sender) > 1: + raise Exception("There should only ever be one default") + return default_sms_sender[0].sms_sender def get_default_reply_to_email_address(self): default_reply_to = [x for x in self.reply_to_email_addresses if x.is_default] @@ -303,7 +309,7 @@ class ServiceSmsSender(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) sms_sender = db.Column(db.String(11), nullable=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=True, index=True, nullable=False) - service = db.relationship(Service, backref=db.backref("service_sms_senders", uselist=False)) + service = db.relationship(Service, backref=db.backref("service_sms_senders", uselist=True)) is_default = db.Column(db.Boolean, nullable=False, default=True) inbound_number_id = db.Column(UUID(as_uuid=True), db.ForeignKey('inbound_numbers.id'), unique=True, index=True, nullable=True) @@ -1002,7 +1008,7 @@ class Notification(db.Model): return NOTIFICATION_STATUS_LETTER_ACCEPTED else: # Currently can only be technical-failure - return status + return self.status def serialize_for_csv(self): created_at_in_bst = convert_utc_to_bst(self.created_at) diff --git a/tests/app/db.py b/tests/app/db.py index b3b6ba61c..fdac0f5c4 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -27,8 +27,7 @@ from app.models import ( INBOUND_SMS_TYPE, KEY_TYPE_NORMAL, ServiceInboundApi, - ServiceEmailReplyTo, - ServiceLetterContact + ServiceEmailReplyTo ) from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification, dao_created_scheduled_notification @@ -71,7 +70,7 @@ def create_service( message_limit=1000, restricted=restricted, email_from=service_name.lower().replace(' ', '.'), - created_by=user or create_user(), + created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())), sms_sender=sms_sender, ) @@ -143,9 +142,6 @@ def create_notification( if not api_key: api_key = create_api_key(template.service, key_type=key_type) - if template.template_type == LETTER_TYPE and reference is None: - reference = create_random_identifier() - data = { 'id': uuid.uuid4(), 'to': to_field, @@ -354,6 +350,24 @@ def create_reply_to_email( return reply_to +def create_service_sms_sender( + service, + sms_sender, + is_default=True +): + data = { + 'service_id': service.id, + 'sms_sender': sms_sender, + 'is_default': is_default, + } + service_sms_sender = ServiceSmsSender(**data) + + db.session.add(service_sms_sender) + db.session.commit() + + return service_sms_sender + + def create_letter_contact( service, contact_block, diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index c88b7f380..8f7685f5d 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -271,15 +271,14 @@ def test_should_not_send_to_provider_when_status_is_not_created( def test_should_send_sms_sender_from_service_if_present( - sample_service, - sample_template, + notify_db_session, mocker): - db_notification = create_notification(template=sample_template, + service = create_service(sms_sender='elevenchars') + template = create_template(service=service) + db_notification = create_notification(template=template, to_field="+447234123123", status='created') - sample_service.sms_sender = 'elevenchars' - mocker.patch('app.mmg_client.send_sms') mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') @@ -289,9 +288,9 @@ def test_should_send_sms_sender_from_service_if_present( mmg_client.send_sms.assert_called_once_with( to=validate_and_format_phone_number("+447234123123"), - content="This is a template:\nwith a newline", + content="Dear Sir/Madam, Hello. Yours Truly, The Government.", reference=str(db_notification.id), - sender=sample_service.sms_sender + sender=service.sms_sender ) @@ -651,12 +650,11 @@ def test_should_set_international_phone_number_to_sent_status( @pytest.mark.parametrize('sms_sender, expected_sender, expected_content', [ ('foo', 'foo', 'bar'), # if 40604 is actually in DB then treat that as if entered manually - ('40604', '40604', 'bar'), + # ('40604', '40604', 'bar'), # 'testing' is the FROM_NUMBER during unit tests - ('testing', 'testing', 'Sample service: bar'), + # ('testing', 'testing', 'Sample service: bar'), ]) def test_should_handle_sms_sender_and_prefix_message( - sample_service, mocker, sms_sender, expected_sender, @@ -664,8 +662,8 @@ def test_should_handle_sms_sender_and_prefix_message( ): mocker.patch('app.mmg_client.send_sms') mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') - sample_service.sms_sender = sms_sender - template = create_template(sample_service, content='bar') + service = create_service(service_name=str(uuid.uuid4()), sms_sender=sms_sender) + template = create_template(service, content='bar') notification = create_notification(template) send_to_providers.send_sms_to_provider(notification) diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 0303782ff..8860975a7 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -26,6 +26,7 @@ from tests.app.db import ( create_service, create_inbound_number, create_reply_to_email, + create_service_sms_sender, create_letter_contact ) from tests.conftest import set_config @@ -279,3 +280,8 @@ def test_service_get_default_contact_letter(sample_service): # this test will need to be removed after letter_contact_block is dropped def test_service_get_default_letter_contact_block_from_service(sample_service): assert sample_service.get_default_letter_contact() == sample_service.letter_contact_block + + +def test_service_get_default_sms_sender(notify_db_session): + service = create_service(sms_sender='new_value') + assert service.get_default_sms_sender() == 'new_value' From 0db39bfac8b01d5a88f8c6022d1eb424d59b9fb8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 2 Oct 2017 15:29:13 +0100 Subject: [PATCH 2/7] Fix the unit tests. I think there was some imports missed when resolving merge conflicts. Also I'm not sure why the test_update_letter_notification_to_sent or error passed, I've updated them so they do pass. --- tests/app/celery/test_ftp_update_tasks.py | 8 ++++---- tests/app/db.py | 4 ++-- tests/app/delivery/test_send_to_providers.py | 9 +++++---- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 05680d32b..dc96ac91d 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -95,8 +95,8 @@ def test_update_letter_notifications_to_sent_to_dvla_updates_based_on_notificati client, sample_letter_template ): - first = create_notification(sample_letter_template) - second = create_notification(sample_letter_template) + first = create_notification(sample_letter_template, reference='first ref') + second = create_notification(sample_letter_template, reference='second ref') dt = datetime.utcnow() with freeze_time(dt): @@ -113,8 +113,8 @@ def test_update_letter_notifications_to_error_updates_based_on_notification_refe client, sample_letter_template ): - first = create_notification(sample_letter_template) - second = create_notification(sample_letter_template) + first = create_notification(sample_letter_template, reference='first ref') + second = create_notification(sample_letter_template, reference='second ref') dt = datetime.utcnow() with freeze_time(dt): diff --git a/tests/app/db.py b/tests/app/db.py index fdac0f5c4..dfa3f6a07 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -27,8 +27,8 @@ from app.models import ( INBOUND_SMS_TYPE, KEY_TYPE_NORMAL, ServiceInboundApi, - ServiceEmailReplyTo -) + ServiceEmailReplyTo, + ServiceLetterContact, ServiceSmsSender) 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 diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 8f7685f5d..30ca1e876 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -650,19 +650,20 @@ def test_should_set_international_phone_number_to_sent_status( @pytest.mark.parametrize('sms_sender, expected_sender, expected_content', [ ('foo', 'foo', 'bar'), # if 40604 is actually in DB then treat that as if entered manually - # ('40604', '40604', 'bar'), + ('40604', '40604', 'bar'), # 'testing' is the FROM_NUMBER during unit tests - # ('testing', 'testing', 'Sample service: bar'), + ('testing', 'testing', 'Sample service: bar'), ]) def test_should_handle_sms_sender_and_prefix_message( mocker, sms_sender, expected_sender, - expected_content + expected_content, + notify_db_session ): mocker.patch('app.mmg_client.send_sms') mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') - service = create_service(service_name=str(uuid.uuid4()), sms_sender=sms_sender) + service = create_service(sms_sender=sms_sender) template = create_template(service, content='bar') notification = create_notification(template) From fb687677202c419cd6c9d4a29fd3af616caed965 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 2 Oct 2017 17:15:15 +0100 Subject: [PATCH 3/7] Update service_schema to return the default sms_sender from the new table rather than the services table. --- app/schemas.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/schemas.py b/app/schemas.py index e880a1589..a8ef2cb2b 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -183,6 +183,7 @@ class ServiceSchema(BaseSchema): permissions = fields.Method("service_permissions") override_flag = False reply_to_email_address = fields.Method(method_name="get_reply_to_email_address") + sms_sender = fields.Method(method_name="get_sms_sender") def get_free_sms_fragment_limit(selfs, service): return service.free_sms_fragment_limit() @@ -193,6 +194,9 @@ class ServiceSchema(BaseSchema): def get_reply_to_email_address(self, service): return service.get_default_reply_to_email_address() + def get_sms_sender(self, service): + return service.get_default_sms_sender() + class Meta: model = models.Service dump_only = ['free_sms_fragment_limit', 'reply_to_email_address'] From eff2a720ead3754736384839799708818e86d0b0 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 2 Oct 2017 17:35:58 +0100 Subject: [PATCH 4/7] Update post_allocate_inbound_number to set the service_sms_sender in the case when a service had the number, then it was set to inactive and now there is a request to turn it back on (or click `allow inbound sms` a couple of times on the front end) --- app/inbound_number/rest.py | 2 ++ tests/app/inbound_number/test_rest.py | 19 ------------------- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/app/inbound_number/rest.py b/app/inbound_number/rest.py index becaf21fc..24b2445c2 100644 --- a/app/inbound_number/rest.py +++ b/app/inbound_number/rest.py @@ -36,6 +36,8 @@ 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) + service = dao_fetch_service_by_id(service_id) + insert_or_update_service_sms_sender(service, inbound_number.number, inbound_number.id) return jsonify(), 204 else: return jsonify(), 200 diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py index c237df448..3ffd691ab 100644 --- a/tests/app/inbound_number/test_rest.py +++ b/tests/app/inbound_number/test_rest.py @@ -114,25 +114,6 @@ def test_rest_set_inbound_number_active_flag_off( assert not inbound_number_from_db.active -def test_allocate_inbound_number_insert_update_service_sms_sender( - admin_request, notify_db_session -): - service = create_service() - inbound_number = create_inbound_number(number='123') - - admin_request.post( - 'inbound_number.post_allocate_inbound_number', - _expected_status=204, - service_id=service.id - ) - - service_sms_senders = ServiceSmsSender.query.all() - assert len(service_sms_senders) == 1 - assert service_sms_senders[0].sms_sender == inbound_number.number - assert service_sms_senders[0].inbound_number_id == inbound_number.id - assert service_sms_senders[0].is_default - - def test_allocate_inbound_number_to_service(admin_request, notify_db_session): service = create_service() inbound_number = create_inbound_number(number='1235468') From d1d655e23d46fe4d6813cbd53256afdeedc3f85f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 3 Oct 2017 11:04:29 +0100 Subject: [PATCH 5/7] Kept the test after all --- tests/app/inbound_number/test_rest.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/app/inbound_number/test_rest.py b/tests/app/inbound_number/test_rest.py index 3ffd691ab..c237df448 100644 --- a/tests/app/inbound_number/test_rest.py +++ b/tests/app/inbound_number/test_rest.py @@ -114,6 +114,25 @@ def test_rest_set_inbound_number_active_flag_off( assert not inbound_number_from_db.active +def test_allocate_inbound_number_insert_update_service_sms_sender( + admin_request, notify_db_session +): + service = create_service() + inbound_number = create_inbound_number(number='123') + + admin_request.post( + 'inbound_number.post_allocate_inbound_number', + _expected_status=204, + service_id=service.id + ) + + service_sms_senders = ServiceSmsSender.query.all() + assert len(service_sms_senders) == 1 + assert service_sms_senders[0].sms_sender == inbound_number.number + assert service_sms_senders[0].inbound_number_id == inbound_number.id + assert service_sms_senders[0].is_default + + def test_allocate_inbound_number_to_service(admin_request, notify_db_session): service = create_service() inbound_number = create_inbound_number(number='1235468') From cefb74c73225475c4b4635877cae1f65a34816a8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 4 Oct 2017 13:49:01 +0100 Subject: [PATCH 6/7] Removed a test that should have been removed during the last merge --- tests/app/test_model.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 8860975a7..a1bc1e343 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -277,11 +277,6 @@ def test_service_get_default_contact_letter(sample_service): assert sample_service.get_default_letter_contact() == 'London,\nNW1A 1AA' -# this test will need to be removed after letter_contact_block is dropped -def test_service_get_default_letter_contact_block_from_service(sample_service): - assert sample_service.get_default_letter_contact() == sample_service.letter_contact_block - - def test_service_get_default_sms_sender(notify_db_session): service = create_service(sms_sender='new_value') assert service.get_default_sms_sender() == 'new_value' From 9d507466ef14b68ba6afb892b1d474bbeb4688d3 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 4 Oct 2017 14:51:02 +0100 Subject: [PATCH 7/7] Remove exception thrown when getting default sender. Having more than one default is checked on insert and update. --- app/models.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/app/models.py b/app/models.py index 670a5d654..a5da33951 100644 --- a/app/models.py +++ b/app/models.py @@ -257,23 +257,15 @@ class Service(db.Model, Versioned): def get_default_sms_sender(self): default_sms_sender = [x for x in self.service_sms_senders if x.is_default] - if len(default_sms_sender) > 1: - raise Exception("There should only ever be one default") return default_sms_sender[0].sms_sender def get_default_reply_to_email_address(self): default_reply_to = [x for x in self.reply_to_email_addresses if x.is_default] - if len(default_reply_to) > 1: - raise Exception("There should only ever be one default") - else: - return default_reply_to[0].email_address if default_reply_to else None + return default_reply_to[0].email_address if default_reply_to else None def get_default_letter_contact(self): default_letter_contact = [x for x in self.letter_contacts if x.is_default] - if len(default_letter_contact) > 1: - raise Exception("There should only ever be one default") - else: - return default_letter_contact[0].contact_block if default_letter_contact else None + return default_letter_contact[0].contact_block if default_letter_contact else None class InboundNumber(db.Model):