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.
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 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.
we now no longer create a job. At the end of the post there is no
action, as we don't have any tasks to queue immediately - if it's a
real notification it'll get picked up in the evening scheduled task.
If it's a test notification, we create it with an initial status of
sending so that we can be sure it'll never get picked up - and then we
trigger the update-letter-notifications-to-sent-to-dvla task to sent
the sent-at/by.
1. No longer create jobs when creating letters from api 🎉
2. Bulk update notifications based on the notification references after
we send them to DVLA - either as success or as error
this means that if the task is accidentally ran twice (eg we autoscale
notify-celery-worker-beat to 2), it won't send letters twice.
Additionally, update some function names and config variables to make
it clear that they are referring to letter jobs, rather than all letter
content
