Closed chris48s closed 4 days ago
This PR is probably not sensible to try and review commit-by-commit as there are multiple commits where I merged in master and resolved a merge conflict. Probably easier to just look at the whole diff for this one. I'll probably just squash-merge this one to tidy up.
I reckon this is also one for @pmk01 too.
The code looks fine, but I think we should remove registration details from the polling station section in all cases. In fact, I'm not really sure it's useful to have an address there at all: if you don't have a poll card, you don't want to be sending the council a postcard about it.
The postcodes are duplicated in the image above, I assume that's an issue with the test data you're using?
I remove the
<p>
tag, it messes up the display
How is it messed up?
The postcodes are duplicated in the image above, I assume that's an issue with the test data you're using?
Yeah the duplicated postcode is from the source data:
{
"council_id": "EAY",
"name": "East Ayrshire Council",
"email": "electionoffice@east-ayrshire.gov.uk",
"phone": "01563 576 555",
"website": "https://www.east-ayrshire.gov.uk",
"postcode": "KA3 7BU",
"address": "London Road\r\nKilmarnock\r\nEast Ayrshire\r\nKA3 7BU",
"identifiers": [
"S12000008"
],
"nation": "Scotland"
}
but that is our real data for that council. It is not test data.
How is it messed up?
It is just missing spacing.
in a <p>
:
not in a <p>
:
It is probably an easy enough CSS fix, but I guess its something where we want to:
which is why I was going to leave it how it is in this PR.
It is probably an easy enough CSS fix
I think you can solve this by making the parent details
a "stack".
But I also think you're right that address
should be included in the default margin rule outlined on that page. I'll make an issue.
I agree with Sym re. addresses. TBH I suspect 99.9% of people will only need the address for postal ballots. But then maybe more people send letters for this sort of thing than I suspected.
OK, so to summarise those 2 bits of feedback.. what you want is:
In the "where to vote" (polling station) section, remove registration details. Just show council electoral services
In the "register to vote" section, continue to show both registration details AND council electoral services if both are present?
In both sections (both "where to vote" and "register to vote"), remove postal address and just show website, email and phone number?
I think we keep both address (when needed) in the "register to vote" section. No addresses in the "where to vote", and only ever council contact details.
Don't forget EONI is a special case here too!
In the data, we just return an object like
"electoral_services": {
// all this is the same for every postcode in NI
"name": "The Electoral Office for Northern Ireland",
"email": "info@eoni.org.uk",
"phone": "02890 446680",
"website": "http://www.eoni.org.uk/",
"postcode": "BT1 1ER",
"address": "The Electoral Office Headquarters\r\nSt Anne's House\r\n15 Church Street\r\nBelfast",
"nation": "Northern Ireland",
// this is actually the Council ID. Ards & North Down in this case
"council_id": "AND",
"identifiers": [
"N09000011"
]
}
in the response for all NI postcodes. In client applications we can just render the contact details as we would for any other "council" (even though it is not a council).
I guess the one thing I need to make sure of is that we're not saying
Contact your council: The Electoral Office for Northern Ireland ...
Anything else I am missing here??
I think that's about it. We have some of this in WDIV:
OK, cool. Ta.
Will do another edit of this taking all that into account.
OK. I've pushed another commit implementing those suggestions 55d19bb
This now looks like
where you've got just one set of contact details
and
where you've got two.
In terms of Northern Ireland, nothing I've done in this PR breaks the existing code for showing the user "contact your council" vs "contact the Electoral Office for Northern Ireland" on the polling station card. That stands as it was.
The other place where we say "contact your council" is on the registration card. This one was previously hard-coded to "contact your council" before this PR, but I've fixed that in 2a50354 too since we're talking about it.
This is looking good. The only thing for me is that I'm a bit confused by the two addresses. On the EC pages we explain what they are:
https://github.com/DemocracyClub/ec-postcode-lookup-pages/blob/6fdc644a9243b983a48875ff984be5e293591a69/postcode_lookup/templates/includes/organisation_contact.html#L2. Are you able to add sub-headings to them?
Here's the latest version:
Bah. I just realised I should have squash-merged that after I pressed merge :(
Quick reminder that none of this will actually take effect until we do https://app.asana.com/0/1204880927741389/1208629276538089/f but once we do that, these changes will become visible.
This PR requires unravelling several layers of bugs.
First problem I hit was: In production, we don't show any council contact details ever because every API call we make returns
null
in place of theelectoral_services
and registration objects. We do need to fix that, but it is not the point of this PR. I've captured that in https://app.asana.com/0/1204880927741389/1208629276538089/fWhen we fix that, the next problem we'll hit is that once your API calls are actually returning some contact details, you get a lot of this sort of nonsense:
This is because WCIVF does not pass a
registration
object to the template layer. So all this code in the templates looking at aregistration
object is always looking at aNone
(and we'll always be silently comparingcouncil.address
toNone
because django templates). I don't think this code ever worked. It looks like it works on the dummy view because you've faked the data https://github.com/DemocracyClub/WhoCanIVoteFor/blob/1a8bd2d4c9854b3094c07b02180ed6d74724f20d/wcivf/apps/elections/views/postcode_view.py#L434-L441 but I don't think we ever actually passed aregistration
object from the API to the template layer. LolzSo the next thing to do is fix that: https://github.com/DemocracyClub/WhoCanIVoteFor/pull/2072/commits/258d993c9148eb812c66d1e01e4fb72cfe6cf05e
Then we also have a perfectly good
_council_contact_details.html
template, which we're using in some places but not others. So lets use that consistently everywhere to DRY up a bit and present them the same way everywhere...and then finally, I've made it so everything does the following:
If we're talking about polling stations:
If we're talking about registration:
..while guarding against one or both of those things possibly being None.
So then that's where the code is now. I think that does implement what was originally intended (and presumably requested) by this PR https://github.com/DemocracyClub/WhoCanIVoteFor/pull/1867 as it was written. However this does lead to a bit of a silly situation where we do have both sets of contact info and it is before the registration deadline:
Yo dawg. I heard you like addresses.
..so I wonder if that is what we really want? Personally I'd be inclined to just present one set of details in each context, but the trail runs cold at https://app.asana.com/0/1204880927741389/1207218844713564/f - there's not really any background on the reasoning there. Maybe you can fill me in.
There is one further issue here (in
_council_contact_details.html
), which is that<address>
is not a valid child of<p>
but if I remove the<p>
tag, it messes up the display so I've just ignored that for now. I reckon for the moment the invalid HTML is the lesser evil here. This feels like something we should fix too though.