This changeset converts the display of dates and times to be just UTC to match the recent changes in the backend. This unwinds a bit of work that was done previously and allows us to start with a clean slate in how we want to approach displaying dates and times going forward. It also adds a bit of explanatory text to help users.
Signed-off-by: Carlo Costino <carlo.costino@gsa.gov>
Co-authored-by: stvnrlly <steven.reilly@gsa.gov>
* Updated header and footer
* Moved files around and updated gulpfile to correct the build process when it goes to production
* Updated fonts
* Adjusted grid templating
* Adding images to assets
* Updated account pages, dashboard, and pages in message sending flow
* Updated the styling for the landing pages in the account section once logged in
It looks like, by default, Flask no longer makes full URLs, for example
`https://example.com/path`. Instead it does `/path`. This will still
work fine, and if anything is better because it reduces the number of
bytes of HTML we are sending.
It won’t mean that requests go over `http` instead of `https` without
the protocol because we set the appropriate HSTS header here:
0c57da7781/ansible/roles/paas-proxy/templates/admin.conf.j2 (L11)
This commit changes all our tests to reflect that URLs no longer have
the protocol and domain in them. `_external=True` is Flask’s way of
saying whether a URL should be generated with the domain and protocol
(`True`) or without it (`False`).
Again, I can’t find the changelog or diff where this was introuduced,
but if you’d like to go spelunking then here’s a starting point:
50374e3cfe/src/flask/helpers.py (L192)
This goal wasn't clear from the original test, which was checking
the entire return value, even though this is covered implicitly by
tests of the usage page itself.
This doesn't need to test variable rates for every postage class,
which is more an aspect of grouping. It only needs to check that
some out-of-order usage gets reordered appropriately.
This is covered sufficiently by the main "test_usage" assertions,
which prove the usage is broken down by postage.
I don't think we need to explicitly test the usage is broken down
by month as we already prove this for SMS and we also check the
usage is associated with the correct month in the "ordering" test.
This is now only used for letters and represents the number sent
[^1]. We could use the chargeable_units field, but using "_sent"
is more consistent with the annual attributes [^2].
In fact, chargeable_units isn't actually used anywhere, but I've
kept it in the test data as it is part of the real API and helps
clarify the other values for SMS - free vs. charged.
Note: for SMS I've used an arbitrary "1234" for "chargeable_units"
to indicate it's not used and may be different to the number sent -
for SMS it's related to the number of fragments.
[^1]: bb62d22f25/app/dao/fact_billing_dao.py (L339)
[^2]: 3a1ac189ff/app/main/views/dashboard.py (L339)
This starts using the sms_{cost, charged, free_allowance_used}
fields in the new API to replace the "get_free_paid_breakdown"
function we had before, which could not support multiple rates.
In order to use "get_free_paid_breakdown" the calling method had
to store a "cumulative" variable to calculate the free allowance
used so far, which is now done by the API.
To calculate the data for conftest.py, I had to start from the
bottom ("April") and manually calculate the free allowance used
to emulate the API - this is what "cumulative" used to do.
Note: I've removed the pricing assertion in the "0_free_allowance"
test as it's covered elsewhere - the value of the test is really to
check that we don't show the remainder if there never was any.
The previous, manual calculation could be incorrect depending on
which SMS rates the free allowance was attributed to.
The new field also supersedes the old "letter_total" bolt-on so we
can get cost information consistently for both types.
This adds missing assertions for email and SMS usage, as well as
letters with the help of some additional test data.
Previously we were only checking monthly usage (in other tests).
The "with_letters" was mostly a duplicate of the one before - no
change in test setup - bar the three assertions at the end.
Having the assertions in a separate test will help keep the one
above manageable as we add more assertions for the annual usage.
At the moment, we put the sms rate on the usage page for each
months billing data by taking the single sms rate for the year.
The assumption that there will be a single sms rate for the year is
no longer going to be true. Therefore, instead we take the sms
rate from the monthly data itself which tells us the rate for
a months worth of sent SMS.
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.
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.
We've added new broadcast roles in the database (`create_broadcasts` and
`approve_broadcasts`).
Adding these has meant we've needed to do a bit of a rewrite of the roles and
permissions code since this had been based on the assumption that each
database permission only belongs to one admin role - this is no longer true.
This means that flipping the roles dict round to create a dict which
contains database permissions as the keys is no longer possible. We can't
necessarily tell which admin role someone has given a database permission.
To check if a user has an admin role given a list of database permissions,
the user must now have ALL the database permissions mapped to that role
(instead of just one). This works because no one has the `manage_users`
permission without also having the `manage_settings` (and similar for
the other admin roles which map to multiple database permissions).
Some test data was changed because it was using admin roles where
database permissions are actually used when the app is running. I've kept
the functionality of the `translate_permissions_from_db_to_admin_roles`
function passing through any unknown roles it is passed as an argument.
This is not necessary, so can be changed later if we decide it will not
ever be used. However, removing it would require updating a lot of
tests since the tests rely on this behaviour.
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)
