v3 POST notification (part 1)

[kalbfled]

User Story - Business Need

• [x] Ticket is understood, and QA has been contacted (if the ticket has a QA label).

User Story(ies)

As a Notify developer, I want creating and sending notifications to be fast So that we can scale to many more messages sent.

As a Notify client, I want clean and descriptive HTTP responses So that we can easily parse and capture the result of our attempt to send a notification.

Additional Info and Resources

• v3 flowchart
• JSON schema documentation
• Understanding JSON Schema

Engineering Checklist

Do not use any code from the app/dao/ folder. Make all queries directly using flask-sqlalchemy.

• [x] Create the folders app/v3/notifications/ and tests/app/v3/notifications/. All new code should be in these folders.
• [x] Implement app/v3/notifications/notification_schema.py. It should contain the JSON schema appropriate for validating a request's POST data. See the v2 schemas as an example, but ensure v3 works for any notification type (don't have a separate schema for sms and e-mail, etc.). Do not permit any field names that are not Notification model attributes. This is a departure from v2, which takes fields like "phone_number" and "email_address" that later map to the "to" field.
• [x] Write unit tests for the v3 schema in tests/app/v3/notifications/test_notification_schemas.py. Tests should directly call jsonschema.validate (example) with various request data.
• [x] Implement the route handler app/v3/notifications/rest.py::post_notification (example) to handle POST requests.
  • [x] Authenticate the request not utilizing the existing auth unless necessary
  • [x] Validate the request data; return 400 on failure
  • [x] Feature flag with only dev/perf allowed in task definitions
  • [x] Create a UUID4
  • [x] Call apply_async to pass the UUID4 and request data to Celery (comment this out for now)
  • [x] Return HTTP status 202 (accepted) with the UUID4 as the response body
• [x] Write unit tests for the new route in tests/app/v3/notifications/test_rest.py
• [x] Create and register a new blueprint here
• [ ] Include JSON request(s) for testing v3 below: to be populated

Acceptance Criteria

• [x] All existing unit tests are untouched and still pass
• [x] New routes are /v3/notification/email and /v3/notification/sms
• [x] Authentication is handled and responds with a 403 if it fails
• [x] Validation is handled and responds with a 400 if it fails
• [x] All validation schema attributes mirror Notification model attributes
• [x] Feature flag is in-place to only allow this to work in Dev and Perf (returns 404)
• [x] A 202 status code and notification id are in the response for successful requests
• [x] Notifications can be sent with any of these individually: phone number, e-mail address, or Recipient ID
• [x] A POST notification request that contains a phone number and e-mail address is not valid
• [x] A POST notification request that contains a Recipient ID and a (phone number or e-mail address) (but not both) is valid
• [x] There are test cases for the above criteria
• [x] Code is setup in an extensible manner - There's clearly defined and reusable functional separation
  • e.g. A generic validation method that is self-contained (does not expect another function to handle its errors)
• [x] The implementation does not use any code from app/dao/

QA Considerations

For QA to populate. Leave blank if QA is not applicable on this ticket.

• [x] regression passes
• [x] swagger is reviewed for accuracy
• [x] postman is review for accuracy
• [x] determine what, if any changes are needed for the QA Suite based on convo with dev

[mjones-oddball]

Hey team! Please add your planning poker estimate with Zenhub @babyjunior56 @cris-oddball @EvanParish @k-macmillan @kalbfled @ldraney @nikolai-efimov

[tabinda-syed]

A note to Engs. from our 8/7 Refinement: We've pointed this at a 5 with QA considered. This means about 3 of the points are available to you - if your effort on this ticket begins to go beyond that, please bring it up to the team ASAP. Thank you.

[kalbfled]

I finished the initial development work for this issue, but I still need to create the new openapi and Postman documentation.

@cris-oddball As discussed, the new unit tests provide relevant post request bodies.

[cris-oddball]

Code approved, merged to master, deployed to Perf. regression: 134 passed, 3 xfailed in 143.46s (0:02:23)

Will do manual testing tomorrow.

[kalbfled]

@mjones-oddball

User-Facing Differences Between V2 and V3 for Notifications

• V3 returns http status 202 for valid request data. V2 return 201.
• V3 error responses do not include the http status code in the response data because that is already part of the http response. In general, a V3 error response looks like this . . .

{
    "errors": [
        {
            "error": "AuthError",
            "message": "Unauthorized, authentication token must be provided"
        }
    ]
}

. . . where the values of "error" reflects the exception that was handled, and "message" provides details.

Other Differences Between V2 and V3 for Notifications

