-
Notifications
You must be signed in to change notification settings - Fork 1
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
chore(site launch form): trim site launch form input #698
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, is defensive trimming something we should have as a default on all our methods which use formsg input? I'm thinking it might be better to trim from getField
in formsg-utils
instead
@alexanderleegs oh hey this is fair! |
src/utils/formsg-utils.ts
Outdated
function trimAllStrings( | ||
responseArray: string[] | string[][] | ||
): string[] | string[][] { | ||
if (Array.isArray(responseArray)) { | ||
responseArray.map((item) => { | ||
if (Array.isArray(item)) { | ||
return trimAllStrings(item) | ||
} | ||
if (typeof item === "string") { | ||
return item.trim() | ||
} | ||
return item | ||
}) | ||
return responseArray | ||
} | ||
return responseArray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on reducing the checks here, given that responseArray is string[] | string[][]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS complains if we dont have this because it is not able to infer that
if (Array.isArray(responseArray)), then responseArray is of type []. I have added in a comment for clarity here:
08433bb
src/utils/formsg-utils.ts
Outdated
let answers = response?.answerArray | ||
if (answers) { | ||
answers = trimAllStrings(answers) | ||
} | ||
return response?.answerArray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking - does this modify response
? I think it might be better to rewrite it so it's clearly regardless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, this is a bug, fixed here 08433bb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Problem
The form expects string values. When typing, it is possible for someone to enter a space in the string
Solution
Defensively trim all inputs