This avoids a bunch of boilerplate and makes it easier to see what
is being passed in the request.
Note that the "invalid_schema" test is slightly different because
it was previously passing an invalid JSON object - "{}" - instead
of a valid JSON but invalid by the schema - "\"{}\"". The new test
seems more valuable than the old one.
`charset-normalizer` is now used by default if installed instead of
`chardet` (https://pyup.io/changelogs/beautifulsoup4/#4.11.0). We do
have `charset-normalizer` installed because it's a subdependency of the
requests library, so it is being used.
This caused the `test_content_too_long_returns_400` to fail since it
now thought that the encoding of `ŵ` is `{'encoding': 'Big5',
'language': 'Chinese', 'confidence': 1.0}`.
There are two options for fixing this
- change the test content so that it doesn't just contain a single
letter - the docs state that you shouldn't run character detection on
very tiny content
- add `chardet` as a requirement, so that the code functions exactly the
same as before
I've chose the first option, since this avoids adding a dependency and
we should never have messages consisting of a single character.
We have three different ways of checking the formats of datetimes.
1. The built-in way that comes with the jsonschema package ("date-time")
2. A new way we added for broadcasts ("datetime") 61a5730596
3. An old way we defined in
"/tests/app/public_contracts/schemas/v0/definitions.json"
In order to simplify things and make it clearer how datetimes are being
validated, this replaces the few places where we were using option 3 with option 1
instead. Option 3 was only being used to validate code that is no longer
used, the initial version of the API.
The big breaking change for our code (not mentioned in the changelog) is
that the built-in validator for the `date-time` format now requires the
`rfc3339-validator` package instead of the `strict-rfc3339` package.
This updates the requirements file to use `rfc3339-validator`. Without
this change, wrong `date-time` formats would always silently pass validation.
We were using the Draft4Validator in one place, so this updates it to
the Draft7Validator instead.
The schemas were mostly using draft 4 of the JSON schema, though there
were a couple of schemas that were already of version 7. This updates
them all to version 7, which is the latest version fully supported by
the jsonschema Python package. There are some breaking changes in the
newer version of the schema, but I could not see anywhere would these
affect us. Some of these schemas were not valid in version 4, but are
now valid in version 7 because `"required": []` was not valid in earlier
versions.
This is to support a migration from Redislabs to PaaS native Redis,
allowing us to toggle between old and new using the env vars for
the instance - without needing to change the code.
This was inconsistent with the source data for the fixture being
overidden in some of the tests. We only need to set it in the env
once, so it makes sense to just put the code there.
Fixes this error (in Admin):
File "/home/vcap/app/app/notify_client/notification_api_client.py", line 74, in send_precompiled_letter
return self.post(url='/service/{}/send-pdf-letter'.format(service_id), data=data)
File "/home/vcap/app/app/notify_client/__init__.py", line 59, in post
return super().post(*args, **kwargs)
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_python_client/base.py", line 48, in post
return self.request("POST", url, data=data)
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_python_client/base.py", line 64, in request
response = self._perform_request(method, url, kwargs)
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_python_client/base.py", line 118, in _perform_request
raise api_error
notifications_python_client.errors.HTTPError: 500 - Internal server error
Due to this error (in API):
File "/home/vcap/app/app/service/send_notification.py", line 178, in send_pdf_letter_notification
raise e
File "/home/vcap/app/app/service/send_notification.py", line 173, in send_pdf_letter_notification
letter = utils_s3download(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location)
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_utils/s3.py", line 53, in s3download
raise S3ObjectNotFound(error.response, error.operation_name)
notifications_utils.s3.S3ObjectNotFound: An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
I checked the DB to verify the letter does actually exist i.e. it
is an instance of the problem we're fixing here.
The new alert happens earlier but is otherwise the same:
- We only create a ticket in Production.
- We only create a ticket on approval.
I took this opportunity to refactor the alert as a private function
and test this specifically in detail to avoid lots of repetitive
mocks, which are required when calling the main "update" function.
One test I haven't preserved was for when the "names" array is empty,
as this was added for a legacy data integrity scenario [^1].
[^1]: bf0bf4e31c
This was creating data in the Notifications table but the funcetion
under test was - now the timestamps are in the past - looking in the
NotificationHistory table. Freezing the time of the test fixes that.
We should lock these tests to run in a particular year [^1].
Fixes e.g.
> assert annual_billing[0].free_sms_fragment_limit == 150000
E assert 40000 == 150000
E + where 40000 = <AnnualBilling 8851fd4b-6316-4a53-b0e0-8202c803ae97>.free_sms_fragment_limit
[^1]: 8402e7c97b (diff-adcd90bfdc6b7777fdf309037ca6948bef4f4b858e22f8b2e46b71865d580fbaR60)
In response to: [^1].
The stacktrace conveys the same and more information. We don't do
anything different for each exception class, so there's no value
in having three of them over one exception.
I did think about DRYing-up the duplicate exception behaviour into
the base class one. This isn't ideal because the base class would
be making assumptions about how inheriting classes make requests,
which might change with future providers. Although it might be nice
to have more info in the top-level message, we'll still get it in
the stacktrace e.g.
ValueError: Expected 'code' to be '0'
During handling of the above exception, another exception occurred:
app.clients.sms.SmsClientResponseException: SMS client error (Invalid response JSON)
requests.exceptions.ReadTimeout
During handling of the above exception, another exception occurred:
app.clients.sms.SmsClientResponseException: SMS client error (Request failed)
[^1]: https://github.com/alphagov/notifications-api/pull/3493#discussion_r837363717
This works in conjunction with the new SMS provider stub [^1].
Local testing:
- Run the migrations to add Reach as an inactive provider.
- Activate the Reach provider locally and deactivate the others.
update provider_details set priority = 100, active = false where notification_type = 'sms';
update provider_details set active = true where identifier = 'reach';
- Tweak your local environment to point at the SMS stub.
export REACH_URL="http://host.docker.internal:6300/reach"
- Start / restart Celery to pick up the config change.
- Send a SMS via the Admin app and see the stub log it.
- Reset your environment so you can send normal SMS.
update provider_details set active = true where notification_type = 'sms';
update provider_details set active = false where identifier = 'reach';
[^1]: https://github.com/alphagov/notifications-sms-provider-stub/pull/10
This avoids duplicating it as we add a new provider and means we
can test it all in one place (although it wasn't tested before).
I'm not sure why the previous code did "super(..)__init__" in a
non-init function - it's a bit late! - so I've just replaced it
with a call to the new "init_app" function in the parent class.
The provider tests are coupled to actual data in the DB, but we
shouldn't have to overhaul the tests when this changes.
Assuming we don't delete old providers, just testing a subset of
the fixture data should give us enough confidence in the code.
Previously we used a combination of "provider.name" and "get_name()"
which was confusing. Using a non-property function also gave me the
impression that the name was more dynamic than it actually is.
This is enough to update a notification in DB:
1. First create a notification in the UI and sent it.
2. Then reset its attributes to pretend it's for Reach.
update notifications set
sent_at = null,
sent_by = null,
notification_status='sending'
where id='some-uuid';
3. Change "notification_id" to "<some-uuid>" in the code.
4. Call the boilerplate endpoint for Reach callbacks.
curl -X POST localhost:6011/notifications/sms/reach
Interestingly there's no foreign key constraint on "sent_by" in the
DB, so this just works: the notification is updated.
These were hard to find by searching as the filename doesn't refer
to SMS or letters. Ideally our tests should be organised to reflect
the structure of files in app/, so this does that.
Fixes:
> reduced_provider = providers[identifier]
E KeyError: 'firetext'
Note that the mock return value in the other test was wrong [^1].
[^1]: bff97f0bbe/app/dao/provider_details_dao.py (L73)
