BUG: missing email subject personalization throws errors

[cris-oddball]

Description

When sending an email using a template that has a subject personalization and that subject personalization is not sent in the POST payload, one of two things may happen:

• If the email body also has a personalization field which is sent in the payload, then the service returns a 500 Internal Server Error and the logs indicate a NullValueForNonConditionalPlaceholderException was raised.
• If the email body contains no personalization then the message is send but the subject is then modified to include the personalization field and span headers:

QA Test Thoughts and no body personalization <span class='placeholder'>((thoughts))</span>

Either way, the service does not correctly handle the missing payload fields for email subjects.

Unclear if this is a Utils repo issue, which it may be based on the logs seen.

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

Steps to Reproduce

First Condition: Email template has personalization fields in the subject and body.
VANotify Service, template ID b2c9ff49-b37b-4352-ab0c-3d3d0710fe4c
The Happy Path payload would be

{
    "template_id": "b2c9ff49-b37b-4352-ab0c-3d3d0710fe4c",
    "email_address": "<your email here>",
    "personalisation": {
        "thoughts": "TEST"
        "body": "QA says Hello"
  }
}

1. In Postman, set service to VANotify with email_template_id set to b2c9ff49-b37b-4352-ab0c-3d3d0710fe4c
2. Use the POST send email route with that template ID and this payload (missing the subject personalization "thoughts"):

{
    "template_id": "b2c9ff49-b37b-4352-ab0c-3d3d0710fe4c",
    "email_address": "<your email here>",
    "personalisation": {
        "body": "Hello There!"
  }
}

3. Note that the response is a 500 Internal Server Error.
   A notification ID will be found in the logs, the sequence is something like this (or just search for NullValueForNonConditionalPlaceholderException)

raise NullValueForNonConditionalPlaceholderException
notifications_utils.field.NullValueForNonConditionalPlaceholderException
[2023-08-04 19:19:11,610: INFO/ForkPoolWorker-4] Notification id: d5ff1b57-df7a-4af6-bb3c-f87acbf73c6e, max retries: 2886, retries: 210

There will also be a stack trace broken out over multiple lines that looks like:

Second Condition: Email template has personalization field in subject, only.
VANotify Service, template ID 64b0953c-b7ea-4b48-b70f-d242ce5b17d2
The Happy Path payload would be

{
    "template_id": "64b0953c-b7ea-4b48-b70f-d242ce5b17d2",
    "email_address": "<your email here>",
    "personalisation": {
        "thoughts": "TEST"
  }
}

1. In Postman, set service to VANotify with email_template_id set to 64b0953c-b7ea-4b48-b70f-d242ce5b17d2
2. Use the POST send email route with that template ID and this payload (missing the subject personalization "thoughts"):

{
    "template_id": "64b0953c-b7ea-4b48-b70f-d242ce5b17d2",
    "email_address": "<your email here>",
    "personalisation": {}
}

3. Observe that the response is a 201 created, the email is sent/received and GET notifications for the notification ID returns a 200 and the relevant data.
4. Observe that the email's subject line contains the personalization field and span headers.
   

Workaround

Is there something we can do to work around this issue in the meantime?

• None - always send the appropriate payloads.

Impact/Urgency

Medium - inconvenient at best.
This is observed in the Staging environment as well, so is unrelated to any work happening in the current sprint.

Expected Behavior

Note: The majority of the work is in utils, but there will be minimal work in API to use the new utils version and throw the appropriate validation error. Please do both together in this ticket.

• Our service should validate any email subject personalization is present.
• When an email subject personalization is missing from an otherwise good payload, the service should return a 400 response about a missing property, not create a notification ID, and not send or retry the email.
• Stretch goal: anytime a NullValueForNonConditionalPlaceholderException is raised, the message should never be retried.
• Update the Slide deck with the key win and a demo slide

QA Considerations

• regression passes
• test cases above in steps to reproduce a 400 error, not a 500 or a 201.

Additional Info & Resources

[k-macmillan]

The issue stems from subjects not being checked for "missing data". That's how we currently report this error when data is missing from the content/body. Since this fails to check for missing data it makes it through the validation checks, then fails when attempting to send.

The utils Template class has validation in the form of the missing_data property, which the API uses to check for missing content. The WithSubjectTemplate class needs to run a similar check on the subject.

The problem is our API code calls template.missing_data, and missing_data is implemented in the base class. The WithSubjectTemplate needs to override the property and take into account both self._subject and self.content, comparing to self.values.

Template class missing_data property:

return list(
            placeholder_name for placeholder_name in Field(self.content).placeholder_names
            if self.values.get(placeholder_name) is None
        )

Probable solution:
WithSubjectTemplate class missing_data property:

@property
def missing_data(self):
  return [placeholder_name for placeholder_name in Field(self.content + self._subject).placeholder_names if self.values.get(placeholder_name) is None]
        )

It may be more complicated because the base class Template modifies self.values (it's a property), so it may need to be handled slightly differently.

[mjones-oddball]

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

[tabinda-syed]

Whoever picks this up, please sync with Kyle first (unless his notes above are clear for you).

[tabinda-syed]

Alert the team asap if unit test-related work begins to get out of hand.

[k-macmillan]

okay to keep