-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[SECURITY_SOLUTION][ENDPOINT] Trusted App Create Form show inline validations errors #78305
Merged
paul-tavares
merged 13 commits into
elastic:master
from
paul-tavares:task/emt-252-trusted-app-create-form-validations
Sep 24, 2020
+498
−1,077
Merged
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
e9f0b73
Updated structure for `ValidationResult` type
paul-tavares b9f0190
show errors on the ui if field is invalid
paul-tavares 3dc041a
Support for tracking visited fields (not Condition builder yet)
paul-tavares d7a85e8
Add `onVisited` to ConditionBuilder components
paul-tavares 671935b
Merge remote-tracking branch 'upstream/master' into task/emt-252-trus…
paul-tavares cd8efb5
Remove use of Snapshots in Trusted Apps tests
paul-tavares e11874b
initial test plan for trusted app create form
paul-tavares 148b014
First set of tests
paul-tavares 8559194
Tests covering the Condition Builder component
paul-tavares 2d1d4b8
Final set of tests for trusted apps create form
paul-tavares d674c6a
Merge remote-tracking branch 'upstream/master' into task/emt-252-trus…
paul-tavares 64b9803
Updated snapshots
paul-tavares 0db501d
Merge remote-tracking branch 'upstream/master' into task/emt-252-trus…
paul-tavares File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
987 changes: 0 additions & 987 deletions
987
...n/public/management/pages/trusted_apps/view/__snapshots__/trusted_apps_page.test.tsx.snap
This file was deleted.
Oops, something went wrong.
77 changes: 77 additions & 0 deletions
77
...ion/public/management/pages/trusted_apps/view/components/create_trusted_app_form.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { | ||
AppContextTestRender, | ||
createAppRootMockRenderer, | ||
} from '../../../../../common/mock/endpoint'; | ||
import * as reactTestingLibrary from '@testing-library/react'; | ||
import React from 'react'; | ||
import { CreateTrustedAppForm, CreateTrustedAppFormProps } from './create_trusted_app_form'; | ||
|
||
describe('When showing the Trusted App Create Form', () => { | ||
let render: () => ReturnType<AppContextTestRender['render']>; | ||
let onChangeCallback: jest.MockedFunction<CreateTrustedAppFormProps['onChange']>; | ||
|
||
beforeEach(() => { | ||
const mockedContext = createAppRootMockRenderer(); | ||
|
||
onChangeCallback = jest.fn(); | ||
render = () => mockedContext.render(<CreateTrustedAppForm onChange={onChangeCallback} />); | ||
}); | ||
|
||
it.todo('should show Name as required'); | ||
|
||
it.todo('should default OS to Windows'); | ||
|
||
it.todo('should allow user to select between 3 OSs'); | ||
|
||
it.todo('should show Description as optional'); | ||
|
||
it.todo('should NOT initially show any inline validation errors'); | ||
|
||
it.todo('should show top-level Errors'); | ||
|
||
describe('the condition builder component', () => { | ||
it.todo('should show an initial condition entry'); | ||
|
||
it.todo('should not allow the entry to be removed if its the only one displayed'); | ||
|
||
it.todo('should display 2 options for Field'); | ||
|
||
it.todo('should show the value field as required'); | ||
|
||
it.todo('should display the `AND` button'); | ||
|
||
it.todo('should add a new condition entry when `AND` is clicked'); | ||
|
||
it.todo('should have remove icons enabled when multiple conditions are present'); | ||
}); | ||
|
||
describe('and the user visits required fields but does not fill them out', () => { | ||
it.todo('should show Name validation error'); | ||
|
||
it.todo('should show Condition validation error'); | ||
|
||
it.todo('should NOT display any other errors'); | ||
|
||
it.todo('should call change callback with isValid set to false and contain the new item'); | ||
}); | ||
|
||
describe('and invalid data is entered', () => { | ||
it.todo('should validate that Name has a non empty space value'); | ||
|
||
it.todo('should validate that a condition value has a non empty space value'); | ||
|
||
it.todo( | ||
'should validate all condition values (when multiples exist) have non empty space value' | ||
); | ||
}); | ||
|
||
describe('and all required data passes validation', () => { | ||
it.todo('should call change callback with isValid set to true and contain the new item'); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
when are we using react's state vs redux 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.
😝 answer here 👉 https://www.elastic.co/about/our-source-code#it-depends
😄 to expand on my reasoning:
For this one - the entire thing - it was developed as a "pure" component and it is not tied to the Store. I did not see the benefit of storing this in Redux while the data is being collected from the user. Another reason, but one that really would probably not have convinced me to change the approach, was that at the time I started it, the trusted apps store setup was not yet available (pending PR).