Update ModelPropertiesDescriptionListGroup component filtering logic (#1297)

* Update ModelPropertiesDescriptionListGroup component filtering logic

Signed-off-by: Juntao Wang <[email protected]>

* Add hasError prop to ModelRegistrySelector

Signed-off-by: Juntao Wang <[email protected]>

* address comments

Signed-off-by: Juntao Wang <[email protected]>

---------

Signed-off-by: Juntao Wang <[email protected]>

Author: DaoDaoNoCode

clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup.tsx

@@ -5,14 +5,9 @@ import { PlusCircleIcon } from '@patternfly/react-icons';
 import text from '@patternfly/react-styles/css/utilities/Text/text';
 import spacing from '@patternfly/react-styles/css/utilities/Spacing/spacing';
 import { DashboardDescriptionListGroup } from 'mod-arch-shared';
-import {
-  filterCustomProperties,
-  getProperties,
-  mergeUpdatedProperty,
-} from '~/app/pages/modelRegistry/screens/utils';
+import { getProperties, mergeUpdatedProperty } from '~/app/pages/modelRegistry/screens/utils';
 import { ModelRegistryCustomProperties } from '~/app/types';
 import ModelPropertiesTableRow from '~/app/pages/modelRegistry/screens/ModelPropertiesTableRow';
-import { pipelineRunSpecificKeys } from './ModelVersionDetails/const';
 
 type ModelPropertiesDescriptionListGroupProps = {
   customProperties: ModelRegistryCustomProperties;
@@ -35,13 +30,9 @@ const ModelPropertiesDescriptionListGroup: React.FC<ModelPropertiesDescriptionLi
   const isEditingSomeRow = isAdding || editingPropertyKeys.length > 0;
 
   const [isSavingEdits, setIsSavingEdits] = React.useState(false);
-  const filteredCustomProperties = filterCustomProperties(
-    customProperties,
-    pipelineRunSpecificKeys,
-  );
 
   // We only show string properties with a defined value (no labels or other property types)
-  const filteredProperties = getProperties(filteredCustomProperties);
+  const filteredProperties = getProperties(customProperties);
 
   const [isShowingMoreProperties, setIsShowingMoreProperties] = React.useState(false);
   const keys = Object.keys(filteredProperties);

clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelRegistrySelector.tsx

@@ -36,13 +36,15 @@ type ModelRegistrySelectorProps = {
   onSelection: (modelRegistry: string) => void;
   primary?: boolean;
   isFullWidth?: boolean;
+  hasError?: boolean;
 };
 
 const ModelRegistrySelector: React.FC<ModelRegistrySelectorProps> = ({
   modelRegistry,
   onSelection,
   primary,
   isFullWidth,
+  hasError,
 }) => {
   const { modelRegistries, updatePreferredModelRegistry } = React.useContext(
     ModelRegistrySelectorContext,
@@ -97,7 +99,7 @@ const ModelRegistrySelector: React.FC<ModelRegistrySelectorProps> = ({
       isScrollable
       placeholder="Select a model registry"
       dataTestId="model-registry-selector-dropdown"
-      toggleProps={{ id: 'download-steps-logs-toggle' }}
+      toggleProps={{ id: 'download-steps-logs-toggle', status: hasError ? 'danger' : undefined }}
       toggleLabel={toggleLabel}
       aria-label="Model registry toggle"
       previewDescription={false}

clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersionDetails/const.ts

@@ -7,8 +7,3 @@ export enum ModelVersionDetailsTabTitle {
   DETAILS = 'Details',
   DEPLOYMENTS = 'Deployments',
 }
-export const pipelineRunSpecificKeys: string[] = [
-  '_registeredFromPipelineProject',
-  '_registeredFromPipelineRunId',
-  '_registeredFromPipelineRunName',
-];

clients/ui/frontend/src/app/pages/modelRegistry/screens/__tests__/utils.spec.ts

@@ -0,0 +1,63 @@
+/* eslint-disable camelcase */
+import {
+  ModelRegistryCustomProperties,
+  ModelRegistryMetadataType,
+  ModelRegistryStringCustomProperties,
+} from '~/app/types';
+import { getProperties } from '~/app/pages/modelRegistry/screens/utils';
+
+describe('getProperties', () => {
+  it('should return an empty object when customProperties is empty', () => {
+    const customProperties: ModelRegistryCustomProperties = {};
+    const result = getProperties(customProperties);
+    expect(result).toEqual({});
+  });
+
+  it('should return a filtered object including only string properties with a non-empty value', () => {
+    const customProperties: ModelRegistryCustomProperties = {
+      property1: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty' },
+      property2: {
+        metadataType: ModelRegistryMetadataType.STRING,
+        string_value: 'another-non-empty',
+      },
+      label1: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' },
+      label2: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' },
+      int1: { metadataType: ModelRegistryMetadataType.INT, int_value: '1' },
+      int2: { metadataType: ModelRegistryMetadataType.INT, int_value: '2' },
+    };
+    const result = getProperties(customProperties);
+    expect(result).toEqual({
+      property1: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty' },
+      property2: {
+        metadataType: ModelRegistryMetadataType.STRING,
+        string_value: 'another-non-empty',
+      },
+    } satisfies ModelRegistryStringCustomProperties);
+  });
+
+  it('should return an empty object when all values in customProperties are empty strings or non-string values', () => {
+    const customProperties: ModelRegistryCustomProperties = {
+      label1: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' },
+      label2: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' },
+      int1: { metadataType: ModelRegistryMetadataType.INT, int_value: '1' },
+      int2: { metadataType: ModelRegistryMetadataType.INT, int_value: '2' },
+    };
+    const result = getProperties(customProperties);
+    expect(result).toEqual({});
+  });
+
+  it('should return with _lastModified and _registeredFrom props filtered out', () => {
+    const customProperties: ModelRegistryCustomProperties = {
+      property1: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty' },
+      _lastModified: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty' },
+      _registeredFromSomething: {
+        metadataType: ModelRegistryMetadataType.STRING,
+        string_value: 'non-empty',
+      },
+    };
+    const result = getProperties(customProperties);
+    expect(result).toEqual({
+      property1: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty' },
+    });
+  });
+});

clients/ui/frontend/src/app/pages/modelRegistry/screens/utils.ts

@@ -45,15 +45,15 @@ export const mergeUpdatedLabels = (
   return customPropertiesCopy;
 };
 
-// Retrives the customProperties that are not labels (they have a defined string_value).
+// Retrieves the customProperties that are not special (_registeredFrom) or labels (they have a defined string_value).
 export const getProperties = <T extends ModelRegistryCustomProperties>(
   customProperties: T,
 ): ModelRegistryStringCustomProperties => {
   const initial: ModelRegistryStringCustomProperties = {};
   return Object.keys(customProperties).reduce((acc, key) => {
     // _lastModified is a property that is required to update the timestamp on the backend and we have a workaround for it. It should be resolved by
     // backend team
-    if (key === '_lastModified') {
+    if (key === '_lastModified' || /^_registeredFrom/.test(key)) {
       return acc;
     }
 
@@ -183,21 +183,4 @@ export const isValidHttpUrl = (value: string): boolean => {
   }
 };
 
-export const filterCustomProperties = (
-  customProperties: ModelRegistryCustomProperties,
-  keys: string[],
-): ModelRegistryCustomProperties => {
-  const filteredCustomProperties: ModelRegistryCustomProperties = {};
-  Object.keys(customProperties).forEach((key) => {
-    if (!keys.includes(key)) {
-      filteredCustomProperties[key] = customProperties[key];
-    }
-  });
-  return filteredCustomProperties;
-};
-
-export const isPipelineRunExist = (
-  customProperties: ModelRegistryCustomProperties,
-  keys: string[],
-): boolean => keys.every((key) => key in customProperties);
 export const isRedHatRegistryUri = (uri: string): boolean => uri.startsWith('oci://example.io/'); // TODO: Change this to a proper check