Skip to content

Conversation

@joaquimds
Copy link
Member

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug in the postcode geocoding functionality where Jersey postcodes (and potentially other postcodes) could return responses from the postcodes.io API with null values for postcode, latitude, or longitude fields, causing the application to crash or behave incorrectly instead of gracefully failing.

Changes:

  • Added additional validation checks to ensure postcode, latitude, and longitude fields are not null/falsy before processing
  • Added a test case with a Jersey postcode (JE3 2DF) to the test data file to validate the fix

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/server/mapping/geocode.ts Added null/falsy value checks for postcode, latitude, and longitude fields in postcodes.io API response validation
tests/resources/members.csv Added Notes column and Jersey postcode test case; also added notes to existing test cases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

George,G115RD,
Ringo,HP20 2QB,
Jane,BT15 3ES,Test Belfast
Mysterion,JE3 2DF, Test Jersey (currently does not geocode as is missing in the database and postcodes.io)
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra space after the comma before 'Test Jersey'. This should be 'JE3 2DF,Test Jersey' to be consistent with line 6 where 'BT15 3ES,Test Belfast' has no space after the comma.

Copilot uses AI. Check for mistakes.
Comment on lines +96 to +97
!postcodesData.result.latitude ||
!postcodesData.result.longitude
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks will fail for valid latitude/longitude values of 0. For example, postcodes on the Prime Meridian have longitude 0, and postcodes on the Equator have latitude 0. Consider using explicit null/undefined checks instead: postcodesData.result.latitude == null || postcodesData.result.longitude == null.

Suggested change
!postcodesData.result.latitude ||
!postcodesData.result.longitude
postcodesData.result.latitude == null ||
postcodesData.result.longitude == null

Copilot uses AI. Check for mistakes.
George,G115RD
Ringo,HP20 2QB
Jane,BT15 3ES
Name,Postcode,Notes
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test expectations in tests/feature/importDataSource.test.ts still expect only 5 records (lines 81-171), but adding a 7th row to members.csv means the test will now import 6 records. The test expectations need to be updated to handle the new Jersey postcode record, or the test should explicitly verify that the Jersey record is excluded due to geocoding failure.

Copilot uses AI. Check for mistakes.
@joaquimds joaquimds force-pushed the fix/jersey-postcodes branch from 5c815f6 to 4cd9427 Compare January 15, 2026 13:43
@joaquimds joaquimds merged commit 2c30498 into main Jan 15, 2026
1 check passed
@joaquimds joaquimds deleted the fix/jersey-postcodes branch January 15, 2026 15:03
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