Fix test cases and update async logic.

Signed-off-by: Aramis Sennyey <sennyeya@amazon.com>

plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx

@@ -14,15 +14,20 @@
  * limitations under the License.
  */
 
-import { Entity, parseEntityRef } from '@backstage/catalog-model';
+import { Entity, stringifyEntityRef } from '@backstage/catalog-model';
 import { fireEvent, screen } from '@testing-library/react';
 import React from 'react';
 import { MockEntityListContextProvider } from '../../testUtils/providers';
 import { EntityOwnerFilter } from '../../filters';
 import { EntityOwnerPicker } from './EntityOwnerPicker';
 import { ApiProvider } from '@backstage/core-app-api';
-import { renderWithEffects, TestApiRegistry } from '@backstage/test-utils';
+import {
+  MockErrorApi,
+  renderWithEffects,
+  TestApiRegistry,
+} from '@backstage/test-utils';
 import { catalogApiRef, CatalogApi } from '../..';
+import { errorApiRef } from '@backstage/core-plugin-api';
 
 const ownerEntities: Entity[] = [
   {
@@ -38,12 +43,18 @@ const ownerEntities: Entity[] = [
     metadata: {
       name: 'some-owner-2',
     },
+    spec: {
+      profile: {
+        displayName: 'Some Owner 2',
+      },
+    },
   },
   {
     apiVersion: '1',
     kind: 'Group',
     metadata: {
       name: 'another-owner',
+      title: 'Another Owner',
     },
   },
 ];
@@ -96,57 +107,20 @@ const sampleEntities: Entity[] = [
 
 const getEntitiesByRefs = jest.fn(async ({ entityRefs }) => ({
   items: entityRefs.map((e: string) =>
-    ownerEntities.find(f => f.metadata.name === e),
+    ownerEntities.find(f => stringifyEntityRef(f) === e),
   ),
 }));
 const mockCatalogApi: Partial<CatalogApi> = {
   getEntitiesByRefs,
 };
+const mockErrorApi = new MockErrorApi();
 
 describe('<EntityOwnerPicker/>', () => {
-  const mockApis = TestApiRegistry.from([catalogApiRef, mockCatalogApi]);
+  const mockApis = TestApiRegistry.from(
+    [catalogApiRef, mockCatalogApi],
+    [errorApiRef, mockErrorApi],
+  );
 
-  it('renders display name when available', async () => {
-    const names: Record<string, string> = {
-      'some-owner': 'Some Team',
-      'some-owner-2': 'Other Team',
-      'another-owner': 'AnotherTeam',
-    };
-    getEntitiesByRefs.mockResolvedValueOnce({
-      items: ownerEntities.map(e => {
-        e.spec = {
-          ...e.spec,
-          profile: {
-            displayName: names[e.metadata.name],
-          },
-        };
-        return e;
-      }),
-    });
-    await renderWithEffects(
-      <ApiProvider apis={mockApis}>
-        <MockEntityListContextProvider
-          value={{ entities: sampleEntities, backendEntities: sampleEntities }}
-        >
-          <EntityOwnerPicker />
-        </MockEntityListContextProvider>
-      </ApiProvider>,
-    );
-    expect(screen.getByText('Owner')).toBeInTheDocument();
-
-    fireEvent.click(screen.getByTestId('owner-picker-expand'));
-    sampleEntities
-      .flatMap(e => e.relations?.map(r => parseEntityRef(r.targetRef).name))
-      .forEach(owner => {
-        expect(screen.getByText(names[owner as string])).toBeInTheDocument();
-      });
-  });
-
-  /**
-   * All previous test cases are still applicable for the case where there is no
-   *  owner entity returned from the API or the owner entity returned does not have
-   *  a display name.
-   */
   it('renders all owners', async () => {
     await renderWithEffects(
       <ApiProvider apis={mockApis}>
@@ -160,11 +134,9 @@ describe('<EntityOwnerPicker/>', () => {
     expect(screen.getByText('Owner')).toBeInTheDocument();
 
     fireEvent.click(screen.getByTestId('owner-picker-expand'));
-    sampleEntities
-      .flatMap(e => e.relations?.map(r => parseEntityRef(r.targetRef).name))
-      .forEach(owner => {
-        expect(screen.getByText(owner as string)).toBeInTheDocument();
-      });
+    ['Another Owner', 'some-owner', 'Some Owner 2'].forEach(owner => {
+      expect(screen.getByText(owner)).toBeInTheDocument();
+    });
   });
 
   it('renders unique owners in alphabetical order', async () => {
@@ -182,9 +154,9 @@ describe('<EntityOwnerPicker/>', () => {
     fireEvent.click(screen.getByTestId('owner-picker-expand'));
 
     expect(screen.getAllByRole('option').map(o => o.textContent)).toEqual([
-      'another-owner',
+      'Another Owner',
       'some-owner',
-      'some-owner-2',
+      'Some Owner 2',
     ]);
   });
 

plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx

@@ -16,7 +16,6 @@
 
 import {
   Entity,
-  parseEntityRef,
   RELATION_OWNED_BY,
   stringifyEntityRef,
 } from '@backstage/catalog-model';
@@ -36,11 +35,10 @@ import React, { useEffect, useMemo, useState } from 'react';
 import { useEntityList } from '../../hooks/useEntityListProvider';
 import { EntityOwnerFilter } from '../../filters';
 import { getEntityRelations } from '../../utils';
-import { humanizeEntityRef } from '../EntityRefLink';
 import useAsync from 'react-use/lib/useAsync';
-import { useApi } from '@backstage/core-plugin-api';
+import { errorApiRef, useApi } from '@backstage/core-plugin-api';
 import { catalogApiRef } from '../../api';
-import { humanizeEntity } from '../EntityRefLink/humanize';
+import { humanizeEntity, humanizeEntityRef } from '../EntityRefLink/humanize';
 
 /** @public */
 export type CatalogReactEntityOwnerPickerClassKey = 'input';
@@ -67,12 +65,74 @@ export const EntityOwnerPicker = () => {
     queryParameters: { owners: ownersParameter },
   } = useEntityList();
   const catalogApi = useApi(catalogApiRef);
+  const errorApi = useApi(errorApiRef);
 
   const queryParamOwners = useMemo(
     () => [ownersParameter].flat().filter(Boolean) as string[],
     [ownersParameter],
   );
 
+  const {
+    loading,
+    error,
+    value: ownerEntities,
+  } = useAsync(async () => {
+    const availableOwners = [
+      ...new Set(
+        backendEntities
+          .flatMap((e: Entity) =>
+            getEntityRelations(e, RELATION_OWNED_BY).map(o =>
+              stringifyEntityRef(o),
+            ),
+          )
+          .filter(Boolean) as string[],
+      ),
+    ];
+    const { items } = await catalogApi.getEntitiesByRefs({
+      entityRefs: availableOwners,
+      fields: [
+        'kind',
+        'metadata.name',
+        'metadata.title',
+        'metadata.namespace',
+        'spec.profile.displayName',
+      ],
+    });
+    return (
+      availableOwners
+        .map(
+          (e, i) =>
+            items[i] || ({ metadata: { name: e }, kind: 'Group' } as Entity),
+        )
+        // Keep the previous sorting logic.
+        .sort((a, b) => {
+          const nameA = humanizeEntity(a).toLocaleUpperCase('en-US'); // ignore upper and lowercase
+          const nameB = humanizeEntity(b).toLocaleUpperCase('en-US'); // ignore upper and lowercase
+          if (nameA < nameB) {
+            return -1;
+          }
+          if (nameA > nameB) {
+            return 1;
+          }
+
+          // names must be equal
+          return 0;
+        })
+    );
+  }, [backendEntities]);
+
+  useEffect(() => {
+    if (error) {
+      errorApi.post(
+        {
+          ...error,
+          message: `EntityOwnerPicker failed to initialize: ${error.message}`,
+        },
+        {},
+      );
+    }
+  }, [error, errorApi]);
+
   const [selectedOwners, setSelectedOwners] = useState(
     queryParamOwners.length ? queryParamOwners : filters.owners?.values ?? [],
   );
@@ -85,55 +145,16 @@ export const EntityOwnerPicker = () => {
     }
   }, [queryParamOwners]);
 
-  const availableOwners = useMemo(
-    () =>
-      [
-        ...new Set(
-          backendEntities
-            .flatMap((e: Entity) =>
-              getEntityRelations(e, RELATION_OWNED_BY).map(o =>
-                humanizeEntityRef(o, { defaultKind: 'group' }),
-              ),
-            )
-            .filter(Boolean) as string[],
-        ),
-      ].sort(),
-    [backendEntities],
-  );
-
-  const {
-    loading,
-    error,
-    value: ownerEntities,
-  } = useAsync(async () => {
-    const { items } = await catalogApi.getEntitiesByRefs({
-      entityRefs: availableOwners.map(ref =>
-        stringifyEntityRef(parseEntityRef(ref, { defaultKind: 'Group' })),
-      ),
-      fields: [
-        'kind',
-        'metadata.name',
-        'metadata.title',
-        'spec.profile.displayName',
-      ],
-    });
-    return availableOwners.map(
-      (e, i) =>
-        items.at(i) || ({ metadata: { name: e }, kind: 'Group' } as Entity),
-    );
-  }, [availableOwners]);
-
   useEffect(() => {
-    updateFilters({
-      owners:
-        selectedOwners.length && availableOwners.length
-          ? new EntityOwnerFilter(selectedOwners)
-          : undefined,
-    });
-  }, [selectedOwners, updateFilters, availableOwners]);
-
-  if (!availableOwners.length) return null;
-  if (error) throw error;
+    if (!loading && ownerEntities) {
+      updateFilters({
+        owners:
+          selectedOwners.length && ownerEntities.length
+            ? new EntityOwnerFilter(selectedOwners)
+            : undefined,
+      });
+    }
+  }, [selectedOwners, updateFilters, ownerEntities, loading]);
 
   return (
     <Box pb={1} pt={1}>
@@ -152,7 +173,9 @@ export const EntityOwnerPicker = () => {
             ) ?? []
           }
           onChange={(_: object, value: Entity[]) =>
-            setSelectedOwners(value.map(e => e.metadata.name))
+            setSelectedOwners(
+              value.map(e => humanizeEntityRef(e, { defaultKind: 'Group' })),
+            )
           }
           getOptionLabel={option =>
             humanizeEntity(option, { defaultKind: 'Group' })
@@ -175,6 +198,7 @@ export const EntityOwnerPicker = () => {
           renderInput={params => (
             <TextField
               {...params}
+              error={!!error}
               className={classes.input}
               variant="outlined"
             />

plugins/catalog-react/src/components/EntityRefLink/humanize.ts

@@ -18,8 +18,8 @@ import {
   Entity,
   CompoundEntityRef,
   DEFAULT_NAMESPACE,
-  UserEntity,
 } from '@backstage/catalog-model';
+import get from 'lodash/get';
 
 /**
  * @param defaultNamespace - if set to false then namespace is never omitted,
@@ -88,14 +88,12 @@ export function humanizeEntity(
     defaultNamespace?: string | false;
   },
 ) {
-  let title: string | undefined = undefined;
-  switch (entity.kind) {
-    case 'User':
-    case 'Group':
-      title = (entity as UserEntity).spec?.profile?.displayName;
-      break;
-    default:
-      title = entity.metadata.title;
+  for (const path of ['spec.profile.displayName', 'metadata.title']) {
+    const value = get(entity, path);
+    if (value && typeof value === 'string') {
+      return value;
+    }
   }
-  return title || humanizeEntityRef(entity, opts);
+
+  return humanizeEntityRef(entity, opts);
 }