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)
This is similar to the corresponding endpoint for services. However,
it is a little simpler since we don't need to worry about always having
at least one team member for an organisation.
The new dao function added, `dao_remove_user_from_organisation`, is also
simpler than `dao_remove_user_from_service` since we don't have any
organisation permissions to deal with.
The way it was done before, the remainder was incorrect in the
billing report and in the org usage query - it was the sms remainder
left at the start of the report period, not at the end of that period.
This became apparent when we tried to show sms_remainder on the org
usage report, where start date is always the start of the financial year.
We saw that sms sent by services did not reduce their free allowance
remainder according to the report. As a result of this, we had to
temporarily remove of sms_remainder column from the report, until
we fix the bug - it has been fixed now, yay!
I think the bug has snuck in partially because our fixtures for testing
this part of the code are quite complex, so it was
harder to see that numbers don't add up. I have added comments
to the tests to try and make it a bit clearer why the results are
as they are.
I also added comments to the code, and renamed some variables,
to make it easier to understand, as there are quite a few
moving parts in it - subqueries and the like.
I also renamed the fetch_sms_free_allowance_remainder method to
fetch_sms_free_allowance_remainder_until_date so it is clearer
what it does.
db update/insert.
Using a savepoint for the multiple transactions allows us to rollback if
there is an error when executing the second db transaction.
However, this does add a bit of complexity. Developers need to manage
the db session when calling multiple nested tranactions.
Unit tests have been added to test this functionality and some end to
end tests have been done to make sure all transactions are rollback if
there is an exception while executing the transaction.
the default free allowance for the organisation type.
The update/insert for the default free allowance is done in a separate
transaction. Updates to services need to happen in a transaction to
trigger the insert into the ServicesHistory table. For that reason the
call to set_default_free_allowance_for_service is done after the service
is updated.
I've added a try/except around the set_default_free_allowance_for_service call to ensure we still get the update to the service but get an exception log if the update to annual_billing fails. I believe it's important to preserve the update to the service in the unlikely event that the annual_billing upsert fails.
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.
This endpoint may need to change, but we'd like to see how this performs, so we'll test this with a real data set. Then come back to make sure the format is correct and check for missing tests for the endpoint,
This will let us do some filtering of this list in the admin. It’s
better to do it there because it means the admin can use the same cached
response from Redis each time.
Although their allowances are the same as what we call `nhs_local` it
makes more sense to store them separately because:
- we already present them as two separate choices to the user
- we may want to handle them differently in the future, eg in terms of
what branding choices are available to them
At the moment this response returns a list of service IDs for hundreds
of organisations.
The admin app doesn’t use this information, but having to wait for it to
be serialized and sent across the network slows it down all the same.
This is changing because we’re going to introduce accepting contracts
and MoUs online.
Previously
---
We had one column for who signed the agreement, which is foreign keyed
to the user table. This is still relevant, because there will always be
a user who is clicking the button.
Now
---
We add two new fields for the name and email address of the person on
whose behalf the agreement is being accepted. This person:
- is different from the one signing the agreement
- won’t necessarily have a Notify account
We need this in the admin app while we still have pages that:
- talk about the data sharing and financial agreement
- but aren’t within a service (so can’t look at the service’s
organisation)
This is a get, but it deliberately won’t work if you pass it an email
address, in order not to put personally identifying information in our
logs.
Currently we have
- a thing in the database called an ‘organisation’ which we don’t use
- the idea of an organisation which we derive from the user’s email
address and is used to set the default branding for their service and
determine whether they’ve signed the MOU
We should make these two things into one thing, by storing everything
we know about an organisation against that organisation in the database.
This will be much less laborious than storing it in a YAML file that
needs a deploy every time it’s updated.
An organisation can now have:
- domains which we can use to automatically associate services with it
(eg anyone whose email address ends in `dwp.gsi.gov.uk` gets services
they create associated to the DWP organisation)
- default letter branding for any new services
- default email branding for any new services
When creating or updating an organisation an itegrity error is raise if the name is already used.
This change adds a new error handler for the organisation to catch the named unique index and return a 400 with a sensible message.
We have an other error handler for unique service names which was caught in the error handler for all blueprints. A new error handler for the service_blueprint has been created for catch those specific unique constraints.
This is a nice way to encapulate the specific errors for a specific blueprint.
If the organisation name that is being inserted or updated is not unique we just want to return a 400 to the admin app.
Updated the code so that we are not logging.exception, this is because a cloud watch alert is set to the support team. This type of error is not something we need to investigate.
- Changed the organisation DAO update method to only make 1 query
- Updated the update rest endpoint to not return an organisation when
the update is successful
notable things that have been kept until migration is complete:
* passing in `organisation` to update_service will update email branding
* both `/email-branding` and `/organisation` hit the same code
* service endpoints still return organisation as well as email branding
