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

Edit for existing project connection instances #3250

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
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,36 @@ describe('Connections', () => {

cy.wait('@createConnection');
});

it('Edit a connection', () => {
initIntercepts();
cy.interceptOdh('GET /api/connection-types', [
mockConnectionTypeConfigMap({
name: 'postgres',
fields: [
{
name: 'field A',
type: ConnectionTypeFieldType.ShortText,
envVar: 'field_env',
properties: {},
},
],
}),
]);
cy.interceptK8s(
'PUT',
SecretModel,
mockSecretK8sResource({
name: 'test2',
}),
).as('editConnection');

projectDetails.visitSection('test-project', 'connections');

connectionsPage.getConnectionRow('test2').findKebabAction('Edit').click();
cy.findByTestId(['field_env']).fill('new data');
cy.findByTestId('modal-submit-button').click();

cy.wait('@editConnection');
});
});
94 changes: 71 additions & 23 deletions frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,21 @@
import * as React from 'react';
import { Form, FormGroup, FormSection, MenuToggleStatus, Title } from '@patternfly/react-core';
import {
Flex,
FlexItem,
Form,
FormGroup,
FormSection,
MenuToggleStatus,
Title,
Truncate,
} from '@patternfly/react-core';
import ConnectionTypeFormFields from '~/concepts/connectionTypes/fields/ConnectionTypeFormFields';
import {
ConnectionTypeConfigMapObj,
ConnectionTypeValueType,
} from '~/concepts/connectionTypes/types';
import {
getDescriptionFromK8sResource,
getDisplayNameFromK8sResource,
getResourceNameFromK8sResource,
} from '~/concepts/k8s/utils';
Expand All @@ -17,6 +27,48 @@
} from '~/concepts/k8s/K8sNameDescriptionField/types';
import { ConnectionTypeDetailsHelperText } from './ConnectionTypeDetailsHelperText';

const getConnectionTypeSelectOptions = (
isPreview: boolean,
selectedConnectionType?: ConnectionTypeConfigMapObj,
connectionTypes?: ConnectionTypeConfigMapObj[],
): TypeaheadSelectOption[] => {
if (isPreview && selectedConnectionType?.metadata.annotations?.['openshift.io/display-name']) {
return [

Check warning on line 36 in frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx#L36

Added line #L36 was not covered by tests
{
value: '',
content: selectedConnectionType.metadata.annotations['openshift.io/display-name'],
isSelected: true,
},
];
}
if (!isPreview && connectionTypes) {
return connectionTypes.map((t) => ({
value: getResourceNameFromK8sResource(t),
content: getDisplayNameFromK8sResource(t),
description: (
<Flex direction={{ default: 'column' }} rowGap={{ default: 'rowGapNone' }}>
{getDescriptionFromK8sResource(t) && (
<FlexItem>
<Truncate content={getDescriptionFromK8sResource(t)} />
</FlexItem>
)}
{t.data?.category?.length && (
<FlexItem>
<Truncate content={`Category: ${t.data.category.join(', ')}`} />
</FlexItem>
)}
</Flex>
),
data: `${getDescriptionFromK8sResource(t)} ${t.data?.category?.join(' ')}`,
isSelected:
!!selectedConnectionType &&
getResourceNameFromK8sResource(t) ===
getResourceNameFromK8sResource(selectedConnectionType),
}));
}
return [];

Check warning on line 69 in frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx#L69

Added line #L69 was not covered by tests
};

type Props = Pick<
React.ComponentProps<typeof ConnectionTypeFormFields>,
'onChange' | 'onValidate'
Expand All @@ -30,6 +82,7 @@
connectionValues?: {
[key: string]: ConnectionTypeValueType;
};
disableTypeSelection?: boolean;
};

