Use personalisation to set client_reference for letters

which were sent through Notify interface only. This is done
to avoid performance dip from additional operation for
other notification types.

app/celery/tasks.py

@@ -58,7 +58,7 @@ from app.models import (
 from app.notifications.process_notifications import persist_notification
 from app.serialised_models import SerialisedService, SerialisedTemplate
 from app.service.utils import service_allowed_to_send_to
-from app.utils import DATETIME_FORMAT
+from app.utils import DATETIME_FORMAT, get_reference_from_personalisation
 
 
 @notify_celery.task(name="process-job")
@@ -383,6 +383,7 @@ def save_letter(
             job_row_number=notification['row_number'],
             notification_id=notification_id,
             reference=create_random_identifier(),
+            client_reference=get_reference_from_personalisation(notification['personalisation']),
             reply_to_text=template.reply_to_text,
             status=status
         )

app/notifications/process_notifications.py

@@ -121,7 +121,7 @@ def persist_notification(
         created_at=notification_created_at,
         job_id=job_id,
         job_row_number=job_row_number,
-        client_reference=client_reference or _get_reference_from_personalisation(personalisation),
+        client_reference=client_reference,
         reference=reference,
         created_by_id=created_by_id,
         status=status,
@@ -159,12 +159,6 @@ def persist_notification(
     return notification
 
 
-def _get_reference_from_personalisation(personalisation):
-    if personalisation:
-        return personalisation.get("reference")
-    return None
-
-
 def send_notification_to_queue_detached(
     key_type, notification_type, notification_id, research_mode, queue=None
 ):

app/notifications/rest.py

@@ -108,6 +108,7 @@ def send_notification(notification_type):
 
     if notification_type == SMS_TYPE:
         check_if_service_can_send_to_number(authenticated_service, notification_form['to'])
+
     # Do not persist or send notification to the queue if it is a simulated recipient
     simulated = simulated_recipient(notification_form['to'], notification_type)
     notification_model = persist_notification(template_id=template.id,
@@ -120,7 +121,7 @@ def send_notification(notification_type):
                                               api_key_id=api_user.id,
                                               key_type=api_user.key_type,
                                               simulated=simulated,
-                                              reply_to_text=template.reply_to_text
+                                              reply_to_text=template.reply_to_text,
                                               )
     if not simulated:
         queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None

app/service/send_notification.py

@@ -81,12 +81,16 @@ def send_one_off_notification(service_id, post_data):
         allow_guest_list_recipients=False,
     )
     postage = None
+    client_reference = None
     if template.template_type == LETTER_TYPE:
         # Validate address and set postage to europe|rest-of-world if international letter,
         # otherwise persist_notification with use template postage
         postage = validate_address(service, personalisation)
         if not postage:
             postage = template.postage
+        from app.utils import get_reference_from_personalisation
+        client_reference = get_reference_from_personalisation(personalisation)
+
     validate_created_by(service, post_data['created_by'])
 
     sender_id = post_data.get('sender_id', None)
@@ -108,7 +112,8 @@ def send_one_off_notification(service_id, post_data):
         created_by_id=post_data['created_by'],
         reply_to_text=reply_to,
         reference=create_one_off_reference(template.template_type),
-        postage=postage
+        postage=postage,
+        client_reference=client_reference
     )
 
     queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None

app/utils.py

@@ -160,3 +160,9 @@ def get_uuid_string_or_none(val):
 
 def format_sequential_number(sequential_number):
     return format(sequential_number, "x").zfill(8)
+
+
+def get_reference_from_personalisation(personalisation):
+    if personalisation:
+        return personalisation.get("reference")
+    return None

tests/app/celery/test_tasks.py

@@ -1070,6 +1070,38 @@ def test_save_letter_saves_letter_to_database_with_correct_postage(
     assert notification_db.international == expected_international
 
 
+@pytest.mark.parametrize('reference_paceholder,', [None, 'ref2'])
+def test_save_letter_saves_letter_to_database_with_correct_client_reference(
+    mocker, notify_db_session, reference_paceholder
+):
+    service = create_service(service_permissions=[LETTER_TYPE])
+    template = create_template(service=service, template_type=LETTER_TYPE)
+    letter_job = create_job(template=template)
+
+    personalisation = {'addressline1': 'Foo', 'addressline2': 'Bar', 'postcode': 'SW1A 1AA'}
+    if reference_paceholder:
+        personalisation['reference'] = reference_paceholder
+
+    mocker.patch('app.celery.tasks.letters_pdf_tasks.get_pdf_for_templated_letter.apply_async')
+    notification_json = _notification_json(
+        template=letter_job.template,
+        to='Foo',
+        personalisation=personalisation,
+        job_id=letter_job.id,
+        row_number=1
+    )
+    notification_id = uuid.uuid4()
+    save_letter(
+        letter_job.service_id,
+        notification_id,
+        encryption.encrypt(notification_json),
+    )
+
+    notification_db = Notification.query.one()
+    assert notification_db.id == notification_id
+    assert notification_db.client_reference == reference_paceholder
+
+
 def test_save_letter_saves_letter_to_database_with_formatted_postcode(mocker, notify_db_session):
     service = create_service(service_permissions=[LETTER_TYPE])
     template = create_template(service=service, template_type=LETTER_TYPE)

tests/app/notifications/rest/test_send_notification.py

@@ -1249,3 +1249,27 @@ def test_send_notification_should_send_international_letters(
     notification = Notification.query.get(notification_id['id'])
     assert notification.postage == expected_postage
     assert notification.international == expected_international
+
+
+@pytest.mark.parametrize('reference_paceholder,', [None, 'ref2'])
+def test_send_notification_should_set_client_reference_from_placeholder(
+    sample_letter_template, mocker, reference_paceholder
+):
+    deliver_mock = mocker.patch('app.celery.tasks.letters_pdf_tasks.get_pdf_for_templated_letter.apply_async')
+    data = {
+        'template_id': sample_letter_template.id,
+        'personalisation': {
+            'address_line_1': 'Jane',
+            'address_line_2': 'Moss Lane',
+            'address_line_3': 'SW1A 1AA',
+        },
+        'to': 'Jane',
+        'created_by': sample_letter_template.service.created_by_id
+    }
+    if reference_paceholder:
+        data['personalisation']['reference'] = reference_paceholder
+
+    notification_id = send_one_off_notification(sample_letter_template.service_id, data)
+    assert deliver_mock.called
+    notification = Notification.query.get(notification_id['id'])
+    assert notification.client_reference == reference_paceholder

tests/app/notifications/test_process_notification.py

@@ -96,42 +96,6 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api
     assert notification_from_db.reply_to_text == sample_template.service.get_default_sms_sender()
 
 
-@pytest.mark.parametrize('client_reference,personalisation,expected_client_reference', [
-    ('clientref1', {}, 'clientref1'),
-    ('clientref1', {'reference': 'ref2'}, 'clientref1'),
-    (None, {'reference': 'ref2'}, 'ref2'),
-    (None, {'foo': 'bar'}, None),
-])
-def test_persist_notification_sets_client_reference_correctly(
-    sample_letter_template, sample_api_key, client_reference, personalisation, expected_client_reference
-):
-
-    assert Notification.query.count() == 0
-    assert NotificationHistory.query.count() == 0
-    notification = persist_notification(
-        template_id=sample_letter_template.id,
-        template_version=sample_letter_template.version,
-        recipient='3 Somerset House',
-        service=sample_letter_template.service,
-        personalisation=personalisation,
-        notification_type='letter',
-        api_key_id=sample_api_key.id,
-        key_type=sample_api_key.key_type,
-        reference="ref",
-        client_reference=client_reference,
-    )
-
-    assert Notification.query.get(notification.id) is not None
-
-    notification_from_db = Notification.query.one()
-
-    assert notification_from_db.id == notification.id
-    assert notification_from_db.template_id == notification.template_id
-    assert notification_from_db.notification_type == notification.notification_type
-    assert notification_from_db.reference == notification.reference
-    assert notification_from_db.client_reference == expected_client_reference
-
-
 def test_persist_notification_throws_exception_when_missing_template(sample_api_key):
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0

tests/app/service/test_send_one_off_notification.py

@@ -100,7 +100,8 @@ def test_send_one_off_notification_calls_persist_correctly_for_sms(
         created_by_id=str(service.created_by_id),
         reply_to_text='testing',
         reference=None,
-        postage=None
+        postage=None,
+        client_reference=None
     )
 
 
@@ -161,7 +162,8 @@ def test_send_one_off_notification_calls_persist_correctly_for_email(
         created_by_id=str(service.created_by_id),
         reply_to_text=None,
         reference=None,
-        postage=None
+        postage=None,
+        client_reference=None
     )
 
 
@@ -210,7 +212,8 @@ def test_send_one_off_notification_calls_persist_correctly_for_letter(
         created_by_id=str(service.created_by_id),
         reply_to_text=None,
         reference='this-is-random-in-real-life',
-        postage='first'
+        postage='first',
+        client_reference=None
     )