1) It's incr not inc on the redis client, so renamed the calls everywhere
2) Redis returns bytes/string rather than an int if the value stored is an int. Cast the result to an int before use. Not you can set up the GET to do this transparently but I've not done this as we *may * use GETS for non-int and the callback sets up the cast for the connection not the call.
These means that the cache count is on Notifications in the database NOT notifications sent to providers. If the provider fails to accept the notification, it still counts.
I think this is correct, as they have done the work to send it so we should count it, though there is an argument that we should count them on sending?
- Uses Redis cache to check for current count
- If not present then sets the value based on the database state
- Any Redis errors are swallowed. Cache failures should NOT fail the request.
- It would be nice to refactor the send_sms and send_email tasks to use these common functions as well, that way I can get rid of the new Notifications.from_v2_api_request method.
- Still not happy with the format of the errors. Would like to find a happy place, where the message is descript enough that we do not need external documentation to explain the error. Perhaps we still only need documentation to explain the trial mode concept.
- Use these validation methods in post_sms_notification and the version 1 of post_notification.
- Create a v2 error handlers.
- InvalidRequest has a to_dict method for private and v1 error responses and a to_dict_v2 method to create the v2 of the error responses.
- Each validation method has extensive unit tests, so the unit test for the endpoint do not need to check every error case, but check that the error handle formats the message correctly.
- The format of the error messages is still a work on progress.
- This version of the api could be deployed without causing a problem to the application.
- The new endpoing is still a work in progress and is not being used yet.
when given any log function with multiple parameters, the python logging utils
assume the first param is a format string and the rest are arguments to pass
in - we were passing in the exception object to `logger.exception`, however,
the purpose of .exception is to add the exception object itself - so we didn't
need to
Currently getting a single notification by ID is restricted to
notifications created with the same key type.
This makes things awkward for the functional tests now we’ve removed the
ability to create live keys in trial mode. So this commit removes the
restriction, so that any key can get any notification, no matter how it
was created.
And you’re never going to guess a UUID, so the chances of this giving
you privileged access to someone’s personal information is none.
This does not change the get all notifications endpoint, which
absolutely should be restricted by key type.
Refactored send_notifications method so that it is more readible.
Refectored the test_send_notificaitons so that it uses parametrized test to avoid duplication.
We wrote to the queue as a performance optimisation , however dev found it confusing to not have immediate access to the notification as it may be perished some minutes later under periods of load. Additionally we had a couple of DB issues which led to us dropping notifications.
Pushing the DB write to earlier in the flow makes the system a little more robust in the early days, we may want to change this when the traffic increases.
Logic:
- live services don't check days limit for now
- restricted services check limits
(caveat) simulate keys aren't checking day limit even in restricted mode.
When you use a simulate API key it should behave like a live service,
except for actually sending the messages. There should be no limits even
if the service is in trial mode.
This commit removes the restriction on sending messages when you’ve sent
up to your daily limit.
When you use a simulate API key it should behave like a live service,
except for actually sending the messages. There should be no limits even
if the service is in trial mode.
This commit removes the restriction on sending messages to peole outside
your team when you’re in trial mode.
Previously there were 4 queues for sending messages
The was based on the fact that each notification has 2 actions - persist in the database and send to provider.
Two queues supported the CSV upload - for the first of these tasks
- bulk-email
- build-sms
And there were two more queues for the tasks that make the 3rd party client calls.
- sms
- email
API Calls just used the latter two queues for both tasks
Added four new queues
- db-email
- db-sms
- send-sms
- send-email
So an API call puts a notification into the db-[type] queue first, which then puts the notification into the send-[type] queue
Build queues stay as before.
This will allow us to target processing of these tasks with separate workers to manage these differently.
- As before this is now driven from the notifications history table
- Removed from updates and create
- Signatures changes to removed unused params hits many files
- Also potential issue around rate limiting - we used to get the number sent per day from the stats table - which was a single row lookup, now we have to count this. This applies to EVERY API CALL. Probably not a good thing and should be addressed urgently.
Although using a team key is functionally the same as your service being
restricted, conflating the two errors is not helpful. What we typically
saw in research was that someone was using a team key, got the error,
used a live key and got the _same_ error.
This commit adds a new error message that specifically mentions the type
of API key that you’re using.
Scenario we saw in research:
- trying to send a message to someone outside your team
- service is in trial mode
Result:
- error message was terrible, no-one understood it
Solution:
- better error message
