Refactor process_firetext_responses
Removed the abstract ClientResponses for firetext and mmg. There is a map for each response to handle the status codes sent by each client.
Since MMG has about 20 different status code, none of which seem to be a pending state (unlike firetext that has 3 status one for pending - network delay).
For MMG status codes, look for 00 as successful, everything else is assumed to be a failure.
The Celery `send_sms` and `send_email` tasks _could_ assume that all the
recipients it gets are safe, because they have been checked either:
- when the admin app processes the CSV
- in the `/notifications/sms|email` endpoint
*However*, it’s probably safer to make the check again when the Celery
task run and passes the message off to the third party.
This also means that changing a service _back_ to restricted would have
an effect on messages that were queued, as well as all subsequent
messages.
This commit:
- restores the test that was removed here:
e56aee5d1d (diff-e5627619e387fccc04783c32a23e6859L346)
- adds checks back into the Celery tasks for sending email and SMS,
using the `allowed_to_send_to` function that was introduced into utils
in https://github.com/alphagov/notifications-utils/pull/16
Implements
https://github.com/alphagov/notifications-utils/pull/16
Once
https://github.com/alphagov/notifications-admin/pull/376
is merged it will no longer be possible for a user to upload a CSV file
containing recipients that they’re not allowed to send to.
So this commit also removes any restricted service checks in the task,
because any public phone numbers/email addresses no longer have any way
of reach this point if the service is restricted.
If a service is in restricted mode then a user can’t send messages to anyone
other than themselves and members of their team. To do this the API has to
compare the numbers they are sending to with those of their team members.
It will (falsely) say the numbers do not match if they are in a different
format, eg 07700 900849 vs +447700900849
This commit uses the code we use elsewhere for formatting phone numbers to
make sure that both numbers are in a consistent format before doing a
comparison.
I have a strong preference for doing it this way, rather than formatting numbers
before we store them:
1. https://en.wikipedia.org/wiki/Robustness_principle
2. It’s confusing to a user to see their own phone number formatted in a
different way to that which they entered it, and the alternative, storing
the phone number in two different formats is grim
- client updated to raise errors with fire text error codes/messages
New endpoint
- /notifications/sms/firetext
For delivery notifications to be sent to.
- If a job starts it MUST be able to fit into the days sending limits
- So if service limit is 10, and we've sent 5 messages and the current job is 4 then it's OK.
- If the job is 6 then it's over the limit and it should fail
- Job should NOT start if can't complete in the limit
Currently when the Celery task processes a CSV it will call the API with the
values for all the non-recipient columns in the `personalisation` field. This
means that those API calls would fail, even though the CSV has been processed
‘successfully’.
This was not being caught by the tests, so this commit adds extra tests to check
what data the task is passing to the API call.
It then updates utils to version 2.0.1 which brings in this fix:
https://github.com/alphagov/notifications-utils/pull/10
- to capture the counts of things that we do
- initial commit captures when we create an email or sms
DOES NOT know about ultimate success only that we asked our partners to ship the notification
Requires some updates when we retry sending in event of error.
This is checked on 3rd party API calls, but jobs (CSV files) were able expected to only allow valid files.
Change in tack means we want to have restricted notification failures reported in the UI.
When building the template it was looking for a placeholder called
((phone number)). This caused it to fail because the template it had did not
match the personalisation it was being given.
`Template` has an optional parameter for specifying personalisation values that
should be ignored. The recipient of a message is an example of such a value.
This commit passes that extra parameter, which fixes that bug.
This commit replaces placeholders in a template with the user’s data, using
the Template class from utils
(https://github.com/alphagov/notifications-utils/tree/master/utils#template)
It also extracts the personalisation data from the CSV, taking account of the
different column headings that SMS and email CSVs will have.
At the point of creating the task to send an individual messages, validation of
the placeholders matching the template is assumed to have been done either:
- in processing the CSV in the admin app
- in the endpoint for the API call
No exceptions should be raised at this point.