• According to their respective schema definitions, V2 and V3 requests both can contain direct contact info (e-mail address or phone number) and a recipient ID. The future V3 implementation (#1361) will use both, as appropriate. V2 will look up the contact info again if a recipient ID is given even if that's in the request data. Changing that behavior for V2 would be non-trivial.

[cris-oddball]

Happy Path email test cases pass (automated) Tests check for the 202 response, that only an "id" is returned and that the "id" is a UUID

Private Zenhub Image

test_send_v3_email[Email: Basic Formatting] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'email_address': 'vanotify.qa@gmail.com'}

test_send_v3_email[Email: Personalization] PASSED

{'template_id': 'c1616f63-2f9a-45e9-8ea9-e00072987451', 'email_address': '<redacted>', 'personalisation': {'name': 'Veteran', 'adjective': 'happy'}}

test_send_v3_email[Email: Basic Formatting and Optional Properties] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'email_address': '<redacted>', 'client_reference': 'string', 'reference': 'string', 'billing_code': 'qa-test-003'}

test_send_v3_email[Email: Basic Formatting w/ VAPROFILEID] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'recipient_identifier': {'id_type': 'VAPROFILEID', 'id_value': '120451'}}

test_send_v3_email[Email: Basic Formatting w/ email and VAPROFILEID] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'email_address': '<redacted>', 'recipient_identifier': {'id_type': 'VAPROFILEID', 'id_value': '120451'}}

test_send_v3_email[Email: Basic Formatting w/ ICN] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'recipient_identifier': {'id_type': 'ICN', 'id_value': '1012858648V984772'}}

test_send_v3_email[Email: Basic Formatting w/ email and ICN] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'email_address': '<redacted>', 'recipient_identifier': {'id_type': 'ICN', 'id_value': '1012858648V984772'}}

test_send_v3_email[Email: Basic Formatting w/ BIRLSID] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'recipient_identifier': {'id_type': 'BIRLSID', 'id_value': '858131208'}}

test_send_v3_email[Email: Basic Formatting w/ email and BIRLSID] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'email_address': '<redacted>', 'recipient_identifier': {'id_type': 'BIRLSID', 'id_value': '858131208'}}

test_send_v3_email[Email: Basic Formatting w/ EDIPI] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'recipient_identifier': {'id_type': 'EDIPI', 'id_value': '1607380704'}}

test_send_v3_email[Email: Basic Formatting w/ email and EDIPI] PASSED

{'template_id': '324a5b71-8f3e-4617-9ba4-9e0da94bfe30', 'email_address': '<redacted>', 'recipient_identifier': {'id_type': 'EDIPI', 'id_value': '1607380704'}}

Test Response Private Zenhub Image

[cris-oddball]

Happy Path sms test cases (automated) Tests check for the 202 response, that only an "id" is returned and that the "id" is a UUID Private Zenhub Image

test_send_v3_sms[SMS: Short] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'phone_number': '+14254147755'}

test_send_v3_sms[SMS: Short Personalized] PASSED

{'template_id': '6a2a27bf-8ede-4f67-9b21-01d4dd007356', 'phone_number': '+14254147755', 'personalisation': {'name': 'Friend', 'place': 'D.C.', 'year': '2023'}}

test_send_v3_sms[SMS: Short w/ optional properties] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'phone_number': '+14254147755', 'reference': 'string', 'billing_code': 'qa-test-004'}

test_send_v3_sms[SMS: Short w/ alternate Pinpoint SMS Sender ID] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'phone_number': '+14254147755', 'sms_sender_id': 'b6288085-824f-4ee5-864e-8f2db5d7d0c9'}

test_send_v3_sms[SMS: Twilio w/ alternate Twilio SMS Sender ID] PASSED

{'template_id': '438a2240-ea2e-42d0-b0b8-4d63fc172a55', 'phone_number': '+14254147755', 'sms_sender_id': '6db4c815-fe8b-4c34-bea8-536a0976993d', 'billing_code': 'vanotify-testing'}

test_send_v3_sms[SMS: Short w/ BIRLSID recipient ID] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'recipient_identifier': {'id_type': 'BIRLSID', 'id_value': '858131208'}}

test_send_v3_sms[SMS: Short w/ sms and BIRLSID recipient ID] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'phone_number': '+14254147755', 'recipient_identifier': {'id_type': 'BIRLSID', 'id_value': '858131208'}}

test_send_v3_sms[SMS: Short w/ ICN recipient ID] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'recipient_identifier': {'id_type': 'ICN', 'id_value': '1012858648V984772'}}

test_send_v3_sms[SMS: Short w/ sms and ICN recipient ID] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'phone_number': '+14254147755', 'recipient_identifier': {'id_type': 'ICN', 'id_value': '1012858648V984772'}}

test_send_v3_sms[SMS: Short w/ EDIPI recipient ID] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'recipient_identifier': {'id_type': 'EDIPI', 'id_value': '1607380704'}}

