Skip to content

Commit

Permalink
[App Search] Refactor AppLogic to initialize data via props rather th…
Browse files Browse the repository at this point in the history
…an action (#92841)

* [Misc cleanup] Move Access type to common

- it was being duplicated in server/check_access and InitialAppData

+ add mock access data to DEFAULT_INITIAL_APP_DATA
+ update server/ tests to account for access in DEFAULT_INITIAL_APP_DATA

* Update AppSearchConfigured to pass props to AppLogic vs calling initializeAppData

+ update tests to rerender a wrapper rather than doing {...DEFAULT_INITIAL_APP_DATA} repeatedly

* Update AppLogic to set values from props rather than a listener

- main goal of this PR is to prevent the flash of state between mount and initializeX being called

- note: I recommend turning off whitespace changes in the test file

* Update AppLogic typing so that app data is always expected

- which it should be in any case in a production environment

- note: I could have changed InitialAppData to remove the ? optional notation, but decided on this route instead since InitialAppData affects more than just App Search (e.g. server, WS), and I didn't want this to have potential far-reaching side effects

* Update type scenarios where AppLogic values were previously thought potentially undefined

- which is mostly just configuredLimits it looks like

* [PR feedback] Type name
  • Loading branch information
Constance authored Mar 2, 2021
1 parent 3f5473e commit fd3b3eb
Show file tree
Hide file tree
Showing 13 changed files with 106 additions and 147 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ export const DEFAULT_INITIAL_APP_DATA = {
},
},
},
access: {
hasAppSearchAccess: true,
hasWorkplaceSearchAccess: true,
},
appSearch: {
accountId: 'some-id-string',
onboardingComplete: true,
Expand Down
10 changes: 6 additions & 4 deletions x-pack/plugins/enterprise_search/common/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,7 @@ export interface InitialAppData {
ilmEnabled?: boolean;
isFederatedAuth?: boolean;
configuredLimits?: ConfiguredLimits;
access?: {
hasAppSearchAccess: boolean;
hasWorkplaceSearchAccess: boolean;
};
access?: ProductAccess;
appSearch?: AppSearchAccount;
workplaceSearch?: WorkplaceSearchInitialData;
}
Expand All @@ -32,6 +29,11 @@ export interface ConfiguredLimits {
workplaceSearch: WorkplaceSearchConfiguredLimits;
}

export interface ProductAccess {
hasAppSearchAccess: boolean;
hasWorkplaceSearchAccess: boolean;
}

export interface MetaPage {
current: number;
size: number;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,74 +6,68 @@
*/

import { DEFAULT_INITIAL_APP_DATA } from '../../../common/__mocks__';
import { LogicMounter } from '../__mocks__';

import { resetContext } from 'kea';

import { AppLogic } from './app_logic';

describe('AppLogic', () => {
const { mount } = new LogicMounter(AppLogic);

beforeEach(() => {
mount();
});

const DEFAULT_VALUES = {
hasInitialized: false,
account: {},
configuredLimits: {},
ilmEnabled: false,
myRole: {},
const mount = (props = {}) => {
AppLogic({ ...DEFAULT_INITIAL_APP_DATA, ...props });
AppLogic.mount();
};

it('has expected default values', () => {
expect(AppLogic.values).toEqual(DEFAULT_VALUES);
beforeEach(() => {
jest.clearAllMocks();
resetContext({});
});

describe('initializeAppData()', () => {
it('sets values based on passed props', () => {
AppLogic.actions.initializeAppData(DEFAULT_INITIAL_APP_DATA);
it('sets values from props', () => {
mount();

expect(AppLogic.values).toEqual({
hasInitialized: true,
ilmEnabled: true,
configuredLimits: {
engine: {
maxDocumentByteSize: 102400,
maxEnginesPerMetaEngine: 15,
},
},
account: {
accountId: 'some-id-string',
onboardingComplete: true,
role: DEFAULT_INITIAL_APP_DATA.appSearch.role,
expect(AppLogic.values).toEqual({
ilmEnabled: true,
configuredLimits: {
engine: {
maxDocumentByteSize: 102400,
maxEnginesPerMetaEngine: 15,
},
myRole: expect.objectContaining({
id: 'account_id:somestring|user_oid:somestring',
roleType: 'owner',
availableRoleTypes: ['owner', 'admin'],
credentialTypes: ['admin', 'private', 'search'],
canAccessAllEngines: true,
canViewAccountCredentials: true,
// Truncated for brevity - see utils/role/index.test.ts for full output
}),
});
},
account: {
accountId: 'some-id-string',
onboardingComplete: true,
role: DEFAULT_INITIAL_APP_DATA.appSearch.role,
},
myRole: expect.objectContaining({
id: 'account_id:somestring|user_oid:somestring',
roleType: 'owner',
availableRoleTypes: ['owner', 'admin'],
credentialTypes: ['admin', 'private', 'search'],
canAccessAllEngines: true,
canViewAccountCredentials: true,
// Truncated for brevity - see utils/role/index.test.ts for full output
}),
});
});

it('gracefully handles missing initial data', () => {
AppLogic.actions.initializeAppData({});
describe('actions', () => {
describe('setOnboardingComplete()', () => {
it('sets true', () => {
mount({ appSearch: { onboardingComplete: false } });

expect(AppLogic.values).toEqual({
...DEFAULT_VALUES,
hasInitialized: true,
AppLogic.actions.setOnboardingComplete();
expect(AppLogic.values.account.onboardingComplete).toEqual(true);
});
});
});

describe('setOnboardingComplete()', () => {
it('sets true', () => {
expect(AppLogic.values.account.onboardingComplete).toBeFalsy();
AppLogic.actions.setOnboardingComplete();
expect(AppLogic.values.account.onboardingComplete).toEqual(true);
describe('selectors', () => {
describe('myRole', () => {
it('falls back to an empty object if role is missing', () => {
mount({ appSearch: {} });

expect(AppLogic.values.myRole).toEqual({});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -14,53 +14,33 @@ import { ConfiguredLimits, Account, Role } from './types';
import { getRoleAbilities } from './utils/role';

interface AppValues {
hasInitialized: boolean;
ilmEnabled: boolean;
configuredLimits: Partial<ConfiguredLimits>;
account: Partial<Account>;
myRole: Partial<Role>;
configuredLimits: ConfiguredLimits;
account: Account;
myRole: Role;
}
interface AppActions {
initializeAppData(props: InitialAppData): Required<InitialAppData>;
setOnboardingComplete(): boolean;
}

export const AppLogic = kea<MakeLogicType<AppValues, AppActions>>({
export const AppLogic = kea<MakeLogicType<AppValues, AppActions, Required<InitialAppData>>>({
path: ['enterprise_search', 'app_search', 'app_logic'],
actions: {
initializeAppData: (props) => props,
setOnboardingComplete: () => true,
},
reducers: {
hasInitialized: [
false,
{
initializeAppData: () => true,
},
],
reducers: ({ props }) => ({
account: [
{},
props.appSearch,
{
initializeAppData: (_, { appSearch: account }) => account || {},
setOnboardingComplete: (account) => ({
...account,
onboardingComplete: true,
}),
},
],
configuredLimits: [
{},
{
initializeAppData: (_, { configuredLimits }) => configuredLimits?.appSearch || {},
},
],
ilmEnabled: [
false,
{
initializeAppData: (_, { ilmEnabled }) => !!ilmEnabled,
},
],
},
configuredLimits: [props.configuredLimits.appSearch, {}],
ilmEnabled: [props.ilmEnabled, {}],
}),
selectors: {
myRole: [
(selectors) => [selectors.account],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,11 @@ export const FlyoutHeader: React.FC = () => {
};

export const FlyoutBody: React.FC = () => {
const { configuredLimits } = useValues(AppLogic);
const maxDocumentByteSize = configuredLimits?.engine?.maxDocumentByteSize;
const {
configuredLimits: {
engine: { maxDocumentByteSize },
},
} = useValues(AppLogic);

const { textInput, errors } = useValues(DocumentCreationLogic);
const { setTextInput } = useActions(DocumentCreationLogic);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,11 @@ export const FlyoutHeader: React.FC = () => {
};

export const FlyoutBody: React.FC = () => {
const { configuredLimits } = useValues(AppLogic);
const maxDocumentByteSize = configuredLimits?.engine?.maxDocumentByteSize;
const {
configuredLimits: {
engine: { maxDocumentByteSize },
},
} = useValues(AppLogic);

const { isUploading, errors } = useValues(DocumentCreationLogic);
const { setFileInput } = useActions(DocumentCreationLogic);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const comboBoxOptionToEngineName = (option: EuiComboBoxOptionOption<string>): st
export const MetaEngineCreation: React.FC = () => {
const {
configuredLimits: {
engine: { maxEnginesPerMetaEngine } = { maxEnginesPerMetaEngine: Infinity },
engine: { maxEnginesPerMetaEngine },
},
} = useValues(AppLogic);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,21 @@
* 2.0.
*/

import '../__mocks__/shallow_useeffect.mock';
import { DEFAULT_INITIAL_APP_DATA } from '../../../common/__mocks__';
import '../__mocks__/enterprise_search_url.mock';
import { setMockValues, setMockActions } from '../__mocks__';
import { setMockValues, rerender } from '../__mocks__';

import React from 'react';

import { Redirect } from 'react-router-dom';

import { shallow } from 'enzyme';
import { shallow, ShallowWrapper } from 'enzyme';

import { Layout, SideNav, SideNavLink } from '../shared/layout';

jest.mock('./app_logic', () => ({ AppLogic: jest.fn() }));
import { AppLogic } from './app_logic';

import { EngineRouter } from './components/engine';
import { EngineCreation } from './components/engine_creation';
import { EnginesOverview } from './components/engines';
Expand Down Expand Up @@ -52,52 +55,34 @@ describe('AppSearchUnconfigured', () => {
});

describe('AppSearchConfigured', () => {
beforeEach(() => {
// Mock resets
let wrapper: ShallowWrapper;

beforeAll(() => {
setMockValues({ myRole: {} });
setMockActions({ initializeAppData: () => {} });
wrapper = shallow(<AppSearchConfigured {...DEFAULT_INITIAL_APP_DATA} />);
});

it('renders with layout', () => {
const wrapper = shallow(<AppSearchConfigured />);

expect(wrapper.find(Layout)).toHaveLength(2);
expect(wrapper.find(Layout).last().prop('readOnlyMode')).toBeFalsy();
expect(wrapper.find(EnginesOverview)).toHaveLength(1);
expect(wrapper.find(EngineRouter)).toHaveLength(1);
});

it('initializes app data with passed props', () => {
const initializeAppData = jest.fn();
setMockActions({ initializeAppData });

shallow(<AppSearchConfigured ilmEnabled />);

expect(initializeAppData).toHaveBeenCalledWith({ ilmEnabled: true });
});

it('does not re-initialize app data', () => {
const initializeAppData = jest.fn();
setMockActions({ initializeAppData });
setMockValues({ myRole: {}, hasInitialized: true });

shallow(<AppSearchConfigured />);

expect(initializeAppData).not.toHaveBeenCalled();
it('mounts AppLogic with passed initial data props', () => {
expect(AppLogic).toHaveBeenCalledWith(DEFAULT_INITIAL_APP_DATA);
});

it('renders ErrorConnecting', () => {
setMockValues({ myRole: {}, errorConnecting: true });

const wrapper = shallow(<AppSearchConfigured />);
rerender(wrapper);

expect(wrapper.find(ErrorConnecting)).toHaveLength(1);
});

it('passes readOnlyMode state', () => {
setMockValues({ myRole: {}, readOnlyMode: true });

const wrapper = shallow(<AppSearchConfigured />);
rerender(wrapper);

expect(wrapper.find(Layout).first().prop('readOnlyMode')).toEqual(true);
});
Expand All @@ -106,14 +91,14 @@ describe('AppSearchConfigured', () => {
describe('canManageEngines', () => {
it('renders EngineCreation when user canManageEngines is true', () => {
setMockValues({ myRole: { canManageEngines: true } });
const wrapper = shallow(<AppSearchConfigured />);
rerender(wrapper);

expect(wrapper.find(EngineCreation)).toHaveLength(1);
});

it('does not render EngineCreation when user canManageEngines is false', () => {
setMockValues({ myRole: { canManageEngines: false } });
const wrapper = shallow(<AppSearchConfigured />);
rerender(wrapper);

expect(wrapper.find(EngineCreation)).toHaveLength(0);
});
Expand All @@ -122,14 +107,14 @@ describe('AppSearchConfigured', () => {
describe('canManageMetaEngines', () => {
it('renders MetaEngineCreation when user canManageMetaEngines is true', () => {
setMockValues({ myRole: { canManageMetaEngines: true } });
const wrapper = shallow(<AppSearchConfigured />);
rerender(wrapper);

expect(wrapper.find(MetaEngineCreation)).toHaveLength(1);
});

it('does not render MetaEngineCreation when user canManageMetaEngines is false', () => {
setMockValues({ myRole: { canManageMetaEngines: false } });
const wrapper = shallow(<AppSearchConfigured />);
rerender(wrapper);

expect(wrapper.find(MetaEngineCreation)).toHaveLength(0);
});
Expand Down
Loading

0 comments on commit fd3b3eb

Please sign in to comment.