-
Notifications
You must be signed in to change notification settings - Fork 2k
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(web): send test email button #10011
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.
Where should we add the endpoint for this? Maybe in the jobs controller?
@HttpCode(HttpStatus.NO_CONTENT) | ||
@Authenticated() | ||
testEmailNotification(@Auth() auth: AuthDto) { | ||
return this.service.handleTestEmailSetup({ id: auth.user.id }); |
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.
Feedback from the web notification is pretty bad. It tells you a test email has been sent, even though the smtp config is empty.
Why isn't NotificationRepository.sendEmail
called directly? Then any errors can be relayed to the user.
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 am reworking to invoke those repository method directly
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.
That's much better already. However, I still think there's room for improvement. When an smtp error like 530 5.7.0 User not authenticated
occurs, it would be beneficial to show this error to troubleshoot the settings.
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 will be a larger rework on how we send the HTTPException and have it displayed correctly with the openapi runtime we use
This PR adds a button to send a test email when setting up STMP configuration, then save the setting if an email is sent succesfully.