Skip to content

Commit

Permalink
Update 'Resource name' fields to meet UX guidelines: Image streams (o…
Browse files Browse the repository at this point in the history
  • Loading branch information
jeff-phillips-18 authored Nov 13, 2024
1 parent 037ecd0 commit f93a992
Show file tree
Hide file tree
Showing 8 changed files with 88 additions and 68 deletions.
7 changes: 4 additions & 3 deletions backend/src/routes/api/images/imageUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,10 @@ export const postImage = async (
'opendatahub.io/dashboard': 'true',
};
const imageStreams = (await getImageStreams(fastify, labels)) as ImageStream[];
const validName = imageStreams.filter((is) => is.metadata.name === body.name);
const name = body.name || `custom-${translateDisplayNameForK8s(body.display_name)}`;
const existingImage = imageStreams.find((is) => is.metadata.name === name);

if (validName.length > 0) {
if (existingImage) {
fastify.log.error('Duplicate name unable to add notebook image');
return {
success: false,
Expand All @@ -287,7 +288,7 @@ export const postImage = async (
body.recommendedAcceleratorIdentifiers ?? [],
),
},
name: `custom-${translateDisplayNameForK8s(body.display_name)}`,
name,
namespace: namespace,
labels: labels,
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { appChrome } from '~/__tests__/cypress/cypress/pages/appChrome';
import { Modal } from '~/__tests__/cypress/cypress/pages/components/Modal';
import { TableRow } from '~/__tests__/cypress/cypress/pages/components/table';
import { K8sNameDescriptionField } from '~/__tests__/cypress/cypress/pages/components/subComponents/K8sNameDescriptionField';
import { DeleteModal } from './components/DeleteModal';
import { TableToolbar } from './components/TableToolbar';

Expand Down Expand Up @@ -86,6 +87,8 @@ class NotebookImageDeleteModal extends DeleteModal {
}

class ImportUpdateNotebookImageModal extends Modal {
k8sNameDescription = new K8sNameDescriptionField('byon-image');

constructor(private edit = false) {
super(`${edit ? 'Update' : 'Import'} notebook image`);
}
Expand All @@ -99,11 +102,11 @@ class ImportUpdateNotebookImageModal extends Modal {
}

findNameInput() {
return this.find().findByTestId('byon-image-name-input');
return this.k8sNameDescription.findDisplayNameInput();
}

findDescriptionInput() {
return this.find().findByTestId('byon-image-description-input');
return this.k8sNameDescription.findDescriptionInput();
}

// Software tab
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,24 @@ describe('Notebook image settings', () => {

importNotebookImageModal.findSaveResourceButton('Software').click();
importNotebookImageModal.findSubmitButton().should('be.enabled');

// test resource name validation
importNotebookImageModal.k8sNameDescription.findResourceEditLink().click();
importNotebookImageModal.k8sNameDescription
.findResourceNameInput()
.should('have.attr', 'aria-invalid', 'false');
// Invalid character k8s names fail
importNotebookImageModal.k8sNameDescription
.findResourceNameInput()
.clear()
.type('InVaLiD vAlUe!');
importNotebookImageModal.k8sNameDescription
.findResourceNameInput()
.should('have.attr', 'aria-invalid', 'true');
importNotebookImageModal.findSubmitButton().should('be.disabled');
importNotebookImageModal.k8sNameDescription.findResourceNameInput().clear().type('image');
importNotebookImageModal.findSubmitButton().should('be.enabled');

let notebookImageTabRow = importNotebookImageModal.getSoftwareRow('software');
notebookImageTabRow.shouldHaveVersionColumn('version');

Expand Down Expand Up @@ -198,6 +216,7 @@ describe('Notebook image settings', () => {
expect(interception.request.body).to.eql({
/* eslint-disable-next-line camelcase */
display_name: 'image',
name: 'image',
url: 'image:latest',
description: '',
recommendedAcceleratorIdentifiers: [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ export const HelperTextItemValidCharacters: HelperTextItemType = ({ k8sName }) =

return (
<HelperTextItem variant={variant} hasIcon>
Must start and end with a letter or number. Valid characters include lowercase letters,
numbers, and hyphens (-).
{k8sName.state.invalidCharsMessage ||
'Must start and end with a letter or number. Valid characters include lowercase letters, numbers, and hyphens (-).'}
</HelperTextItem>
);
};
8 changes: 8 additions & 0 deletions frontend/src/concepts/k8s/K8sNameDescriptionField/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ export type K8sNameDescriptionFieldData = {
invalidCharacters: boolean;
/** If the maxLength is exceeded */
invalidLength: boolean;
/** Optional regexp for the resource name */
regexp?: RegExp;
/** Optional invalid characters message */
invalidCharsMessage?: string;
/**
* Optional safe prefix for translation.
* @see AdditionalCriteriaForTranslation
Expand Down Expand Up @@ -52,6 +56,10 @@ export type UseK8sNameDescriptionDataConfiguration = {
* @see AdditionalCriteriaForTranslation
*/
staticPrefix?: boolean;
/** Optional regexp for the resource name */
regexp?: RegExp;
/** Optional invalid characters message */
invalidCharsMessage?: string;
};

type K8sNameDescriptionFieldUpdateFunctionTemplate<T> = (
Expand Down
11 changes: 8 additions & 3 deletions frontend/src/concepts/k8s/K8sNameDescriptionField/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ export const setupDefaults = ({
limitNameResourceType,
safePrefix,
staticPrefix,
regexp,
invalidCharsMessage,
}: UseK8sNameDescriptionDataConfiguration): K8sNameDescriptionFieldData => {
let initialName = '';
let initialDescription = '';
Expand Down Expand Up @@ -76,6 +78,8 @@ export const setupDefaults = ({
maxLength: configuredMaxLength,
safePrefix,
staticPrefix,
regexp,
invalidCharsMessage,
touched: false,
},
},
Expand Down Expand Up @@ -111,7 +115,8 @@ export const handleUpdateLogic =
case 'k8sName':
changedData.k8sName = {
state: {
invalidCharacters: value.length > 0 ? !isValidK8sName(value) : false,
invalidCharacters:
value.length > 0 ? !isValidK8sName(value, existingData.k8sName.state.regexp) : false,
invalidLength: value.length > existingData.k8sName.state.maxLength,
touched: true,
},
Expand All @@ -130,7 +135,7 @@ export const isK8sNameDescriptionDataValid = ({
name,
k8sName: {
value,
state: { invalidCharacters, invalidLength },
state: { invalidCharacters, invalidLength, regexp },
},
}: K8sNameDescriptionFieldData): boolean =>
name.trim().length > 0 && isValidK8sName(value) && !invalidLength && !invalidCharacters;
name.trim().length > 0 && isValidK8sName(value, regexp) && !invalidLength && !invalidCharacters;
6 changes: 4 additions & 2 deletions frontend/src/concepts/k8s/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,10 @@ export const translateDisplayNameForK8s = (
additionalCriteria: AdditionalCriteriaForTranslation = {},
): string => translateDisplayNameForK8sAndReport(name, additionalCriteria)[0];

export const isValidK8sName = (name?: string): boolean =>
name === undefined || (name.length > 0 && /^[a-z0-9]([-a-z0-9]*[a-z0-9])?$/.test(name));
export const isValidK8sName = (
name?: string,
regExp = /^[a-z0-9]([-a-z0-9]*[a-z0-9])?$/,
): boolean => name === undefined || (name.length > 0 && regExp.test(name));

type ResourceWithConditions = K8sResourceCommon & { status?: { conditions?: K8sCondition[] } };

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import React from 'react';
import {
Form,
FormGroup,
TextInput,
Modal,
ModalVariant,
Tabs,
Expand All @@ -18,6 +17,10 @@ import DashboardModalFooter from '~/concepts/dashboard/DashboardModalFooter';
import { filterBlankPackages } from '~/pages/BYONImages/utils';
import { AcceleratorIdentifierMultiselect } from '~/pages/BYONImages/BYONImageModal/AcceleratorIdentifierMultiselect';
import DashboardPopupIconButton from '~/concepts/dashboard/DashboardPopupIconButton';
import K8sNameDescriptionField, {
useK8sNameDescriptionFieldData,
} from '~/concepts/k8s/K8sNameDescriptionField/K8sNameDescriptionField';
import { isK8sNameDescriptionDataValid } from '~/concepts/k8s/K8sNameDescriptionField/utils';
import ImageLocationField from './ImageLocationField';
import DisplayedContentTabContent from './DisplayedContentTabContent';

Expand All @@ -37,8 +40,6 @@ const ManageBYONImageModal: React.FC<ManageBYONImageModalProps> = ({ existingIma
);
const [isProgress, setIsProgress] = React.useState(false);
const [repository, setRepository] = React.useState('');
const [displayName, setDisplayName] = React.useState('');
const [description, setDescription] = React.useState('');
const [recommendedAcceleratorIdentifiers, setRecommendedAcceleratorIdentifiers] = React.useState<
string[]
>([]);
Expand All @@ -52,11 +53,22 @@ const ManageBYONImageModal: React.FC<ManageBYONImageModalProps> = ({ existingIma

const isEditing = editIndex !== undefined;

const { data: byonNameDesc, onDataChange: setByonNameDesc } = useK8sNameDescriptionFieldData({
initialData: existingImage
? {
name: existingImage.display_name,
k8sName: existingImage.name,
description: existingImage.description,
}
: undefined,
regexp: /^[a-z0-9]+(?:[._-][a-z0-9]+)*[a-z0-9]?$/,
invalidCharsMessage:
'Must start and end with a letter or number. Valid characters include lowercase letters, numbers, and the special characters: period (.), hyphen (-), and underscore (_). Special characters cannot appear consecutively.',
});

React.useEffect(() => {
if (existingImage) {
setRepository(existingImage.url);
setDisplayName(existingImage.display_name);
setDescription(existingImage.description);
setPackages(existingImage.packages);
setSoftware(existingImage.software);
setTempPackages(existingImage.packages);
Expand All @@ -65,46 +77,34 @@ const ManageBYONImageModal: React.FC<ManageBYONImageModalProps> = ({ existingIma
}
}, [existingImage]);

const onBeforeClose = (submitted: boolean) => {
onClose(submitted);
setIsProgress(false);
setActiveTabKey(DisplayedContentTab.SOFTWARE);
setRepository('');
setDisplayName('');
setDescription('');
setRecommendedAcceleratorIdentifiers([]);
setSoftware([]);
setPackages([]);
setTempSoftware([]);
setTempPackages([]);
setError(undefined);
};

const handleResponse = (response: ResponseStatus) => {
setIsProgress(false);
if (response.success === false) {
setError(new Error(response.error));
} else {
onBeforeClose(true);
onClose(true);
}
};

const submit = () => {
setIsProgress(true);
if (existingImage) {
updateBYONImage({
name: existingImage.name,
// eslint-disable-next-line camelcase
display_name: displayName,
description,
display_name: byonNameDesc.name,
description: byonNameDesc.description,
recommendedAcceleratorIdentifiers,
packages: filterBlankPackages(packages),
software: filterBlankPackages(software),
}).then(handleResponse);
} else {
importBYONImage({
// eslint-disable-next-line camelcase
display_name: displayName,
display_name: byonNameDesc.name,
name: byonNameDesc.k8sName.value,
url: repository,
description,
description: byonNameDesc.description,
recommendedAcceleratorIdentifiers,
provider: userName,
packages: filterBlankPackages(packages),
Expand All @@ -119,16 +119,21 @@ const ManageBYONImageModal: React.FC<ManageBYONImageModalProps> = ({ existingIma
variant={ModalVariant.medium}
title={`${existingImage ? 'Update' : 'Import'} notebook image`}
isOpen
onClose={() => onBeforeClose(false)}
onClose={() => onClose(false)}
showClose={!isEditing}
footer={
<DashboardModalFooter
error={error}
alertTitle={`Error ${existingImage ? 'updating' : 'importing'} notebook image`}
submitLabel={existingImage ? 'Update' : 'Import'}
isSubmitDisabled={isProgress || displayName === '' || repository === '' || isEditing}
isSubmitDisabled={
isProgress ||
!isK8sNameDescriptionDataValid(byonNameDesc) ||
repository === '' ||
isEditing
}
onSubmit={submit}
onCancel={() => onBeforeClose(false)}
onCancel={() => onClose(false)}
isCancelDisabled={isEditing}
/>
}
Expand All @@ -145,34 +150,11 @@ const ManageBYONImageModal: React.FC<ManageBYONImageModalProps> = ({ existingIma
location={repository}
setLocation={setRepository}
/>

<FormGroup label="Name" isRequired fieldId="byon-image-name-input">
<TextInput
id="byon-image-name-input"
isRequired
type="text"
data-testid="byon-image-name-input"
name="byon-image-name-input"
value={displayName}
onChange={(e, value) => {
setDisplayName(value);
}}
/>
</FormGroup>
<FormGroup label="Description" fieldId="byon-image-description-input">
<TextInput
id="byon-image-description-input"
isRequired
type="text"
data-testid="byon-image-description-input"
name="byon-image-description-input"
aria-describedby="byon-image-description-input"
value={description}
onChange={(e, value) => {
setDescription(value);
}}
/>
</FormGroup>
<K8sNameDescriptionField
data={byonNameDesc}
onDataChange={setByonNameDesc}
dataTestId="byon-image"
/>
<FormGroup
label="Accelerator identifier"
labelIcon={
Expand Down

0 comments on commit f93a992

Please sign in to comment.