-
Notifications
You must be signed in to change notification settings - Fork 7
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(ui): enhance and refactor validation across UI components #1956
Conversation
61834f6
to
227ac24
Compare
227ac24
to
1c834a4
Compare
webapp/src/components/App/Singlestudy/HomeView/InformationView/CreateVariantDialog.tsx
Show resolved
Hide resolved
webapp/src/components/App/Singlestudy/explore/Modelization/Map/CreateAreaDialog.tsx
Show resolved
Hide resolved
webapp/src/components/App/Singlestudy/explore/TableModeList/dialogs/TableTemplateFormDialog.tsx
Show resolved
Hide resolved
...c/components/App/Singlestudy/explore/Modelization/Map/MapConfig/Layers/UpdateLayerDialog.tsx
Outdated
Show resolved
Hide resolved
1c834a4
to
516e19d
Compare
516e19d
to
0e539eb
Compare
cf2a965
to
9ad92b6
Compare
webapp/src/components/App/Settings/Groups/dialog/GroupFormDialog/GroupForm.tsx
Outdated
Show resolved
Hide resolved
value: PASSWORD_MIN_LENGTH, | ||
message: t("form.field.minLength", { 0: PASSWORD_MIN_LENGTH }), | ||
}, | ||
validate: (v) => validatePassword(v) || undefined, |
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.
Remove || undefined
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.
didn't notice the state is managed by react hook form, thanks !
webapp/src/components/App/Settings/Users/dialog/UserFormDialog/UserForm.tsx
Outdated
Show resolved
Hide resolved
webapp/src/utils/validationUtils.ts
Outdated
//////////////////////////////////////////////////////////////// | ||
|
||
// Function to escape special characters in allowedChars | ||
const escapeSpecialChars = (chars: 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.
function
please
webapp/src/utils/validationUtils.ts
Outdated
|
||
// Compiles a regex pattern based on allowed characters and flags. | ||
const allowedCharsPattern = new RegExp( | ||
generatePattern(allowSpaces, allowSpecialChars, allowedChars), |
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.
allowSpaces
is checked twice
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.
generatePattern
function, conditionally includes spaces in the allowed character set. This does not directly check if the input contains spaces but rather allows them in the validation process if allowSpaces
is true.
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.
e.g. you can have a case where some special chars are needed but spaces forbidden, you need to make sure the validation regex don't allow spaces too
validateString(v, {
existingValues: existingCandidates,
allowSpaces: false,
allowedChars: "&_*",
}),
webapp/src/utils/validationUtils.ts
Outdated
* @param options.existingValues - An array of strings to check against for duplicates. Comparison is case-insensitive by default. | ||
* @param options.excludedValues - An array of strings that the value should not match. | ||
* @param options.isCaseSensitive - Whether the comparison with `existingValues` and `excludedValues` is case-sensitive. Defaults to false. | ||
* @param options.allowSpecialChars - Flags if special characters are permitted in the value. |
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.
allowSpecialChars
and allowedChars
can be fusioned:
allowSpecialChars?: boolean | string
If true, all special chars are allowed, if string, only the specified ones.
allowedChars
name is confused.
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.
Don't know about this, 🤔 it's confusing to have one parameter handle 2 data types and use cases in my opinion.
While combining parameters might seem like a simplification, it can actually complicate usage and understanding, specific parameters can help separate concerns and have more granular control.
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.
It is common to have parameters with mutilple types.
allowedChars
is not clear because letters and digit are chars too.
And it's dependant of allowSpecialChars
value.
validateString(v, {
allowSpecialChars: true,
allowedChars: "&_*",
})
Becomes
validateString(v, { allowSpecialChars: "&_*" })
validateString(v, {
allowSpecialChars: true,
allowedChars: "&?*()'/.................", // add all special chars possible
})
Becomes
validateString(v, { allowSpecialChars: true }) // all special chars possible
validateString(v, {
allowSpecialChars: false,
allowedChars: "&?", // ignored
})
Becomes
validateString(v, { allowSpecialChars: false })
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.
your example is not accurate it don't account default values allowSpecialChars: true
is by default true allowing alphanum chars + default special chars set to "&()_-"
.
In general you don't need to allow all special characters this as no sense for validation purposes, where the goal is to restrict user inputs.
it's true that allowedChars
name can be confusing thought.
I will think about this change if this not complicates the actual implementation it could simplify the special chars management 🤔
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.
Edit: the allowedChars
parameter is now renamed specialChars
for more clarity as you suggested, thanks.
I tried to merge the flag allowSpecialChars
and specialChars
parameter, but this change doesn't align with the original design, I don't want the possibility to allow all special characters for obvious raison, this is against the concept of validation, and can cause potential security risks.
So for now we have 2 possible cases:
- 1 Special chars blocked:
validateString(v, { allowSpecialChars: false })
OR
- 2 Only a specific set of special chars are allowed (by default
"&()_-"
to avoid repeating this common pattern all around):
validateString(v) // will allow only `"&()_-"` as special chars + alphanumeric pattern
and that can be adjusted as needed, e.g. for an email input:
validateString(v, { specialChars: "@" }) // will allow only `"@"` as special char + alphanumeric pattern
Note that we could pass an empty string to disable
specialChars
, and removeallowSpecialChars
but it's confusing to me, a dedicated flag to separate concerns is better in my opinion
What do you think about that @skamril ?
…l backtracking issues
9ad92b6
to
d354916
Compare
d354916
to
c59d810
Compare
Summary
This pull request introduces a comprehensive set of validation utilities aimed at enhancing data integrity and security across our application. By implementing these utilities, we are centralizing our validation logic, thereby ensuring consistency and maintainability.
This PR not only adds new validation functions for strings and passwords but also refactors various sections of our codebase to utilize these new utilities.
Features
validateString
utility offers extensive validation capabilities, including checks for minimum and maximum length, allowance for special characters and spaces, and the option for case-sensitive comparison. It's designed to be flexible, accommodating a wide range of validation scenarios.validatePassword
utility, we enforce strong security criteria for passwords, including requirements for lowercase and uppercase letters, digits, and special characters, along with configurable minimum and maximum length checks.Impacts
Table Mode: The creation and modification of Tables are now more robust with the integration of the new validation utilities.