const ConnectionTypeForm: React.FC<Props> = ({
Expand All @@ -42,26 +95,12 @@
connectionValues,
onChange,
onValidate,
disableTypeSelection,
}) => {
const options: TypeaheadSelectOption[] = React.useMemo(() => {
if (isPreview && connectionType?.metadata.annotations?.['openshift.io/display-name']) {
return [
{
value: '',
content: connectionType.metadata.annotations['openshift.io/display-name'],
isSelected: true,
},
];
}
if (!isPreview && connectionTypes) {
return connectionTypes.map((t) => ({
value: getResourceNameFromK8sResource(t),
content: getDisplayNameFromK8sResource(t),
isSelected: t.metadata.name === connectionType?.metadata.name,
}));
}
return [];
}, [isPreview, connectionType?.metadata, connectionTypes]);
const options: TypeaheadSelectOption[] = React.useMemo(
() => getConnectionTypeSelectOptions(isPreview, connectionType, connectionTypes),
[isPreview, connectionType, connectionTypes],
);

return (
<Form>
Expand All @@ -73,7 +112,7 @@
onSelect={(_, selection) =>
setConnectionType?.(connectionTypes?.find((c) => c.metadata.name === selection))
}
isDisabled={isPreview || connectionTypes?.length === 1}
isDisabled={isPreview || disableTypeSelection}
placeholder={
isPreview && !connectionType?.metadata.annotations?.['openshift.io/display-name']
? 'Unspecified'
Expand All @@ -84,13 +123,22 @@
? { status: MenuToggleStatus.danger }
: undefined
}
isScrollable
popperProps={{ maxWidth: 'trigger' }}
filterFunction={(filterValue: string, filterOptions: TypeaheadSelectOption[]) =>
filterOptions.filter(
(o) =>

Check warning on line 130 in frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx#L130

Added line #L130 was not covered by tests
String(o.content).toLowerCase().includes(filterValue.toLowerCase()) ||
String(o.data).toLowerCase().includes(filterValue.toLowerCase()),

Check warning on line 132 in frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx#L132

Added line #L132 was not covered by tests
)
}
/>
{connectionType && (
<ConnectionTypeDetailsHelperText connectionType={connectionType} isPreview={isPreview} />
)}
</FormGroup>
{(isPreview || connectionType?.metadata.name) && (
<FormSection title="Connection details" style={{ marginTop: 0 }}>
<FormSection title="Connection details">
<K8sNameDescriptionField
dataTestId="connection-name-desc"
nameLabel="Connection name"
Expand All @@ -102,7 +150,7 @@
k8sName: {
value: '',
state: {
immutable: true,
immutable: false,
invalidCharacters: false,
invalidLength: false,
maxLength: 0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
ConnectionTypeField,
ConnectionTypeFieldType,
ConnectionTypeValueType,
isConnectionTypeDataField,
SectionField,
} from '~/concepts/connectionTypes/types';

Expand Down Expand Up @@ -47,6 +48,22 @@ const ConnectionTypeFormFields: React.FC<Props> = ({
[fields],
);

const unmatchedValues: ConnectionTypeDataField[] = React.useMemo(() => {
const unmatched: ConnectionTypeDataField[] = [];
for (const key in connectionValues) {
const matching = fields?.find((f) => isConnectionTypeDataField(f) && f.envVar === key);
if (!matching) {
unmatched.push({
type: ConnectionTypeFieldType.ShortText,
Copy link
Contributor

Choose a reason for hiding this comment

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

we could be smart. If it contains \n make it long text, or if it is true|false make it a boolean. But otherwise I think this works.

envVar: key,
name: key,
properties: {},
});
}
}
return unmatched;
}, [connectionValues, fields]);

const renderDataFields = (dataFields: ConnectionTypeDataField[]) =>
dataFields.map((field, i) => (
<DataFormFieldGroup key={i} field={field}>
Expand Down Expand Up @@ -74,6 +91,7 @@ const ConnectionTypeFormFields: React.FC<Props> = ({
<React.Fragment key={i}>{renderDataFields(fieldGroup.fields)}</React.Fragment>
),
)}
{unmatchedValues.length > 0 && renderDataFields(unmatchedValues)}
Copy link
Contributor

Choose a reason for hiding this comment

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

@simrandhaliw when editing a connection instance where the env var has no matching field in the connection type, should we section these off with a header and description which informs the user of their situation?

</>
);
};
Expand Down
45 changes: 44 additions & 1 deletion frontend/src/concepts/connectionTypes/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,12 @@
},
): Connection => {
const connectionValuesAsStrings = Object.fromEntries(
Object.entries(values).map(([key, value]) => [key, String(value)]),
Object.entries(values).map(([key, value]) => {
if (Array.isArray(value)) {
return [key, JSON.stringify(value)]; // multi select
}
return [key, String(value)];
}),
);
return {
apiVersion: 'v1',
Expand All @@ -158,3 +163,41 @@
stringData: connectionValuesAsStrings,
};
};

export const parseConnectionSecretValues = (
connection: Connection,
connectionType?: ConnectionTypeConfigMapObj,
): { [key: string]: ConnectionTypeValueType } => {
const response: { [key: string]: ConnectionTypeValueType } = {};

for (const [key, value] of Object.entries(connection.data ?? {})) {
const decodedString = window.atob(value);
const matchingField = connectionType?.data?.fields?.find(
(f) => isConnectionTypeDataField(f) && f.envVar === key,
);

if (matchingField?.type === ConnectionTypeFieldType.Boolean) {
response[key] = decodedString === 'true';
} else if (matchingField?.type === ConnectionTypeFieldType.Numeric) {
response[key] = Number(decodedString);
} else if (
matchingField?.type === ConnectionTypeFieldType.Dropdown &&
matchingField.properties.variant === 'multi'
) {
try {
const parsed = JSON.parse(decodedString);
if (Array.isArray(parsed)) {
response[key] = parsed.map((v) => String(v));
} else {
response[key] = [decodedString];

Check warning on line 192 in frontend/src/concepts/connectionTypes/utils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/concepts/connectionTypes/utils.ts#L191-L192

Added lines #L191 - L192 were not covered by tests
}
} catch {
response[key] = [decodedString];

Check warning on line 195 in frontend/src/concepts/connectionTypes/utils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/concepts/connectionTypes/utils.ts#L195

Added line #L195 was not covered by tests
}
} else {
response[key] = decodedString;
}
}

return response;
};
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ const K8sNameDescriptionField: React.FC<K8sNameDescriptionFieldProps> = ({
value={name}
onChange={(event, value) => onDataChange?.('name', value)}
/>
{!showK8sField && !!onDataChange && !k8sName.state.immutable && (
{!showK8sField && !k8sName.state.immutable && (
<FormHelperText>
{k8sName.value && (
<HelperText>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const ResourceNameField: React.FC<ResourceNameFieldProps> = ({
return <FormGroup {...formGroupProps}>{k8sName.value}</FormGroup>;
}

if (!allowEdit || !onDataChange) {
if (!allowEdit) {
return null;
}

Expand All @@ -52,7 +52,7 @@ const ResourceNameField: React.FC<ResourceNameFieldProps> = ({
name={`${dataTestId}-resourceName`}
isRequired
value={k8sName.value}
onChange={(event, value) => onDataChange('k8sName', value)}
onChange={(event, value) => onDataChange?.('k8sName', value)}
validated={validated}
/>
<HelperText>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import DashboardPopupIconButton from '~/concepts/dashboard/DashboardPopupIconBut
import { ProjectObjectType, typedEmptyImage } from '~/concepts/design/utils';
import { Connection } from '~/concepts/connectionTypes/types';
import { useWatchConnectionTypes } from '~/utilities/useWatchConnectionTypes';
import { createSecret } from '~/api';
import { createSecret, replaceSecret } from '~/api';
import ConnectionsTable from './ConnectionsTable';
import { ManageConnectionModal } from './ManageConnectionsModal';

Expand All @@ -26,6 +26,7 @@ const ConnectionsList: React.FC = () => {

const [manageConnectionModal, setManageConnectionModal] = React.useState<{
connection?: Connection;
isEdit?: boolean;
}>();

return (
Expand Down Expand Up @@ -75,6 +76,9 @@ const ConnectionsList: React.FC = () => {
connections={connections}
connectionTypes={connectionTypes}
refreshConnections={refreshConnections}
setManageConnectionModal={(modalConnection?: Connection) =>
setManageConnectionModal({ connection: modalConnection, isEdit: true })
}
/>
{manageConnectionModal && (
<ManageConnectionModal
Expand All @@ -87,7 +91,10 @@ const ConnectionsList: React.FC = () => {
refreshConnections();
}
}}
onSubmit={(connection: Connection) => createSecret(connection)}
onSubmit={(connection: Connection) =>
manageConnectionModal.isEdit ? replaceSecret(connection) : createSecret(connection)
}
isEdit={manageConnectionModal.isEdit}
/>
)}
</DetailsSection>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ type ConnectionsTableProps = {
connections: Connection[];
connectionTypes?: ConnectionTypeConfigMapObj[];
refreshConnections: () => void;
setManageConnectionModal: (connection: Connection) => void;
};

const ConnectionsTable: React.FC<ConnectionsTableProps> = ({
connections,
connectionTypes,
refreshConnections,
setManageConnectionModal,
}) => {
const [deleteConnection, setDeleteConnection] = React.useState<Connection>();

Expand All @@ -30,7 +32,7 @@ const ConnectionsTable: React.FC<ConnectionsTableProps> = ({
key={connection.metadata.name}
obj={connection}
connectionTypes={connectionTypes}
onEditConnection={() => undefined}
onEditConnection={() => setManageConnectionModal(connection)}
onDeleteConnection={() => setDeleteConnection(connection)}
/>
)}
Expand Down
Loading
Loading