Skip to content
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

fix #5314 Email Address Friendly Name Error #5318

Conversation

ABHIGYAN-MOHANTA
Copy link
Contributor

Description

Fixed: When selecting the SendGrid alert time and entering the "From Email" in the: name format, this gives a validation error

Fixes #5314

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • User interface (UI)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

uptime-kuma.mp4

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.
I have left a few comments below (most of them try to make this part of the frontend less special)

src/components/notifications/SendGrid.vue Outdated Show resolved Hide resolved
src/components/notifications/SendGrid.vue Outdated Show resolved Hide resolved
src/components/notifications/SendGrid.vue Outdated Show resolved Hide resolved
src/components/notifications/SendGrid.vue Outdated Show resolved Hide resolved
src/components/notifications/SendGrid.vue Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label Nov 6, 2024
@louislam
Copy link
Owner

louislam commented Nov 6, 2024

In my opinion, just make it as a normal text field without regex checking, let "Test" button do the checking.

Because the email format regex usually will not be that simple, it may cause unpredictable bugs.

RFC 5322 / RFC 822

If really want to check it, finding a well-known library to verify the email address will be better.

@ABHIGYAN-MOHANTA
Copy link
Contributor Author

I agree, even if we use a good email validation library like Validator JS, we will still need to use regex for friendly name format validation, and if we are not going in the regex route, better to get rid of it. Sorry for introducing the complexity, I will just change the type to text. Thanks! Let's keep it straightforward, but if you decide otherwise, I am happy to implement validator.js as well! Thanks again!

@ABHIGYAN-MOHANTA
Copy link
Contributor Author

@louislam, I have just kept it as a normal text field like you mentioned. Also, I realised, I can't really make ui changes just for SendGrid, it has to be for all providers, sorry about that 😅

@louislam louislam added this to the 2.0.0-beta.1 milestone Nov 8, 2024
@louislam louislam removed the pr:please address review comments this PR needs a bit more work to be mergable label Nov 9, 2024
@louislam louislam merged commit 6899603 into louislam:master Nov 9, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SendMail] Email Address Friendly Name Error
3 participants