Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use senedd.c not senedd #706

Closed
wants to merge 5 commits into from
Closed

Conversation

VirginiaDooley
Copy link
Contributor

No description provided.

@VirginiaDooley VirginiaDooley marked this pull request as ready for review June 20, 2024 18:22
Copy link

This PR has been deployed.

@chris48s
Copy link
Member

I am surprised that the should render the ballot information for a constituency test doesn't fail with this change. Note the ballot_paper_id value.

it('should render the ballot information for a constituency', () => {
const ballot = {
ballot_paper_id: 'senedd.2022-05-06',
cancelled: false,
election_name: 'Welsh Election',
post_name: 'Cardiff Central',
candidates: ['Candidate 1', 'Candidate 2'],
candidates_verified: true,
};

@VirginiaDooley
Copy link
Contributor Author

I am surprised that the should render the ballot information for a constituency test doesn't fail with this change. Note the ballot_paper_id value.

it('should render the ballot information for a constituency', () => {
const ballot = {
ballot_paper_id: 'senedd.2022-05-06',
cancelled: false,
election_name: 'Welsh Election',
post_name: 'Cardiff Central',
candidates: ['Candidate 1', 'Candidate 2'],
candidates_verified: true,
};

Yikes, this fails locally but isn't caught in CI.

@chris48s
Copy link
Member

Just having a quick look at your CI log, it outputs

PASS src/tests/unit/DemocracyClubAPIHandler.test.js
PASS src/tests/unit/SecondaryComponents.test.js
PASS src/tests/unit/PollingDate.test.js
PASS src/tests/unit/TranslationCheck.test.js
PASS src/tests/integration/ElectionInformationWidget.test.js

Test Suites: 5 passed, 5 total
Tests:       66 passed, 66 total
Snapshots:   0 total
Time:        5.287s
Ran all test suites matching /a/i.

I reckon

https://github.com/DemocracyClub/WhereDoIVote-Widget/blob/master/.github/workflows/stage_deploy.yml#L27

is applying a filter that is not matching all tests.

I think changing that to npm test in the GH workflow should run all your tests but I've not checked it..

@VirginiaDooley
Copy link
Contributor Author

I think the issue here is how we are rendering props that are structured as a dict. I've confirmed that the props in question do in fact exist and are accessible in the relevant component. I suspect this effects several areas of the app.

@chris48s
Copy link
Member

chris48s commented Jun 23, 2024

The code itself is actually fine here. Fundamentally, the problem is that the ballot object you're constructing in your

  • 'should render the ballot information for a constituency' and
  • 'should render the ballot information for a region'

tests is malformed.

  1. The app is expecting the ballot object to have a voting_system object with a uses_party_lists boolean. e.g:
"voting_system": {
  "slug": "FPTP",
  "name": "First-past-the-post",
  "uses_party_lists": false
}

It is throwing

TypeError: Cannot read properties of undefined (reading 'uses_party_lists')

under test because your test object doesn't define one. Implicitly voting_system == undefined

You're also passing candidates as a list of strings, but in reality this would be a list of candidate objects e.g:

{
  "party": {
    "party_name": "UK Independence Party (UKIP)",
    "party_id": "party:85"
  },
  "list_position": null,
  "person": {
    "ynr_id": 39510,
    "name": "Alun Elder-Brown",
    "absolute_url": "https://whocanivotefor.co.uk/person/39510/alun-elder-brown"
  }
}

which will also throw an error once you get past the voting system issue. Remember internally the app is just passing around bits of an API response from dev.DC e.g:

https://github.com/DemocracyClub/WhereDoIVote-Widget/blob/master/public/example-responses/address-10000066465.json#L17-L46

I have submitted a PR at #707 which

  • runs all the tests everywhere under CI
  • fixes the ballot objects
  • fixes this issue (senedd.c)
  • uses the 'Constituency' suffix in all the places it applies

and requested review. Suggest closing this one.

@VirginiaDooley
Copy link
Contributor Author

Fixed with #707

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants