to get the data for a day can be reasonably slow (a few hundred
milliseconds), and if someone's viewing a service with no activity we
don't want to do that query seven times every two seconds. So if there
is no data in redis, when we get the data out of the database, we
should put it in redis so we can just grab it from there next time.
This'll happen in two cases:
* redis data is deleted
* the service sent no messages that day
additionally, make sure that we convert nicely from redis' return
values (ascii strings) to unicode keys and integer counts.
we should be very careful with when we get data from
NotificationHistory - this should probably only be from scheduled
tasks. `dao_get_template_usage` is only called from the template
statistics rest endpoint, so shouldn't ever hit template history.
also, moved tests out to new file to break up the 2k test file a bit
This PR will optimize this query to use a more efficient index.
- Add notification_type to the dao_get_last_template_usage to optimize the query.
- Tested and analyzed query on production database with very significant results.
Before:
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Limit (cost=0.43..1711.35 rows=1 width=935) (actual time=21186.053..21186.053 rows=0 loops=1)
-> Index Scan Backward using ix_notifications_created_at on notifications (cost=0.43..4607493.80 rows=2693 width=935) (actual time=21186.052..21186.052 rows=0 loops=1)
Filter: (((key_type)::text <> 'test'::text) AND (template_id = 'xxxxxx'::uuid))
Rows Removed by Filter: 8244071
Planning time: 0.112 ms
Execution time: 21186.082 ms
After:
QUERY PLAN
---------------------------------------------------------------------------------------------------------------------
Limit (cost=5323.10..5323.10 rows=1 width=935)
-> Sort (cost=5323.10..5323.74 rows=258 width=935)
Sort Key: created_at DESC
-> Index Scan using ix_notifications_template_id on notifications (cost=0.56..5321.81 rows=258 width=935)
Index Cond: (template_id = 'xxxxx'::uuid)
Filter: (((key_type)::text <> 'test'::text) AND (notification_type = 'sms'::notification_type))
Planning time: 1.102 ms
Execution time: 0.584 ms
SQLAlchemy handles escaping anything that could allow a SQL injection
attack. But it doesn’t escape the characters used for wildcard
searching. This is the reason we’re able to do `.like('%example%')`
at all.
But we shouldn’t be letting our users search with wildcard characters,
so we need to escape them. Which is what this commit does.
It is possible to search for a phone number when from the email notification page and get a SMS message in return.
This also helps to optimise the query.
Phone numbers sometimes contain stuff we normalise out. This matches
perfectly if we have a full phone number, because we can normalise the
thing we’re searching for in the same way as the search term.
With partial search terms we can’t do this completely, because we can’t
work out if ‘123’ is part of a UK number, an international number, the
start of the phone number, the last 3 digits, etc.
What we can do is remove some stuff that we can know will cause partial
search terms to not match:
- leading pluses
- leading `0`s
- any brackets
- any spaces
Users expect the search to work on partial email addresses ‘similar to
Gov.Pay’. We can’t have a situation where Pay are doing something better
than us 😜
This will continue to update the notification history for letter notifications.
We currently have an issue where the responses to letters from the provider is taking a long time.
This is due to the manual nature of their process.
Updating the status of the letter will still work if the notification has been purged.
Also turned back on the purge letter notification scheduled task.
if the international_billing_rates.yml has `dlr: null`, that means we
don't know what delivery receipts they provide - they might not provide
any. So if we do get an update, we don't know for sure that the message
was actually delivered - lets not update it.
- Introduce a `_raise` flag for `get_notification_by_id` so that sql alchemy will raise the NoResults error rather than the app
- Refactor `dao_set_created_live_letter_api_notifications_to_pending` to use a join for getting services that don't have `letters_as_pdf` as marginally faster.
- check if service_callback_api exist before putting tasks on queue
- create_service_callback_api in tests before asserting if send_delivery_status_to_service has been called.
`Notification.template` changed from being a Template relationship
to TemplateHistory. When a relationship attribute is being assigned,
SQLAlchemy checks that the assigned value type matches the relationship
type. Since most tests at the moment create a notification using a
Template instance this check fails.
Rewriting the tests to use TemplateHistory objects would require
changes to the majority of tests. Instead, when creating a notification
objects we assign the foreign key attributes directly. This skips the
SQLAlchemy type check, but we still get the constraint check on the
foreign keys, so a matching TemplateHistory object needs to exist in
the database.
NotificationStatistics was added as a spike but didn't work out as expected. This is finally removing all that unused code.
I'll drop the table in the next PR
Comments are PR review. Updated code style in a few places to make it
more consistent with other code, added tests for letters and emails
so they are testedt, refactored some database queries to dao file
- Fixed code style
- Refactored database queries to dao code
- Added tests for emails and sms.
- Added code in `delete_notifications_created_more_than_a_week_ago_by_type` to remove notifications to email_reply_to older than 7 days
- Added transactional to `delete_notifications_created_more_than_a_week_ago_by_type`
- Added log for when a job starts so that we will know when the processing of a job starts with the number of notifications
- Added dao method to get total notifications for a job id
- Added a test to check whether the number of notifications in the table matches the job notification_count
Added clarification to an error message to give better debugging information.
Removed using dao_get_reply_to_by_service_id in tests to be more consistent with other code and use the test db functions or remove the need for a call altogether making the code less complex.
