Skip to content

Commit

Permalink
#2014 - Add callback url to GET and POST requests
Browse files Browse the repository at this point in the history
  • Loading branch information
MackHalliday authored Oct 8, 2024
1 parent f2a0feb commit c63a559
Show file tree
Hide file tree
Showing 9 changed files with 108 additions and 7 deletions.
2 changes: 2 additions & 0 deletions .talismanrc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ fileignoreconfig:
checksum: a80aa727586db82ed1b50bdb81ddfe1379e649a9dfc1ece2c36047486b41b83d
- filename: tests/app/notifications/test_process_notifications_for_profile_v3.py
checksum: 4e15e63d349635131173ffdd7aebcd547621db08de877ef926d3a41fde72d065
- filename: tests/app/v2/notifications/test_post_notifications.py
checksum: 3181930a13e3679bb2f17eaa3f383512eb9caf4ed5d5e14496ca4193c6083965
- filename: tests/app/callback/test_webhook_callback_strategy.py
checksum: 288841d3209dc3ca885cd0bb08591221f7f15e5b3406fb7140505096db212554
- filename: app/callback/webhook_callback_strategy.py
Expand Down
1 change: 1 addition & 0 deletions app/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1631,6 +1631,7 @@ def serialize(self):
'sms_sender_id': self.sms_sender_id,
'segments_count': self.segments_count,
'cost_in_millicents': self.cost_in_millicents,
'callback_url': self.callback_url,
}

if self.notification_type == LETTER_TYPE:
Expand Down
2 changes: 2 additions & 0 deletions app/notifications/process_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ def persist_notification(
recipient_identifier=None,
billing_code=None,
sms_sender_id=None,
callback_url=None,
) -> Notification:
notification_created_at = created_at or datetime.utcnow()

Expand Down Expand Up @@ -116,6 +117,7 @@ def persist_notification(
billable_units=billable_units,
billing_code=billing_code,
sms_sender_id=sms_sender_id,
callback_url=callback_url,
)

if accept_recipient_identifiers_enabled() and recipient_identifier:
Expand Down
1 change: 1 addition & 0 deletions app/v2/notifications/create_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,5 @@ def __create_notification_response(
},
'billing_code': notification.billing_code if notification.billing_code else None,
'scheduled_for': scheduled_for if scheduled_for else None,
'callback_url': notification.callback_url if notification.callback_url else None,
}
10 changes: 9 additions & 1 deletion app/v2/notifications/notification_schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
NOTIFICATION_STATUS_LETTER_RECEIVED,
TEMPLATE_TYPES,
)
from app.schema_validation.definitions import nullable_uuid, uuid, personalisation, letter_personalisation
from app.schema_validation.definitions import (
nullable_uuid,
uuid,
personalisation,
letter_personalisation,
)
from app.va.identifier import IdentifierType


Expand Down Expand Up @@ -55,6 +60,7 @@
'sent_by': {'type': ['string', 'null']},
'completed_at': {'type': ['string', 'null']},
'scheduled_for': {'type': ['string', 'null']},
'callback_url': {'type': ['string', 'null'], 'format': 'uri', 'pattern': '^https.*', 'maxLength': 255},
},
'required': [
# technically, all keys are required since we always have all of them
Expand Down Expand Up @@ -153,6 +159,7 @@
'scheduled_for': {'type': ['string', 'null'], 'format': 'datetime_within_next_day'},
'sms_sender_id': nullable_uuid,
'billing_code': {'type': ['string', 'null'], 'maxLength': 256},
'callback_url': {'type': ['string', 'null'], 'format': 'uri', 'pattern': '^https.*', 'maxLength': 255},
},
# This is necessary to get the content of the message and who it's from.
'required': ['template_id'],
Expand Down Expand Up @@ -202,6 +209,7 @@
'scheduled_for': {'type': ['string', 'null'], 'format': 'datetime_within_next_day'},
'email_reply_to_id': uuid,
'billing_code': {'type': ['string', 'null'], 'maxLength': 256},
'callback_url': {'type': ['string', 'null'], 'format': 'uri', 'pattern': '^https.*', 'maxLength': 255},
},
'required': ['template_id'],
'anyOf': [{'required': ['email_address']}, {'required': ['recipient_identifier']}],
Expand Down
5 changes: 2 additions & 3 deletions app/v2/notifications/post_notifications.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import base64
import functools

import werkzeug
Expand All @@ -10,15 +9,13 @@
from app.attachments.store import AttachmentStoreError
from app.attachments.types import UploadedAttachmentMetadata

