upgrade uk-election-timetables, add notice_of_election_deadline and sopn_publish_deadline fields#2613
upgrade uk-election-timetables, add notice_of_election_deadline and sopn_publish_deadline fields#2613chris48s wants to merge 7 commits into
Conversation
- handle ref/NoE edge case in one place - allow for the possibility of other optional timetable fields
|
coverage: 71.484% (+0.4%) from 71.069% — timetables5 into master |
There was a problem hiding this comment.
This is another migration that is going to touch the modified date on basically every ballot object in EE
I do at least know this time round that it will break the EE --> YNR sync and what to do about it when it does
| self.assert_timetable_fields_all_none(election_group) | ||
| self.assert_timetable_fields_all_none(org_group) | ||
|
|
||
| def test_nothern_ireland_ballot(self): |
There was a problem hiding this comment.
This is a test I should have really added in #2608
Figured I might as well do it here
| self.assert_timetable_fields_all_none(org_group) | ||
|
|
||
|
|
||
| class TestCleanMethod(TestCase): |
There was a problem hiding this comment.
We didn't have tests for the clean() method at all and this PR adds complexity to it
I told a LLM to generate tests for it and then did a review/tidy up on it. So this block of ~200 lines is mostly LLM output with a few edits from me.
1. make migration faster by using bulk_update to reduce the number of round-trips to the db 2. fix backwards migration to restore City of London SOPN dates to previous state
symroe
left a comment
symroe
left a comment
There was a problem hiding this comment.
Approving even though I had a comment about the migration. Everything else looks fine and that migration doesn't really matter if you think it'll run ok.
|
|
||
| def backfill_new_timetable_fields(apps, schema_editor): | ||
| Election = apps.get_model("elections", "Election") | ||
| for election in Election.private_objects.using( |
There was a problem hiding this comment.
Maybe this doesn't matter for a one off query, but I reckon you can use exclude() here to catch some of the continue cases below? I also suspect it would help to use annotate to attach the territory_code(s), or at least use select_related with only() to get the fields, rather than making n+1 queries in the migration. If this times out, it could be a bit of a pain to unpick.
There was a problem hiding this comment.
I wrote the above before seeing the later commit, but I think some of these points stand still? Feel free to ignore if you think it's ok
3 participants
PR notes
This PR upgrades EE to use
uk-election-timetables==5.0.0Release notes: https://github.com/DemocracyClub/uk-election-timetables/releases/tag/5.0.0
Most of the churn here is related to
notice_of_election_deadlineandsopn_publish_deadlinepropertiesI'll leave some comments inline calling out specific things.
Once this is in, DemocracyClub/aggregator-api#642 makes sense
So
Make sense to all merge as a block
Then we're on to moving downstream apps to remove use of uk-election-timetables + consume the API instead