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

fix: Ensure input postcode is always parsed #229

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Jan 7, 2025

What's the problem?

Whilst working on #228 I noticed that postcode district, area, and sector were undefined.

This is due to a regression introduced here - #65

Without using the calculationSchema the input postcode isn't parsed into its constituent parts, allowing queries to fail.

This happens due to this behaviour - https://www.prisma.io/docs/orm/prisma-client/special-fields-and-types/null-and-undefined#null-and-undefined-in-queries-that-affect-many-records

The returned itl3 is always the same one, TLH23.

Also - the frontend is not correctly validating postcodes, it's currently possible to pass an invalid postcode to the API.

What's the solution?

Revert change to always parse input via the calculation schema.

image

Copy link

vercel bot commented Jan 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fairhold-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 8, 2025 11:26am

@DafyddLlyr DafyddLlyr marked this pull request as draft January 7, 2025 15:55
@DafyddLlyr DafyddLlyr requested a review from a team January 7, 2025 16:24
@DafyddLlyr DafyddLlyr marked this pull request as ready for review January 7, 2025 16:24
@DafyddLlyr DafyddLlyr marked this pull request as draft January 7, 2025 16:26
} else {
input = data;
}
const input = calculationSchema.parse(data);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main change in this PR - falling back to parsing the input.

@@ -13,8 +13,7 @@ const getItl3ByPostcodeDistrict = async (
},
});

// Cast to string as 'not: null' clause in Prisma query does not type narrow
return itl3 as string;
return itl3;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small unrelated tidy up sorry - this column in now non-nullable.

@DafyddLlyr DafyddLlyr force-pushed the dp/parse-input-postcode branch from f877033 to 5c9fc79 Compare January 7, 2025 17:02
houseType: HouseTypeEnum.refine(
(value) => HouseTypeEnum.options.includes(value),
{
message: `houseType is required and must be one of ${HOUSE_TYPES}`,
message: `House type is required`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error messages here are using facing and more a bit more user-friendly.

Copy link
Collaborator

@gabrielegranello gabrielegranello left a comment

Choose a reason for hiding this comment

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

Thanks @DafyddLlyr !
Can I quickly double check if I get the logic right here?
We have two schemas: calculationSchema and formSchema.
The main difference is that:

  1. formSchema checks if the the string is a valid postcode with the following:
housePostcode: z
    .string()
    .min(1, "Postcode is required")
    .refine(isValidPostcode, "Invalid postcode"),

We are using this for form validation.
( side note, it is interesting to me that we can check if a postcode is valid without transforming the string into a postcode object, great thing)

  1. calculationSchema:
    it checks the string to be a valid postcode AND it transforms it into a postcode object
housePostcode: z
   .string()
   .min(1, "housePostcode is required")
   .refine(isValidPostcode, "Invalid postcode")
   .transform(parsePostcode)
   .refine((postcode): postcode is ValidPostcode => postcode.valid), 

we are using this schema to ALWAYS parse the data before sending it to the API. Meaning to the API we are passing always a postcode object.

Question:
what does this extra final line do?
.refine((postcode): postcode is ValidPostcode => postcode.valid),

Is it like a double check that after transforming a valid string into an object, it is still a valid postcode?

@gabrielegranello
Copy link
Collaborator

Oh side note. I can see a third schema called apiSchema in the folder. I don't think we are using it in route.ts so I tried deleting it. Is it perhaps an old file that stuck around?

I am scary to say the words ' everything seems working without it', bit it actually does

@DafyddLlyr
Copy link
Contributor Author

@gabrielegranello Yep that's correct - we always want to parse in the backend as it could technically be accessed from anywhere. We also need the postcode object here so we're doing this transformation as part of the schema.

This line is a type predicate (docs) which narrows down the type for us (the library isn't doing this automatically).

.refine((postcode): postcode is ValidPostcode => postcode.valid), 

@DafyddLlyr DafyddLlyr merged commit 16500af into main Jan 8, 2025
5 checks passed
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