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.
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.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 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.
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.
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.
This moves things out of the parametrization that didn't need to be
in there and also makes the test match the test below,
test_shows_delete_link_for_error_on_post_request_for_edit_email_reply_to_address,
more closely.
https://www.pivotaltracker.com/story/show/180026726
There was a bug where if you enter an invalid email address in
to the edit reply to email address form and click save, the
form you get shown with your error message will always contain
the field to set as default the reply to and also delete. This should
not have been the case. If you make an error on the form when
changing a reply to that is already a default, then you should not
be given the chance to change it to not default, nor should you
be able to delete it.
This commit fixes that bug by making sure the additional form fields
are only shown if the reply to being changed is not the default.
Since we’ve introduced the ‘on behalf of’ wording to the go live ticket
(to talk about who the agreement has been signed on behalf of) it’s
confusing to use the same terminology to talk about the organisation
for whom the agreement has been accepted.
The new way of creating support tickets can be seen in
[notifications-utils](https://github.com/alphagov/notifications-utils/pull/899).
This changes tickets created when making a request to go live to use
the new way, while other tickets stay the same for now.
The go live tags have been removed. Some of these had become
unneccessary since you can't make the request to go live unless they are
true (e.g. `notify_go_live_email_reply_to`). Others will always get
added by a Zendesk macro when the ticket is replied to, so we don't need
to add them here.
Add additional instructions for the service name - this is more consistent with the local government version of this page
Also update tests to use the new content.
Previously the backend would never validate permissions because the
"not service.active" part would (usually) fail. I've updated it to
match the (inverse of the) conditional we have in the HTML [1].
[1]: 6ac593aa5f/app/templates/views/service-settings.html (L455)
When the broadcast service settings form is submitted it now removes all
permissions for users in notifications-api. This means it should be
clearing the user cache.
This makes the tests consistent with those for suspend / archive,
logging in with different users to make it clearer who can/not do
this action in the backend.
Note that we think this functionality may be too permissive [1],
so we may restrict it in a future PR.
[1]: https://github.com/alphagov/notifications-admin/pull/3959#issuecomment-878291295
For most events this makes the purpose of each argument clearer at
the point the event is called. It's still worth having a function
for each event type, as this abstracts knowledge of the event label.
Using a schema approach will make adding new events easier.
In the next commit we'll DRY-up the duplication in the tests as well.
Previously these only tested with a Platform Admin user, but service
admins can suspend a service too. I've rewritten the tests to match
the 'archive_service' ones, which use the client_request fixture to
make changing the user easier.
Note that the return value of the service API client wasn't used for
anything, so it's safe to remove it from the mock.
This is particularly important for broadcast services, where a rogue
service or platform admin could launch a DoS attack by suspending a
service at a critical moment when it needs to send alerts.
We put some content in the go live ticket which is for our benefit, for
example notes about the organisation.
It’s hard for us to be able to say what we want here if we know that the
person making a go live request is going to see those notes.
This commit changes go live requests so that the initial content of the
ticket is hidden from the person raising it (in Zendesk it will appear
as an ‘internal note’, rather than a ‘public reply’).
---
Depends on:
- [ ] https://github.com/alphagov/notifications-utils/pull/877/files
We have a label saying "other live services". This label means
other live services for a user making the request, but it could
also be interpreted as other live services for an organisation.
Hence, we are changing the label to "other live services for
that user" to avoid confusion
Was just in one of those meetings where it felt like writing this would
take less time than I’d already spent talking about its relative
priority…
---
In the admin app you can already set the broadcast channel as 'test', 'severe' or 'government'.
Aim:
- Add the 'operator' channel to the list of channels you can pick for the admin app broadcast services
Note:
- The API already supports this - https://github.com/alphagov/notifications-api/pull/3262
- The CBC proxy does not yet support the operator channel and this will need a separate card. That card has not yet been written because the interface has not been agreed between us and the MNOs yet.
- Will need to have the ability to select the operator channel for just a single MNO like we do for the other channels
- If we add this, we shouldn't actually start using it until the MNO in question gives us the go ahead.
---
https://www.pivotaltracker.com/story/show/178485177
This takes a similar approach as in the previous commit. Since the
"training channel" doesn't really exist, we need some extra code
to pre-select it if a service is already in training mode. As in
the previous commit, I've removed a few non-critical test cases
where we really don't need to test exhaustively.
Note that we also need some specific code to avoid pre-selecting an
option for non-broadcast services, which only used to work by fluke:
we would try to populate the field with (False, None, 'all'), which
isn't a valid combination, so nothing was selected.
Previously this field had to mimic the final hyphenated string of
the broadcast account type, even though it was only used to select
one of its components. The new, shorter choices make it easier to
simplify the test for the POST request.
I've also deleted a number of test cases for pre-selected radios.
This functionality isn't critical, so we don't need to exhaustively
test every single possible combination of values.
Previously we had to cope with two forms of the hyphenated string
we use to represent a pending change in broadcast account type.
Using "all" to mean "all providers" matches the behaviour in the
API [1], and means we can remove some complexity.
"training-test-all" isn't ideal, since the provider is irrelevant
for a training mode service. However, this isn't much worse than
the previous "training-test", noting that the channel also has no
relevance. We'll iterate this in later commits.
[1]: 8e1a144f87/migrations/versions/0352_broadcast_provider_types.py (L14)
This is inconsistent with all the other tests in the same file, and
one of them was incorrect ('_post' was testing a GET). I don't think
we get any value from them, given the inconsistency.
Previously the network selection case was tested here and also by
'test_post_service_set_broadcast_network_makes_you_choose'.
I've renamed the test to be consistent and more specific.
'all' isn't a valid channel. It should be one of government, severe or
test. I think this is a mistake and therefore this commit changes it to
what it should be
I don’t think we need to say (all networks) in the header. The real
benefit of this change is forcing the platform admin person making the
change to explicitly give their choice of network.
And by not putting it in the label we don’t have future users from other
organisations wondering if there’s some option other than ‘all networks’
they need to think about.
without the need for verification.
This is for when the email takes too long to arrive and the service
users cannot update it as a result.
A more streamlined solution has been proposed where we could send
a link in the verification email to the users and clicking that
link would add/update reply-email-to address.
That would require a bit more work so right now I am proposing this
as a quick stop gap so that we don't have to go to the database
manually to add the reply-to email address.
Previously we could only select a provider when using the test
channel, but this is also required for others channels when we
do tests on the production network with individual MNOs.
In order to reduce duplication and improve consistency, I've reused
the new broadcast_service_name_tag macro to show the setting.
This naming was introduced in 2016 without explanation [1]. I find it
confusing because:
- It's reminiscent of "_app", which is a Python convention indicating
the variable is internal, so maybe avoid using it.
- It suggests there's some other "app" fixture I should be using (there
isn't, though).
The Python style guide describes using an underscore suffix to avoid
clashes with inbuilt names [1], which is sort of applicable if we need
to import the "app" module [2]. However, we can also avoid clashes by
choosing a different name, without the strange underscore.
[1]: 3b1d521c10
[2]: 78824f54fd/tests/app/main/views/test_forgot_password.py (L5)
Adding ‘all networks’ whenever we mention the using the test channel
without a restriction to a single network should help reinforce that
this sends real alerts.
