Generic boundary changes

[awdem]

This PR makes changes to the current_boundary_changes pipeline so that it includes generic LGBC reviews.

• BRs are included based on their public_visibility field
• I've mimicked the concept of election currency, so that BRs are excluded from the pipeline once their organisation has a 'non-current' election that used the new boundaries (i.e. an election with a poll open date after the BR's effective date)
• I've also added related ballots to boundary changes.

I've tested the first two points by deploying to dev, running the state machine, and checking the output. The boundary reviews for 2027 don't have elections yet, so in order check that related ballots were included in the parquet file, I created the 2027 local elections in my local DB and generated a boundary changes CSV from it, then uploaded that CSV to S3 and ran an altered state machine which skipped the CSV generation step and used my locally generated one instead.

[awdem]

I'm ignoring the old divisions here because the relevant ballots will only be made with the new divisions. I'm also unsure if regex matching the division names to the ballots is the best/most efficient way to do this or if there is a better optimized way.

[chris48s]

I'm more concerned about accuracy than efficiency here.

To give a real-word example port-talbot is a substring of local.neath-port-talbot.sandfields-east.2022-05-05

Also piddlington-south would be a substring of piddlington-south-east, etc

If we're going to do string matching, we need to be more robust about it
Or, we need to actually use the FK relationship Ballot --> Division --> SubdividedDivision to link ballot and subdivided Division

[GeoWill]

So I was wondering about this too.
I was thinking of ways to get round it like doing a substring match with dots: b LIKE '%.' || d.slug || '.%' or maybe d.slug = ANY(string_to_array(b, '.')).

But I think what Chris is suggesting is probably better. There will be issues for elections which don't have a division_id.

[awdem]

So I've re-rewritten this case statement as:

    CASE
        WHEN ds.id = r.old_divisionset_id THEN NULL
        WHEN ds.id = r.new_divisionset_id THEN COALESCE(
            (
                SELECT
                    json_agg(e.election_id)::text
                FROM
                    elections_election AS e
                WHERE
                    e.division_id = d.id
                    AND d.divisionset_id = r.new_divisionset_id
                    AND e.poll_open_date = r.effective_date
            ),
            '[]'
        )
        ELSE NULL
    END AS division_related_ballots,

Doing it like this has also let me remove the review_related_ballots field from the review sub-query. However, there is something I want to check with you both - Will this ever return >1 election_id? Can a division in a divisionset have more than one ballot on the same day?

Sorry if this is an obvious one - I've possibly been thinking too hard about this.

[GeoWill]

I thought the answer was 'no'.
However I didn't actually know that, just 'how I thought it worked'.

Our database doesn't entirely agree, but I think these might be errors. However the query I used probably shows some extra bits you want for the WHERE clause:

• e.group_type IS NULL
• NOT e.cancelled
• e.current_status = 'Approved'

To see if there were any cases where there were multiple ballots for a single division I did this:

SELECT e1.election_id, e1.group_type, e2.election_id, e2.group_type
FROM
    elections_election e1 JOIN elections_election e2
        ON e1.division_id=e2.division_id
            AND e1.poll_open_date = e2.poll_open_date
            AND e1.election_id != e2.election_id
WHERE e1.group_type IS NULL
  AND e2.group_type IS NULL
  AND NOT e1.cancelled
  AND NOT e2.cancelled
  AND e1.current_status = 'Approved'
  AND e2.current_status = 'Approved';

Which I think is right.

With the duplicates manually removed it gives:

local.castle-point.st-georges.2016-05-05,local.castle-point.st-georges.by.2016-05-05
local.castle-point.st-georges.by.2021-05-06,local.castle-point.st-georges.2021-05-06
local.castle-point.boyce.by.2021-05-06,local.castle-point.boyce.2021-05-06

I'm curious why these are all in Castle Point and twice in St. Georges. Have I done something stupid in the query?
These either need tidying up, or are the counter examples you're asking about. Hopefully the former. Or the query isn't right.

[GeoWill]

🤔 https://candidates.democracyclub.org.uk/media/official_documents/local.castle-point.st-georges.2021-05-06/statement-of-persons-nominated.pdf and https://candidates.democracyclub.org.uk/elections/local.castle-point.st-georges.by.2021-05-06/sopn/

[awdem]

thanks I've got e.current_status = 'Approved' in the latest query, but I missed NOT e.cancelled. I left out e.group_type IS NULL, because I assumed only ballots have a FK to a division, is that wrong?

