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..f7abdc4db9 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,82 @@ describe('Model version details', () => { modelVersionDetails.findModelVersionDropdownItem('Version 2').click(); modelVersionDetails.findVersionId().contains('2'); }); + + it('should handle label editing', () => { + modelVersionDetails.findEditLabelsButton().click(); + + modelVersionDetails.findAddLabelButton().click(); + cy.findByTestId('editable-label-group') + .should('exist') + .within(() => { + cy.contains('New Label').should('exist').click(); + cy.focused().type('First Label{enter}'); + }); + + modelVersionDetails.findAddLabelButton().click(); + cy.findByTestId('editable-label-group') + .should('exist') + .within(() => { + cy.contains('New Label').should('exist').click(); + cy.focused().type('Second Label{enter}'); + }); + + cy.findByTestId('editable-label-group').within(() => { + cy.contains('First Label').should('exist').click(); + cy.focused().type('Updated First Label{enter}'); + }); + + cy.findByTestId('editable-label-group').within(() => { + cy.contains('Second Label').parent().find('[data-testid^="remove-label-"]').click(); + }); + + modelVersionDetails.findSaveLabelsButton().should('exist').click(); + }); + + it('should validate label length', () => { + modelVersionDetails.findEditLabelsButton().click(); + + const longLabel = 'a'.repeat(64); + modelVersionDetails.findAddLabelButton().click(); + cy.findByTestId('editable-label-group') + .should('exist') + .within(() => { + cy.contains('New Label').should('exist').click(); + cy.focused().type(`${longLabel}{enter}`); + }); + + cy.findByTestId('label-error-alert') + .should('be.visible') + .within(() => { + cy.contains(`can't exceed 63 characters`).should('exist'); + }); + }); + + it('should validate duplicate labels', () => { + modelVersionDetails.findEditLabelsButton().click(); + + modelVersionDetails.findAddLabelButton().click(); + cy.findByTestId('editable-label-group') + .should('exist') + .within(() => { + cy.get('[data-testid^="editable-label-"]').last().click(); + cy.focused().type('{selectall}{backspace}Testing label{enter}'); + }); + + modelVersionDetails.findAddLabelButton().click(); + cy.findByTestId('editable-label-group') + .should('exist') + .within(() => { + cy.get('[data-testid^="editable-label-"]').last().click(); + cy.focused().type('{selectall}{backspace}Testing label{enter}'); + }); + + cy.findByTestId('label-error-alert') + .should('be.visible') + .within(() => { + cy.contains('Testing label already exists').should('exist'); + }); + }); }); describe('Registered deployments tab', () => { diff --git a/frontend/src/components/DashboardDescriptionListGroup.tsx b/frontend/src/components/DashboardDescriptionListGroup.tsx index 68d3425a58..6ef5dcb6ba 100644 --- a/frontend/src/components/DashboardDescriptionListGroup.tsx +++ b/frontend/src/components/DashboardDescriptionListGroup.tsx @@ -33,6 +33,7 @@ type EditableProps = { editButtonTestId?: string; saveButtonTestId?: string; cancelButtonTestId?: string; + discardButtonTestId?: string; }; export type DashboardDescriptionListGroupProps = { @@ -43,6 +44,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) => { @@ -64,6 +66,7 @@ const DashboardDescriptionListGroup: React.FC @@ -81,7 +84,7 @@ const DashboardDescriptionListGroup: React.FC diff --git a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx index 309a4dca9b..759e4e8579 100644 --- a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx +++ b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx @@ -1,223 +1,249 @@ -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'; - -type EditableTextDescriptionListGroupProps = Partial< - Pick -> & { +import React, { useState } from 'react'; +import { Label, LabelGroup, Alert, AlertVariant } from '@patternfly/react-core'; +import spacing from '@patternfly/react-styles/css/utilities/Spacing/spacing'; +import DashboardDescriptionListGroup from './DashboardDescriptionListGroup'; + +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 [hasSavedEdits, setHasSavedEdits] = useState(false); + const [unsavedLabels, setUnsavedLabels] = useState(labels); - const editUnsavedLabel = (newText: string, index: number) => { - if (isSavingEdits) { + const validateLabels = (): string[] => { + const errors: string[] = []; + + const duplicatesMap = new Map(); + unsavedLabels.forEach((label) => { + duplicatesMap.set(label, (duplicatesMap.get(label) || 0) + 1); + }); + + const duplicateLabels: string[] = []; + duplicatesMap.forEach((count, label) => { + if (count > 1) { + duplicateLabels.push(label); + } + }); + + if (duplicateLabels.length > 0) { + if (duplicateLabels.length === 1) { + const label = duplicateLabels[0] ?? ''; + errors.push(`**${label}** already exists as a label. Ensure that each label is unique.`); + } else { + const lastLabel = duplicateLabels.pop() ?? ''; + const formattedLabels = duplicateLabels.map((label) => `**${label}**`).join(', '); + errors.push( + `${formattedLabels} and **${lastLabel}** already exist as labels. Ensure that each label is unique.`, + ); + } + } + unsavedLabels.forEach((label) => { + if (allExistingKeys.includes(label) && !labels.includes(label)) { + errors.push( + `**${label}** already exists as a property key. Labels cannot use the same name as existing properties.`, + ); + } + if (label.length > 63) { + errors.push(`**${label}** can't exceed 63 characters`); + } + }); + + return errors; + }; + + const handleEditComplete = ( + _event: MouseEvent | KeyboardEvent, + newText: string, + currentLabel?: string, + ) => { + if (!newText) { return; } - const copy = [...unsavedLabels]; - copy[index] = newText; - setUnsavedLabels(copy); + + setUnsavedLabels((prev) => { + if (currentLabel) { + const index = prev.indexOf(currentLabel); + if (index === -1) { + return [...prev, newText]; + } + + const newLabels = [...prev]; + newLabels[index] = newText; + return newLabels; + } + return [...prev, newText]; + }); }; + 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]); - }; + const baseLabel = 'New Label'; + let counter = 1; + let newLabel = baseLabel; - // 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 [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); + while (unsavedLabels.includes(newLabel)) { + newLabel = `${baseLabel} ${counter}`; + counter++; + } + + setUnsavedLabels((prev) => { + const updated = [...prev, newLabel]; + return updated; + }); }; - React.useEffect(() => { - if (isAddLabelModalOpen && addLabelInputRef.current) { - addLabelInputRef.current.focus(); + + const labelErrors = validateLabels(); + const shouldBeRed = (label: string, index: number): boolean => { + const firstIndex = unsavedLabels.findIndex((l) => l === label); + + if (firstIndex !== index) { + return true; } - }, [isAddLabelModalOpen]); - - const addLabelModalSubmitDisabled = !addLabelInputValue || !!addLabelValidationError; - const submitAddLabelModal = (event?: React.FormEvent) => { - event?.preventDefault(); - if (!addLabelModalSubmitDisabled) { - addUnsavedLabel(addLabelInputValue); - toggleAddLabelModal(); + + if (allExistingKeys.includes(label) && !labels.includes(label)) { + return true; } + + return false; }; + // Add a ref for the alert + const alertRef = React.useRef(null); + return ( - <> - Add label - ) + ) : undefined } > {unsavedLabels.map((label, index) => ( ))} + {labelErrors.length > 0 && ( + +
    + {labelErrors.map((error, index) => ( +
  • + {error + .split('**') + .map((part, i) => (i % 2 === 0 ? part : {part}))} +
  • + ))} +
+
+ )} + + } + onEditClick={() => { + setUnsavedLabels(labels); + setIsEditing(true); + }} + onSaveEditsClick={async () => { + if (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 { + setHasSavedEdits(true); + setIsSavingEdits(false); setIsEditing(false); - }} + } + }} + onDiscardEditsClick={() => { + setUnsavedLabels(labels); + setIsEditing(false); + }} + > + - - {labels.map((label) => ( - - ))} - -
- {isAddLabelModalOpen ? ( - - Save - , - , - ]} - > -
- - , value: string) => - setAddLabelInputValue(value) - } - ref={addLabelInputRef} - isRequired - validated={addLabelValidationError ? 'error' : 'default'} - /> - {addLabelValidationError && ( - - - } variant="error"> - {addLabelValidationError} - - - - )} - -
-
- ) : null} - + {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 75303f36e5..a358a72f2f 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx @@ -10,7 +10,7 @@ import { 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'; @@ -67,7 +67,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( {}, @@ -78,6 +80,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( {},