Currently templates are ordered by the newest created first. This made
sense when, after creating a new template, you were landed on the page
that listed all the templates. In other words, you needed to see
confirmation of the thing that you’ve just done.
Now (since https://github.com/alphagov/notifications-admin/pull/1195)
you get landed on the page for just that template. So you always see
the template you’ve just created, no matter how the list of templates is
ordered. So we are free to change the order of the templates.
Ordering by time created is not great, because it gives users no control
over which templates appear first. For example, our research reported
this from one team:
> One frustration they have is that when you add a new template it
> automatically goes to the top of the list. To get round this, whenever
> they add a new template they delete all of the existing ones and start
> again because they want to keep their templates in numerical order.
> They'd like to be able to control the order of templates in the list.
We _could_ give people some sort of drag-and-drop template ordering
thing. But this feels like overkill. I think that alphabetical order is
better because:
- it’s easily discoverable – anyone who wants to know how a list is
ordered can quickly tell just by looking at it
- it’s universal – everyone knows how alphabetical ordering works
- it’s familiar – this is how people documents on their computer are
ordered; there’s no new UI to learn
- it’s what users are doing already – from the same service as above:
> They number their templates 1,2a, 2b, 3a etc
So this commit changes the ordering from newest created first to
alphabetical.
Previous changes to template order and navigation:
- https://github.com/alphagov/notifications-admin/pull/1163
- https://github.com/alphagov/notifications-admin/pull/1195
- https://github.com/alphagov/notifications-admin/pull/1330
Implementation notes
---
I refactored some of the tests here. I still don’t think they’re great
tests, but they’re a little more Pythonic now at least.
I also added a sort by template type, so that the order is deterministic
when you have, for example, an email template and a text message
template with the same name. If you have two text message templates with
the same name you’re on your own.
reply_to requires template_type to be already set, but the order
of attribute assignment is not defined when a model object is created
from a dictionary.
This adds a constructor to Template model that makes sure that
template_type is set first when multiple arguments are passed to the
constructor at once.
The problem might still exist when the template is created through the
API, so this is a temporary fix to unblock the release.
We're hiding the `service_letter_contact_id` column since it should
only be readable and writable using the `.reply_to` wrapper.
Schemas are defined using `fields.Method` since the fields are
represented by a property on the Template model that requires
template type to be set. When creating a template, if `reply_to`
is defined using `fields.String` it gets assigned at the same time
as `template_type` (or the order of assignments is not defined),
so the schema loader attempts to set `.reply_to` on a Template
object with a `None` `template_type`, which raises an exception.
Using `fields.Method` seems to delay `.reply_to` assignment until
the Template object is created, which means it already has a
valid type.
Adds a relationship between Template models and service letter contact
blocks.
Depending on template type, we can have a reference to either a letter
contact block, email reply-to address or SMS sender record. This means
that in order to enforce foreign key constraints we need to define three
separate foreign key columns on the template model.
To hide this implementation detail and make it easier to access the
sender/reply-to information we define a wrapper property that returns
the value from the correct column.
The relationship and the property are only defined for letter templates
at the moment.
The setter raises an error when trying to assign a reply_to value for
non-letter templates. The exception isn't raised if the value being
assigned is `None` since it can get assigned by marshmallow schemas
and as it matches the value returned for other template types it
doesn't need to be written anywhere.
This allows us to avoid duplication between Template and TemplateHistory
classes and makes it easier to ensure that all columns are copied
to the TemplateHistory objects.
When createing a history instance of the updated object `create_history`
sets attributes using `setattr`. Since SQLAlchemy model instances are
Python objects they don't prevent new attributes being created by setattr,
which means that if history models are missing some of the columns the
attributes will still be assigned, but their values will not be persisted
by SQLAlchemy since database columns for them do not exist.
To avoid this, we check that the attribute is defined on the `history_cls`
and raise an error if it isn't.
Checks authentication header value on inbound SMS requests from
Firetext against a list of allowed API keys set in the application
config.
At the moment, we're only logging the attempts without aborting the
requests. Once this is rolled out to production and we've checked
the logs we'll switch on the aborts and add the tests for 401 and 403
responses.
The integration tests did test for zero return. Added a method to test
for a year which has no data and also tweaked the existing tests
to ensure they are testing the year more fully.
Added a check to ensure that the current date falls in between the
financial year for the year supplied by the method, so that the todays
stats will only be appended in that situation.
correctly
The dao_fetch_monthly_historical_usage_by_template_for_service code
wasn't being tested properly due to an bug on the test which created a
notification with the same template and hence was not testing that
a specific service would have a different template id.
- Fixed the bug in the test
- Update services_dao so that the service id check is made
Removes "NOT VALID" mark from the notifications -> template_history
foreign key constraint by validating existing records.
From postgres docs:
> This form validates a foreign key or check constraint that was
> previously created as NOT VALID, by scanning the table to ensure there
> are no rows for which the constraint is not satisfied. Nothing happens
> if the constraint is already marked valid.
> Validation can be a long process on larger tables. The value of
> separating validation from initial creation is that you can defer
> validation to less busy times, or can be used to give additional time
> to correct pre-existing errors while preventing new errors. Note also
> that validation on its own does not prevent normal write commands
> against the table while it runs.
> Validation acquires only a SHARE UPDATE EXCLUSIVE lock on the table
> being altered. If the constraint is a foreign key then a ROW SHARE
> lock is also required on the table referenced by the constraint.
are not returned from queries
- Updated stats_template_usage_by_month_dao.py to return the results for
financial year not calendar, as the report os for FY only and hence
only the FY data is required
- Updated services_dao.py to ensure double precision values are converted
to an int as the 'exact' function returns double precision from the
database query, as the admin code requires the value for month to be an
int