Your query looks good to me. Very weird about castle point, maybe one to check with Peter?

[chris48s]

Just to put the discussion we had about this yesterday down in text:

A division can have more than one election on the same day
but it has to be a by-election on the same day as a scheduled election
and if there are new boundaries that forces an all-up election, so you can't have that if there has also been a new divisionset

[awdem]

I'm assuming here that the first elections to use a review's boundaries will always have its polling day on the review's effective date. I think that's safe to assume?

[chris48s]

We should also use the Ballot --> Division --> DivisionSet FK relationship to make sure the election is actually attached to the DivisionSet we care about here. Not just an election to the same organisation on the same date.

[chris48s]

I think this need some kind of sanity checks on the data we produce at the end of this:

• Every subdivided Division should be attached to exactly one Ballot ID. If any subdivided Division is attached to 0 or >1 ballots, throw
• Every ballot ID we found should be attached to at least one subdivided Division. If we have any ballots "left over", throw

[chris48s]

I had another thought on this. Where we are collecting up the ballots for each division, does this only look at approved elections (i.e: not suggested or soft-deleted)?

[GeoWill]

Is this the right way round?
Would obr.public_visibility = 'MAP' be better?
This might have to be updated to obr.public_visibility IN ( 'MAP', 'NEW_VALUE'), but I guess that's better than having something come through unintentionally?

[awdem]

I sort of assumed that we'd want to include any potential future value that is not HIDDEN, and since we have to manually set the values on the records in EE anyway, we're avoid having a review pulled through before the data baker layer is ready or whatever. But maybe it's better to be explicit?

[GeoWill]

Chris' point about election status matters here I think. I.e. we only want approved elections.
We can also pin elections as current outside of the time check. Do we want to respect that here so that we're consistent with what get_current() in EE does?

[GeoWill]

In aggregator api I called the field ballots.
If we keep this then it will need to be updated in the model there.:

[GeoWill]

Do we want reviews to be taken out of the csv once they're a certain age. This only get's rid of them if there's a new poll.

[awdem]

I've updated this filtering in fc851f5

It's now:

AND NOT EXISTS (
    SELECT
        e.election_id
    FROM
        elections_election e
    WHERE
        e.current_status = 'Approved'
        AND e.current IS NOT TRUE
        AND e.organisation_id = o.id
        AND e.poll_open_date >= obr.effective_date
        AND e.poll_open_date <= CURRENT_DATE  - INTERVAL '20 days'
    LIMIT
        1
)

This means we exclude a boundary review if it's organisation has a non-current, approved election with a polling day that is on or after the effective date and the polling day at least 20 days ago.

So it should get rid of them as elections age basically.

[GeoWill]

I think your parquet files got steamrollered, so I've failed to confirm this on dev.
But.. this is a string. I think this will stay as string-y json all the way through. The model in aggregator api is expecting a list of strings. We can decode the json twice, but it might be worth checking that however the quote escaping works is okay with that. Another approach might be to make it a delimited string.
Given it's an array of ballot ids, we could pick a delimiter to call split() on in aggregator api because the the ballot ids use the org/division slugs. So , or ; would probably be OK.

[awdem]

Yeah so I changed the aggregator API model to

    @classmethod
    def from_dict(cls, data: dict):
        return cls(
            ...,
            ballots=json.loads(data["related_ballots"]),
        )

So I am decoding the json twice and the quote escaping seemed to work fine. Here is an abridged API response:

{
  "address_picker": false,
  "addresses": [],
  "dates": [
   ...
  ],
  "electoral_services": {
   ...
  },
  "registration": {
   ....
  },
  "postcode_location": {
    "type": "Feature",
    "properties": {
      "postcode": "CT1 1AA"
    },
    "geometry": {
    ...
    }
  },
  "boundary_reviews": [
    {
      "id": "785",
      "consultation_url": "http://www.lgbce.org.uk/all-reviews/canterbury",
      "effective_date": "2027-05-06",
      "legislation_title": "The Canterbury (Electoral Changes) Order 2025",
      "organisation_name": "Canterbury",
      "organisation_official_name": "Canterbury City Council",
      "organisation_gss": "E07000106",
      "boundary_changes": [
        {
          "change_scenario": "BOUNDARY_CHANGED",
          "division_type": "DIW",
          "new_division_official_identifier": "CAT:northgate",
          "new_division_slug": "northgate",
          "new_division_name": "Northgate",
          "new_divisionset_pmtiles_url": "https://s3.eu-west-2.amazonaws.com/ee.public.data/pmtiles-store/canterbury_843_1febe76039eee7b315a07e4df60b4109.pmtiles",
          "old_division_official_identifier": "gss:E05010402",
          "old_division_slug": "northgate",
          "old_division_name": "Northgate",
          "old_divisionset_pmtiles_url": "https://s3.eu-west-2.amazonaws.com/ee.public.data/pmtiles-store/canterbury_603_34aaede56e403ef9b156a978a480e794.pmtiles",
          "ballots": [
            "local.canterbury.northgate.2027-05-06"
          ]
        }
      ]
    }
  ]
}

