diff --git a/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts b/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts index 08fd807b70..8361a47ae5 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts @@ -126,6 +126,30 @@ class ModelVersionDetails { this.findTable().find(`[data-label=Key]`).contains(name).parents('tr'), ); } + + findEditLabelsButton() { + return cy.findByTestId('editable-labels-group-edit'); + } + + findAddLabelButton() { + return cy.findByTestId('add-label-button'); + } + + findLabelInput(label: string) { + return cy.findByTestId(`edit-label-input-${label}`); + } + + findLabel(label: string) { + return cy.findByTestId(`editable-label-${label}`); + } + + findLabelErrorAlert() { + return cy.findByTestId('label-error-alert'); + } + + findSaveLabelsButton() { + return cy.findByTestId('editable-labels-group-save'); + } } class PropertyRow extends TableRow {} diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts index 3a0e6917e0..200a41253e 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts @@ -238,19 +238,6 @@ describe('Model version details', () => { it('Model version details tab', () => { modelVersionDetails.findVersionId().contains('1'); modelVersionDetails.findDescription().should('have.text', 'Description of model version'); - modelVersionDetails.findMoreLabelsButton().contains('6 more'); - modelVersionDetails.findMoreLabelsButton().click(); - modelVersionDetails.shouldContainsModalLabels([ - 'Testing label', - 'Financial', - 'Financial data', - 'Fraud detection', - 'Machine learning', - 'Next data to be overflow', - 'Label x', - 'Label y', - 'Label z', - ]); modelVersionDetails.findStorageEndpoint().contains('test-endpoint'); modelVersionDetails.findStorageRegion().contains('test-region'); modelVersionDetails.findStorageBucket().contains('test-bucket'); @@ -346,6 +333,31 @@ describe('Model version details', () => { modelVersionDetails.findModelVersionDropdownItem('Version 2').click(); modelVersionDetails.findVersionId().contains('2'); }); + + it('should handle label editing', () => { + modelVersionDetails.findEditLabelsButton().click(); + + modelVersionDetails.findAddLabelButton().click(); + + modelVersionDetails.findLabel('New Label').should('exist'); + + modelVersionDetails.findLabel('New Label').click().type('Updated Label{enter}'); + + modelVersionDetails.findSaveLabelsButton().should('exist').click(); + }); + + it('should handle label validation', () => { + modelVersionDetails.findEditLabelsButton().click(); + + modelVersionDetails.findLabelInput('Testing label').click(); + + modelVersionDetails.findLabelInput('Testing label').should('exist').should('be.visible'); + + const longLabel = 'a'.repeat(64); + modelVersionDetails.findLabelInput('Testing label').clear().type(`${longLabel}{enter}`); + modelVersionDetails.findLabelErrorAlert().should('contain', "can't exceed 63 characters"); + modelVersionDetails.findLabel('New Label').should('not.exist'); + }); }); describe('Registered deployments tab', () => { diff --git a/frontend/src/components/DashboardDescriptionListGroup.tsx b/frontend/src/components/DashboardDescriptionListGroup.tsx index 607f3b0920..9282eb8dc6 100644 --- a/frontend/src/components/DashboardDescriptionListGroup.tsx +++ b/frontend/src/components/DashboardDescriptionListGroup.tsx @@ -26,6 +26,7 @@ type EditableProps = { editButtonTestId?: string; saveButtonTestId?: string; cancelButtonTestId?: string; + discardButtonTestId?: string; }; export type DashboardDescriptionListGroupProps = { @@ -36,6 +37,7 @@ export type DashboardDescriptionListGroupProps = { contentWhenEmpty?: React.ReactNode; children: React.ReactNode; groupTestId?: string; + isSaveDisabled?: boolean; } & (({ isEditable: true } & EditableProps) | ({ isEditable?: false } & Partial)); const DashboardDescriptionListGroup: React.FC = (props) => { @@ -57,6 +59,7 @@ const DashboardDescriptionListGroup: React.FC @@ -74,7 +77,7 @@ const DashboardDescriptionListGroup: React.FC diff --git a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx index 309a4dca9b..027e19a6b0 100644 --- a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx +++ b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx @@ -1,223 +1,191 @@ -import * as React from 'react'; -import { - Button, - Form, - FormGroup, - FormHelperText, - HelperText, - HelperTextItem, - Label, - LabelGroup, - Modal, - TextInput, -} from '@patternfly/react-core'; -import { ExclamationCircleIcon } from '@patternfly/react-icons'; -import DashboardDescriptionListGroup, { - DashboardDescriptionListGroupProps, -} from './DashboardDescriptionListGroup'; +import React, { useState } from 'react'; +import { Label, LabelGroup, Alert, AlertVariant, Button } from '@patternfly/react-core'; +import DashboardDescriptionListGroup from './DashboardDescriptionListGroup'; -type EditableTextDescriptionListGroupProps = Partial< - Pick -> & { +interface EditableLabelsProps { labels: string[]; - saveEditedLabels: (labels: string[]) => Promise; - allExistingKeys?: string[]; + onLabelsChange: (labels: string[]) => Promise; isArchive?: boolean; -}; + title?: string; + contentWhenEmpty?: string; + allExistingKeys: string[]; +} -const EditableLabelsDescriptionListGroup: React.FC = ({ +export const EditableLabelsDescriptionListGroup: React.FC = ({ title = 'Labels', contentWhenEmpty = 'No labels', labels, - saveEditedLabels, + onLabelsChange, isArchive, - allExistingKeys = labels, + allExistingKeys, }) => { - const [isEditing, setIsEditing] = React.useState(false); - const [unsavedLabels, setUnsavedLabels] = React.useState(labels); - const [isSavingEdits, setIsSavingEdits] = React.useState(false); + const [isEditing, setIsEditing] = useState(false); + const [isSavingEdits, setIsSavingEdits] = useState(false); + const [unsavedLabels, setUnsavedLabels] = useState(labels); + const [labelErrors, setLabelErrors] = useState<{ [key: string]: string }>({}); - const editUnsavedLabel = (newText: string, index: number) => { - if (isSavingEdits) { - return; + const validateLabel = (text: string, currentLabel?: string): string | null => { + if (currentLabel === text) { + return null; + } + + const isDuplicate = + unsavedLabels.some((key) => key !== currentLabel && key === text) || + allExistingKeys.some((key) => key !== currentLabel && key === text); + + if (isDuplicate) { + return `"${text}" already exists. Use a unique name that doesn't match any existing key or property`; + } + if (text.length > 63) { + return "Label text can't exceed 63 characters"; } - const copy = [...unsavedLabels]; - copy[index] = newText; - setUnsavedLabels(copy); + return null; }; + + const handleEditComplete = ( + _event: MouseEvent | KeyboardEvent, + newText: string, + currentLabel?: string, + ) => { + const error = validateLabel(newText, currentLabel); + if (error) { + setLabelErrors({ [newText]: error }); + setUnsavedLabels((prev) => { + const filtered = prev.filter((label) => label !== currentLabel); + return [...filtered, newText]; + }); + } else if (newText) { + setUnsavedLabels((prev) => { + if (currentLabel) { + return [...prev, newText]; + } + const filtered = prev.filter((label) => label !== currentLabel); + return [...filtered, newText]; + }); + setLabelErrors({}); + } + }; + const removeUnsavedLabel = (text: string) => { if (isSavingEdits) { return; } setUnsavedLabels(unsavedLabels.filter((label) => label !== text)); }; - const addUnsavedLabel = (text: string) => { + + const addNewLabel = () => { if (isSavingEdits) { return; } - setUnsavedLabels([...unsavedLabels, text]); - }; - - // Don't allow a label that matches a non-label property key or another label (as they stand before saving) - // Note that this means if you remove a label and add it back before saving, that is valid - const reservedKeys = [ - ...allExistingKeys.filter((key) => !labels.includes(key)), - ...unsavedLabels, - ]; + const baseLabel = 'New Label'; + let counter = 1; + let newLabel = baseLabel; - const [isAddLabelModalOpen, setIsAddLabelModalOpen] = React.useState(false); - const [addLabelInputValue, setAddLabelInputValue] = React.useState(''); - const addLabelInputRef = React.useRef(null); - let addLabelValidationError: string | null = null; - if (reservedKeys.includes(addLabelInputValue)) { - addLabelValidationError = 'Label must not match an existing label or property key'; - } else if (addLabelInputValue.length > 63) { - addLabelValidationError = "Label text can't exceed 63 characters"; - } - - const toggleAddLabelModal = () => { - setAddLabelInputValue(''); - setIsAddLabelModalOpen(!isAddLabelModalOpen); - }; - React.useEffect(() => { - if (isAddLabelModalOpen && addLabelInputRef.current) { - addLabelInputRef.current.focus(); + while (unsavedLabels.includes(newLabel)) { + newLabel = `${baseLabel} ${counter}`; + counter++; } - }, [isAddLabelModalOpen]); - const addLabelModalSubmitDisabled = !addLabelInputValue || !!addLabelValidationError; - const submitAddLabelModal = (event?: React.FormEvent) => { - event?.preventDefault(); - if (!addLabelModalSubmitDisabled) { - addUnsavedLabel(addLabelInputValue); - toggleAddLabelModal(); - } + setUnsavedLabels((prev) => [...prev, newLabel]); }; return ( - <> - 0} + contentWhenEditing={ + <> - Add label - - ) - } + numLabels={10} + expandedText="Show Less" + collapsedText="Show More" > - {unsavedLabels.map((label, index) => ( + {unsavedLabels.map((label, index, array) => ( ))} + + {Object.keys(labelErrors).length > 0 && ( + + )} + + } + onEditClick={() => { + setUnsavedLabels(labels); + setLabelErrors({}); + setIsEditing(true); + }} + onSaveEditsClick={async () => { + if (Object.keys(labelErrors).length > 0) { + return; } - onEditClick={() => { - setUnsavedLabels(labels); - setIsEditing(true); - }} - onSaveEditsClick={async () => { - setIsSavingEdits(true); - try { - await saveEditedLabels(unsavedLabels); - } finally { - setIsSavingEdits(false); - } - setIsEditing(false); - }} - onDiscardEditsClick={() => { - setUnsavedLabels(labels); + setIsSavingEdits(true); + try { + await onLabelsChange(unsavedLabels); + } finally { + setIsSavingEdits(false); setIsEditing(false); - }} - > - - {labels.map((label) => ( - - ))} - - - {isAddLabelModalOpen ? ( - - Save - , - , - ]} - > -
- - , value: string) => - setAddLabelInputValue(value) - } - ref={addLabelInputRef} - isRequired - validated={addLabelValidationError ? 'error' : 'default'} - /> - {addLabelValidationError && ( - - - } variant="error"> - {addLabelValidationError} - - - - )} - -
-
- ) : null} - + } + }} + onDiscardEditsClick={() => { + setUnsavedLabels(labels); + setIsEditing(false); + }} + > + + {labels.map((label) => ( + + ))} + + ); }; - -export default EditableLabelsDescriptionListGroup; diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx index 7bf00b05e0..d9e00408fa 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx @@ -3,7 +3,7 @@ import { DescriptionList, Flex, FlexItem, TextVariants, Title } from '@patternfl import { ModelVersion } from '~/concepts/modelRegistry/types'; import DashboardDescriptionListGroup from '~/components/DashboardDescriptionListGroup'; import EditableTextDescriptionListGroup from '~/components/EditableTextDescriptionListGroup'; -import EditableLabelsDescriptionListGroup from '~/components/EditableLabelsDescriptionListGroup'; +import { EditableLabelsDescriptionListGroup } from '~/components/EditableLabelsDescriptionListGroup'; import ModelPropertiesDescriptionListGroup from '~/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup'; import { getLabels, mergeUpdatedLabels } from '~/pages/modelRegistry/screens/utils'; import useModelArtifactsByVersionId from '~/concepts/modelRegistry/apiHooks/useModelArtifactsByVersionId'; @@ -61,7 +61,9 @@ const ModelVersionDetailsView: React.FC = ({ labels={getLabels(mv.customProperties)} isArchive={isArchiveVersion} allExistingKeys={Object.keys(mv.customProperties)} - saveEditedLabels={(editedLabels) => + title="Labels" + contentWhenEmpty="No labels" + onLabelsChange={(editedLabels) => apiState.api .patchModelVersion( {}, @@ -72,6 +74,7 @@ const ModelVersionDetailsView: React.FC = ({ ) .then(refresh) } + data-testid="model-version-labels" /> = ({ labels={getLabels(rm.customProperties)} isArchive={isArchiveModel} allExistingKeys={Object.keys(rm.customProperties)} - saveEditedLabels={(editedLabels) => + title="Labels" + contentWhenEmpty="No labels" + onLabelsChange={(editedLabels) => apiState.api .patchRegisteredModel( {},