-
Notifications
You must be signed in to change notification settings - Fork 346
Bug6451 email without top domain#6781
Conversation
Summary
Validate email addresses to ensure they contain a top domain and that the top domain is at least 2 characters long
Work Item(s)
Fixes #6451
duichwer
left a 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.
Could you add some tests with error assertions?
src/System Application/App/Email/src/Account/EmailAccountImpl.Codeunit.al
Show resolved
Hide resolved
I can't find any tests of those functions, so I'm not sure where to put them. And I'm also not sure how to write them to not risk failure with .Net-calls in different environments and similar. |
@Dan-Karlsson I think you're correct. There are some tests in the Base Application for the procedure ');
// Check that only one address is validated.
if EmailAddress.Split('@').Count() <> 2 then
Error(InvalidEmailAddressErr, EmailAddress);
if not EmailAccount.ValidateEmailAddress(EmailAddress) then
Error(InvalidEmailAddressErr, EmailAddress);
end;"> [TryFunction]
procedure CheckValidEmailAddress(EmailAddress: Text) var EmailAccount: Codeunit "Email Account"; IsHandled: Boolean; begin IsHandled := false; OnBeforeCheckValidEmailAddr(EmailAddress, IsHandled); if IsHandled then exit; EmailAddress := DelChr(EmailAddress, '<>'); // Check that only one address is validated. if EmailAddress.Split('@').Count() <> 2 then Error(InvalidEmailAddressErr, EmailAddress); if not EmailAccount.ValidateEmailAddress(EmailAddress) then Error(InvalidEmailAddressErr, EmailAddress); end; Therefore I would suggest to some tests to https://github.com/microsoft/BCApps/blob/main/src/System%20Application/Test/Email/src/EmailAccountsTest.Codeunit.al. |
Thank you! I didn't think to check in Base Application. But I think the tests should be in Base Application, with added tests in the procedure procedure TestInvalidEmailAddress()
var MailManagement: Codeunit "Mail Management"; begin asserterror MailManagement.CheckValidEmailAddress('@a'); asserterror MailManagement.CheckValidEmailAddress('b@'); asserterror MailManagement.CheckValidEmailAddress('ab)@c.d'); asserterror MailManagement.CheckValidEmailAddress('a@@b'); end; But I don't know how to add it there (without creating a new bug report in Base and a separate pull request for the tests, that won't work until this is approved). |
|
You can put the tests in https://github.com/microsoft/BCApps/blob/main/src/System%20Application/Test/Email/src/EmailAccountsTest.Codeunit.al. I've tested the change myself and it works. Once the tests are in, I'll approve the PR. |
Thank you! I have added tests now. But I still think they should be merged with the tests in https://github.com/microsoft/BusinessCentralApps/blob/main/App/Layers/W1/Tests/Integration-Internal/MailManagementTest.Codeunit.al |