-
Notifications
You must be signed in to change notification settings - Fork 108
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(invalid_test): show error popup if test data is invalid #516
Conversation
Hello @enudler , could you check this PR? |
Hi @anusha5695 |
@manorlh would you review this too please? |
@@ -271,6 +309,7 @@ class getTests extends React.Component { | |||
autoHideDuration={4000} | |||
onRequestClose={this.handleSnackbarClose} | |||
/>} | |||
{this.state.testActionError.isError && <ActionErrorPopup onClose={this.resetTestActionError} message={this.state.testActionError.errorMessage} />} | |||
{error && <ErrorDialog closeDialog={this.onCloseErrorDialog} showMessage={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.
as you can see we already have a component for error.
also you can use the variable error that already exist.
example:
const error = your error || errorOnDeleteTest ;
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.
@manorlh make sense. done!
ui/src/features/get-tests.js
Outdated
testActionError: { | ||
isError: false, | ||
errorMessage: EMPTY_STRING, | ||
}, |
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.
it will be nicer to use one variable for that - something like renderTestError
and its value would be the error text if error accrued and undefined/null if it didn'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.
done!
ui/src/features/get-tests.js
Outdated
@@ -118,13 +128,36 @@ class getTests extends React.Component { | |||
this.setState({ openViewTest: data }); | |||
}; | |||
|
|||
updateTestActionError = ({ isError, errorMessage }) => { | |||
this.setState({ | |||
...this.state, |
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.setState update only the keys that you send, so you don't need "...this.state "
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.
done again!
thank you for your contribution! it's awesome:) |
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.
Fixed all feedback 👍
thanks. 🙏 I will review it again tomorrow |
Issue:
Invalid test data created thro API when rendered on UI crashes
Fix:
IMPORTANT: Error handling done for both "EDIT" and "CLONE"
Show error pop up if test data is invalid
Related issues: #498
![image](https://user-images.githubusercontent.com/33545283/96991708-7b921f80-1546-11eb-87b9-985ef8679724.png)
Have extracted constants for error messages, titles, etc
Created isValidTest function that reuses existing "createStateForEditTest" function. [Creating new function would require refactoring existing flows, which we may do separately]
Created ActionErrorComponent for showing error that can be used for showing any error in Test Actions