Previously we made a call to S3 to list objects for a letter, even
though we already had the precise key of the single object to hand.
This removes the one usage of "get_s3_bucket_objects" and uses the
filename directly in the call to remove the object.
Previously, the function would just return a presumed filename. Now that
it actually checks s3, if the file doesn't exist it'll raise an
exception. By default that's a StopIteration at the end of the bucket
iterator, which isn't ideal as this will get supressed if the function
is called within a generator loop further up or anything.
There are a couple of places where we expect the file may not exist, so
we define a custom exception to rescue specifically here. I did consider
subclassing boto's ClientError, but this wasn't straightforward as the
constructor expects to know the operation that failed, which for me is a
signal that it's not an appropriate (re-)use of the class.
Previously we generated the filename we expected a letter PDF to be
stored at in S3, and used that to retrieve it. However, the generated
filename can change over the course of a notification's lifetime e.g.
if the service changes from crown ('.C.') to non-crown ('.N.').
The prefix of the filename is stable: it's based on properties of the
notification - reference and creation - that don't change. This commit
changes the way we interact with letter PDFs in S3:
- Uploading uses the original method to generate the full file name.
The method is renamed to 'generate_' to distinguish it from the new one.
- Downloading uses a new 'find_' method to get the filename using just
its prefix, which makes it agnostic to changes in the filename suffix.
Making this change helps to decouple our code from the requirements DVLA
have on the filenames. While it means more traffic to S3, we rely on S3
in any case to download the files. From experience, we know S3 is highly
reliable and performant, so don't anticipate any issues.
In the tests we favour using moto to mock S3, so that the behaviour is
realistic. There are a couple of places where we just mock the method,
since what it returns isn't important for the test.
Note that, since the new method requires a notification object, we need
to change a query in one place, the columns of which were only selected
to appease the original method to generate a filename.
We no longer will send them any stats so therefore don't need the code
- the code to work out the nightly stats
- the performance platform client
- any configuration for the client
- any nightly tasks that kick off the sending off the stats
We will require a change in cronitor as we no longer will have this task
run meaning we need to delete the cronitor check.
We current do this as part of send-daily-performance-platform-stats but
now this moves it into its own separate task. This is for two reasons
- we will shortly get rid of the send-daily-performance-platform-stats
task as we no longer will need to send anything to performance
platform
- even if we did decide to keep the task
send-daily-performance-platform-stats and remove the specific bits
that relate to the performance platform, it's probably nicer to
rewrite the new task from scratch to make sure it's all clear and easy
to understand
having `/invite/service/<token>` and `/invite/service/<id>` as two
separate routes (the first to validate an invite token, the second to
retrieve invite metadata) technically works. Routes are matched from
first to last until a match is found. The metadata endpoint only accepts
UUIDs, so requests with a UUID will be picked up by the correct
endpoint, while requests that don't look like a UUID will carry on
searching for an endpoint, and will find the token validation endpoint.
So while this works correctly for our normal expected input, it only
does so _because the UUID endpoint is first in the file_. This isn't
great, and it makes it harder to reason about the URLs when looking at
them.
To solve this, create the new `invite/service/check/<token>` endpoint.
For backwards compatibility, assign this in parallel with the existing
route - once the admin uses the new route we can remove the old route
and make better guarantees about what endpoint is being hit.
this functions the same as `validate_invitation_token`, but without
having the signed token, instead just the ID. This is so later endpoints
within the invite flow can also fetch the invited user
nb: the routes are not changing as part of this, only file paths and
blueprint names.
invite -> service_invite
this blueprint handles fetching invites for a service, creating invites,
etc.
accept_invite -> global_invite
this blueprint handles accepting invites for now, but will also involve
retrieving service/org user invite data without knowing the service/org
id associated. i'm not in love with this name and open to suggestions,
but i wanted to contrast it from service_invite and
organisation/invite_rest.py.
We are using the `set_broadcast_service_type` route to make changes to
service objects. However, we had forgotten to add the `version_class`
decorator to it which will mean the changing of a service going from
training to live mode will also be recorded in the services_history
table for free. Whilst not essential, this easy change makes things more
consistent for how we update other services.
the existing endpoint is a GET, and so leaves email addresses in log
files.
we've got an existing POST find_users_by_partial_email, but not one that
matches on a whole email address.
Spotted that we aren't testing all our permission types here so added
this one in.
It includes the TODO for allowing the API to give a service the
broadcast permission. We don't want this to happen, we want them to use
the `set_as_broadcast_service` route instead. We will probably get away
with it for the moment for it would be tighter validation we should add
to reduce the risk of letting a service get in a dodgy state.
We accidently were removing the ability for a service to do email auth
if it was a broadcast service with email auth. This fixes it.
Note, it might be up for debate later whether we let broadcast services
use email auth (I think we should) so this might change in time, but we
will fix this bug regardless.
Note, worth glancing at `SERVICE_PERMISSION_TYPES` which contains a list
of permissions that a service might have to make sure I haven't missed
any others. The one that looks potentially dodgy is the
`EDIT_FOLDER_PERMISSIONS` permission but I can't see this being used
anywhere in the API or the admin app so think it is likely now defunct
and a user level permission so we don't need to worry about it.
The performance platform is going away soon. The only stat that we do not have in our database is the processing time. Let me clarify the only statistic we don't have in our database that we can query efficiently is the processing time. Any queries on notification_history are too inefficient to use on a web page.
Processing time = the total number of normal/team emails and text messages plus the number of messages that have gone from created to sending within 10 seconds per whole day. We can then easily calculate the percentage of messages that were marked as sending under 10 seconds.
SMS.
This is not a catch all for international SMS, the rules are quite
complex and still not completely understood. We are talking with our
provider who maybe able to sort this out for us. But in the meantime,
this should solve for the case that we understand.
Now that every service has a row in the service_broadcast_settings
table, we want all our tests to use the `sample_broadcast_service`
fixture as this ensures it has a row in that table and is correctly
representitive of what a real broadcast service looks like.
previously we would retry if the task was queued up for retry but the
status is in "received-ack" or "received-err". We don't expect that a
task will be retried after getting this status, but if there are
duplicate tasks that could happen. Lets plan for the worst by saying
"only process a retry if the task is currently in sending".
this way, if a duplicate task is on retry and the first task goes
through succesfully, the duplicate task will give up.
boto returns a `StreamingBody`[1] response rather than a json struct.
We're currently just logging things like "Error calling lambda
o2-1-proxy with function error <botocore.response.StreamingBody object
at 0x7f74cd6e02e8>" which is obviously less than ideal. Also make the
tests properly reflect this - annoyingly it appears like we can't use
moto to reliably test this interface as the moto `mock_lambda` decorator
needs you to be running inside a docker container??
[1] https://botocore.amazonaws.com/v1/documentation/api/latest/reference/response.html#botocore.response.StreamingBody
This was passing locally, but failing on Concourse due to a different
order of TemplateHistory items being returned. This changes the
test so that it can't randomly fail based on the order of template
history items returned.
We've decided we don't get any value from these emails any more, so this
stops us (Notify support) receiving them. We still let teams know an MOU
has been signed.
Update `send_user_2fa_code` to send from number when recipient is international
Update `update_user_attribute` to send from number when recipient is international
Add caching by using the SeriralisedTemplate and SerialisedService objects
Removed extra call to the database to fetch the notification after the commit by saving the created_at and key_type to a local variable. After the update to the notification to mark it as sending the db.session is committed. Any reference to the the Notification data model after that will require a query to fetch the object again because it is considered "dirty" or out of date.
Added name, sms_prefix and email branding to SerialisedService.
Refactor the get_html_options to work with the SerialisedService object.
Removed the need to validate and format the to field by using `normalised_to`, since when persisting the notification the `normalised_to` field has already had this done.
Removed the validate and format for reply_to_text for email reply_to, this has been done when the email address has been added via the frontend, no need to validate this address every time a services sends an email.
Note, I haven't added anything for the `go_live_user` because it doesn't
quite make sense because here a user isn't requesting to go live. So
there should be no reason to record this.
We will in time though want to add audit events to capture every change
to the service broadcast settings, that will actually capture who has
done what.
We are in a weird situation where at the moment, we have services with
the broadcast permission that do not have a row in the
service_broadcast_settings table and therefore do not have defined
whether they should send messages on the 'test' or 'severe' channel.
We currently get around this when we send broadcast messages out as
such:
https://github.com/alphagov/notifications-api/blob/master/app/celery/broadcast_message_tasks.py#L51
We need to something equivalent for the broadcast channel that the API
says the service is on. In time, when we have added a row in the
service_broadcast_settings table for every service with the broadcast
permission then we can remove both of these two hardcodings.
Note, one option would have been to move the default of `test` on to the
`Service` model rather than having it in both the
broadcast_message_tasks file and the `ServiceSchema` class. However, I
went for the quickest thing which was to add it here.
