The updateContent JS was changed in this commit so
the replacement of the original HTML (with GOVUK
modules data-attributes) was moved into the start
method rather than being a slightly odd side
effect of the render function diffing:
476ed1593c
This adds a test to make it more clear this
happens, as requested in this comment:
https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804689618
Wrap the code that updates the HTML with changes
from the server with code that stores and
re-applies specified classes.
This is to allow other JS to add classes which
change the visual state of the HTML without them
being considered by the code that diffs our
in-page HTML against that from the server.
They are called classesToPersist because this
should make the visual state they create persist
between updates.
Includes the addition of tests for updateContent
that cover the addition/deletion of elements so we
can write a test for classNames persisting through
updates. The existing tests only cover updates
that change the content of elements. Just adding
the test for these changes to those would simulate
a scenario that doesn't exist in the app. Writing
extra tests for the kind of updates these changes
act on keeps them in line with the app code.
The endpoint to change the email branding to "GOV.UK" branding and
"GOV.UK and organisation" branding was the same but with a query string
used to determine which of the two options had been selected. This makes
them two separate endpoints, which makes the code a bit simpler and
hopefully means there is less chance of things not working as expected.
This changes the URLs for someone to request new email or letter
branding to match the new URLs we've agreed for the new email branding
changes. The old URLs are still in place for now too to keep backwards
compatibility.
It shouldn't be possible to view the page to confirm that you want a
particular type of email branding if that branding is not allowed for
your service. Although we don't show banned branding options on the
branding form, it would have been possible to visit the relevant URLs
directly.
We now give a `404` status page if you visit a page to select branding
that isn't allowed.
For the "Something else" branding form we want the form label to be the
title. This brings in the textarea component from GOV.UK Frontend in
order to do this since that contains code to set a the textarea label as
the page heading in an accessible way.
The rest of the textarea fields have not been switched to use the new
component yet.
We were showing the form to request email branding with a button which
submits your choice immediately. Now, we only submit the form
immediately if "Something else" is the only branding option available to
you. If you select any other radio button (or select "Something else"
when it's not the only option) we take you to another page which either
contains more information or a textbox to fill in the details for the
branding you want.
There is currently some duplication between the new pages and their
tests, but these will be changed in future versions of the work so will
start to differ more.
The existing tests were parameterized to contain the cases both where
the branding form is successfully submitted and those where it isn't. We
were using `pytest.mark.xfail` to check that the tests fail as expected
if there were errors on the form. However, since I'll be changing how
the form validation works, I want to make sure that these tests were
actually failing because of the form validation and not because of
another reason, such as a slight difference in Zendesk ticket output.
This creates separate tests for cases where data entered in the form
is invalid.
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.
We added domdiff to replace the DiffDOM library
here:
87f54d1e88
DiffDOM had updated its code to be written to the
ECMAScript 6 (ES6) standard and so needed extra
work to work with the older browsers in our
support matrix. This was recorded as an issue
here:
https://www.pivotaltracker.com/n/projects/1443052/stories/165380360
Domdiff didn't work (see below for more details)
so this replaces it with the morphdom library.
Morphdom supports the same browsers as us and is
relied on by a range of large open source
projects:
https://github.com/patrick-steele-idem/morphdom#what-projects-are-using-morphdom
It was tricky to find alternatives to DiffDOM so
if we have to source alternatives in future, other
options could be:
- https://github.com/choojs/nanomorph
- https://diffhtml.org/index.html (using its
outerHTML method)
Why domdiff didn't work
Turns out that domdiff was replacing the page HTML
with the HTML from the AJAX response every time,
not just when they differed. This isn't a bug.
Domdiff is bare bones enough that it compares old
DOM nodes to new DOM nodes with ===. With our
code, this always results to false because our new
nodes are made from HTML strings from AJAX
response so are never the same node as the old
one.
Having this as a function which does string parsing and manipulation
surprised me a bit when I was trying to figure out why something wasn’t
working.
It’s more in line with the way we do other config like this (for example
`ASSET_PATH`) to make it a simple config variable, rather than trying to
be clever and guess things based on other config variables.
It’s also less code, and is explicit enough that it doesn’t need tests.
This field caused some confusion and lots of unnecessary work
to our colleague because of unclear name.
The field was named sms_fragments, where in fact the value of
the field is: those sms fragments that go above free allowance
multiplied by the rate multiplier.
The new name was chosen through consultation with colleagues
who use billing report the most.
for a job to be finished there are two requirements:
* the status to be "finished"
* the percentage complete to be 100%
The job status is set to finished when the process_job task finishes
(even though not all process_row may have finished). The
percentage_complete is calculated by comparing the number of
notifications in the database with the number of rows in the
spreadsheet.
This was inadvertently changed from an "and" to an "or" clause two years
ago. This meant that people could download a report when the status was
finished but not all notifications were present in the database. Lets
change it back.
7d52ac97f1 (diff-44b012cad205379c481bed244ddb2294bae5ee85dcd01f4aee932a2bd85b67b2L86-R100)
This deletes the `test_view_team_members` test since it is now
duplicated by a new test. It also moves a test relating to org
invites to test_organisation_invites.py and one that wasn't related to
invites from that file to test_organisations.py.
This adds a link next to the organisation team members which lets
them be removed from the organisation. Service team members have
their own page and the link to remove them appears there. For
organisation team members, we don't currently have any other
information we want to show or any other actions to perform. As
a result, this change uses the 'Team members' page to show the
confirmation banner.
The endpoint called 'edit_user_org_permissions' was renamed to
'edit_organisation_user' and some of the existing code around deleting
org users (which didn't work) was changed.
This stops adding the current user to the data sent to the API when
removing a user from an organisation. API only needs to know the
organisation_id and the id of the user we want to remove from
the organisation, so we don't need to pass through the id of the
current user too.
The other change made is to clear the user cache of the user who has
been removed from the org. We don't need to clear any of the organisation
caches, since these values don't contain lists of users for the orgs.
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.
It’s weird to have a method just for updating one attribute. I think
the reason for doing this was to only invalidate the
`organisation-{}-name` cache when absolutely necessary, but:
- we don’t need a separate method to check whether it’s the name being
updated
- it was easy to get around this by calling
`OrganisationsClient.update_organisation` directly, leaving a stale
value in the cache
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)
No tests are now using the `client` fixture directly so we can rename
it.
Python convention is to use an `_underscore` for things which should be
considered semi private.
This should discourage people from writing new tests with these old
fixtures.
New tests should always use `client_request`.
Want to be logged in with a different user? Call
`client_request.login(user)` first.
Don’t want to be logged in? Call `client_request.logout()` first (most
of our tests need to be logged in).
Need an instance of `Response` object not an instance of
`BeautifulSoup`? Use `client_request.get_response` or
`client_request.post_response`.
Need to pass in a URL, not arguments to `url_for`? Use
`client_request.get_url(…)` or `client_request.post_url(…)`.
Need to pass in a URL and get a response back? Use
`client_request.get_response_from_url(…)` or
`client_request.post_response_from_url(…)`.
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 a letters that has been posted via the API has more than 10 pages it would not get a validation-failed status. This also happens for letters in a CSV upload, only the first row has been validated for having too many pages, because you need to created the pdf before getting an accurate page count.
The API has been updated to mark these letters as invalid and move the
letter to the invalid s3 bucket, the meta data is also set with the
error message and page count.
This PR updates the notification page to display the validation error.
https://www.pivotaltracker.com/story/show/169209742
This updates the `get_notifications_for_service` of the
`NotificationApiClient` to make POST request to the api when the `to`
field is provided. This is done so that we avoid logging personal
details such as email addreses.
If the `to` field is not present, the method will make a GET reqeust and
there will be no change to how it works.
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.
