If a subclass of `JSONModel` defines a property then we shouldn’t try
to override it with the value from the underlying dictionary.
Rather than silently fail we should raise an exception because it will
help keep our list of `ALLOWED_PROPERTIES` nice and tidy.
We wrote custom `__getattr__` and `__getitem__` implementation to make
it easier to find code that was accessing fields in the dictionary that
we weren’t aware of. See this commit for details:
b48305c50d
Now that no view-layer code accesses the service dictionary directly we
don’t need to support this behaviour any more. By removing it we can
make the model code simpler, and closer to the `SerialisedModel` it
inherits from.
It still needs some custom implementation because:
- a lot of our test fixtures are lazy and don’t set up all the expected
fields, so we need to account for fields sometimes being present in
the underlying dictionary and sometimes not
- we often implement a property that has the same name as one of the
fields in the JSON, so we have to be careful not to try to override
this property with the value from the underlying JSON
I emailed the Geography team at the ONS:
> Hi geography team,
>
> I work on GOV.UK Notify, which is a service run by Government Digital Service (part of the Cabinet Office). I was given your email address by [redacted] who’s been helping answer some of my questions on the cross-government Slack.
>
> We’re using some of the boundary datasets from the Open Geography Portal, and mostly they’ve been excellent.
>
> In the abstract, the problem we’re trying to solve is, given a point outside an area, what is the minimum distance to a point within that area. So, for example, if a crow was somewhere in Cardiff, what’s the shortest distance it would have to fly to reach somewhere in the Bristol local authority district?
>
> We’ve noticed some problems with the data that means our calculations would be wrong. We’ve noticed this around Torquay, Norwich and Bristol. Here are some screenshots of Bristol, from the generalised and full resolution boundaries:
>
> The artefacts I’ve highlighted are closer to Cardiff than any actual part of the land area of Bristol. They are either:
> - in the sea
> - land that’s part of North Somerset
>
> I suspect that this is being caused by the process of clipping the actual region of Bristol (which, unusually, extends into the water) to the mean high water line.
>
> I’ve worked around this by filtering out any polygons that are smaller than ~7,500m². It’s a bit hacky because parts of the Scilly Isles start disappearing. That’s not a problem for what I’m working on, but it would be nice to not need the hack.
>
> So my questions would be:
>
> - Is there a better way to remove these artefacts than filtering by area?
> - Is there a plan to remove these artefacts from the data in future releases?
>
> Thanks in advance,
> Chris
They emailed back to say:
> Hi Chris
>
> Thank you for your enquiry.
>
> We have completed the amendments to the LAD MAY 2020 BFC and BGC boundaries as mentioned so you should be able to download them from the portal now.
>
> Hope this helps.
>
> Kind regards
> [redacted]
This commit brings in the files they’ve updated. We still have to do
some filtering (but now at a higher resolution) because they haven’t
fixed Norwich yet. I’ll email them separately about that.
We have a bunch of stuff for doing lat/long transformation in the
`BroadcastMessage` class. This is not a good separation of concerns, now
that we have a separate class for dealing with polygons and coordinates.
We’ve observed people using ‘national’ and ‘local’ during user research.
It has less tongue-twisting ambiguity than county vs country.
But we think that maybe just getting rid of ‘counties’ is enough to
disambiguate them. So this commit just takes the ‘local’ concept.
This commit also gives the libraries and areas new IDs, which means if
we want to rename them in the future it won’t be a breaking change.
Broadcasting is not a precise technology, because:
- cell towers are directional
- their range varies depending on whether they are 2, 3, 4, or 5G
(the higher the bandwidth the shorter the range)
- in urban areas the towers are more densely packed, so a phone is
likely to have a greater choice of tower to connect to, and will
favour a closer one (which has a stronger signal)
- topography and even weather can affect the range of a tower
So it’s good for us to visually indicate that the broadcast is not as
precise as the boundaries of the area, because it gives the person
sending the message an indication of how the technology works.
At the same time we have a restriction on the number of polygons we
think and area can have, so we’ve done some work to make versions of
polygons which are simplified and buffered (see
https://github.com/alphagov/notifications-utils/pull/769 for context).
Serendipitously, the simplified and buffered polygons are larger and
smoother than the detailed polygons we’ve got from the GeoJSON files. So
they naturally give the impression of covering an area which is wider
and less precise.
So this commit takes those simple polygons and uses them to render the
blue fill. This makes the blue fill extend outside the black stroke,
which is still using the detailed polygons direct from the GeoJSON.
Brings in:
- re-usable `SerialisedModel`
- speed improvements to processing CSVs against email templates
I chose not to rename `JSONModel` or `ModelList` to keep the diff nice
and small.
`dir(object)` is a useful Python function that tells you what attributes
and methods an object has. It’s also used by tools like iPython and IDEs
for code completion.
Some of the attributes of a `JSONModel` are dynamic, based on what
fields we expect in the underlying JSON. Therefore they don’t
automatically end up in the result of calling `dir`. To get around this
we can implement our own `__dir__` method, which also returns the names
of the fields we’re expecting the the JSON.
Inspired by this Raymond Hettinger tweet:
> #python tip: If you add attributes to an API with __getattr__() or
> __getattribute__(), remember to update __dir__() to make the extension
> introspectable.
— https://twitter.com/raymondh/status/1249860863525146624
We’re caching the organisation name, but still talking to the API
to see if the organisation exists.
`Service().organisation_id` only goes to the JSON for the service.
`Service().organisation` makes a separate API call.
We only need the former to know if a service belongs to an organisation.
A lot of pages in the admin app are now generated entirely from Redis,
without touching the API.
The one remaining API call that a lot of pages make, when the user is
platform admin or a member of an organisation, is to get the name of
the current service’s organisation.
This commit adds some code to start caching that as well, which should
speed up page load times for when we’re clicking around the admin app
(it’s typically 100ms just to get the organisation, and more than that
when the API is under load).
This means changing the service model to get the organisation from the
API by ID, not by service ID. Otherwise it would be very hard to clear
the cache if the name of the organisation ever changed.
We can’t cache the whole organisation because it has a
`count_of_live_services` field which can change at any time, without an
update being made.
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
`__getattr__` is called whenever an attribute error is raised.
This means that if something deep inside a property on a model raised
an attribute error, that error would be caught by `__getattr__`, which
would then raise an exception that looked like the property itself
didn’t exist. Very confusing.
The solution seems to be to override `__getattribute__` instead, which
handles _all_ attributes, not just those that aren’t explicitly defined.
We then only intervene if the desired attribute is one of the
`ALLOWED_PROPERTIES`, otherwise falling through to the built in methods
of the underlying `object`.
It looks weird to have two different visual treatments for showing a
navigable hierarchy.
I reckon losing the slash won’t make things less folder like – Windows
for example uses chevrons as foler separators.
Rather than force us to write the decorators in a specific order let’s
just have one decorator call the other. This should make fewer lines of
code, and fewer annoying test failures. It also means that the same way
of raising a `401` (through the `current_app` method) is used
everywhere.
so that platform admins (us) can view pages as regular users do easily.
Simply adds a flag in the session cookie that overrides the actual
platform admin flag on the user model if set. This way it's safe, since
this only downgrades existing functionality, so if someone managed to
alter it they could only get less permissions, not more.
You can change this value from the user profile page if either:
* you're a platform admin
* the flag is set (to any value) on the cookie.
This slightly weird check means that we don't check the underlying
`user._platform_admin` flag anywhere in the code, even when toggling
the suppression.
The data flow of other bits of our application looks like this:
```
API (returns JSON)
⬇
API client (returns a built in type, usually `dict`)
⬇
Model (returns an instance, eg of type `Service`)
⬇
View (returns HTML)
```
The user API client was architected weirdly, in that it returned a model
directly, like this:
```
API (returns JSON)
⬇
API client (returns a model, of type `User`, `InvitedUser`, etc)
⬇
View (returns HTML)
```
This mixing of different layers of the application is bad because it
makes it hard to write model code that doesn’t have circular
dependencies. As our application gets more complicated we will be
relying more on models to manage this complexity, so we should make it
easy, not hard to write them.
It also means that most of our mocking was of the User model, not just
the underlying JSON. So it would have been easy to introduce subtle bugs
to the user model, because it wasn’t being comprehensively tested. A lot
of the changed lines of code in this commit mean changing the tests to
mock only the JSON, which means that the model layer gets implicitly
tested.
For those reasons this commit changes the user API client to return
JSON, not an instance of `User` or other models.
This removes the edit_folder_permission checks from the code, enabling
the folder permissions for all services.
This also fixes folder-related tests to set up appropriate user
permissions.
This should only be merged right after alphagov/notifications-api#2428,
when all other permission stories are done.
User model is the most natural place for a permission check method,
however this means that we need to pass the full user object to
service model methods and TemplateList instead of user_id.
Putting the permission check in the get_user_template_folders allows
us to replace `all_template_folders` usage with the new method without
having to worry about the temporary service permission flag.
