Skip to content
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.

Sign up for GitHub

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

[control group] apply selections on reset #189830

Merged
merged 16 commits into from
Aug 8, 2024
Merged

[control group] apply selections on reset #189830

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3412ec1
[control group] apply selections on reset
nreese Aug 2, 2024
635540c
fix untilFiltersReady
nreese Aug 2, 2024
110b029
avoid static timer
nreese Aug 2, 2024
fa376f7
tslint
nreese Aug 2, 2024
787d71f
ContextStateManager
nreese Aug 2, 2024
a2a8232
Merge branch 'main' into apply_selections_on_reset
elasticmachine Aug 5, 2024
18fdffa
move comparators into selections files
nreese Aug 5, 2024
5742e92
revert changes to options list clear selections
nreese Aug 5, 2024
b9b8226
restore
nreese Aug 5, 2024
8dda053
move setExclude to component api
nreese Aug 5, 2024
757a6f1
fix test
nreese Aug 5, 2024
c013489
clean up
nreese Aug 5, 2024
d54a8e9
reset filters ready on dataViewId and field name change
nreese Aug 5, 2024
f4a9962
Merge branch 'main' into apply_selections_on_reset
elasticmachine Aug 7, 2024
b78a301
fix issue where reset was not finishing
nreese Aug 7, 2024
f1a4c63
only applySelections when not auto apply
nreese Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions examples/controls_example/public/app/react_control_example/react_control_example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import React, { useEffect, useMemo, useState } from 'react';
import { BehaviorSubject, combineLatest, Subject } from 'rxjs';

