Using `+` to concatenate strings isn’t very memory efficient. Not sure
if there are real-world implications for how it’s being used here, but
can’t hurt to use `.join` instead.
Rewriting it as a generator also lets us remove some unneeded variable
assignment.
when we made the change to async persist notifications, we forgot to
pass through api_key_id and key_type. in send_sms/email, for legacy
reasons, they default to None/KEY_TYPE_NORMAL, so regardless of what
your api key was set up as, we would send real messages!
TODO: Once the PaaS transition is complete and the task changes are
reverted, remove the api_key_id and key_type params from the send_*
tasks entirely, as those are only called from the csv job flow, and
don't need them
This is being done for the PaaS migration to allow us to keep traffic coming in whilst we migrate the database.
uses the same tasks as the CSV uploaded notifications. Simple changes to not persist the notification, and call into a different task.
This will transform each notification in a job to a row in a file.
The file is then uploaded to S3.
The files will later be aggregated by the notifications-ftp app to send to dvla.
The method to upload the file to S3 should be pulled into notifications-utils package.
It is the same method used in notifications-admin.
Update the PermissionsDao.get_permissions_by_user_id to only return permissions for active services,
this will make the admin app return a 403 if someone (otherthan platform admin) tries to look at an inactive service.
Removed the active flag in sample_service the dao_create_service overiddes this attribute.
the `to` field stores either the phone number or the email address
of the recipient - it's a bit more complicated for letters, since
there are address lines 1 through 6, and a postcode. In utils, they're
stored alongside the personalisation, and we have to ensure that when
we persist to the database we keep as much parity with utils to make
our work easier. Aside from sending, the `to` field is also used to
show recipients on the front end report pages - we've decided that the
best thing to store here is address_line_1 - which is probably going to
be either a person's name, company name, or PO box number
Also, a lot of tests and test cleanup - I added create_template and
create_notification functions in db.py, so if you're creating new
fixtures you can use these functions, and you won't need to pass
notify_db and notify_db_session around, huzzah!
also removed create param from sample_notification since it's not used
anywhere
it's almost entirely duplicated so share it across.
also clean up retrying - `task.retry(...)` raises a
celery.exceptions.Retry object, so you do not need to `raise` its
response. additionally, cleaned up tests around that since raising
Exception and asserting Exception is raised is dangerous as it could
mask actual programming errors
it now switches utils.template.Template type, since the base Template
type now no longer has a subject attribute.
updated test case to use `sample_email_template_with_placeholders`
instead of `sample_email_template`
- note this is an unexpectedly big change.
- When we create a service we pass the service id to the persist method. This means that we don't have the service available to check if in research mode.
- All calling methods (expecting the one where we use the notify service) have the service available. So rather than reload it I changed the method signature to pass the service, not the ID to persist.
- Touches a few places.
Note this means that the update or create methods will fall over on a null service. But this seems correct.
Goes back to the story which we need to play to make the service available as the API user so that the need to load and pass around services is minimised.
In this PR the id for the notification is passed in and used to created the notification, which causes a integrity error.
Normally when we get a SQLAlchemy error here we send the message to the retry queue, but if the notification already exists
we just ignore it.
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.
After we have written to the database and placed it on a deliver queue we count it in the cache against the service.
This is the equivalent of doing it at the end of the API call.
From a support ticket:
> it's possible to add a personalisation token with trailing whitespace
> (eg. "key " rather than "key"). Can this be trimmed in the UI to guard
> against this? (one of our devs copied and pasted it from a document
> and inadvertently included the space)
> Nothing major but caused a few hours of investigations!
Rather than trim the placeholder in the template, we should treat
placeholders in API calls the same way we do with CSV files, ie we
ignore case and spacing in the name of the placeholder. So
`(( First Name))` is equivalent to `((first_name))`, and both would be
populated with a dictionary like `{'firstName': 'Chris'}`.
Depends on:
- [x] https://github.com/alphagov/notifications-utils/pull/77
help prevent issues where scheduled jobs are processed twice. note this is NOT
a watertight solution - it holds no locks, and there is no guarantee that the
status won't have updated between asserting that its status is 'pending' and
updating it to be 'in progress'
- It seems that when we changed the name of the job.status column that we didn't update the code to use job.job_status.
- Therefore none of the jobs since then have had the job status updated.
- Now that this is fix we can show the job status when there is an error like "sending exceeds limits"
- This could happen if a job is scheduled to run at the top of the hour, so at the time of the job creation the limit was not exceed, but at the time of processing the job the limit is exceed.
Refactored send_notifications method so that it is more readible.
Refectored the test_send_notificaitons so that it uses parametrized test to avoid duplication.
