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.

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

Hide notebook feature when MDS enabled and remove security dashboard API dependency calling #201

Merged
merged 9 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Integrate chatbot with sidecar service ([#164](https://github.com/opensearch-project/dashboards-assistant/pull/164))
- Add data source service ([#191](https://github.com/opensearch-project/dashboards-assistant/pull/191))
- Update router and controller to support MDS ([#190](https://github.com/opensearch-project/dashboards-assistant/pull/190))
- Refactor default data source retriever ([#197](https://github.com/opensearch-project/dashboards-assistant/pull/197))
- Hide notebook feature when MDS enabled and remove security dashboard plugin dependency ([#201](https://github.com/opensearch-project/dashboards-assistant/pull/201))
- Refactor default data source retriever ([#197](https://github.com/opensearch-project/dashboards-assistant/pull/197))
3 changes: 3 additions & 0 deletions common/constants/llm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@ export const ASSISTANT_API = {
ABORT_AGENT_EXECUTION: `${API_BASE}/abort`,
REGENERATE: `${API_BASE}/regenerate`,
TRACE: `${API_BASE}/trace`,
ACCOUNT: `${API_BASE}/account`,
} as const;

export const NOTEBOOK_API = {
CREATE_NOTEBOOK: `${NOTEBOOK_PREFIX}/note`,
SET_PARAGRAPH: `${NOTEBOOK_PREFIX}/set_paragraphs/`,
};

export const DEFAULT_USER_NAME = 'User';
3 changes: 1 addition & 2 deletions opensearch_dashboards.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@
"opensearchDashboardsUtils"
],
"optionalPlugins": [
"securityDashboards",
"dataSource",
"dataSourceManagement"
],
"configPath": [
"assistant"
"assistant"
]
}
26 changes: 4 additions & 22 deletions public/chat_header_button.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,10 @@ describe('<HeaderChatButton />', () => {
render(
<HeaderChatButton
application={applicationStart}
userHasAccess={true}
messageRenderers={{}}
actionExecutors={{}}
assistantActions={{} as AssistantActions}
currentAccount={{ username: 'test_user', tenant: 'test_tenant' }}
currentAccount={{ username: 'test_user' }}
/>
);

Expand Down Expand Up @@ -142,11 +141,10 @@ describe('<HeaderChatButton />', () => {
render(
<HeaderChatButton
application={applicationServiceMock.createStartContract()}
userHasAccess={true}
messageRenderers={{}}
actionExecutors={{}}
assistantActions={{} as AssistantActions}
currentAccount={{ username: 'test_user', tenant: 'test_tenant' }}
currentAccount={{ username: 'test_user' }}
/>
);
screen.getByLabelText('chat input').focus();
Expand All @@ -166,11 +164,10 @@ describe('<HeaderChatButton />', () => {
render(
<HeaderChatButton
application={applicationServiceMock.createStartContract()}
userHasAccess={true}
messageRenderers={{}}
actionExecutors={{}}
assistantActions={{} as AssistantActions}
currentAccount={{ username: 'test_user', tenant: 'test_tenant' }}
currentAccount={{ username: 'test_user' }}
/>
);
expect(screen.getByLabelText('chat input')).not.toHaveFocus();
Expand All @@ -183,29 +180,14 @@ describe('<HeaderChatButton />', () => {
expect(screen.getByLabelText('chat input')).toHaveFocus();
});

it('should disable chat input when no access', () => {
render(
<HeaderChatButton
application={applicationServiceMock.createStartContract()}
userHasAccess={false}
messageRenderers={{}}
actionExecutors={{}}
assistantActions={{} as AssistantActions}
currentAccount={{ username: 'test_user', tenant: 'test_tenant' }}
/>
);
expect(screen.getByLabelText('chat input')).toBeDisabled();
});

it('should not focus on chat input when no access and pressing global shortcut', () => {
render(
<HeaderChatButton
application={applicationServiceMock.createStartContract()}
userHasAccess={false}
messageRenderers={{}}
actionExecutors={{}}
assistantActions={{} as AssistantActions}
currentAccount={{ username: 'test_user', tenant: 'test_tenant' }}
currentAccount={{ username: 'test_user' }}
/>
);
expect(screen.getByLabelText('chat input')).not.toHaveFocus();
Expand Down
9 changes: 1 addition & 8 deletions public/chat_header_button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import { MountPointPortal } from '../../../src/plugins/opensearch_dashboards_rea

interface HeaderChatButtonProps {
application: ApplicationStart;
userHasAccess: boolean;
messageRenderers: Record<string, MessageRenderer>;
actionExecutors: Record<string, ActionExecutor>;
assistantActions: AssistantActions;
Expand Down Expand Up @@ -74,7 +73,6 @@ export const HeaderChatButton = (props: HeaderChatButtonProps) => {
flyoutFullScreen,
setFlyoutVisible,
setFlyoutComponent,
userHasAccess: props.userHasAccess,
messageRenderers: props.messageRenderers,
actionExecutors: props.actionExecutors,
currentAccount: props.currentAccount,
Expand All @@ -92,7 +90,6 @@ export const HeaderChatButton = (props: HeaderChatButtonProps) => {
flyoutFullScreen,
selectedTabId,
preSelectedTabId,
props.userHasAccess,
props.messageRenderers,
props.actionExecutors,
props.currentAccount,
Expand Down Expand Up @@ -157,9 +154,6 @@ export const HeaderChatButton = (props: HeaderChatButtonProps) => {
}, []);

useEffect(() => {
if (!props.userHasAccess) {
return;
}
const onGlobalMouseUp = (e: KeyboardEvent) => {
if (e.ctrlKey && e.key === '/') {
inputRef.current?.focus();
Expand All @@ -170,7 +164,7 @@ export const HeaderChatButton = (props: HeaderChatButtonProps) => {
return () => {
document.removeEventListener('keydown', onGlobalMouseUp);
};
}, [props.userHasAccess]);
}, []);

useEffect(() => {
const handleSuggestion = (event: { suggestion: string }) => {
Expand Down Expand Up @@ -235,7 +229,6 @@ export const HeaderChatButton = (props: HeaderChatButtonProps) => {
)}
</span>
}
disabled={!props.userHasAccess}
/>
<ChatContext.Provider value={chatContextValue}>
<ChatStateProvider>
Expand Down
21 changes: 19 additions & 2 deletions public/components/__tests__/chat_window_header_title.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ import { DataSourceServiceMock } from '../../services/data_source_service.mock';
const setup = ({
messages = [],
...rest
}: { messages?: IMessage[]; conversationId?: string | undefined } = {}) => {
}: {
messages?: IMessage[];
conversationId?: string | undefined;
dataSource?: DataSourceServiceMock;
} = {}) => {
const useCoreMock = {
services: {
...coreMock.createStart(),
Expand All @@ -38,7 +42,7 @@ const setup = ({
}),
reload: jest.fn(),
},
dataSource: new DataSourceServiceMock(),
dataSource: rest.dataSource ?? new DataSourceServiceMock(false),
},
};
useCoreMock.services.http.put.mockImplementation(() => Promise.resolve());
Expand Down Expand Up @@ -170,6 +174,19 @@ describe('<ChatWindowHeaderTitle />', () => {
});
});

it('should hide save to notebook button when MDS enabled', async () => {
const { renderResult } = setup({
messages: [{ type: 'input', contentType: 'text', content: 'foo' }],
dataSource: new DataSourceServiceMock(true),
});

fireEvent.click(renderResult.getByLabelText('toggle chat context menu'));

expect(
renderResult.queryByRole('button', { name: 'Save to notebook' })
).not.toBeInTheDocument();
});

it('should disable "Save to notebook" button when message does not include input', async () => {
const { renderResult } = setup({
messages: [{ type: 'output', content: 'bar', contentType: 'markdown' }],
Expand Down
31 changes: 19 additions & 12 deletions public/components/chat_window_header_title.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
EuiButtonIcon,
EuiToolTip,
} from '@elastic/eui';
import React, { useCallback, useState } from 'react';
import React, { useCallback, useMemo, useState } from 'react';
import { useChatContext } from '../contexts';
import { useChatActions, useChatState, useSaveChat } from '../hooks';
import { NotebookNameModal } from './notebook/notebook_name_modal';
Expand Down Expand Up @@ -57,6 +57,11 @@ export const ChatWindowHeaderTitle = React.memo(() => {
[chatContext, core.services.conversations]
);

const displayNotebookFeature = useMemo(() => {
// Notebook dashboard API doesn't support MDS for now, so we hide saving to notebook feature when MDS enabled.
return !core.services.dataSource.isMDSEnabled();
}, [core.services.dataSource.isMDSEnabled]);

const handleSaveNotebookModalClose = () => {
setSaveNotebookModalOpen(false);
};
Expand Down Expand Up @@ -87,17 +92,19 @@ export const ChatWindowHeaderTitle = React.memo(() => {
>
New conversation
</EuiContextMenuItem>,
<EuiContextMenuItem
key="save-as-notebook"
onClick={() => {
closePopover();
setSaveNotebookModalOpen(true);
}}
// User only can save conversation when he send a message at least.
disabled={chatState.messages.every((item) => item.type !== 'input')}
>
Save to notebook
</EuiContextMenuItem>,
displayNotebookFeature && (
<EuiContextMenuItem
key="save-as-notebook"
onClick={() => {
closePopover();
setSaveNotebookModalOpen(true);
}}
// User only can save conversation when he send a message at least.
disabled={chatState.messages.every((item) => item.type !== 'input')}
>
Save to notebook
</EuiContextMenuItem>
),
];

return (
Expand Down
3 changes: 1 addition & 2 deletions public/contexts/__tests__/chat_context.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ describe('useChatContext', () => {
flyoutFullScreen: true,
setFlyoutVisible: jest.fn(),
setFlyoutComponent: jest.fn(),
userHasAccess: true,
messageRenderers: {},
actionExecutors: {},
currentAccount: { username: 'foo', tenant: '' },
currentAccount: { username: 'foo' },
setTitle: jest.fn(),
setInteractionId: jest.fn(),
};
Expand Down
1 change: 0 additions & 1 deletion public/contexts/chat_context.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export interface IChatContext {
flyoutFullScreen: boolean;
setFlyoutVisible: React.Dispatch<React.SetStateAction<boolean>>;
setFlyoutComponent: React.Dispatch<React.SetStateAction<React.ReactNode | null>>;
userHasAccess: boolean;
messageRenderers: Record<string, MessageRenderer>;
actionExecutors: Record<string, ActionExecutor>;
currentAccount?: UserAccount;
Expand Down
2 changes: 0 additions & 2 deletions public/hooks/use_chat_actions.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,9 @@ describe('useChatActions hook', () => {
setInteractionId: setInteractionIdMock,
flyoutVisible: false,
flyoutFullScreen: false,
userHasAccess: false,
messageRenderers: {},
currentAccount: {
username: '',
tenant: '',
},
};

Expand Down
38 changes: 12 additions & 26 deletions public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
} from './services';
import { ConfigSchema } from '../common/types/config';
import { DataSourceService } from './services/data_source_service';
import { ASSISTANT_API, DEFAULT_USER_NAME } from '../common/constants/llm';

export const [getCoreStart, setCoreStart] = createGetterSetter<CoreStart>('CoreStart');

Expand All @@ -46,7 +47,7 @@ export const IncontextInsightComponent: React.FC<{ props: any }> = (props) => (
);

interface UserAccountResponse {
data: { roles: string[]; user_name: string; user_requested_tenant?: string };
user_name: string;
}

export class AssistantPlugin
Expand Down Expand Up @@ -76,27 +77,15 @@ export class AssistantPlugin
const actionExecutors: Record<string, ActionExecutor> = {};
const assistantActions: AssistantActions = {} as AssistantActions;
/**
* Returns {@link UserAccountResponse}. Provides default roles and user
* name if security plugin call fails.
* Returns {@link UserAccountResponse}. Provides user name.
*/
const getAccount: () => Promise<UserAccountResponse> = (() => {
let account: UserAccountResponse;
return async () => {
if (setupDeps.securityDashboards === undefined)
return { data: { roles: ['all_access'], user_name: 'dashboards_user' } };
if (account === undefined) {
account = await core.http
.get<UserAccountResponse>('/api/v1/configuration/account')
.catch((e) => {
console.error(`Failed to request user account information: ${String(e.body || e)}`);
return { data: { roles: [], user_name: '' } };
});
}
return account;
};
})();
const checkAccess = (account: Awaited<ReturnType<typeof getAccount>>) =>
account.data.roles.some((role) => ['all_access', 'assistant_user'].includes(role));
const getAccount: () => Promise<UserAccountResponse> = async () => {
const account = await core.http.get<UserAccountResponse>(ASSISTANT_API.ACCOUNT).catch((e) => {
console.error(`Failed to request user account information: ${String(e.body || e)}`);
return { user_name: DEFAULT_USER_NAME };
});
return account;
};

const dataSourceSetupResult = this.dataSourceService.setup({
uiSettings: core.uiSettings,
Expand All @@ -116,8 +105,7 @@ export class AssistantPlugin
dataSource: this.dataSourceService,
});
const account = await getAccount();
const username = account.data.user_name;
const tenant = account.data.user_requested_tenant ?? '';
const username = account.user_name;
this.incontextInsightRegistry?.setIsEnabled(this.config.incontextInsight.enabled);

coreStart.chrome.navControls.registerRight({
Expand All @@ -126,11 +114,10 @@ export class AssistantPlugin
<CoreContext.Provider>
<HeaderChatButton
application={coreStart.application}
userHasAccess={checkAccess(account)}
messageRenderers={messageRenderers}
actionExecutors={actionExecutors}
assistantActions={assistantActions}
currentAccount={{ username, tenant }}
currentAccount={{ username }}
/>
</CoreContext.Provider>
),
Expand All @@ -152,7 +139,6 @@ export class AssistantPlugin
actionExecutors[actionType] = execute;
},
chatEnabled: () => this.config.chat.enabled,
userHasAccess: async () => await getAccount().then(checkAccess),
assistantActions,
registerIncontextInsight: this.incontextInsightRegistry.register.bind(
this.incontextInsightRegistry
Expand Down
17 changes: 14 additions & 3 deletions public/services/data_source_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,23 @@
*/

export class DataSourceServiceMock {
constructor() {}
private _isMDSEnabled = true;
constructor(isMDSEnabled?: boolean) {
this._isMDSEnabled = isMDSEnabled ?? true;
}

getDataSourceQuery() {
wanglam marked this conversation as resolved.
Show resolved Hide resolved
const result = this._isMDSEnabled
? {
dataSourceId: '',
}
: {};
return new Promise((resolve) => {
resolve({ dataSourceId: '' });
resolve(result);
});
// return { dataSourceId: '' };
}

isMDSEnabled() {
return this._isMDSEnabled;
}
}
Loading
Loading