Second attempt [1]. This increases the threshold so:
- It's a more substantial amount of money lost (£16).
- It's 10% of the minimum free allowance for a service.
- It's greater than the threshold we have for TV numbers (500).
Having a higher threshold for this alert will help prevent wasted
effort investigating more negligible failures, and reduces the
ambiguity of whether we should take action: we should.
[1]: https://github.com/alphagov/notifications-api/pull/3221
When deploying to paas the database postgres environment variables are set using VCAP_SERVICES provided by PaaS. When we start up the app and set the properties we need to replace the postgres string with postgresql for the app to start up properly.
This wasn't caught locally or with the unit tests because we were setting this property with postgresql.
`service_ids_to_purge` is a list of `row` object rather than a list of `UUID`.
NOTE: db.session.query(Service).filter(Service.id.notin_(services_with_data_retention)).all() would have also worked. It seems that only selecting attributes from the db.Model has caused the change.
- sqlalchemy.sql.expression.case must include an else statement.
- clearly define list of columns for inbound_sms_history insert, getting the list from InboundSmsHistory.__table__.c was causing data type errors.
- remove relationships when not needed, the foreign key relationship is established in the creation of the column. This will get rid of the warnings referenced here: http://sqlalche.me/e/14/qzyx.
- update queries now that he user relationship in ServiceUser db model has been removed.
- move the check that a template is archived to the view instead of the dao method. The check was clearing the session before the version history could be done.
Deleting notifications in the night tasks still needs to be
investigated. The raw sql is causing an error.
if you run `make help` or just `make` then you get a nice list of the
tasks that you can run (or at least, the ones with help text added to
them.
We were missing these for some of the key commands that a developer
would want to know about.
By adding help text to them, they will now show up in `make` or `make
help` and saves a developer needing to go either look in the README or
go look in the Makefile to figure out what commands are available.
Note, there is no particular convention around which commands have help
comments. I don't think we need to figure out this but at the least, the
ones which developers may want to run locally I think should show up.
If this alert goes off in the morning, it usually means we need to do
something, ideally quite quickly as it indicates a potential problem
with the sending of letters over to DVLA the night before.
Given this goes off at 9am at the moment, but actually some people start
work earlier, if we alert at 7am it means it will likely be looked at
earlier in the day and we can potentially fix any problems with letters
sooner than later.
This applies the same change we made in other apps [1][2]. Adding
the override here is special, though, because it means the others
will now get triggered, since this app is the start of the chain
of tasks for a request. We will also retain existing request_id
tracing for tasks within this app, since "apply_async" calls the
"send_task" method internally, which is the one we're overriding.
[1]: 6f3c118a1e
[2]: 2e08b7aa95
`service_ids_to_purge` is a list of `row` object rather than a list of `UUID`.
NOTE: db.session.query(Service).filter(Service.id.notin_(services_with_data_retention)).all() would have also worked. It seems that only selecting attributes from the db.Model has caused the change.
- sqlalchemy.sql.expression.case must include an else statement.
- clearly define list of columns for inbound_sms_history insert, getting the list from InboundSmsHistory.__table__.c was causing data type errors.
- remove relationships when not needed, the foreign key relationship is established in the creation of the column. This will get rid of the warnings referenced here: http://sqlalche.me/e/14/qzyx.
- update queries now that he user relationship in ServiceUser db model has been removed.
- move the check that a template is archived to the view instead of the dao method. The check was clearing the session before the version history could be done.
Deleting notifications in the night tasks still needs to be
investigated. The raw sql is causing an error.
This is a belt-and-braces check because the admin app already checks
this. But since we do it for SMS already it makes sense to replicate it
for broadcast templates.
This changes the content length validation of the internal API to match
the validation of the public broadcast API[1].
This removes the length check from JSONSchema, which isn’t sophisticated
enough to deal with things like normalising newlines or handling
different encodings.
The admin app should catch these errors before they’re raised here, but
it’s best to be belt and braces.
1.7ab0403ae7/app/v2/broadcast/post_broadcast.py (L53-L63)
This ensures that the log messages both contain broadcast_event id and
broadcast_provider_message id. It also removes the broadcast_event
reference since this isn't particularly useful in helping to find an
event.
It wasn't clear what the ID in the message was. It's not possible to add
more details to the message - we don't create a broadcast message or
event for a link test.
We want to start using Firetext for sending international SMS. They
require us to use a different API key for international SMS because it
requires a new code path to switch the sender ID to something that the
country will accept.
This PR does not include switching the sender of international SMS to
Firetext but sets us up to do so.
This is an extra precaution for the table to ensure data integrity. Since we only update/insert the data using the annual_billing_dao methods the integrity is in tact. I've check the data on preview, staging and prod there are no violations of this unique key.
Also update error message for when someone does not have permissions.
The message referenced approving broadcasts specifically, whereas
people would also see it if they try to cancel or reject
broadcast without permission.
Why we allow platform admins to cancel broadcasts:
we do this so they can react quickly if a broadcast was
approved by accident.
While both of these are integrity errors (since we should never
reach this point in the code + data), this just means the original
method comment is still relevant to what immediately follows it.
Since the checks are only performed in one place we can easily take
extra care to ensure this in the tests, noting that we don't need to
do any additional setup, except if no exception is raised - I've left
these tests as-is, to avoid doing more setup.
Note that we still check the happy path for when a provider message is
already sending - just in a different test [1].
[1]: 3d71815956/tests/app/celery/test_broadcast_message_tasks.py (L263)
This mirrors the check we do for jobs, which are also a high-impact
task [1]. While this shouldn't be possible, just like other checks
we're adding it here to be doubly certain.
[1]: 3d71815956/app/celery/tasks.py (L74)
We only actually use this when the data we're working with is in an
unexpected state, which is unrelated to the CBC Proxy. Using this
name also means we can re-use this exception in the next commits.
Note that we may still care if a broadcast message has expired, since
it's not expected that someone would send one in this condition.
