-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[OnWeek][Discover] Allow to change current sample size and save it with a saved search #157269
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
Merged
jughosta
merged 49 commits into
elastic:main
from
jughosta:onweek-configurable-sample-size
Oct 19, 2023
Merged
Changes from all commits
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
fd399ce
[Discover] Allow to change current sample size and save it with a sav…
jughosta 6fdae5c
[Discover] Search embaddable supports a custom sample size
jughosta fcb469b
Merge remote-tracking branch 'upstream/main' into onweek-configurable…
jughosta 5ca785c
[Discover] Update test
jughosta 8d80dbe
[Discover] Fix context page
jughosta 8150dd7
[Discover] Disable in SQL mode
jughosta 517cd51
[Discover] Fix redundant state updates
jughosta e39f191
[Discover] Fix the max value
jughosta b25b974
[Discover] Discard old value
jughosta 469a3ac
Merge branch 'main' into onweek-configurable-sample-size
jughosta 1fbf4eb
[Discover] Fix the default case
jughosta f070a1c
Merge branch 'main' into onweek-configurable-sample-size
jughosta f5b6ae7
Merge remote-tracking branch 'upstream/main' into onweek-configurable…
jughosta 96572bb
[Discover] Update UI
jughosta 05a72b7
[Discover] Update icon
jughosta 2095b0c
Merge remote-tracking branch 'upstream/main' into onweek-configurable…
jughosta ba2297d
[Discover] Fix tests
jughosta 0755a37
Merge branch 'main' into onweek-configurable-sample-size
jughosta d05a506
Merge branch 'main' into onweek-configurable-sample-size
jughosta cedb282
Merge remote-tracking branch 'upstream/main' into onweek-configurable…
jughosta d468443
[Discover] Fix reading from saved search SO
jughosta 5bbe61e
[Discover] Update search hash
jughosta 44c3da0
[Discover] Add tests
jughosta 19e60ff
[Discover] Fix tests
jughosta b18c5d9
Merge remote-tracking branch 'upstream/main' into onweek-configurable…
jughosta 8f2679e
[Discover] Use new Eui props
jughosta 8b6cee9
[Discover] Update version
jughosta fbf56b3
Merge branch 'main' into onweek-configurable-sample-size
jughosta 3b1650b
[Discover] Update tests
jughosta 54a3449
Merge remote-tracking branch 'upstream/main' into onweek-configurable…
jughosta e2a9d30
[Discover] Update scheme version
jughosta 4f55005
[Discover] Update UI
jughosta e1bcb2a
[Discover] Fix props
jughosta 7a31208
[Discover] Add jest tests
jughosta 60baf90
[Discover] Update search SO hash
jughosta 40ba04c
Merge branch 'main' into onweek-configurable-sample-size
jughosta 7194e6b
[Discover] Extend tests suite
jughosta 79712df
[Discover] Add functional tests
jughosta 6866eb4
[Discover] Update code style
jughosta a636a95
[Discover] Fix messaging for legacy table embeddable
jughosta b58bff1
Merge branch 'main' into onweek-configurable-sample-size
jughosta 7feb2ae
[Discover] Switch back to EuiRange
jughosta 9d38df8
Merge branch 'main' into onweek-configurable-sample-size
jughosta 9766615
[Discover] Limit fetched sample by settings and 10_000
jughosta b398fdd
[Discover] Add tests
jughosta d6c17ad
[Discover] Fix tests
jughosta cc1fbc0
Merge branch 'main' into onweek-configurable-sample-size
jughosta 1bbb2e6
Merge branch 'main' into onweek-configurable-sample-size
jughosta c25f598
Merge branch 'main' into onweek-configurable-sample-size
jughosta 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
This file contains hidden or 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
This file contains hidden or 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
111 changes: 111 additions & 0 deletions
111
...ges/kbn-unified-data-table/src/components/data_table_additional_display_settings.test.tsx
This file contains hidden or 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,111 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import React from 'react'; | ||
| import { mountWithIntl } from '@kbn/test-jest-helpers'; | ||
| import { act } from 'react-dom/test-utils'; | ||
| import { findTestSubject } from '@elastic/eui/lib/test'; | ||
| import { UnifiedDataTableAdditionalDisplaySettings } from './data_table_additional_display_settings'; | ||
| import lodash from 'lodash'; | ||
|
|
||
| jest.spyOn(lodash, 'debounce').mockImplementation((fn: any) => fn); | ||
|
|
||
| describe('UnifiedDataTableAdditionalDisplaySettings', function () { | ||
| describe('sampleSize', function () { | ||
| it('should work correctly', async () => { | ||
| const onChangeSampleSizeMock = jest.fn(); | ||
|
|
||
| const component = mountWithIntl( | ||
| <UnifiedDataTableAdditionalDisplaySettings | ||
| sampleSize={10} | ||
| onChangeSampleSize={onChangeSampleSizeMock} | ||
| /> | ||
| ); | ||
| const input = findTestSubject(component, 'unifiedDataTableSampleSizeInput').last(); | ||
| expect(input.prop('value')).toBe(10); | ||
|
|
||
| await act(async () => { | ||
| input.simulate('change', { | ||
| target: { | ||
| value: 100, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| expect(onChangeSampleSizeMock).toHaveBeenCalledWith(100); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
| component.update(); | ||
|
|
||
| expect( | ||
| findTestSubject(component, 'unifiedDataTableSampleSizeInput').last().prop('value') | ||
| ).toBe(100); | ||
| }); | ||
|
|
||
| it('should not execute the callback for an invalid input', async () => { | ||
| const invalidValue = 600; | ||
| const onChangeSampleSizeMock = jest.fn(); | ||
|
|
||
| const component = mountWithIntl( | ||
| <UnifiedDataTableAdditionalDisplaySettings | ||
| maxAllowedSampleSize={500} | ||
| sampleSize={50} | ||
| onChangeSampleSize={onChangeSampleSizeMock} | ||
| /> | ||
| ); | ||
| const input = findTestSubject(component, 'unifiedDataTableSampleSizeInput').last(); | ||
| expect(input.prop('value')).toBe(50); | ||
|
|
||
| await act(async () => { | ||
| input.simulate('change', { | ||
| target: { | ||
| value: invalidValue, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
| component.update(); | ||
|
|
||
| expect( | ||
| findTestSubject(component, 'unifiedDataTableSampleSizeInput').last().prop('value') | ||
| ).toBe(invalidValue); | ||
|
|
||
| expect(onChangeSampleSizeMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should render value changes correctly', async () => { | ||
| const onChangeSampleSizeMock = jest.fn(); | ||
|
|
||
| const component = mountWithIntl( | ||
| <UnifiedDataTableAdditionalDisplaySettings | ||
| sampleSize={200} | ||
| onChangeSampleSize={onChangeSampleSizeMock} | ||
| /> | ||
| ); | ||
|
|
||
| expect( | ||
| findTestSubject(component, 'unifiedDataTableSampleSizeInput').last().prop('value') | ||
| ).toBe(200); | ||
|
|
||
| component.setProps({ | ||
| sampleSize: 500, | ||
| onChangeSampleSize: onChangeSampleSizeMock, | ||
| }); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
| component.update(); | ||
|
|
||
| expect( | ||
| findTestSubject(component, 'unifiedDataTableSampleSizeInput').last().prop('value') | ||
| ).toBe(500); | ||
|
|
||
| expect(onChangeSampleSizeMock).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
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.
Why do we set this to
false?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.
@davismcphee Since we have issues with the button #131130 and now we introduce additional settings which are not handled by eui code, we better hide the button. Originally suggested by @kertal
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.
yes, it did not work as expected, and introducing new settings, wouldn't make it better, so I suggested to remove it (not having a good way to handle it, at least back in the days)
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.
Ok, makes sense to remove it for now if it wasn't working as intended. Might be worth figuring out if we can fix it and add it back in later though. It would be convenient to be able to reset the sample size back to the saved search default after changing it on a dashboard and saving, which persists the updated value with the dashboard.