import useMountedState from 'react-use/lib/useMountedState';
import {
EuiBadge,
EuiButton,
Expand Down Expand Up @@ -76,6 +76,7 @@ export const ReactControlExample = ({
core: CoreStart;
dataViews: DataViewsPublicPluginStart;
}) => {
const isMounted = useMountedState();
const dataLoading$ = useMemo(() => {
return new BehaviorSubject<boolean | undefined>(false);
}, []);
Expand Down Expand Up @@ -112,6 +113,7 @@ export const ReactControlExample = ({
const [controlGroupApi, setControlGroupApi] = useState<ControlGroupApi | undefined>(undefined);
const [isControlGroupInitialized, setIsControlGroupInitialized] = useState(false);
const [dataViewNotFound, setDataViewNotFound] = useState(false);
const [isResetting, setIsResetting] = useState(false);

const dashboardApi = useMemo(() => {
const query$ = new BehaviorSubject<Query | AggregateQuery | undefined>(undefined);
Expand Down Expand Up @@ -361,9 +363,15 @@ export const ReactControlExample = ({
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButtonEmpty
isDisabled={!controlGroupApi}
onClick={() => {
controlGroupApi?.resetUnsavedChanges();
isDisabled={!controlGroupApi || isResetting}
isLoading={isResetting}
onClick={async () => {
if (!controlGroupApi) {
return;
}
setIsResetting(true);
await controlGroupApi.asyncResetUnsavedChanges();
if (isMounted()) setIsResetting(false);
Heenawter marked this conversation as resolved.
Show resolved Hide resolved
}}
>
Reset
Expand Down
16 changes: 14 additions & 2 deletions ...controls_example/public/react_controls/control_group/control_group_unsaved_changes_api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
import { combineLatest, map } from 'rxjs';
import { ControlsInOrder, getControlsInOrder } from './init_controls_manager';
import { ControlGroupRuntimeState, ControlPanelsState } from './types';
import { apiPublishesAsyncFilters } from '../data_controls/publishes_async_filters';

export type ControlGroupComparatorState = Pick<
ControlGroupRuntimeState,
Expand All @@ -33,6 +34,7 @@ export type ControlGroupComparatorState = Pick<
};

export function initializeControlGroupUnsavedChanges(
applySelections: () => void,
children$: PresentationContainer['children$'],
comparators: StateComparators<ControlGroupComparatorState>,
snapshotControlsRuntimeState: () => ControlPanelsState,
Expand Down Expand Up @@ -68,12 +70,22 @@ export function initializeControlGroupUnsavedChanges(
return Object.keys(unsavedChanges).length ? unsavedChanges : undefined;
})
),
resetUnsavedChanges: () => {
asyncResetUnsavedChanges: async () => {
controlGroupUnsavedChanges.api.resetUnsavedChanges();

const filtersReadyPromises: Array<Promise<void>> = [];
Object.values(children$.value).forEach((controlApi) => {
if (apiPublishesUnsavedChanges(controlApi)) controlApi.resetUnsavedChanges();
if (apiPublishesAsyncFilters(controlApi)) {
filtersReadyPromises.push(controlApi.untilFiltersReady());
}
});

await Promise.all(filtersReadyPromises);
applySelections();
Copy link
Contributor

@Heenawter Heenawter Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably only call applySelections if auto-apply is disabled - otherwise, we are publishing twice when it is enabled due to the unpublishedFilters$ subscription in the control group selections manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve with f1a4c63

Copy link
Contributor Author

@nreese nreese Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an if statement around applySelections. applySelections will not publish filters if there are no changes. See the below implementation. So I am not sure if the if statement is needed. Let me know if I should take it out.

function applySelections() {
    if (!deepEqual(filters$.value, unpublishedFilters$.value)) {
      filters$.next(unpublishedFilters$.value);
    }
    if (!deepEqual(timeslice$.value, unpublishedTimeslice$.value)) {
      timeslice$.next(unpublishedTimeslice$.value);
    }
  }

Copy link
Contributor

@Heenawter Heenawter Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point - I personally still think we should keep the applySelections in an if (there is more overhead to calling applySelections twice than there is to check the value of applySelections). But ultimately I'm fine either way, since the at least the filters$ subject won't emit twice 👍

},
} as PublishesUnsavedChanges<ControlGroupRuntimeState>,
} as Pick<PublishesUnsavedChanges, 'unsavedChanges'> & {
asyncResetUnsavedChanges: () => Promise<void>;
},
};
}
1 change: 1 addition & 0 deletions examples/controls_example/public/react_controls/control_group/get_control_group_factory.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ export const getControlGroupEmbeddableFactory = (services: {
const dataLoading$ = new BehaviorSubject<boolean | undefined>(false);

const unsavedChanges = initializeControlGroupUnsavedChanges(
selectionsManager.applySelections,
controlsManager.api.children$,
{
...controlsManager.comparators,
Expand Down
3 changes: 2 additions & 1 deletion examples/controls_example/public/react_controls/control_group/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,11 @@ export type ControlGroupApi = PresentationContainer &
HasSerializedChildState<ControlPanelState> &
HasEditCapabilities &
PublishesDataLoading &
PublishesUnsavedChanges &
Pick<PublishesUnsavedChanges, 'unsavedChanges'> &
PublishesControlGroupDisplaySettings &
PublishesTimeslice &
Partial<HasParentApi<PublishesUnifiedSearch> & HasSaveNotification> & {
asyncResetUnsavedChanges: () => Promise<void>;
autoApplySelections$: PublishingSubject<boolean>;
controlFetch$: (controlUuid: string) => Observable<ControlFetchContext>;
getLastSavedControlState: (controlUuid: string) => object;
Expand Down
82 changes: 49 additions & 33 deletions examples/controls_example/public/react_controls/data_controls/initialize_data_control.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { isEqual } from 'lodash';
import { BehaviorSubject, combineLatest, first, switchMap } from 'rxjs';
import { BehaviorSubject, combineLatest, debounceTime, first, skip, switchMap, tap } from 'rxjs';

import { CoreStart } from '@kbn/core-lifecycle-browser';
import {
Expand Down Expand Up @@ -45,9 +45,12 @@ export const initializeDataControl = <EditorState extends object = {}>(
api: ControlApiInitialization<DataControlApi>;
cleanup: () => void;
comparators: StateComparators<DefaultDataControlState>;
setters: {
onSelectionChange: () => void;
setOutputFilter: (filter: Filter | undefined) => void;
};
stateManager: ControlStateManager<DefaultDataControlState>;
serialize: () => SerializedPanelState<DefaultControlState>;
untilFiltersInitialized: () => Promise<void>;
} => {
const defaultControl = initializeDefaultControlApi(state);

Expand All @@ -57,6 +60,7 @@ export const initializeDataControl = <EditorState extends object = {}>(
const fieldName = new BehaviorSubject<string>(state.fieldName);
const dataViews = new BehaviorSubject<DataView[] | undefined>(undefined);
const filters$ = new BehaviorSubject<Filter[] | undefined>(undefined);
const filtersReady$ = new BehaviorSubject<boolean>(false);
const field$ = new BehaviorSubject<DataViewField | undefined>(undefined);
const fieldFormatter = new BehaviorSubject<DataControlFieldFormatter>((toFormat: any) =>
String(toFormat)
Expand All @@ -69,14 +73,14 @@ export const initializeDataControl = <EditorState extends object = {}>(
title: panelTitle,
};

function clearBlockingError() {
if (defaultControl.api.blockingError.value) {
defaultControl.api.setBlockingError(undefined);
}
}

const dataViewIdSubscription = dataViewId
.pipe(
tap(() => {
filtersReady$.next(false);
if (defaultControl.api.blockingError.value) {
defaultControl.api.setBlockingError(undefined);
}
}),
switchMap(async (currentDataViewId) => {
let dataView: DataView | undefined;
try {
Expand All @@ -90,14 +94,17 @@ export const initializeDataControl = <EditorState extends object = {}>(
.subscribe(({ dataView, error }) => {
if (error) {
defaultControl.api.setBlockingError(error);
} else {
clearBlockingError();
}
dataViews.next(dataView ? [dataView] : undefined);
});

const fieldNameSubscription = combineLatest([dataViews, fieldName]).subscribe(
([nextDataViews, nextFieldName]) => {
const fieldNameSubscription = combineLatest([dataViews, fieldName])
.pipe(
tap(() => {
filtersReady$.next(false);
})
)
.subscribe(([nextDataViews, nextFieldName]) => {
const dataView = nextDataViews
? nextDataViews.find(({ id }) => dataViewId.value === id)
: undefined;
Expand All @@ -115,8 +122,8 @@ export const initializeDataControl = <EditorState extends object = {}>(
})
)
);
} else {
clearBlockingError();
} else if (defaultControl.api.blockingError.value) {
defaultControl.api.setBlockingError(undefined);
}

field$.next(field);
Expand All @@ -125,8 +132,7 @@ export const initializeDataControl = <EditorState extends object = {}>(
if (spec) {
fieldFormatter.next(dataView.getFormatterForField(spec).getConverterFor('text'));
}
}
);
});

const onEdit = async () => {
// get the initial state from the state manager
Expand Down Expand Up @@ -172,6 +178,13 @@ export const initializeDataControl = <EditorState extends object = {}>(
});
};

const filtersReadySubscription = filters$.pipe(skip(1), debounceTime(0)).subscribe(() => {
// Set filtersReady$.next(true); in filters$ subscription instead of setOutputFilter
// to avoid signaling filters ready until after filters have been emitted
// to avoid timing issues
filtersReady$.next(true);
});

const api: ControlApiInitialization<DataControlApi> = {
...defaultControl.api,
panelTitle,
Expand All @@ -181,24 +194,41 @@ export const initializeDataControl = <EditorState extends object = {}>(
fieldFormatter,
onEdit,
filters$,
setOutputFilter: (newFilter: Filter | undefined) => {
filters$.next(newFilter ? [newFilter] : undefined);
},
isEditingEnabled: () => true,
untilFiltersReady: async () => {
return new Promise((resolve) => {
combineLatest([defaultControl.api.blockingError, filtersReady$])
.pipe(
first(([blockingError, filtersReady]) => filtersReady || blockingError !== undefined)
)
.subscribe(() => {
resolve();
});
});
},
};

return {
api,
cleanup: () => {
dataViewIdSubscription.unsubscribe();
fieldNameSubscription.unsubscribe();
filtersReadySubscription.unsubscribe();
},
comparators: {
...defaultControl.comparators,
title: [panelTitle, (value: string | undefined) => panelTitle.next(value)],
dataViewId: [dataViewId, (value: string) => dataViewId.next(value)],
fieldName: [fieldName, (value: string) => fieldName.next(value)],
},
setters: {
onSelectionChange: () => {
filtersReady$.next(false);
},
setOutputFilter: (newFilter: Filter | undefined) => {
filters$.next(newFilter ? [newFilter] : undefined);
},
},
stateManager,
serialize: () => {
return {
Expand All @@ -217,19 +247,5 @@ export const initializeDataControl = <EditorState extends object = {}>(
],
};
},
untilFiltersInitialized: async () => {
return new Promise((resolve) => {
combineLatest([defaultControl.api.blockingError, filters$])
.pipe(
first(
([blockingError, filters]) =>
blockingError !== undefined || (filters?.length ?? 0) > 0
)
)
.subscribe(() => {
resolve();
});
});
},
};
};
16 changes: 13 additions & 3 deletions examples/controls_example/public/react_controls/data_controls/mocks/api_mocks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,16 @@ import { BehaviorSubject } from 'rxjs';
import { OptionsListSuggestions } from '@kbn/controls-plugin/common/options_list/types';
import { DataViewField } from '@kbn/data-views-plugin/common';

import { PublishingSubject } from '@kbn/presentation-publishing';
import { OptionsListSelection } from '../../../../common/options_list/options_list_selections';
import { OptionsListSearchTechnique } from '../../../../common/options_list/suggestions_searching';
import { OptionsListSortingType } from '../../../../common/options_list/suggestions_sorting';
import { OptionsListDisplaySettings } from '../options_list_control/types';

export const getOptionsListMocks = () => {
const selectedOptions$ = new BehaviorSubject<OptionsListSelection[] | undefined>(undefined);
const exclude$ = new BehaviorSubject<boolean | undefined>(undefined);
const existsSelected$ = new BehaviorSubject<boolean | undefined>(undefined);
return {
api: {
uuid: 'testControl',
Expand All @@ -30,17 +34,23 @@ export const getOptionsListMocks = () => {
},
fieldFormatter: new BehaviorSubject((value: string | number) => String(value)),
makeSelection: jest.fn(),
setExclude: (next: boolean | undefined) => exclude$.next(next),
},
stateManager: {
searchString: new BehaviorSubject<string>(''),
searchStringValid: new BehaviorSubject<boolean>(true),
fieldName: new BehaviorSubject<string>('field'),
exclude: new BehaviorSubject<boolean | undefined>(undefined),
existsSelected: new BehaviorSubject<boolean | undefined>(undefined),
exclude: exclude$ as PublishingSubject<boolean | undefined>,
existsSelected: existsSelected$ as PublishingSubject<boolean | undefined>,
sort: new BehaviorSubject<OptionsListSortingType | undefined>(undefined),
selectedOptions: new BehaviorSubject<OptionsListSelection[] | undefined>(undefined),
selectedOptions: selectedOptions$ as PublishingSubject<OptionsListSelection[] | undefined>,
searchTechnique: new BehaviorSubject<OptionsListSearchTechnique | undefined>(undefined),
},
displaySettings: {} as OptionsListDisplaySettings,
// setSelectedOptions and setExistsSelected are not exposed via API because
// they are not used by components
// they are needed in tests however so expose them as top level keys
setSelectedOptions: (next: OptionsListSelection[] | undefined) => selectedOptions$.next(next),
setExistsSelected: (next: boolean | undefined) => existsSelected$.next(next),
};
};
21 changes: 10 additions & 11 deletions ...eact_controls/data_controls/options_list_control/components/options_list_control.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ import React from 'react';

import { DataViewField } from '@kbn/data-views-plugin/common';
import { render } from '@testing-library/react';
import { ControlStateManager } from '../../../types';
import { getOptionsListMocks } from '../../mocks/api_mocks';
import { OptionsListControlContext } from '../options_list_context_provider';
import { OptionsListComponentApi, OptionsListComponentState } from '../types';
import { ContextStateManager, OptionsListControlContext } from '../options_list_context_provider';
import { OptionsListComponentApi } from '../types';
import { OptionsListControl } from './options_list_control';

describe('Options list control', () => {
Expand All @@ -31,7 +30,7 @@ describe('Options list control', () => {
value={{
api: api as unknown as OptionsListComponentApi,
displaySettings,
stateManager: stateManager as unknown as ControlStateManager<OptionsListComponentState>,
stateManager: stateManager as unknown as ContextStateManager,
}}
>
<OptionsListControl controlPanelClassName="controlPanel" />
Expand All @@ -42,8 +41,8 @@ describe('Options list control', () => {
test('if exclude = false and existsSelected = true, then the option should read "Exists"', async () => {
const mocks = getOptionsListMocks();
mocks.api.uuid = 'testExists';
mocks.stateManager.exclude.next(false);
mocks.stateManager.existsSelected.next(true);
mocks.api.setExclude(false);
mocks.setExistsSelected(true);
const control = mountComponent(mocks);
const existsOption = control.getByTestId('optionsList-control-testExists');
expect(existsOption).toHaveTextContent('Exists');
Expand All @@ -52,8 +51,8 @@ describe('Options list control', () => {
test('if exclude = true and existsSelected = true, then the option should read "Does not exist"', async () => {
const mocks = getOptionsListMocks();
mocks.api.uuid = 'testDoesNotExist';
mocks.stateManager.exclude.next(true);
mocks.stateManager.existsSelected.next(true);
mocks.api.setExclude(true);
mocks.setExistsSelected(true);
const control = mountComponent(mocks);
const existsOption = control.getByTestId('optionsList-control-testDoesNotExist');
expect(existsOption).toHaveTextContent('DOES NOT Exist');
Expand All @@ -68,7 +67,7 @@ describe('Options list control', () => {
{ value: 'bark', docCount: 10 },
{ value: 'meow', docCount: 12 },
]);
mocks.stateManager.selectedOptions.next(['woof', 'bark']);
mocks.setSelectedOptions(['woof', 'bark']);
mocks.api.field$.next({
name: 'Test keyword field',
type: 'keyword',
Expand All @@ -87,7 +86,7 @@ describe('Options list control', () => {
{ value: 2, docCount: 10 },
{ value: 3, docCount: 12 },
]);
mocks.stateManager.selectedOptions.next([1, 2]);
mocks.setSelectedOptions([1, 2]);
mocks.api.field$.next({
name: 'Test keyword field',
type: 'number',
Expand All @@ -105,7 +104,7 @@ describe('Options list control', () => {
{ value: 'bark', docCount: 10 },
{ value: 'meow', docCount: 12 },
]);
mocks.stateManager.selectedOptions.next(['woof', 'bark']);
mocks.setSelectedOptions(['woof', 'bark']);
mocks.api.invalidSelections$.next(new Set(['woof']));
mocks.api.field$.next({
name: 'Test keyword field',
Expand Down
Loading