This results in some new errors from flake8-bugbear:
```
B020: Loop control variable overrides iterable it iterates
```
I can't see an issue with the places that we do this, so have ignored
these warnings. If we keep getting these and they look fine, we can
create a global rule to ignore B020.
The endpoint used to handle both email and letter branding, but this
replaces `.branding_request` with `.email_branding_request` and
`.letter_branding_request` instead. This is in preparation for changing
how email branding works.
The `from_template` arg was only possible for letter branding, so I've
removed that from the `.email_branding_request` endpoint.
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 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
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.
previously we'd skip the template page entirely if someone didnt have
manage templates/api keys permission. however, if the template is
deleted you'd then go through the flow entering placeholders and stuff
before it would then crash when trying to send.
instead, just bounce the user to the template page. It has the content
and says when the template was deleted.
Previously this lead to 2 support tickets because the user didn't
understand why their messages was being split into 2 fragments. We
tried modifying the message about charges, but that made it more
complicated. Adding a hint should hopefully be enough.
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)
For someone who has retrieved a template ID from their system the only
way to find it in Notify is:
- hack the URL
- click through every template, visually inspecting the ID shown on the
page until you find the right one
Neither of these is ideal.
This commit adds searching by ID, for those services who have an API
integration. This means we don’t need to confuse teams who aren’t using
the API by talking about IDs.
This is similar to how we let these teams search for notifications by
reference[1]
1. https://github.com/alphagov/notifications-admin/pull/3223/files
If a user has only send_message permissions, when they click on a
template name they are currently taken to the `send_one_off` page. This
is incorrect as if there is more than one SMS sender or email reply to
address, then they should pick the address they wish to use.
This commit fixes that bug by redirecting them to the `set_sender`
route. Note, if there is only one sender then the `set_sender` will
redirect the user on to the `send_one_off` route.
https://www.pivotaltracker.com/story/show/176541486
The content length message was making the page jumpy and causing reflows
in three ways. This commit addresses each of those ways:
As the user scrolled
---
The footer went from fixed to sticky and the spacing around the message
changed. This change in spacing was needed so that the message looked
right in both contexts.
I think the best way to resolve this is to not use the sticky footer
when editing text message or broadcast templates.
On my 1440×900 screen I can fit a 5 fragment text message, plus the
‘will be charged as 5 text messages’ message, plus the save button.
Our top 10 screen resolutions according to our analytics are:
Position | Resolution | Percentage of users
---------|------------|--------------------
1 | 1920x1080 | 27.37%
2 | 1280×720 | 11.07%
3 | 1366×768 | 8.88%
4 | 1536×864 | 5.79%
5 | 1440×900 | 4.52%
6 | 1600×900 | 3.71%
7 | 1280×1024 | 3.10%
8 | 1680×1050 | 2.42%
9 | 1920×1200 | 2.33%
10 | 2560×1440 | 1.99%
When the page first loaded
---
The message is empty so takes up no space, then the javascript fires
and inserts the message, taking up a line of space.
This is resolved by making the empty message take up space with a
non-breaking space character.
When the user first typed
---
We previously didn’t show any message until the user started typing.
This meant that, with the above fix, there was a larger than normal
empty space between the textarea and the save button.
This is resolved by always showing the message, even when the user
hasn’t typed anything yet.
***
These are design decisions which made sense when the message was
displayed along side the button, but we’ve had to change now that the
message is above the button.
We feel that this is more appropriate because it’s part of the
information you’re agreeing to before you hit submit.
Sometimes users can missing information that doesn’t start left-aligned
to the column they’re interacting with.
It also makes it closer to the Design System component.
We’re keeping it in the sticky footer, so that it’s always visible no
matter where in the message you’re scrolled to (this means you won’t
have to edited to content then scroll down to check whether you’ve
made it fit).
This looks tidy, and because of the sticky footer it means the message
is always visible, even if your template is quite long. So no matter
where you’re scrolled to in the template you don’t have to scroll to the
bottom to see the count update.
This commit copies the same ARIA attributes that are added to the
character count component[1] in the GOV.UK Design System.
This means that screen reader users will hear the count message when
they stop typing.
1. https://design-system.service.gov.uk/components/character-count/
This commit adds some Javascript that makes AJAX requests as the users
changes the content of their template.
It then takes the content returned by the backend and inserts it in the
page.
Users sending text messages are sometimes unaware that long messages
will cost more.
Users sending broadcast messages need to be aware that there’s a
character limit, so they can take this into account when planning their
messages.
This commit adds an endpoint which counts the number of characters in
some template content, and returns a snippet of useful info about how
long the message is.
In subsequent commits we’ll be able to use AJAX to fetch this snippet as
the user types.
There’s a surprising amount of complexity in counting the length of
messages. So we’ll need to do this in Python because it would be too
convoluted to re-implement the length counting in client side code, let
alone ensuring it had parity with its Python equivalent.
It’s fiddly having to scroll within a small textbox to see all the
content. Let’s make the box expand to fit the contents like we do
elsewhere. This was removed by accident when we stopped highlighting
placeholders in broadcast templates in
https://github.com/alphagov/notifications-admin/pull/3672/files
Depends on:
- [ ] https://github.com/alphagov/notifications-utils/pull/826/files
Adds error messages for when the content of a broadcast template is too
long.
The error message is explicit when this is cause by non-GSM characters.
We may not want to expose this complexity to our users, but it’s useful
for now while we’re testing things out.
I'm mainly making this change because it's useful
for the CSS that styles the hint text when the
link is focused for the link to have no parent
container.
That being said, there isn't really enough content
underneath these headings to justify them as it is.
I've wrapped them in a list instead because:
- they're structured like a list
- we already called them a `template-list`
This commit also replaces the `message-type` class
on the paragraph below where the headings went,
for consistency. It also removes the CSS for that
class as I couldn't find anywhere else that used
it now.
Broadcast services only have broadcast templates. But we show the
template type under the name of the template. This is redundant. It
would be better to preview the content of the template instead.
This then makes the templates page consistent with the dashboard.
Depends on:
- [ ] https://github.com/alphagov/notifications-api/pull/2996
We don't need these anymore as all users will use the `one-off/step`
routes.
This has mostly involved tidying up the tests which are still a little
disorganised and not as good as I'd like but it's a step in the right
direction.
More refactoring is still possible to the routes, it may come in a later
PR if I have time.
We no longer need the `start_tour` page as this has been replaced with
the new `begin_tour` page.
We also no longer need to handle the `help` argument in the
`send_test_step` or `send_one_off_step` as these no longer are
responsible for the tour and don't need to show the help text.
Worth pointing out, the new tour joins into the send one off flow. When
doing a GET `check_tour_notification`, and submitting the form shown on
this page you are POSTed to `send_notification` with `help=3`. Also for
general sending of one off notifications, the POST to
`send_notification` is done with `help=0` which is a bit of a hack to
make sure that we don't show a back link on the `view_notification` page
for when someone gets there having just sent a one off notification.
This use of `help=0` may be a candidate for a refactor in the future as
it feels like a bit of a hacky way of doing things and is therefore not
as clear to developers what is going on.
Also removes the help argument from the csv routes used here. There is
no reason that we need to ever show help for CSVs and this is leftover
code from when we used to do the tour that way.
Since broadcast services can only have one type of template we probably
don’t need to disambiguate what kind of template you’re creating.
And you’ve just come from a page where the button says ‘New template’,
without the choice of radios after, so it’s nice for the page title to
match that.
Adds the extra text added to each checkbox label.
It's a copy of the text of the link in the same
list item which does add a lot of duplication to
the test data.
This reformats a lot of the test data, stacking it
to separate out the duplicate items.
This conditional comes from before we launched the letters feature.
Since we were only giving the letters permission to teams that we were
inviting, we didn’t want to confuse new users by mentioning letters.
Nowadays all new services will have the letters permission, so this
check is redundant.