test_send_v3_sms[SMS: Short w/ sms and EDIPI recipient ID] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'phone_number': '+14254147755', 'recipient_identifier': {'id_type': 'EDIPI', 'id_value': '1607380704'}}

test_send_v3_sms[SMS: Short w/ VAPROFILEID] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'recipient_identifier': {'id_type': 'VAPROFILEID', 'id_value': '120451'}}

test_send_v3_sms[SMS: Short w/ sms and VAPROFILEID] PASSED

{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'phone_number': '+14254147755', 'recipient_identifier': {'id_type': 'VAPROFILEID', 'id_value': '120451'}}

test_send_v3_sms[SMS: Twilio w/ VAPROFILEID w/ Twilio SMS Sender ID] PASSED

{'template_id': '438a2240-ea2e-42d0-b0b8-4d63fc172a55', 'recipient_identifier': {'id_type': 'VAPROFILEID', 'id_value': '120451'}, 'sms_sender_id': '6db4c815-fe8b-4c34-bea8-536a0976993d', 'billing_code': 'vanotify-testing'}

Test Response Private Zenhub Image

[cris-oddball]

TEST FAILURE on negative test cases (email and sms)

The template_id should be a valid UUID. There is a unit test for this in the code.

However, sending a template_id with some string that is not a valid UUID is allowed, and the response is a 202, created, with a notification ID returned.

Private Zenhub Image

Private Zenhub Image

[cris-oddball]

TEST FAILURE Same as above, except for email validation. Unit test also exists for this.

However, posting an email_address with some string that is not a valid email is allowed, and the response is a 202, created, with a notification ID returned.

[cris-oddball]

TEST FAILURE ? Same as above, except for phone_number validation. I could not find a unit test so not sure what to expect here.

However, posting a phone_number with some string that is not a valid number is allowed, and the response is a 202, created, with a notification ID returned. By valid, I means of the format "+11234567890"

[kalbfled]

The phone number failure is expected because Json-schema does not have a formatter for phone numbers. (See here.) Valid phone numbers should be checked downstream. I could try using a regex formatter, but that would probably be error prone given that we support international numbers.

The UUID failures probably have something to do with my use of Draft202012Validator not working the same as jsonschema.validator, which is what the unit tests use, although it's not obvious to me from the docs why there would be a difference.

According to these docs, Draft202012Validator should verify "email" and "uuid" formats out of the box. ------- Original Message ------- On Thursday, August 31st, 2023 at 2:10 PM, Cris Stoddard @.***> wrote:

TEST FAILURE ? Same as above, except for phone_number validation. I could not find a unit test so not sure what to expect here.

However, posting a phone_number with some string that is not a valid number is allowed, and the response is a 202, created, with a notification ID returned. By valid, I means of the format "+11234567890"

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were assigned.Message ID: @.***>

[cris-oddball]

TEST FAILURE ? Sending a valid UUID for template_id but for a template that does not exist results in a 202 and a notification_id is created.

[cris-oddball]

@kalbfled

The phone number failure is expected because Json-schema does not have a formatter for phone numbers. (See here.) Valid phone numbers should be checked downstream. I could try using a regex formatter, but that would probably be error prone given that we support international numbers.

There is a python library to validate phone numbers

Let's discuss when you get back.

[cris-oddball]

TEST FAILURE

Sending an invalid email_reply_to_id (one that does not exist in the service) also fails, a 202 is returned and a notification_id is created.

Expected something like...

{
  "errors": [
    {
      "error": "ValidationError",
      "message": "email_reply_to_id 3720b5c1-f7af-4220-bb28-7f132fc011aa does not exist in database for service id 17adae4d-8207-40b4-ba4c-1a24a5369bce"
    }
  ],
}

Also, email_reply_to_id should be a UUID, not a string. When sent as a string and not a UUID, a 202 is returned and a notification_id is created.

[cris-oddball]

End result email testing. I have XFAILED and not run the failing email tests for now.

[cris-oddball]

End result sms testing. I have XFAILED and not run the failing sms tests

[cris-oddball]

@mjones-oddball and @EvanParish and I discussed holding this ticket into the next sprint when @kalbfled is back and we are back from holiday to revisit what should and should not be working here.

Testing is complete, but with (I think) failed test cases.

Added blocker internal just to remind me that there is nothing more to do until next Tuesday.

A draft QA PR is up here.

[cris-oddball]

Swagger change needed for email_address, in v3, this is just listed as string it should be listed as string($email_address) as seen in v2

Private Zenhub Image

More swagger changes for email and sms:

scheduled_for is present in example but missing from schema and should be added asstring($datetime) with a descriptor as to the date parameters (not before today, not after 96 hours).

scheduled_for example should be a datetime, not "string" (please change)

client_reference is present in schema but missing from example (please add)

@kalbfled

[cris-oddball]

More swagger issues