I kept it as a string because you can't mix data types in glue.Schema.map here (I think we ran into this last time as well).

I guess a using delimited string is better because we wouldn't need to decode the json twice?

[GeoWill]

I don't think the decode really matters - I was just worried there was going to be some quote mangling.

[awdem]

I've rebased this branch on to main to take in account changes I made in #54 that made dev a bit easier.

I think this need some kind of sanity checks on the data we produce at the end of this:

Every subdivided Division should be attached to exactly one Ballot ID. If any subdivided Division is attached to 0 or >1 ballots, throw
Every ballot ID we found should be attached to at least one subdivided Division. If we have any ballots "left over", throw

I've added map step to the pipeline that checks each that each division has only one ballot in 8a4c45e. The CI deploy has failed, because the state machine failed this check since the boundary reviews I've marked to be visible in EE haven't had their elections created yet so the divisions won't match to any ballots. It means that boundary reviews must have division sets and those divisions must have ballots for the pipeline to succeed. Obviously, we want to eventually include reviews before they have division boundaries, but if we're assuming that will be part of a different pipeline than this is OK.

One thing I'm wondering here, if we're assuming each division should only have one ballot, does the ballots field on a boundary change need to be an array?

I haven't added the second check you suggested because I'm no longer getting all the related ballots for a review. This is because I changed how I match ballots to divisions in b81772a to use the foreign key relationship like you suggested. Doing it that way meant I didn't need to find a division's ballot in the reviews related ballots anymore.

[awdem]

I had another thought on this. Where we are collecting up the ballots for each division, does this only look at approved elections (i.e: not suggested or soft-deleted)?

I think I've now taken this into account in both places where I need to in fc851f5 and 0d411e3

We can also pin elections as current outside of the time check. Do we want to respect that here so that we're consistent with what get_current() in EE does?

I've also done this in the first commit above.

[chris48s]

if we're assuming each division should only have one ballot, does the ballots field on a boundary change need to be an array?

When we did the Scotland review (Constituencies and Regions), how would we have modelled that?

• One review with two related ballots or
• Two reviews, each with one related ballot

?

edit: Actually, I might be asking the wrong question here.. I think for a given division we would expect a max of one but a single review might be connected to >1?

[chris48s]

I've added map step to the pipeline that checks each that each division has only one ballot..

Right, yes, of course.. I guess maybe we say 0 or 1 ballots is allowed, but not >1 ?

[chris48s]

can we include the division name or id in the message here

[awdem]

So because I've changed condition here to just check if we have any rows from the query in 13dead8, I can't include a name or id for a specific division. But, if it fails, you can go back a few steps in the state machine and see what the query returned to get the division ID(s) from there.

[chris48s]

Instead of going through every one here, can we do something like

WHERE division_ballot_count != 1;

(or in light of previous comment)

WHERE division_ballot_count > 1;

if that query gives us zero rows, we're good. If not go through them and fail

[awdem]

Right, so in the ominously named commit 13dead8 I've changed the SQL query to just return divisions with > 1 ballots and I've adjusted the quality check accordingly.

[awdem]

When we did the Scotland review (Constituencies and Regions), how would we have modelled that?

• One review with two related ballots or
• Two reviews, each with one related ballot

?

edit: Actually, I might be asking the wrong question here.. I think for a given division we would expect a max of one but a single review might be connected to >1?

In the case of a scottish postcode, we would have a review with two boundary change objects, one for each division type. And those boundary change objects would each have one ballot in the related_ballots list, an sp.c or an sp.r depending on which division type they're for.

So yeah, a single review would be connected to > 1 ballot. On further thought, I think it's fine to keep the related_ballots as an array, even if we'd only ever expect one ballot. It's more flexible and you never know what could pop up.

[chris48s]

OK. I reckon lets roll with this.