-
Notifications
You must be signed in to change notification settings - Fork 373
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
upcoming: [DI-23769] - multiple error handling #11874
base: develop
Are you sure you want to change the base?
upcoming: [DI-23769] - multiple error handling #11874
Conversation
…s in forms, Added a Notice based component to render list or single accordingly
</AlertNoticeErrorState> | ||
); | ||
} else { | ||
return ( |
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.
@santoshp210-akamai , I think we don't need else, we can just return here, lets remove this branching
errors: APIError[], | ||
errorFieldMap: Record<string, FieldPath<T>>, | ||
multiLineErrorSeparator: string, | ||
singleLineErrorSeparator: string, | ||
setError: UseFormSetError<T> |
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.
Since this has more than 2 arguments, better to have it in interface
continue; | ||
} | ||
|
||
const errorField = error.field.split('.')[0]; |
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.
@santoshp210-akamai , the function is good, lets add comments above each step, explaining what it does
const { queryAllByTestId, queryByTestId } = renderWithTheme( | ||
<AlertListNoticeMessages | ||
errorMessage={errorMessage} | ||
separator={''} |
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.
separator={''} | |
separator="" |
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.
@santoshp210-akamai , Looks good, we can address minor comment I left
fontFamily: theme.tokens.typography.Body.Bold, | ||
})} | ||
data-testid="alert_message_notice" | ||
variant="body2" |
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.
optional: remove this
variant="body2" |
if (rootError) { | ||
enqueueSnackbar(`Creating alert failed: ${rootError.reason}`, { | ||
variant: 'error', | ||
}); |
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.
we had a fallback err msg from ui for enable/disable error handling, but we don't have it here?
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 is the fall back message, for fields we are handling manually everywhere. But if no fields are given, we show in snackbar.
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.
still the reason is from api, cc: @venkymano-akamai
Cloud Manager UI test results🔺 1 failing test on test run #5 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/stackscripts/smoke-community-stackscripts.spec.ts" |
There seems to be a case that's missing from this and an existing component can be enhanced to work with new component and reduce redundancy. So converted to draft to work on it. |
Description 📝
Handling multiple error messages from API and displaying it in the form accordingly.
Changes 🔄
AlertListNoticeMessages.tsx
handleMultipleError
to handle the error map logicAlertListNoticeMessages
component inAddChannelListing
&MetricCriteria
componentshandleSubmit
method in Create and Edit flow to include the error handlingTarget release date 🗓️
Please specify a release date (and environment, if applicable) to guarantee timely review of this PR. If exact date is not known, please approximate and update it as needed.
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
serverHandler.ts
use the given snippet below as the return response for eitherhttp.post('*/monitor/services/:service_type/alert-definitions')
requestor
http.put('*/monitor/services/:serviceType/alert-definitions/:id')
requestVerification steps
(How to verify changes)
channel_ids
,rule_criteria.rules
should be shown in AlertListNoticeMessages componentchannel_ids
,rule_criteria.rules
should be captured shown in AlertListNoticeMessages component.channel_ids
,rule_criteria.rules
should be displayed in the relevant errorText handlers of the componentschannel_ids
,rule_criteria.rules
should be concatenated if they are multiple with a space in between.Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