EMAIL & SMS The 401 authorization message shown in the example is incorrect, it includes the status_code property Private Zenhub Image

The response is actually

{
  "errors": [
    {
      "error": "AuthError",
      "message": "Unauthorized, authentication token must be provided"
    }
  ]
}

The schema is also incorrect for this example and includes a required property. Private Zenhub Image

[cris-oddball]

QA FAILS

So to summarize...

• Posting the template_id should be a valid UUID, should return a 400, it returns a 202
• Posting the email_address with some string that is not a valid email format (like test@example.com vs string) should return a 400, it returns a 202
• Posting the phone_number with some string not in a valid phone number format (like +12345678900 vs string) should return a 400, it returns a 202
• Posting the sms_sender_id should be a valid UUID, should return a 400, it returns a 202
• Swagger issues (see comments above) and adding that the personalisation schema in swagger seems incorrect. We don't just accept a string, but the schema says oneof string (this should be property: string or something) or {file: string($base64), filename: string} - do we really only accept one or the other?
• Now we get to the tricky stuff:
  • posting an sms_sender_id that is not in the service should return what? Note that the swagger example shows this returning a 400 so not clear if this should return a 202 (which it does) and swagger should be changed or if it is supposed to return a 400, in which case, what failure will be reported status in the (future) GET notifications response.
  • posting a template_id that is not part of the service should return? Same as above except this example is not in swagger.

This ticket fails validation, sending back to the TODO column.

[kalbfled]

I recreated the validation problems in the unit tests and am working on a revision. I will also use "phonenumbers" to test for a valid phone number.

[cris-oddball]

Hey @kalbfled , I just caught one more issue.

For /sms, if I pass a payload without the required property phone_number, the response is as follows:

{
    "errors": [
        {
            "error": "ValidationError",
            "message": "{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'notification_type': 'sms'} is not valid under any of the given schemas"
        }
    ]
}

I had expected that the response would indicate that a required property is missing, as it does when you omit the template_id. The error is more understandable to a human that the above, can this be changed?

{
    "errors": [
        {
            "error": "ValidationError",
            "message": "'template_id' is a required property"
        }
    ]
}

[cris-oddball]

One more issue sending an sms with an email_address gives a very unexpected error message, I would have thought more along the lines of ""Additional properties are not allowed ('email_address' was unexpected)"). The same holds for sending an email with a phone_number

{
    "errors": [
        {
            "error": "ValidationError",
            "message": "{'template_id': 'd46ee87b-8a71-4c5c-8654-ed766a9599ff', 'phone_number': '+17192445254', 'email_address': 'test@example.com', 'notification_type': 'sms'} should not be valid under {'anyOf': [{'properties': {'notification_type': {'const': 'email'}}, 'anyOf': [{'required': ['phone_number']}, {'required': ['sms_sender_id']}]}, {'properties': {'notification_type': {'const': 'sms'}}, 'anyOf': [{'required': ['email_address']}, {'required': ['email_reply_to_id']}]}]}"
        }
    ]
}

[cris-oddball]

Deployed to DEV, there are still swagger issues and one route error message that needs to be changed. Left comments on the PR with requests for changes.

Encountering some errors with the v3 routes only

requests.exceptions.ReadTimeout: HTTPSConnectionPool(host='dev-api.va.gov', port=443): Read timed out. (read timeout=30)

This seems to be related to my local network not getting responses back from the public routes in sufficient time. I pushed my branches container to up and ran the v3 regression from the cloud, and did not see any timeout messages. The two failures are expected since they relate to a change requested in the PR.

 =========================== short test summary info ============================
XFAIL tests/test_service_api_key.py::test_create_api_key_incorrect_value_key_type_400 - reason: [NOTRUN] App returns 500, not 400, defect in API-1031
XFAIL tests/test_v2_notifications_email.py::test_send_email_expired_bearer_token_403 - reason: [NOTRUN] Need to write a new method to test expired
XFAIL tests/test_v2_notifications_sms.py::test_send_sms_expired_bearer_token_403 - reason: [NOTRUN] Need to write a new method to test expired
FAILED tests/test_v3_notifications_email.py::test_send_v3_email_invalid_payload_400[400 EMAIL: Missing required property 'email_address' or 'recipient_identifier]
FAILED tests/test_v3_notifications_sms.py::test_send_v3_sms_invalid_payload_400[400 SMS: Missing required property 'phone_number' or 'recipient_identifier]
============= 2 failed, 188 passed, 3 xfailed in 185.10s (0:03:05) =============

Sitting on this one until tomorrow since the QA Suite in the cloud against the v2 routes ran just fine.

[cris-oddball]

Successfully merged to master and deployed to Perf. Regression passes. ================== 190 passed, 3 xfailed in 154.09s (0:02:34) ==================