When checking the service or organisation name for uniqueness before
changing it, it would be necessary to exclude the current name from
this check. However now we are changing it immediately we don’t need
to guard around this behaviour of the uniqueness check.
So this commit removes the guard for both renaming a service and an
organisation.
Note that this is copied from the same change made to the rename service
page:
1190e4541b
The original idea behind was to always ask users to re-enter their
password any time:
- we want them to be sure that they want to do what they’re about to do
- we want to be sure it’s really the user trying to do the thing (and
not someone malicious)
In reality we:
- removed this from the initial place it was added (a descendent of the
‘suspend service’ feature)
- only ever added it to the ‘rename service’ and ‘rename organisation’
features
So in reality it’s not a pattern we have persisted with. Arguably there
are several things you can now do in the admin app without re-entering
your password which are much more high consequence than changing the
service name.
Also, with browser autofill there’s a lot less chance that forcing
someone to re-enter a password really gives much defence against an
unattended laptop, for example.
So this commit removes the need to re-enter your password when renaming
an organisation.
If you submit the rename organisation form without making any changes
you will get an error saying that the name is currently in use. This is
true because it’s being used by the current organisation.
However your intention is probably not to actually change anything, so
we can just redirect back to the settings page.
This is the same thing we do when renaming services:
60f5b74904/app/main/views/service_settings.py (L99-L100)
Some tests use the `client` fixture but don’t call any of its methods.
The reason for doing this is because the test depends on something in
the request context.
This commit replaces all those instances with `client_request`, which
also sets the request context.
These tests are the last ones that still use the `client` fixture. By
replacing it with `client_request` we will be able to say that no tests
should be using the `client` fixture directly.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
Lots of our tests still use an older fixture called `client`. This is
not as good because it:
- returns a raw `Response` object
- doesn’t do the additional checks
- means our tests contain a lot of repetetive boilerplate like `page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')`
This commit converts all the tests which had a `client.get(…)` or
`client.post(…)` statement to use their equivalents on `client_request`
instead.
Subsequent commits will remove uses of `client` in other tests, but
doing it this way means the work can be broken up into more manageable
chunks.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
Lots of our tests still use an older fixture called `client`. This is
not as good because it:
- returns a raw `Response` object
- doesn’t do the additional checks
- means our tests contain a lot of repetetive boilerplate like `page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')`
This commit converts all the tests which had a `client.login(…)`
statement to use `client_request` (which is already logged in by
default).
Subsequent commits will remove uses of `client` in other tests, but
doing it this way means the work can be broken up into more manageable
chunks.
We added a new argument to `client_request.get` and
`client_request.post` to specify that it should return a raw `Response`
object rather than an instance of `BeautifulSoup`.
This is useful because sometimes we need to look at stuff like the
response headers.
However it turns out we already have a separate method for this, so
rather than invent something new I think it’s better to stick with the
thing we already have.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
A few of our tests still use an older fixture called
`logged_in_client_with_session`. It’s not clear how this is different
from `logged_in_client`, which we have replaced with `client_request`.
So this commit goes ahead and converts all the tests using
`logged_in_client_with_session` to use `client_request` instead.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
Lots of our tests still use an older fixture called `logged_in_client`.
This is not as good because:
- it returns a raw `Response` object
- doesn’t do the additional checks
- means our tests contain a lot of repetetive boilerplate like `page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')`
This commit converts all the tests using `logged_in_client` to:
use `client_request` instead.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
For most tests of a platform admin view we used `platform_admin_client`
instead. This is not as good because it returns a raw `Response` object
and doesn’t do the additional checks.
This commit converts all the tests using `platform_admin_client` to:
use new `client_request` and log in as `platform_admin_user` before
making any requests.
This is also nice because it makes any test easy to parametrize with
additional users, for example to test differences in behaviour dependant
on being platform admin or not.
To render text in an SVG consistently the system rendering the SVG must
have the fonts specified by the SVG installed.
If the fonts are not installed then the renderer will fall back to a
system font and the text will look different. This is especially bad
news for branding where the right font is an integral part of any brand.
To fix this, the text should instead be converted to `<path>` elements.
This process is sometimes called ‘outlining’.
A few of our logos had this problem, and I’ve fixed most of them by
hand. Adding this validation will stop the problem, coming up again.
Some users have reported a problem with the received text message
report:
> I have tested the reply service but in the excel report the mobile
> number is showing as 4.47900E+23. How can I change the format so that
> it is show the mobile number that has replied?
This is happening because Excel is interpreting a phone number in the
format `447900900123` as a number in
[scientific notation](https://en.wikipedia.org/wiki/Scientific_notation),
in other words 4.479 × 10<sup>23</sup>.
`447900900123` is the format that our provider is giving us the number
in – there’s no guarantee it will always be in this format.
We can prevent this behaviour by putting spaces in the numbers. Excel
and Google Sheets won’t try to convert a string with spaces into a
number.
I think we used to do this for the sent text messages report but
probably stopped because we decided it was better to keep the phone
number in the same format as it had been supplied to us for
reconcilliation purposes.
If an organisation doesn’t have any live services then there’s no data
to download. To make things less confusing we should hide the link in
this case.
This commit also modifies the existing test so that the assertions are
consistent.
If the organisation table contains an entry for `agreement_signed_by_id`
and for `agreement_signed_on_behalf_of_name` then we should the person
who signed the MOU as being the `agreement_signed_on_behalf_of_name`.
This was wrongly showing the `agreement_signed_by_id` as the person who
signed the agreement.
We use the `ChangeEmailForm` if you want to change your own email
address or someone else's email address. This has various validators
which get run. We check if the email address is valid (by using a
function from utils) and if the email address is already in use
(by calling API).
If the email address is not valid, we should not call API to see if it's
already in use because this will cause an exception in API leading to a
`500` in admin. We now only call API if there were no other errors with
the email address.
(The `test_should_redirect_after_name_change` test didn't need the
`mock_email_is_not_already_in_use` fixture, so this has been removed.)
If there were multiple errors, this widget was joining the messages
together and displaying all error messages. If a text input field does
have more than one validation error, we only want to show one.
This adds a link to the user profile of the person who requested to go
live for "Request to go live" Zendesk tickets. Viewing a user's profile
page helps us to check for duplicate organisations and services from
that user.
We want organisation team members to be able to see the MOU details for
their organisation. This change creates a new page called billing, which
contains these details. It's only visible to platform admin users now -
the plan is to add more information to this page, then to make it visible
to all organisation users.
The page showing the MOU covers the case of when agreement_signed is
True, when an agreement_signed is False, and when agreement_signed is
None. The case when an agreement_signed is None is very rare - it
signifies that the agreement is not signed but that we have some
service-specific agreements in place. We only have a few organisations
in this state, so it's unlikely that the content for this scenario will
be seen.
When an organisation has signed the agreement we may know the full
details (signing date, version signed, the person who signed it or who it
was signed on behalf of), or we may only have the name of the person who
signed the agreement. We show the more detailed content if possible, and
a less detailed version of the content if not.
There's a new route for downloading the agreement which is almost
identical to the existing `.service_download_agreement` route (plus the
test is almost the same), except that it takes an organisation ID
instead of a service ID.
This is a quick additional check to protect the user:
- From getting a CloudFront 502 error if the file takes too
long to upload. I was surprised to find it takes about 1 minute
to upload a 70Mb file to S3.*
- From getting a CloudFront 502 error when we follow the redirect
and run through the slow processing code in utils that builds a
RecipientCSV [1].
For context, a CSV with 100K rows and a few columns is around 5Mb,
so a 10Mb limit should be enough. Analysis over the past week shows
that the vast majority of CSV uploads are actually < 2.5Mb.
I haven't added any tests for this because:
- The check isn't critical, as the worst case scenario is the user
gets a worse error than this in-app one.
- There's no easy way to mock the validation, and I didn't want to
have a test that depends on a 10Mb+ file.
*We're using "key.put" to upload the file, when we could be doing
a multipart upload [2]. However, I tried this myself with a chunk
size of 1000 bytes and found it only led to a marginal improvement.
[1]: https://github.com/alphagov/notifications-utils/pull/930
[2]: https://boto3.amazonaws.com/v1/documentation/api/latest/guide/s3-uploading-files.html
This has several advantages:
- It gives us more room to explain the error and actions. This will
be useful for upcoming work we want to do, which will add yet more
validations for CSV uploads.
- We already use a flash to show certain kinds of errors on these
pages (just above). This is more consistent.
- It's potentially more accessible. Previously the error and the
button text used to be read out as a single sentence. Now the page
reloads and reads the flash error alone.
In theory we should show an error in both places, but this can be
confusing on pages where there's only a single form control, and
especially if the error is long.
The original idea behind was to always ask users to re-enter their
password any time:
- we want them to be sure that they want to do what they’re about to do
- we want to be sure it’s really the user trying to do the thing (and
not someone malicious)
In reality we:
- removed this from the initial place it was added (a descendent of the
‘suspend service’ feature)
- only ever added it to the ‘rename service’ feature
So in reality it’s not a pattern we have persisted with. Arguably there
are several things you can now do in the admin app without re-entering
your password which are much more high consequence than changing the
service name.
Also, with browser autofill there’s a lot less chance that forcing
someone to re-enter a password really gives much defence against an
unatteneded laptop, for example.
I also wonder whether we might get people to give better service names
if we make the process of renaming the service less intimidating.
So this commit removes the need to re-enter your password when renaming
a service.
Note that re-naming an organisation still has the same check, but I
haven’t removed that too for the sake of keeping scope of the PR small.
Brings in https://github.com/alphagov/notifications-utils/pull/889/files
At the moment, we are not doing any transformation of features before
applying geometric algorithms to them. This is, in effect, assuming that
the earth is flat.
This new version of utils implements the transformation of our polygons
to a Cartesian plane. In other words, it converts them from being
defined in spherical degrees to metres.
For the admin app this means we need to convert places where the code
expects things to be measured in degrees to work in metres instead.
We think that the API is returning incorrect data for this column.
It’s going to take a while to figure out what’s going on with the
queries in the API, so this pull request temporarily removes the column
so we’ve not giving people incorrect data.
We are starting to see lots of 100.0%s in the current table
and we think this looks suspiciously too good so think it is
beneficial to change it to be 2dp such that we get a few more
non 100.0% values.
This will put all the values in to having 2dp, however it will
also require the API to have a change to
https://github.com/alphagov/notifications-api/blob/master/app/performance_dashboard/rest.py#L81
where it is currently losing the granularity down to a single
decimal place (meaning that if we were really at 99.894% then that
would be shown on the page as 99.90% rather than 99.89%). However,
I don't think it is a blocker that we get that sorted before this
can be merged.
This means that the data in the report will match what’s on the page,
where the values are rounded to the nearest penny.
This uses the same string formatting to round the numbers which the
`big_number` component does, so it should round the numbers in the same
way.
This is so org level users can use this data easier for things
like determining spending per service.
We do not include sms fragments sent column and remove other sms columns
consistency.
Do not add sms fragments sent column for now until we agree on an
unambiguous name for it. The data in this column is sms billing units
multiplied by international sms weighing. My favourite for a clear
name would be 'text message credits used', but we need a naming
strategy for this.
