-
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
feat: create ssn input component tckt-364 #386
Conversation
Terraform plan for tts-10x-atj-dev Plan: 0 to add, 2 to change, 0 to destroy.Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
!~ update in-place
Terraform will perform the following actions:
# cloudfoundry_app.tts-10x-atj-dev-server-doj_tts-10x-atj-dev-server-doj-app_380DB029 will be updated in-place
!~ resource "cloudfoundry_app" "tts-10x-atj-dev-server-doj_tts-10x-atj-dev-server-doj-app_380DB029" {
!~ docker_image = "ghcr.io/gsa-tts/forms/server-doj:c02d98dd965257d47b42daab60b10d5ea9b4d0a6" -> "ghcr.io/gsa-tts/forms/server-doj:557c301127d734a64e19c60da7fa51b74ea8d690"
id = "8a9fc8b6-af5e-45a2-abb6-2c24ecbcdfaa"
name = "tts-10x-atj-dev-server-doj-app"
# (17 unchanged attributes hidden)
# (3 unchanged blocks hidden)
}
# cloudfoundry_app.tts-10x-atj-dev-server-kansas_tts-10x-atj-dev-server-kansas-app_337A9CF1 will be updated in-place
!~ resource "cloudfoundry_app" "tts-10x-atj-dev-server-kansas_tts-10x-atj-dev-server-kansas-app_337A9CF1" {
!~ docker_image = "ghcr.io/gsa-tts/forms/server-kansas:c02d98dd965257d47b42daab60b10d5ea9b4d0a6" -> "ghcr.io/gsa-tts/forms/server-kansas:557c301127d734a64e19c60da7fa51b74ea8d690"
id = "e885e531-11b7-4906-9cc3-0ddf483868f5"
name = "tts-10x-atj-dev-server-kansas-app"
# (17 unchanged attributes hidden)
# (3 unchanged blocks hidden)
}
Plan: 0 to add, 2 to change, 0 to destroy. 📝 Plan generated in Post Terraform plan to PR comment #443 |
const isValidFormat = /^\d{3}-\d{2}-\d{4}$|^\d{9}$/.test(value); | ||
const stripped = value.replace(/[^0-9]/g, ''); | ||
return isValidFormat && stripped.length === 9; | ||
}, 'Social Security Number must contain exactly 9 digits and be formatted as XXX-XX-XXXX or XXXXXXXXX'); |
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.
This is a good start for validation. There's some additional guidance on validation rules that USWDS recommends checking for. Here is the link to the validation rules that the SSA provides. https://www.ssa.gov/kc/SSAFactSheet--IssuingSSNs.pdf. I think it would be good to capture these in a .refine
statement that's chained on to the rule you have here.
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.
Thank you for sharing the pdf! I will definitely update the validation hook.
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.
Cool. Thank you. I'll review the rest of the changes. I just gave it a quick look earlier today when I left my previous comment.
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.
I'm impressed with how quickly you're picking this code base up and moving through all of these stories! I gave this a good look through and have some suggestions in here about validation and general comments.
I'm also curious if you've seen the suggestion on this page: https://designsystem.digital.gov/patterns/create-a-user-profile/social-security-number/ where it says "Consider using an input mask." You don't have to use one for SSN, but I'm wondering what your opinion is on what you think would offer a better experience.
htmlFor={ssnId} | ||
> | ||
{label || 'Social Security Number'} | ||
{required && <span className="required-indicator">*</span>} |
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.
I just opened a bug for this to address in a different sprint because it's a larger issue we have in various places in the application, but if we're doing the asterisk to indicate a required field, let's add title="required"
to the abbr
tag.
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.
This is the bug I opened for a future sprint for reference: #388
type="text" | ||
defaultValue={value} | ||
{...register(ssnId, { required })} | ||
aria-describedby={error ? `${ssnId} ${errorId}` : ssnId} |
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.
Nice work adding the aria-describedby
attribute to this field. I think we can further enhance it with the following logic, and I notice that it should probably be hint-${ssnId}
instead of ssnId
:
error && hint
->hint-${ssnId} ${errorId}
error
->${errorId}
hint
->hint-${ssnId} ${errorId}
!error && !hint
-> omit thearia-describedby
attribute from the element
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.
Thank you! I am just curious to know why do you think it should be hint? 🤔 when I used ssnId
I referred to the input Id on line 40. Also hint doesn't get an error. Error message is separate from the hint message. Please let me know if I missing something here.
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.
Great question! When you add aria-describedby
, that is saying that there is an additional element somewhere else on the page that provides information about the element where the aria-describedby
appears. The element with error on lines 31-35 describes why the input is invalid, and the element with the hint on line 26-30 describes what type of information we're looking for in the input.
It wouldn't be ssnId
on line 40 because that's the same element that has the aria-describedby
attribute and like saying that the input describes itself.
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.
Thank you for the clarification, got it now! Updated the code accordingly.
const hasValidSuffix = stripped.slice(5) !== '0000'; | ||
|
||
return hasValidPrefix && hasValidMiddle && hasValidSuffix; | ||
}, 'Social Security Number must contain exactly 9 digits, be formatted as XXX-XX-XXXX or XXXXXXXXX, and meet SSA issuance criteria'); |
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.
We might be able to validate this in user testing, but it may be better to break up the checks here to provide a more targeted error message.
Zod allows you to chain validation rules with its refine
function, and it'll return the error message of the first rule that returns false
. You could do something like:
z.string()
.transform(value => // return the cleaned string)
.refine(value => !(data.required && value.length === 0), 'This field is required')
.refine(value => // check for length, 'Error message for the correct amount of characters')
.refine(value => // check for valid pattern, 'Error message for invalid number')
I'd encourage the use of the transform
function especially because that will help normalize how the data is stored.
You would also be able to shed the .or
operator in the line below if you did it this way.
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.
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.
Also when I have multiple .refines() I am getting type errors for nesting:
Type 'ZodEffects<ZodEffects<ZodEffects<ZodEffects<ZodEffects<ZodEffects<ZodString, string, string>, string, string>, string, string>, string, string>, string, string>, string, string>' is not assignable to type 'ZodEffects<ZodEffects<ZodEffects<ZodEffects<ZodEffects<ZodString, string, string>, string, string>, string, string>, string, string>, string, string>'.
Type 'ZodEffects<ZodEffects<ZodEffects<ZodEffects<ZodEffects<ZodString, string, string>, string, string>, string, string>, string, string>, string, string>' is not assignable to type 'ZodEffects<ZodEffects<ZodEffects<ZodEffects<ZodString, string, string>, string, string>, string, string>, string, string>'.
Type 'ZodEffects<ZodEffects<ZodEffects<ZodEffects<ZodString, string, string>, string, string>, string, string>, string, string>' is not assignable to type 'ZodEffects<ZodEffects<ZodEffects<ZodString, string, string>, string, string>, string, string>'.
Type 'ZodEffects<ZodEffects<ZodEffects<ZodString, string, string>, string, string>, string, string>' is not assignable to type 'ZodEffects<ZodEffects<ZodString, string, string>, string, string>'.
Type 'ZodEffects<ZodEffects<ZodString, string, string>, string, string>' is not assignable to type 'ZodEffects<ZodString, string, string>'.
Type 'ZodEffects<ZodString, string, string>' is missing the following properties from type 'ZodString': _regex, _addCheck, email, url, and 39 more.ts(2322)
let schema: z.ZodEffects<z.ZodEffects<z.ZodEffects<z.ZodEffects<z.ZodEffects<z.ZodString, string, string>, string, string>, string, string>, string, string>, 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.
I can refactor the code block to:
export const createSSNSchema = (data: SocialSecurityNumberPattern['data']) => {
const baseSchema = z.string()
.transform(value => value.replace(/[^0-9]/g, '')) // Normalize the SSN by removing non-numeric characters
.superRefine((value, ctx) => {
let issues = [];
if (value.length !== 9) {
issues.push('have exactly 9 digits');
} else {
if (value.startsWith('9') || value.startsWith('666') || value.startsWith('000')) {
issues.push('start with a valid prefix (not 9, 666, or 000)');
}
if (value.slice(3, 5) === '00') {
issues.push('have a valid middle segment (not 00)');
}
if (value.slice(5) === '0000') {
issues.push('have a valid suffix (not 0000)');
}
}
if (issues.length > 0) {
let enhancedMessage = 'Social Security Number must ';
if (issues.length === 1) {
enhancedMessage += issues[0];
} else if (issues.length === 2) {
enhancedMessage += `${issues[0]} and ${issues[1]}`;
} else {
enhancedMessage += `${issues.slice(0, -1).join(', ')}, and ${issues[issues.length - 1]}`;
}
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: enhancedMessage,
});
}
});
if (data.required) {
return z.string()
.refine(value => value.trim().length > 0, {
message: 'This field is required',
})
.superRefine((value, ctx) => {
const result = baseSchema.safeParse(value.trim());
if (!result.success) {
result.error.issues.forEach(issue =>
ctx.addIssue(issue)
);
}
});
} else {
return baseSchema.optional();
}
};
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.
Thanks for taking a look at this. My concern before was that the error was complex with all the criteria that a valid SSN had to meet. I think for now, we can call this good enough for our current epic, but we might want to get some validation on what's the right balance of helping the user figure out why their input is invalid vs. cognitive load.
I think the version you have with .superRefine
is good for now. If you wouldn't mind, can you create a design issue for the next epic so we can get some testing on the language?
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.
Created the ticket: #389
@ethangardner I actually tried it before and for some reason I removed it later. I don't remember exactly what was the issue. However, I am glad you reminded it to me. I will apply it again and try one more time as I already have a working code now. 🙂 |
da9ddb7
to
5930c69
Compare
@ethangardner as of now we have masking for the SSN input! :) Just pushed the code up. |
6ef7fee
to
c3d3745
Compare
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.
Thanks for addressing the suggestions. This looks good to me.
Created Social Security Number Pattern for Forms Users.
Created Social Security Number Pattern Edit for Form Builders.
Created Social Security Number input and config validations.
Added unit tests for Social Security Number config file.