Allow using username and email in SMTP configuration
#3398
Conversation
samaradel
requested review from
AhmedHanafy725,
zaelgohary,
maayarosama,
MohamedElmdary,
Mahmoud-Emad,
amiraabouhadid,
AlaaElattar,
0oM4R and
mohamedamer453
as code owners
samaradel
changed the title
username or email in SMTP configuration
samaradel
changed the title
username or email in SMTP configurationusername and email in SMTP configuration
Co-authored-by: Zainab Elgohary <[email protected]>
Co-authored-by: Zainab Elgohary <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
please mention the idea behind the validation rules, 69 characters, and the regex of the username |
Co-authored-by: Omar Kassem <[email protected]>
|
I can't test the solutions, #3449 and discourse not responding for more than one hour |
|
@0oM4R you should test the field validation to see if it accepts email or username in the discourse without problems |
There was a problem hiding this comment.
i think we need to include those changes in manual Please open an issue for it in https://github.com/threefoldtech/info_grid
| }, | ||
| validators.required('Email or Username is required.'), | ||
| (v: string) => { | ||
| return validators.isValidSmtp(v); |
There was a problem hiding this comment.
as we accepts a username now i think we should add min char validation to be at least two chars
There was a problem hiding this comment.
@AhmedHanafy725 agree?
There was a problem hiding this comment.
I discussed it with him earlier today, you can go a head :D
I see all mail server apps in the manual don't mention anything about validation, it is just about filling fields, I don't think it's necessary |
what i mean is to mention that we accepts username not only email, |
|
@0oM4R you mean the |
- Add space checker in validator function - Add unit test for smtp validation function
| * @returns {{ message: string }} - An object with an error message if the input is invalid, or undefined if the input is valid. | ||
| */ | ||
|
|
||
| export function isValidSmtp(input: string) { |
There was a problem hiding this comment.
1- where is the return type of the function?
2-if the function is named isX is expect it to return a boolean. True if X and false if not. To return null that may denote a failure e.g failure to allocate memory, or no data available in such context.
So we have multiple options
1- return a boolean (and discard the error messages, keep it constant to invalid smtp information)
2- return a boolean with a context, e.g boolean, null in case of success and boolean and error string in case of failure so [boolean, string|null] 3- return a boolean (success) or an error message (failure)boolean | string`
4- return a new type e.g Result that encapsulates the success/failure status, and message/error fields
type Result<T> = {
success: true;
value: T;
} | {
success: false;
error: string;
};
|
|
||
| import { isValidSmtp } from "../../src/utils/validators"; | ||
|
|
||
| describe("isValidSmtp", () => { |
There was a problem hiding this comment.
Probably need more cases e.g empty string, multiple invalid forms of email as well, usage of special characters
| /** | ||
| * Validates an SMTP input string. | ||
| * | ||
| * Checks if the input string is a valid email address and does not contain special characters. |
There was a problem hiding this comment.
Is it only email? if so change the input parameter to email
| export function isValidSmtp(input: string) { | ||
| const emailValidation = isEmail("Please provide a valid email address.")(input); | ||
| const username = /[!@#$%^&*()\s_+\-={}:<>?,./]/.test(input); | ||
| if (username && emailValidation) { |
There was a problem hiding this comment.
I don't understand this condition, does that mean if it's email and a username (doesn't have special characters) then it's true?
shouldn't it be like that
const isValidEmail = isEmail("Please provide a valid email address.")(input);
const hasSpecialCharacters = /[!@#$%^&*()\s_+\-={}:<>?,./]/.test(input);
if (hasSpecialCharacters && isValidEmail) {
return { message: "Please provide a valid username or email" };
}
Which I still don't understand.
|
|
||
| export function isValidSmtp(input: string) { | ||
| const emailValidation = isEmail("Please provide a valid email address.")(input); | ||
| const username = /[!@#$%^&*()\s_+\-={}:<>?,./]/.test(input); |
There was a problem hiding this comment.
This could have been simplified instead of matching against special characters you could have just made it match against valid characters which are a-z[A-Z0-9]+
There was a problem hiding this comment.
This could have been simplified instead of
matching against special charactersyou could have just made itmatch against valid characterswhich area-z[A-Z0-9]+
Please note this is the right way, given there's a very long of list of unhandled utf8 input, that's why you should state the allowed ones as long as you can iterate them, instead of chasing a very long list of symbols.
| import { isValidSmtp } from "../../src/utils/validators"; | ||
|
|
||
| describe("isValidSmtp", () => { | ||
| it("returns nothing for valid username and email", () => { |
There was a problem hiding this comment.
I truly don't understand can you point out the username and the email because there's only just one input name input which looks like an email.
| }); | ||
|
|
||
| it("returns an error message for special characters in username", () => { | ||
| const input = "invalid_email"; |
There was a problem hiding this comment.
_ is not the only special character
| }); | ||
|
|
||
| it("returns an error message for white spaces in username", () => { | ||
| const input = "invalid username"; |
There was a problem hiding this comment.
there're more types of spaces beyond " " there's is \t and \v
Description
Remove
isDiscoursecheck and apply adding email or username for all apps with SMTP.Create a function to validate applications with SMTP.
validate the email input to accept username too
limit the password length to 69 chars as per
sendgriddocumentationChanges
Related Issues
Tested Scenarios
Checklist