from app.config import QueueNames
from app.feature_flags import accept_recipient_identifiers_enabled, is_feature_enabled, FeatureFlag
from app.models import (
SCHEDULE_NOTIFICATIONS,
SMS_TYPE,
EMAIL_TYPE,
LETTER_TYPE,
UPLOAD_DOCUMENT,
PRIORITY,
)
from app.notifications.process_notifications import (
persist_notification,
Expand Down Expand Up @@ -196,6 +193,7 @@ def process_sms_or_email_notification(
recipient_identifier=recipient_identifier,
billing_code=form.get('billing_code'),
sms_sender_id=form.get('sms_sender_id'),
callback_url=form.get('callback_url'),
)

if 'scheduled_for' in form:
Expand Down Expand Up @@ -233,6 +231,7 @@ def process_notification_with_recipient_identifier(
recipient_identifier=form.get('recipient_identifier'),
billing_code=form.get('billing_code'),
sms_sender_id=form.get('sms_sender_id'),
callback_url=form.get('callback_url'),
)

send_to_queue_for_recipient_info_based_on_recipient_identifier(
Expand Down
2 changes: 2 additions & 0 deletions tests/app/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ def create_notification( # noqa: C901
recipient_identifiers=None,
billing_code=None,
sms_sender_id=None,
callback_url=None,
):
assert job or template
if job:
Expand Down Expand Up @@ -351,6 +352,7 @@ def create_notification( # noqa: C901
'postage': postage,
'billing_code': billing_code,
'sms_sender_id': sms_sender_id,
'callback_url': callback_url,
}
notification = Notification(**data)

Expand Down
8 changes: 7 additions & 1 deletion tests/app/v2/notifications/test_get_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def test_get_notification_by_id_returns_200(
sent_by=provider,
scheduled_for='2017-05-12 15:15',
billing_code='billing_code',
callback_url='https://www.test.com',
)

second_notification = create_notification(
Expand Down Expand Up @@ -80,6 +81,7 @@ def test_get_notification_by_id_returns_200(
'sms_sender_id': None,
'cost_in_millicents': 0.0,
'segments_count': 0,
'callback_url': 'https://www.test.com',
}

assert json_response == expected_response
Expand Down Expand Up @@ -110,7 +112,10 @@ def test_get_notification_by_id_with_placeholders_and_recipient_identifiers_retu
):
template = sample_template(template_type=EMAIL_TYPE, content='Hello ((name))\nThis is an email from va.gov')
notification = create_notification(
template=template, personalisation={'name': 'Bob'}, recipient_identifiers=recipient_identifiers
template=template,
personalisation={'name': 'Bob'},
recipient_identifiers=recipient_identifiers,
callback_url='https://www.test.com',
)

auth_header = create_authorization_header(sample_api_key(service=template.service))
Expand Down Expand Up @@ -160,6 +165,7 @@ def test_get_notification_by_id_with_placeholders_and_recipient_identifiers_retu
'sms_sender_id': None,
'cost_in_millicents': 0.0,
'segments_count': 0,
'callback_url': 'https://www.test.com',
}

assert json_response == expected_response
Expand Down
84 changes: 82 additions & 2 deletions tests/app/v2/notifications/test_post_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ def test_post_sms_notification_returns_201(
mocker,
):
template = sample_template(content='Hello (( Name))\nYour thing is due soon')
data.update({'template_id': str(template.id), 'personalisation': {' Name': 'Jo'}})
data.update(
{'template_id': str(template.id), 'personalisation': {' Name': 'Jo'}, 'callback_url': 'https://www.test.com'}
)
if reference is not None:
# Have to set reference for the asserts below, can't add it to data in the None case
reference = str(uuid.uuid4())
Expand All @@ -89,6 +91,7 @@ def test_post_sms_notification_returns_201(
assert len(notifications) == 1
assert notifications[0].status == NOTIFICATION_CREATED
assert notifications[0].postage is None
assert notifications[0].callback_url == 'https://www.test.com'

# endpoint checks
assert resp_json['id'] == str(notifications[0].id)
Expand All @@ -100,7 +103,7 @@ def test_post_sms_notification_returns_201(
assert resp_json['template']['version'] == template.version
assert 'services/{}/templates/{}'.format(template.service_id, template.id) in resp_json['template']['uri']
assert not resp_json['scheduled_for']

assert resp_json['callback_url'] == 'https://www.test.com'
if 'recipient_identifier' not in data:
assert mock_deliver_sms.called
# Else, for sending with a recipient ID, the delivery function won't get called because the preceeding
Expand Down Expand Up @@ -392,6 +395,76 @@ def test_notification_returns_400_and_for_schema_problems(
} in error_resp['errors']


def test_post_sms_notification_without_callback_url(
client,
notify_db_session,
sample_api_key,
sample_template,
):
template = sample_template(content='Hello (( Name))\nYour thing is due soon')

data = {'phone_number': '+16502532222', 'template_id': str(template.id), 'personalisation': {' Name': 'Jo'}}

response = post_send_notification(client, sample_api_key(service=template.service), SMS_TYPE, data)

assert response.status_code == 201
resp_json = response.get_json()
assert validate(resp_json, post_sms_response) == resp_json

notifications = notify_db_session.session.scalars(
select(Notification).where(Notification.service_id == template.service_id)
).all()

assert len(notifications) == 1

notification = notifications[0]
assert notification.callback_url is None

assert resp_json['id'] == str(notification.id)
assert resp_json['content']['body'] == template.content.replace('(( Name))', 'Jo')
assert resp_json['callback_url'] is None


@pytest.mark.parametrize(
'callback_url, expected_error',
[
('invalid-url', 'is not a valid URI.'),
('http://wrongformat.com', 'does not match ^https.*'),
('www.missingprotocol.com', 'does not match ^https.*'),
(
'https://example.com/search?query=this_is_a_search_term_to_reach_exactly_256_charactersaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&filter=type=all&status=active&sort=ascending&page=1234567890&session_token=abc123&tracking_id=unique_user_tracking_value',
'is too long',
),
],
)
def test_notification_returns_400_if_invalid_callback_url(
client,
sample_api_key,
sample_template,
callback_url,
expected_error,
):
template = sample_template(content='Hello (( Name))\nYour thing is due soon')

data = {
'phone_number': '+16502532222',
'template_id': str(template.id),
'personalisation': {' Name': 'Jo'},
'callback_url': callback_url,
}

response = post_send_notification(client, sample_api_key(service=template.service), SMS_TYPE, data)

assert response.status_code == 400
assert response.headers['Content-type'] == 'application/json'
error_resp = response.get_json()

assert error_resp['status_code'] == 400
assert {'error': 'ValidationError', 'message': f'callback_url {callback_url} {expected_error}'} in error_resp[
'errors'
]


@pytest.mark.parametrize('reference', [None, 'reference_from_client'])
def test_post_email_notification_returns_201(
client,
Expand All @@ -407,6 +480,7 @@ def test_post_email_notification_returns_201(
'template_id': template.id,
'personalisation': {'name': 'Bob'},
'billing_code': 'TESTCODE',
'callback_url': 'https://www.test.com',
}

if reference is not None:
Expand All @@ -421,6 +495,8 @@ def test_post_email_notification_returns_201(
)
assert notification.status == NOTIFICATION_CREATED
assert notification.postage is None
assert notification.callback_url == 'https://www.test.com'

assert resp_json['id'] == str(notification.id)
assert resp_json['billing_code'] == 'TESTCODE'
assert resp_json['reference'] == reference
Expand All @@ -432,6 +508,7 @@ def test_post_email_notification_returns_201(
assert resp_json['template']['id'] == str(template.id)
assert resp_json['template']['version'] == template.version
assert 'services/{}/templates/{}'.format(str(template.service_id), str(template.id)) in resp_json['template']['uri']
assert resp_json['callback_url'] == 'https://www.test.com'
assert not resp_json['scheduled_for']
assert mock_deliver_email.called

Expand All @@ -456,6 +533,7 @@ def test_post_email_notification_with_reply_to_returns_201(
'template_id': template.id,
'personalisation': {'name': 'Bob'},
'billing_code': 'TESTCODE',
'callback_url': 'https://www.test.com',
}

if reference is not None:
Expand All @@ -474,6 +552,7 @@ def test_post_email_notification_with_reply_to_returns_201(
assert notification.status == NOTIFICATION_CREATED
assert notification.postage is None
assert notification.reply_to_text == '[email protected]'
assert notification.callback_url == 'https://www.test.com'

# endpoint checks
assert resp_json['id'] == str(notification.id)
Expand All @@ -486,6 +565,7 @@ def test_post_email_notification_with_reply_to_returns_201(
assert resp_json['template']['version'] == template.version
assert 'services/{}/templates/{}'.format(str(template.service_id), str(template.id)) in resp_json['template']['uri']
assert not resp_json['scheduled_for']
assert resp_json['callback_url'] == 'https://www.test.com'
assert mock_deliver_email.called


Expand Down

0 comments on commit c63a559

Please sign in to comment.