Store timetable in EE#2606
Conversation
| close_of_nominations = models.DateField( | ||
| blank=True, null=True, help_text="Close of Nominations" | ||
| ) |
There was a problem hiding this comment.
Today on the naming things is hard channel..
For the registration_deadline, postal_vote_application_deadline and vac_application_deadline I've kept the names consistent with the timetables library.
I haven't done that with sopn_publish_date.
The reason why I have decided to do something different there is because there's actually 2 dates involved here:
There's the close of nominations (first point at which a SOPN can be published),
and then there's the SOPN deadline (last point by which a SOPN must be published)
I think our sopn_publish_date is the first one of those (except I think there is a weird edge case for City of London.. lets put a pin in that for a moment)
My thinking here is: devs.DC actually has a userbase outside Democracy Club and once we put something in a public API it is hard to change. So lets get this terminology right now, and also give ourselves the flexibility to add the SOPN deadline as a separate data point.
Does that seem right to you?
There was a problem hiding this comment.
Yes, I've wanted both dates here for a while 👍
| if getattr(self, field) > self.poll_open_date: | ||
| raise ValidationError(f"{field} must be before poll_open_date") | ||
|
|
||
| if getattr(self, field) < self.poll_open_date - timedelta(days=50): |
There was a problem hiding this comment.
This is a very arbitrary number - it is intended as a basic sanity check and we can edit the validation rules if we want to add more dates to the timetable. We could make this 100 days, if you want.
There was a problem hiding this comment.
This migration will probably take a minute or two to apply when we deploy this
Its not ideal, but also.. it'll be fine
There was a problem hiding this comment.
I guess there are ways we could optimise this, but they're not really worth doing for a one off.
I do wonder if we might need a cached timetable concept for bulk-adding elections though. I don't know how well the library performs, but making a new timetable for every ballot in a local election might take ages to re-produce exactly the same timetable. If we know e.g that local.date + nation=ENG have the same timetable in every case, we could return a cached timetable.
Worth considering?
There was a problem hiding this comment.
Yes this is a really good point. I hadn't really thought about it until you said, this, but putting this in Election.save() might have a noticeable performance impact if we are creating ~1,000 ballot objects at once.
At least, I do want to go and check what that looks like before we merge this anyway.
There was a problem hiding this comment.
So I did an unscientific local test by creating an all-up election for 40 councils before and after this change. Before, it took 8.2 seconds (which is admittedly slow). After applying this change it took 8.4 seconds. So although we are giving it a bit more work to do here, I don't think it really meaningfully changes the game performance-wise tbh.
I think the core thing here is that we're calling .save() individually on every record because we have other bits of logic in there as well as this, and bulk_create() would bypass all of it so we make a crap-ton of round-trips to the DB. In that context, adding a little more work on the frontend to each of those round-trips doesn't really make much difference. We might be able to optimise this a little by working out the dates once and setting them before we call .save() but merging this with the existing naive approach doesn't look like it will make the performance loads worse than it already is.
| "Timetable", | ||
| { | ||
| "fields": Election.TIMETABLE_FIELDS, |
There was a problem hiding this comment.
Making these dates editable via the admin seems like it is useful. In reality, it kind of is, and it kind of isn't.
If we need to manually edit a timetable event for a single ballot or small number of ballots then this is quite convenient. We can just do it.
Hypothetically if we were to make all the scheduled elections for next May and then realise one of the timetable dates is wrong for every ward in Wales, "go and edit 762 ballots by hand" isn't a useful solution to that problem. We still need a dev to go and do it in the backend, although that is still an improvement over the current process.
I don't think this is a unique problem to timetable dates. Anything where we say "store it on the ballot because we can't guarantee it will always be the same for every ballot in a group" has this problem.
I think it is probably out of scope to try and solve this problem here, but also I think there is some mileage in thinking about whether we can provide tooling to apply the same edit to N different ballots in the admin, or have certain data points that can optionally exist higher up the tree but be overridden at ballot level if necessary.. One to ponder.
Here I am coming at the timetables stuff from the opposite angle.
This PR:
Once we are happy with this, there will be more work to do downstream of this: Devs.DC API, Response builder, Client apps, etc.
I will leave some more details comments inline.