The test makes use of list item unpacking and expects the parameter data structure to allow for that. It does not make sense to modify the code in this case just to make test format linting pass.
Signed-off-by: Carlo Costino <carlo.costino@gsa.gov>
This syntax makes it clearer what is being tested here, because it’s
unusual to see a decorator being manually called with function as its
first argument.
It’s also consistent with how the later tests in this file are written.
This user is only re-used once, which isn’t a big saving. By putting it
inside the test it’s easy to see what special conditions are being set
up that result in the expected outcometest result.
By making the one platform admin case a separate test we no longer
need to pass in a `user` or `kwargs` to the parametrize every time,
making it easier to read.
It’s easier to see what the different test cases are when they are laid
out with `parametrize`, rather than separate functions with lots of
boilerplate.
The helper function handles both tests that pass, and tests that are
expected to fail (either by raising an exception or returning a redirect
to the login page).
By moving the handling of cases which are expected to fail out of the
helper function we can make the helper function less complex, which will
make further refactoring easier.
The failing test here[1] does two things:
1. makes a request to /sign-out
2. calls the index route, without actually making a request
This means that when the `login_manager.unauthorized_handler`[2] looks
at Flask’s `request` object it gets the request context from 1., not 2.,
because 2. isn’t actually a request. The means it sets the value of the
`next` parameter to that of the request, not of the index route.
Basically at some point Flask has changed and decided that 2. isn’t a
proper request, so won’t set new request context.
This isn’t a realistic test because nothing would call the index
function directly, it would always be as part of a request to that page.
But to make the minimal change to fix the breaking tests this commit
makes the check a bit more general, i.e. that the redirect is to the
sign in page with any `next` parameter, not a specific `next` parameter.
1. 9111a7fc86/tests/app/utils/test_user.py (L130-L138)
2. 9111a7fc86/app/main/views/sign_in.py (L86)
This is possible now we're testing letters and emails separately.
I've added a few missing cases for NHS and non-central branding.
In the next commits we'll look at the remaining special cases.
These all behave the same as each other so there's little value in
testing all of them - if we had 100 org types we wouldn't test them
all, but it's easy to get carried away when there are fewer.
We already had different functionality for email branding and will
soon be adding more for email branding pools.
Note that the "get_available_choices" class method was only used for
email branding - we can do it in the constructor for letters.
Central orgs have more options than others, including the option
to revert back to GOV.UK once branding is set. Combining the tests
together should make that a bit clearer.
All of the mock / UI assertions in these tests are covered by the
tests above them - these tests were mostly targetting which options
were being shown, which we can check at a lower level.
This is much simpler than trying to test the function via the page,
although there are still two scenarios to test there:
- The page with radio buttons (using NHS as an example).
- The page with a text form (using "other" as an example).
In future work we could split this test in two to make it clearer
what it's trying to test. For now, this keeps the diff simple.
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.
This renames the two functions we have to translate between UI and
DB permissions, as well as some of their associated variables to
make it clearer which kind of permission they contain.
