Because ModelList implements `__add__` we can do the following:
```python
ImmediateJobs() + ScheduledJobs()
ImmediateJobs() + []
```
Both of these call the `__add__` method of `ImmediateJobs`.
What we can’t do is this:
```python
[] + ScheduledJobs()
```
That tries to call the `__add__` method of list, which doesn’t know what
to do with an instance of `ModelList`.
The Pythonic way to deal with this is to implement `__radd__` (right
add) which is invoked when our instance is on the right hand side of the
addition operator.
I’m hoping that if I can design something that clearly differentiates
them then we won’t need to do so by putting them in separate tables,
which then need labelling, which would clutter up the page.
Currently you have no way of getting to the returned letter page. This
commit adds a link to it from the dashboard, following the pattern of
the new received text messages banner.
We found another scenario where signing out of the db can cause a 500.
If the user archives their trial mode service, current_service.active = false, then signs out, the current user was being signed out client side first, meaning current_user is now an Anonymous user, next the call to the API is made to log out user on db, all calls to NotifyApiClient `check_inactive_service`, which is only authorised if user is platform_admin, but an AnonymousUser does not have that attribute, so a 500 is raise.
Seemed a bit cleaner to change the User.signout method to rather than the `check_inactive_service` method for current_user.is_authenticated.
Anytime a user clicks "sign out" we should be signing them out server side as well. This can be accomplished by setting the Users.current_session_id = null.
I found that the method User.logged_in_elsewhere doesn't need to check if the current_session_id is None. The current_session_ids in the cookie and db (redis or postgres) then the user should be forced to log in again.
service contact blocks contain new lines - and jinja2 normally ignores
newlines (as in it keeps them as new lines) - but we need to turn them
into `<br>` tags so that we can show the formatting that the user has
added. We were previously just doing `{{ block | nl2br | safe }}`. nl2br
turns the new lines into `<br>` tags, and then `safe` tells jinja that
it doesn't need to escape the html.
this causes issues if the user adds `<script>alert(1)</script>` to their
contact block (or some other evil xss hack), where that will get let
through due to the safe flag
To solve this, use `Markup(html='escape')` to sanitise any html, and
then convert new lines to <br>.
bump utils
another xss
`recently_created` says it would just be looking at the `created_at`
field to see if it's been created recently. Technically this method
isn't doing that, whilst its behaviour would be similar, it's actually
different and maybe therefore a bit misleading.
Rather than hard-coding a format string in a bunch of different places
we can use the function we already have in utils.
This commit also refactors some logic around password resets to put the
date-parsing changes in the most sensible bit of the codebase, so it’s
clearer what the intention of the view-layer code is.
The API response for jobs includes a field called `job_status`. The API
response for uploads doesn’t.
The `Job` mode handles uploads and jobs, so it needs to account for the
possibility of the field not being there.
Notifications won’t exist for a job if:
- it’s just started
- it started a long time ago (older than the retention period)
We have a bug where:
1. Job starts processing, puts notifications on queue
2. Job finishes processing, sets status to `finished`
3. First notification gets picked up off the queue and put in the
database
In between 2. and 3. it’s possible for a job to be finished, but also to
have no notifications. We’re saying this is because the notifications
have been deleted, whereas really it’s because they haven’t been created
yet.
This commit fixes that bug by introducing the concept of recency for
jobs.
‘Recent’ is defined as 1 day, which is:
- a lot longer than it takes to create any notifications
- a bit shorter than anyone’s retention time
N.B. `processing_started` is defined here:
879ba1d5f0/app/models.py (L1194)
It can be `None` for scheduled jobs that haven’t started yet.
Jobs have a `scheduled_for` field. Single letter uploads don’t.
At the moment we treat both of them as `Job`s. So the `Job` model needs
to account for when the `scheduled_for` field is missing.
The property doesn’t represent the whole client, but just one method on
it. So this commit renames the property to better describe what it is
designed to store.
It returned the same value (and had the same code as
`.notifications_sent`).
I think `.notifications_sent` is a better name because it’s closer to
the language (‘sending’ and ‘sent’) that we use in the interface.
The API clients should just deal with calling the API and returning the
data from it.
Inferring things from the data is more logically done at the model
layer. But we couldn’t do that before, because we didn’t have a model
layer for jobs.
By moving it from the view we reduce the complexity of the methods in
the view layer, so it’s easier to see what they do.
This also renames the variable `finished` to the property
`processing_finished` to disambiguate from the `job_status` field in the
JSON, which can also have a value of `finished`.
This follows the pattern of what we’ve done with services, users and
events.
It gives us a way of neatly instantiating a model for each item in the
list we get back from the API and reduces the complexity of the view
layer code.
Now is a good time to do this because we’re going to be making a bunch
of changes to the jobs pages, and those changes will be easier to code
and understand with a sensible model behind them.
This follows the pattern of what we’ve done with services, users and
events.
It gives us a better interface to the data we get back from the API than
dealing with the raw JSON directly.
Now is a good time to do this because we’re going to be making a bunch
of changes to the jobs pages, and those changes will be easier to code
and understand with a sesnsible model behind them.
The new taxonomy doesn't have a `notify_go_live_incomplete` tag. We
replaced this with `notify_go_live_incomplete_mou` because the only way
users can submit an incomplete request is if they do not agree to the
MOU.
These are the incomplete tags:
- `notify_go_live_incomplete_mou`
- `notify_go_live_incomplete_reply_to`
- `notify_go_live_incomplete_shared_email`
- `notify_go_live_incomplete_templates`
Of those, only the first one is applied automatically.
Requests to go live and email branding requests come through to Zendesk
with tags attached automatically.
With the revised taxonomy some of these tags need to be updated, as
summarised in this spreadsheet.
In addition, `notify_action` tag has to be added in each of those cases.
Old|New
---|---
`notify_request_to_go_live_complete`|`notify_go_live_complete`
`notify_request_to_go_live_incomplete`|`notify_go_live_incomplete`
`notify_action_add_branding`|`notify_branding`
`notify_request_to_go_live_incomplete_mou`|`notify_go_live_incomplete_mou`
`notify_request_to_go_live`|`notify_go_live`
– https://docs.google.com/spreadsheets/d/1o5ATsFsVK8Qpj7x8QvxX-SfEuBZ75028GEySVcdBFYU/edit#gid=0
– https://www.pivotaltracker.com/story/show/169842970
Because it means you often have to cast to string in your application
code just to get your tests passing.
The method being monkey patched is originally defined here: b81aa0f18c/src/werkzeug/routing.py (L1272)
Flake8 Bugbear checks for some extra things that aren’t code style
errors, but are likely to introduce bugs or unexpected behaviour. A
good example is having mutable default function arguments, which get
shared between every call to the function and therefore mutating a value
in one place can unexpectedly cause it to change in another.
This commit enables all the extra warnings provided by Flake8 Bugbear,
except for the line length one (because we already lint for that
separately).
It disables:
- _B003: Assigning to os.environ_ because I don’t really understand this
- _B306: BaseException.message is removed in Python 3_ because I think
our exceptions have a custom structure that means the `.message`
attribute is still present
This commit introduces a slightly hacky way of putting usernames against
events, given that the API only returns user IDs.
It does so without:
- making changes to the API
- making a pages that could potentially fire off dozens of API calls (ie
one per user)
This comes with the limitation that it can only get names for those team
members who are still in the team. Otherwise it will say ‘Unknown’.
In the future the API should probably return the name and email address
for the user who initiated the event, and whether that user was acting
in a platform admin capacity.
Directly referencing the `ModelList` instances will let us more easily
make choices at the view layer about which kinds of events to show, and
is one less layer of indirection to jump through.
We store our audit history in two ways:
1. A list of versions of a service
2. A list of events to do with API keys
In the future there could be auditing data which we want to display that
is stored in other formats (for example the event table).
This commit adds some objects which wrap around the different types of
auditing data, and expose a consistent interface to them. This
architecture will let us:
- write clean code in the presentation layer to display these events on
a page
- add more types of events in the future by subclassing the `Event` data
type, without having to rewrite anything in the presentation layer
Updating an organisation’s branding might now also update the branding
of services associated to that organisation. This is similar to how
updating an organisation’s type can update the organisation type for its
services.
In the latter case we already make sure to clear the cached version of
these services which is held in Redis.
This commit does the same clearing of the caches when updating an
organisation’s branding (and does a bit of refactoring to do so without
duplication of code.)
In some cases it’s not appropriate for teams to have GOV.UK branding.
But they all start with it by default, if we can’t make a better guess.
We should be more explicit about this to reduce the number of teams
sending emails with the wrong branding.
Users who work in local government can’t have GOV.UK branding on their
emails. And only those working for Companies House (for example) can
request the Companies House branding.
This commit adds:
- new choices of email branding, which offer the name of the branding,
rather than the style
- logic to filter this list to only the applicable options, based on
what we know about the user, service and organisation
This is a change from the previous approach which put the onus on users
to figure out the style of branding they wanted, when we might already
know that a lot of the options weren’t available to them, or would be
inconsistent with the branding of other services in their organisation.
