Merge pull request #3986 from alphagov/fix-callbacks

Fix delivery callbacks sometimes updating complaints callbacks instead

app/dao/service_callback_api_dao.py

@@ -27,8 +27,10 @@ def reset_service_callback_api(service_callback_api, updated_by_id, url=None, be
     db.session.add(service_callback_api)
 
 
-def get_service_callback_api(service_callback_api_id, service_id):
-    return ServiceCallbackApi.query.filter_by(id=service_callback_api_id, service_id=service_id).first()
+def get_service_callback_api(service_callback_api_id, service_id, callback_type):
+    return ServiceCallbackApi.query.filter_by(
+        id=service_callback_api_id, service_id=service_id, callback_type=callback_type
+    ).first()
 
 
 def get_service_delivery_status_callback_api_for_service(service_id):

app/schemas.py

@@ -229,6 +229,14 @@ class ServiceSchema(BaseSchema, UUIDsAsStringsMixin):
     custom_email_sender_name = fields.String(allow_none=True)
     # this can only be set via custom_email_sender_name or name
     email_sender_local_part = fields.String(dump_only=True)
+    service_callback_api = fields.Method("service_delivery_status_callback_api")
+
+    def service_delivery_status_callback_api(self, service):
+        return [
+            callback.id
+            for callback in service.service_callback_api
+            if callback.callback_type == app.constants.DELIVERY_STATUS_CALLBACK_TYPE
+        ]
 
     def _get_allowed_broadcast_provider(self, service):
         return service.allowed_broadcast_provider
@@ -324,6 +332,14 @@ class DetailedServiceSchema(BaseSchema):
     name = fields.String()
     custom_email_sender_name = fields.String(required=False)
     email_sender_local_part = fields.String()
+    service_callback_api = fields.Method("service_delivery_status_callback_api")
+
+    def service_delivery_status_callback_api(self, service):
+        return [
+            callback.id
+            for callback in service.service_callback_api
+            if callback.callback_type == app.constants.DELIVERY_STATUS_CALLBACK_TYPE
+        ]
 
     class Meta(BaseSchema.Meta):
         model = models.Service

app/service/callback_rest.py

@@ -96,7 +96,7 @@ def update_service_callback_api(service_id, callback_api_id):
     data = request.get_json()
     validate(data, update_service_callback_api_schema)
 
-    to_update = get_service_callback_api(callback_api_id, service_id)
+    to_update = get_service_callback_api(callback_api_id, service_id, DELIVERY_STATUS_CALLBACK_TYPE)
 
     reset_service_callback_api(
         service_callback_api=to_update,
@@ -109,14 +109,14 @@ def update_service_callback_api(service_id, callback_api_id):
 
 @service_callback_blueprint.route("/delivery-receipt-api/<uuid:callback_api_id>", methods=["GET"])
 def fetch_service_callback_api(service_id, callback_api_id):
-    callback_api = get_service_callback_api(callback_api_id, service_id)
+    callback_api = get_service_callback_api(callback_api_id, service_id, DELIVERY_STATUS_CALLBACK_TYPE)
 
     return jsonify(data=callback_api.serialize()), 200
 
 
 @service_callback_blueprint.route("/delivery-receipt-api/<uuid:callback_api_id>", methods=["DELETE"])
 def remove_service_callback_api(service_id, callback_api_id):
-    callback_api = get_service_callback_api(callback_api_id, service_id)
+    callback_api = get_service_callback_api(callback_api_id, service_id, DELIVERY_STATUS_CALLBACK_TYPE)
 
     if not callback_api:
         error = "Service delivery receipt callback API not found"

tests/app/dao/test_service_callback_api_dao.py

@@ -4,6 +4,7 @@
 from sqlalchemy.exc import SQLAlchemyError
 
 from app import encryption
+from app.constants import COMPLAINT_CALLBACK_TYPE, DELIVERY_STATUS_CALLBACK_TYPE
 from app.dao.service_callback_api_dao import (
     get_service_callback_api,
     get_service_delivery_status_callback_api_for_service,
@@ -146,22 +147,45 @@ def test_update_service_callback_api(sample_service):
 
 
 def test_get_service_callback_api(sample_service):
-    service_callback_api = ServiceCallbackApi(
+    service_delivery_callback_api = ServiceCallbackApi(
         service_id=sample_service.id,
-        url="https://some_service/callback_endpoint",
-        bearer_token="some_unique_string",
+        url="https://some_service/delivery_callback_endpoint",
+        bearer_token="delivery_unique_string",
         updated_by_id=sample_service.users[0].id,
+        callback_type=DELIVERY_STATUS_CALLBACK_TYPE,
     )
-    save_service_callback_api(service_callback_api)
+    save_service_callback_api(service_delivery_callback_api)
 
-    callback_api = get_service_callback_api(service_callback_api.id, sample_service.id)
-    assert callback_api.id is not None
-    assert callback_api.service_id == sample_service.id
-    assert callback_api.updated_by_id == sample_service.users[0].id
-    assert callback_api.url == "https://some_service/callback_endpoint"
-    assert callback_api.bearer_token == "some_unique_string"
-    assert callback_api._bearer_token != "some_unique_string"
-    assert callback_api.updated_at is None
+    service_complaint_callback_api = ServiceCallbackApi(
+        service_id=sample_service.id,
+        url="https://some_service/complaint_callback_endpoint",
+        bearer_token="complaint_unique_string",
+        updated_by_id=sample_service.users[0].id,
+        callback_type=COMPLAINT_CALLBACK_TYPE,
+    )
+    save_service_callback_api(service_complaint_callback_api)
+
+    delivery_callback_api = get_service_callback_api(
+        service_delivery_callback_api.id, sample_service.id, DELIVERY_STATUS_CALLBACK_TYPE
+    )
+    assert delivery_callback_api.id is not None
+    assert delivery_callback_api.service_id == sample_service.id
+    assert delivery_callback_api.updated_by_id == sample_service.users[0].id
+    assert delivery_callback_api.url == "https://some_service/delivery_callback_endpoint"
+    assert delivery_callback_api.bearer_token == "delivery_unique_string"
+    assert delivery_callback_api._bearer_token != "delivery_unique_string"
+    assert delivery_callback_api.updated_at is None
+
+    complaint_callback_api = get_service_callback_api(
+        service_complaint_callback_api.id, sample_service.id, COMPLAINT_CALLBACK_TYPE
+    )
+    assert complaint_callback_api.id is not None
+    assert complaint_callback_api.service_id == sample_service.id
+    assert complaint_callback_api.updated_by_id == sample_service.users[0].id
+    assert complaint_callback_api.url == "https://some_service/complaint_callback_endpoint"
+    assert complaint_callback_api.bearer_token == "complaint_unique_string"
+    assert complaint_callback_api._bearer_token != "complaint_unique_string"
+    assert complaint_callback_api.updated_at is None
 
 
 def test_get_service_delivery_status_callback_api_for_service(sample_service):

tests/app/test_schemas.py

@@ -2,11 +2,13 @@
 from marshmallow import ValidationError
 from sqlalchemy import desc
 
+from app.constants import COMPLAINT_CALLBACK_TYPE, DELIVERY_STATUS_CALLBACK_TYPE
 from app.dao.provider_details_dao import (
     dao_update_provider_details,
     get_provider_details_by_identifier,
 )
-from app.models import ProviderDetailsHistory
+from app.dao.service_callback_api_dao import save_service_callback_api
+from app.models import ProviderDetailsHistory, ServiceCallbackApi
 from tests.app.db import create_api_key
 
 
@@ -149,3 +151,29 @@ def test_provider_details_history_schema_returns_user_details(
     data = provider_details_schema.dump(current_sms_provider_in_history)
 
     assert sorted(data["created_by"].keys()) == sorted(["id", "email_address", "name"])
+
+
+def test_service_schema_only_returns_delivery_status_callback_api(sample_service):
+    from app.schemas import service_schema
+
+    service_delivery_callback_api = ServiceCallbackApi(
+        service_id=sample_service.id,
+        url="https://some_service/delivery_callback_endpoint",
+        bearer_token="delivery_unique_string",
+        updated_by_id=sample_service.users[0].id,
+        callback_type=DELIVERY_STATUS_CALLBACK_TYPE,
+    )
+    save_service_callback_api(service_delivery_callback_api)
+
+    service_complaint_callback_api = ServiceCallbackApi(
+        service_id=sample_service.id,
+        url="https://some_service/complaint_callback_endpoint",
+        bearer_token="complaint_unique_string",
+        updated_by_id=sample_service.users[0].id,
+        callback_type=COMPLAINT_CALLBACK_TYPE,
+    )
+    save_service_callback_api(service_complaint_callback_api)
+
+    data = service_schema.dump(sample_service)
+
+    assert data["service_callback_api"] == [str(service_delivery_callback_api.id)]