-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: add form validation to alert channel form #7135
base: main
Are you sure you want to change the base?
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.
❌ Changes requested. Reviewed everything up to 74577e6 in 3 minutes and 4 seconds
More details
- Looked at
151
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. frontend/src/container/FormAlertChannels/index.tsx:167
- Draft comment:
Field name mismatch: The FormValues type defines 'field_channel_type' but the Form.Item uses 'name="type"'. Use a consistent key for the channel type. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/src/container/FormAlertChannels/index.tsx:99
- Draft comment:
The 'isValidForm' function returns true when required fields are missing, causing the buttons to be disabled. Consider renaming it to 'isInvalidForm' for clarity or adjusting its logic. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/container/FormAlertChannels/Settings/Slack.tsx:14
- Draft comment:
Similar to MsTeams, for consistency and better UX, add form validation rules to required fields instead of relying solely on the 'required' attribute in Form.Item. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/container/FormAlertChannels/Settings/MsTeams.tsx:12
- Draft comment:
Consider using Form.Item’s 'rules' property to display inline validation errors rather than relying solely on the 'required' prop. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/container/FormAlertChannels/Settings/Slack.tsx:14
- Draft comment:
Adding the 'required' attribute is useful, but consider using Form.Item’s 'rules' to provide explicit error messages for missing values. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/container/FormAlertChannels/Settings/Webhook.tsx:12
- Draft comment:
Consider using Form.Item’s 'rules' prop for inline validation messages instead of solely relying on the 'required' attribute. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/container/FormAlertChannels/index.tsx:210
- Draft comment:
Disabling the Save/Test buttons based on isValidForm() hides inline feedback. Consider leveraging Ant Design Form validation (using 'rules') to provide immediate error messages instead of only disabling the buttons. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/src/container/FormAlertChannels/index.tsx:167
- Draft comment:
There is a field name mismatch: FormValues defines the channel type as 'field_channel_type' but the Select Form.Item uses 'name="type"'. Align these names so that validation and state mapping work correctly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_2wmzOZLJY6lkdVpi
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -9,7 +9,7 @@ function MsTeams({ setSelectedConfig }: MsTeamsProps): JSX.Element { | |||
|
|||
return ( | |||
<> | |||
<Form.Item name="webhook_url" label={t('field_webhook_url')}> | |||
<Form.Item required name="webhook_url" label={t('field_webhook_url')}> |
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.
Using 'required' in Form.Item is a UI hint. Consider adding validation rules (e.g., rules with a required message) to enforce validation and provide proper feedback.
<Form.Item required name="webhook_url" label={t('field_webhook_url')}> | |
<Form.Item required name="webhook_url" label={t('field_webhook_url')} rules={[{ required: true, message: t('field_webhook_url_required') }]}> |
@@ -84,12 +96,45 @@ function FormAlertChannels({ | |||
} | |||
}; | |||
|
|||
const isValidForm = (): boolean => { |
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.
The function name 'isValidForm' is misleading since it returns true when required fields are missing. Consider renaming it (e.g., 'isFormIncomplete') or inverting its logic to improve clarity.
@@ -84,12 +96,45 @@ function FormAlertChannels({ | |||
} | |||
}; | |||
|
|||
const isValidForm = (): boolean => { |
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.
May have missed some more fields that are required for validation. I have added checks to the fields which I think are important at first glance, but let me know if I have missed any! :)
Summary
When adding an alarm channel, the alarm channel name, web address, and email are not verified in the form, resulting in an interface error.
Related Issues / PR's
#6614 - bug 1
Screenshots
Screen.Recording.2025-02-17.at.11.27.28.a.m.mov
Affected Areas and Manually Tested Areas
frontend/src/container/FormAlertChannels/index.tsx
Important
Add form validation to alert channel form by marking required fields and implementing a validation function in
index.tsx
.required
attribute toForm.Item
forwebhook_url
inMsTeams.tsx
,api_url
andchannel
inSlack.tsx
, andapi_url
inWebhook.tsx
.isValidForm()
inindex.tsx
to validate form fields based onChannelType
.index.tsx
if form is invalid.MsTeamsChannel
toFormValues
type inindex.tsx
.Form.Item
forname
inindex.tsx
to be required.This description was created by
for 74577e6. It will automatically update as commits are pushed.