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

[Multiple DataSource]Refactor authentication type selection in create and edit data source forms #3693

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Vis Builder] Removed Hard Coded Strings and Used i18n to transalte([#2867](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2867))
- [Console] Replace jQuery.ajax with core.http when calling OSD APIs in console ([#3080](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3080))
- [I18n] Fix Listr type errors and error handlers ([#3629](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3629))
- [Multiple DataSource]Refactor authentication type selection in create and edit data source forms to use EuiSelect ([#3693](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3693))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- [Multiple DataSource]Refactor authentication type selection in create and edit data source forms to use EuiSelect ([#3693](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3693))
- [Multiple DataSource] Present the authentication type choices in a drop-down ([#3693](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3693))

How does this sound?

Copy link
Member Author

Choose a reason for hiding this comment

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

this sound nicer!


### 🔩 Tests

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,12 @@ describe('Datasource Management: Create Datasource form', () => {
component.find(testSubjId).last().simulate('blur');
};

const setAuthTypeValue = (
comp: ReactWrapper<any, Readonly<{}>, React.Component<{}, {}, any>>,
value: AuthType
) => {
// @ts-ignore
comp
.find(authTypeIdentifier)
.first()
// @ts-ignore
.prop('onChange')(value);
comp.update();
const setAuthTypeValue = (testSubjId: string, value: string) => {
component.find(testSubjId).last().simulate('change', {
target: {
value,
},
});
};

beforeEach(() => {
Expand Down Expand Up @@ -98,15 +93,15 @@ describe('Datasource Management: Create Datasource form', () => {
/* Change option: No Authentication */
test('should validate when auth type changed & previously submit button clicked', () => {
/* Update Eui Super Select Value to No Auth*/
setAuthTypeValue(component, AuthType.NoAuth);
setAuthTypeValue(authTypeIdentifier, AuthType.NoAuth);
component.update();

/* Click on submit without any user input */
findTestSubject(component, 'createDataSourceButton').simulate('click');

const { authType, username, password } = getFields(component);

expect(authType.prop('idSelected')).toBe(AuthType.NoAuth);
expect(authType.prop('value')).toBe(AuthType.NoAuth);
expect(username.exists()).toBeFalsy(); // username field does not exist when No Auth option is selected
expect(password.exists()).toBeFalsy(); // password field does not exist when No Auth option is selected
});
Expand Down Expand Up @@ -138,7 +133,7 @@ describe('Datasource Management: Create Datasource form', () => {
/* Username & Password */
test('should create data source with username & password when all fields are valid', () => {
/* set form fields */
setAuthTypeValue(component, AuthType.UsernamePasswordType);
setAuthTypeValue(authTypeIdentifier, AuthType.UsernamePasswordType);
changeTextFieldValue(titleIdentifier, 'test');
changeTextFieldValue(descriptionIdentifier, 'test');
changeTextFieldValue(endpointIdentifier, 'https://test.com');
Expand All @@ -155,7 +150,7 @@ describe('Datasource Management: Create Datasource form', () => {
/* No Auth - Username & Password */
test('should create data source with No Auth when all fields are valid', () => {
/* set form fields */
setAuthTypeValue(component, AuthType.NoAuth); // No auth
setAuthTypeValue(authTypeIdentifier, AuthType.NoAuth); // No auth
changeTextFieldValue(titleIdentifier, 'test');
changeTextFieldValue(descriptionIdentifier, 'test');
changeTextFieldValue(endpointIdentifier, 'https://test.com');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
EuiForm,
EuiFormRow,
EuiPageContent,
EuiRadioGroup,
EuiSelect,
EuiSpacer,
EuiText,
} from '@elastic/eui';
Expand Down Expand Up @@ -120,8 +120,8 @@ export class CreateDataSourceForm extends React.Component<
});
};

onChangeAuthType = (value: string) => {
this.setState({ auth: { ...this.state.auth, type: value as AuthType } });
onChangeAuthType = (e: React.ChangeEvent<HTMLSelectElement>) => {
this.setState({ auth: { ...this.state.auth, type: e.target.value as AuthType } });
};

onChangeUsername = (e: { target: { value: any } }) => {
Expand Down Expand Up @@ -555,10 +555,10 @@ export class CreateDataSourceForm extends React.Component<
{/* Credential source */}
<EuiSpacer size="l" />
<EuiFormRow>
<EuiRadioGroup
<EuiSelect
options={credentialSourceOptions}
idSelected={this.state.auth.type}
onChange={(id) => this.onChangeAuthType(id)}
value={this.state.auth.type}
onChange={(e) => this.onChangeAuthType(e)}
name="Credential"
data-test-subj="createDataSourceFormAuthTypeSelect"
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const titleFormRowIdentifier = '[data-test-subj="editDataSourceTitleFormRow"]';
const endpointFieldIdentifier = '[data-test-subj="editDatasourceEndpointField"]';
const descriptionFieldIdentifier = 'dataSourceDescription';
const descriptionFormRowIdentifier = '[data-test-subj="editDataSourceDescriptionFormRow"]';
const authTypeRadioIdentifier = '[data-test-subj="editDataSourceSelectAuthType"]';
const authTypeSelectIdentifier = '[data-test-subj="editDataSourceSelectAuthType"]';
const usernameFieldIdentifier = 'datasourceUsername';
const usernameFormRowIdentifier = '[data-test-subj="editDatasourceUsernameFormRow"]';
const passwordFieldIdentifier = '[data-test-subj="updateDataSourceFormPasswordField"]';
Expand All @@ -49,6 +49,14 @@ describe('Datasource Management: Edit Datasource Form', () => {
comp.update();
};

const setAuthTypeValue = (testSubjId: string, value: string) => {
component.find(testSubjId).last().simulate('change', {
target: {
value,
},
});
};

describe('Case 1: With Username & Password', () => {
beforeEach(() => {
component = mount(
Expand Down Expand Up @@ -147,10 +155,7 @@ describe('Datasource Management: Edit Datasource Form', () => {
expect(component.find('UpdatePasswordModal').exists()).toBe(false);
});
test("should hide username & password fields when 'No Authentication' is selected as the credential type", () => {
act(() => {
// @ts-ignore
component.find(authTypeRadioIdentifier).first().prop('onChange')(AuthType.NoAuth);
});
setAuthTypeValue(authTypeSelectIdentifier, AuthType.NoAuth);
component.update();
expect(component.find(usernameFormRowIdentifier).exists()).toBe(false);
expect(component.find(passwordFieldIdentifier).exists()).toBe(false);
Expand Down Expand Up @@ -255,13 +260,7 @@ describe('Datasource Management: Edit Datasource Form', () => {
/* functionality */

test("should show username & password fields when 'Username & Password' is selected as the credential type", () => {
act(() => {
// @ts-ignore
component.find(authTypeRadioIdentifier).first().prop('onChange')(
// @ts-ignore
AuthType.UsernamePasswordType
);
});
setAuthTypeValue(authTypeSelectIdentifier, AuthType.UsernamePasswordType);
component.update();
expect(component.find(usernameFormRowIdentifier).exists()).toBe(true);
expect(component.find(passwordFieldIdentifier).exists()).toBe(true);
Expand All @@ -270,13 +269,7 @@ describe('Datasource Management: Edit Datasource Form', () => {
/* validation - Password */

test('should validate password as required field', () => {
act(() => {
// @ts-ignore
component.find(authTypeRadioIdentifier).first().prop('onChange')(
// @ts-ignore
AuthType.UsernamePasswordType
);
});
setAuthTypeValue(authTypeSelectIdentifier, AuthType.UsernamePasswordType);
component.update();

/* Validate empty username - required */
Expand All @@ -297,7 +290,7 @@ describe('Datasource Management: Edit Datasource Form', () => {
});

/* Save Changes */
test('should update the form with NoAUth on click save changes', async () => {
test('should update the form with NoAuth on click save changes', async () => {
await new Promise((resolve) =>
setTimeout(() => {
updateInputFieldAndBlur(component, descriptionFieldIdentifier, '');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
EuiFormRow,
EuiHorizontalRule,
EuiPanel,
EuiRadioGroup,
EuiSelect,
EuiSpacer,
EuiText,
} from '@elastic/eui';
Expand Down Expand Up @@ -162,8 +162,8 @@ export class EditDataSourceForm extends React.Component<EditDataSourceProps, Edi
});
};

onChangeAuthType = (value: string) => {
this.setState({ auth: { ...this.state.auth, type: value as AuthType } }, () => {
onChangeAuthType = (e: React.ChangeEvent<HTMLSelectElement>) => {
this.setState({ auth: { ...this.state.auth, type: e.target.value as AuthType } }, () => {
this.onChangeFormValues();
});
};
Expand Down Expand Up @@ -738,10 +738,10 @@ export class EditDataSourceForm extends React.Component<EditDataSourceProps, Edi
defaultMessage: 'Credential',
})}
>
<EuiRadioGroup
<EuiSelect
options={credentialSourceOptions}
idSelected={this.state.auth.type}
onChange={(id) => this.onChangeAuthType(id)}
value={this.state.auth.type}
onChange={(e) => this.onChangeAuthType(e)}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onChange={(e) => this.onChangeAuthType(e)}
onChange={this.onChangeAuthType}

nit: not blocking the PR for this though

Copy link
Member Author

Choose a reason for hiding this comment

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

made the change

name="Credential"
data-test-subj="editDataSourceSelectAuthType"
/>
Expand Down
6 changes: 3 additions & 3 deletions src/plugins/data_source_management/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ export enum AuthType {
}

export const credentialSourceOptions = [
{ id: AuthType.NoAuth, label: 'No authentication' },
{ id: AuthType.UsernamePasswordType, label: 'Username & Password' },
{ id: AuthType.SigV4, label: 'AWS SigV4' },
{ value: AuthType.NoAuth, text: 'No authentication' },
{ value: AuthType.UsernamePasswordType, text: 'Username & Password' },
{ value: AuthType.SigV4, text: 'AWS SigV4' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use i18n for these?

Copy link
Member Author

Choose a reason for hiding this comment

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

of course

];

export interface DataSourceAttributes extends SavedObjectAttributes {
Expand Down