diff --git a/.changeset/metal-jokes-add.md b/.changeset/metal-jokes-add.md new file mode 100644 index 0000000000..faef714d87 --- /dev/null +++ b/.changeset/metal-jokes-add.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-react': patch +--- + +Make EntityOwnerPicker display metadata.title or spec.profile.displayName for mode=only-owners instead of metadata.name diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx index a7b98e890f..4eb957cc16 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx @@ -395,6 +395,10 @@ describe('', () => { ], }, }); + + mockedGetEntitiesByRef.mockResolvedValue({ + items: [...ownerEntitiesBatch1, ...ownerEntitiesBatch2], + }); }); it('renders all users and groups', async () => { @@ -410,14 +414,16 @@ describe('', () => { fireEvent.click(screen.getByTestId('owner-picker-expand')); await waitFor(() => - expect(screen.getByText('another-owner')).toBeInTheDocument(), + expect(screen.getByText('Another Owner')).toBeInTheDocument(), ); - ['some-owner', 'some-owner-2', 'test-namespace/another-owner-2'].forEach( - owner => { - expect(screen.getByText(owner)).toBeInTheDocument(); - }, - ); + [ + 'some-owner', + 'Some Owner 2', + 'Another Owner in Another Namespace', + ].forEach(owner => { + expect(screen.getByText(owner)).toBeInTheDocument(); + }); expect(mockedGetEntityFacets).toHaveBeenCalledTimes(1); @@ -429,8 +435,8 @@ describe('', () => { [ 'some-owner-batch-2', - 'some-owner-2-batch-2', - 'test-namespace/another-owner-2-batch-2', + 'Some Owner Batch 2', + 'Another Owner in Another Namespace Batch 2', ].forEach(owner => { expect(screen.getByText(owner)).toBeInTheDocument(); }); @@ -470,7 +476,11 @@ describe('', () => { , ); - expect(mockedGetEntitiesByRef).not.toHaveBeenCalled(); + expect(mockedGetEntitiesByRef).toHaveBeenCalledWith({ + entityRefs: [...ownerEntitiesBatch1, ...ownerEntitiesBatch2].map(entity => + stringifyEntityRef(entity), + ), + }); expect(updateFilters).toHaveBeenLastCalledWith({ owners: undefined, }); diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/useFacetsEntities.test.ts b/plugins/catalog-react/src/components/EntityOwnerPicker/useFacetsEntities.test.ts index 1d0d1130a6..742efaab9f 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/useFacetsEntities.test.ts +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/useFacetsEntities.test.ts @@ -16,12 +16,17 @@ import { renderHook, waitFor } from '@testing-library/react'; import { useFacetsEntities } from './useFacetsEntities'; import { CatalogApi } from '@backstage/catalog-client'; +import { Entity, parseEntityRef } from '@backstage/catalog-model'; const mockedGetEntityFacets: jest.MockedFn = jest.fn(); +const mockedGetEntitiesByRefs: jest.MockedFn = + jest.fn(); + const mockCatalogApi: Partial = { getEntityFacets: mockedGetEntityFacets, + getEntitiesByRefs: mockedGetEntitiesByRefs, }; jest.mock('@backstage/core-plugin-api', () => ({ @@ -34,6 +39,31 @@ describe('useFacetsEntities', () => { jest.resetAllMocks(); }); + const facetsFromEntityRefs = (entityRefs: string[]) => ({ + facets: { + 'relations.ownedBy': entityRefs.map(value => ({ count: 1, value })), + }, + }); + + const entitiesFromEntityRefs = ( + entityRefs: string[], + enrichedEntities: { [key: string]: Entity } = {}, + ) => ({ + items: entityRefs.map(ref => { + const compoundRef = parseEntityRef(ref); + return ( + enrichedEntities[ref] || { + apiVersion: 'backstage.io/v1beta1', + kind: compoundRef.kind, + metadata: { + name: compoundRef.name, + namespace: compoundRef.namespace, + }, + } + ); + }), + }); + it(`should return empty items when facets are loading`, () => { mockedGetEntityFacets.mockReturnValue(new Promise(() => {})); const { result } = renderHook(() => useFacetsEntities({ enabled: true })); @@ -41,14 +71,11 @@ describe('useFacetsEntities', () => { }); it(`should return the owners`, async () => { - mockedGetEntityFacets.mockResolvedValue({ - facets: { - 'relations.ownedBy': [ - { count: 1, value: 'component:default/e2' }, - { count: 1, value: 'component:default/e1' }, - ], - }, - }); + const entityRefs = ['component:default/e1', 'component:default/e2']; + mockedGetEntityFacets.mockResolvedValue(facetsFromEntityRefs(entityRefs)); + mockedGetEntitiesByRefs.mockResolvedValue( + entitiesFromEntityRefs(entityRefs), + ); const { result } = renderHook(() => useFacetsEntities({ enabled: true })); @@ -74,20 +101,46 @@ describe('useFacetsEntities', () => { }); }); - it(`should return the owners sorted by namespace, name and kind`, async () => { + it(`should return the owners sorted by namespace, (displayName or title or name) and kind`, async () => { const entityRefs = [ 'group:namespace/team-b', 'component:default/c', 'group:default/a', 'component:default/a', 'component:default/b', + 'group:default/d', + 'group:default/e', ]; - mockedGetEntityFacets.mockResolvedValue({ - facets: { - 'relations.ownedBy': entityRefs.map(value => ({ count: 1, value })), + const enrichedEntities: { [key: string]: Entity } = { + 'group:default/a': { + apiVersion: 'backstage.io/v1beta1', + kind: 'group', + metadata: { name: 'a', namespace: 'default', title: 'My title A' }, }, - }); + 'component:default/a': { + apiVersion: 'backstage.io/v1beta1', + kind: 'component', + metadata: { name: 'a', namespace: 'default', title: 'My title B' }, + }, + 'group:default/d': { + apiVersion: 'backstage.io/v1beta1', + kind: 'group', + metadata: { name: 'd', namespace: 'default' }, + spec: { profile: { displayName: 'My display name D' } }, + }, + 'group:default/e': { + apiVersion: 'backstage.io/v1beta1', + kind: 'group', + metadata: { name: 'e', namespace: 'default' }, + spec: { profile: { displayName: 'My display name E' } }, + }, + }; + + mockedGetEntityFacets.mockResolvedValue(facetsFromEntityRefs(entityRefs)); + mockedGetEntitiesByRefs.mockResolvedValue( + entitiesFromEntityRefs(entityRefs, enrichedEntities), + ); const { result } = renderHook(() => useFacetsEntities({ enabled: true })); @@ -96,16 +149,6 @@ describe('useFacetsEntities', () => { expect(result.current[0]).toEqual({ value: { items: [ - { - apiVersion: 'backstage.io/v1beta1', - kind: 'component', - metadata: { name: 'a', namespace: 'default' }, - }, - { - apiVersion: 'backstage.io/v1beta1', - kind: 'group', - metadata: { name: 'a', namespace: 'default' }, - }, { apiVersion: 'backstage.io/v1beta1', kind: 'component', @@ -116,6 +159,36 @@ describe('useFacetsEntities', () => { kind: 'component', metadata: { name: 'c', namespace: 'default' }, }, + { + apiVersion: 'backstage.io/v1beta1', + kind: 'group', + metadata: { name: 'd', namespace: 'default' }, + spec: { profile: { displayName: 'My display name D' } }, + }, + { + apiVersion: 'backstage.io/v1beta1', + kind: 'group', + metadata: { name: 'e', namespace: 'default' }, + spec: { profile: { displayName: 'My display name E' } }, + }, + { + apiVersion: 'backstage.io/v1beta1', + kind: 'group', + metadata: { + name: 'a', + namespace: 'default', + title: 'My title A', + }, + }, + { + apiVersion: 'backstage.io/v1beta1', + kind: 'component', + metadata: { + name: 'a', + namespace: 'default', + title: 'My title B', + }, + }, { apiVersion: 'backstage.io/v1beta1', kind: 'group', @@ -137,11 +210,10 @@ describe('useFacetsEntities', () => { 'component:default/b', ]; - mockedGetEntityFacets.mockResolvedValue({ - facets: { - 'relations.ownedBy': entityRefs.map(value => ({ count: 1, value })), - }, - }); + mockedGetEntityFacets.mockResolvedValue(facetsFromEntityRefs(entityRefs)); + mockedGetEntitiesByRefs.mockResolvedValue( + entitiesFromEntityRefs(entityRefs), + ); const { result } = renderHook(() => useFacetsEntities({ enabled: true })); @@ -249,11 +321,25 @@ describe('useFacetsEntities', () => { 'component:default/nade', ]; - mockedGetEntityFacets.mockResolvedValue({ - facets: { - 'relations.ownedBy': entityRefs.map(value => ({ count: 1, value })), + mockedGetEntityFacets.mockResolvedValue(facetsFromEntityRefs(entityRefs)); + const enrichedEntities: { [key: string]: Entity } = { + 'group:default/go': { + apiVersion: 'backstage.io/v1beta1', + kind: 'group', + metadata: { name: 'go', namespace: 'default', title: 'Hidden Spider' }, }, - }); + 'component:default/lemon': { + apiVersion: 'backstage.io/v1beta1', + kind: 'component', + metadata: { name: 'lemon', namespace: 'default' }, + spec: { + profile: { displayName: 'Lemon Spider' }, + }, + }, + }; + mockedGetEntitiesByRefs.mockResolvedValue( + entitiesFromEntityRefs(entityRefs, enrichedEntities), + ); const { result } = renderHook(() => useFacetsEntities({ enabled: true })); @@ -262,6 +348,8 @@ describe('useFacetsEntities', () => { expect(result.current[0]).toEqual({ value: { items: [ + enrichedEntities['group:default/go'], + enrichedEntities['component:default/lemon'], { apiVersion: 'backstage.io/v1beta1', kind: 'component', diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/useFacetsEntities.ts b/plugins/catalog-react/src/components/EntityOwnerPicker/useFacetsEntities.ts index 33b96d2494..10a5c5a8c2 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/useFacetsEntities.ts +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/useFacetsEntities.ts @@ -17,7 +17,8 @@ import { useApi } from '@backstage/core-plugin-api'; import useAsyncFn from 'react-use/esm/useAsyncFn'; import { catalogApiRef } from '../../api'; import { useState } from 'react'; -import { Entity, parseEntityRef } from '@backstage/catalog-model'; +import { Entity } from '@backstage/catalog-model'; +import get from 'lodash/get'; type FacetsCursor = { start: number; @@ -48,29 +49,37 @@ export function useFacetsEntities({ enabled }: { enabled: boolean }) { return []; } const facet = 'relations.ownedBy'; + const facetsResponse = await catalogApi.getEntityFacets({ + facets: [facet], + }); + const entityRefs = facetsResponse.facets[facet].map(e => e.value); + return catalogApi - .getEntityFacets({ facets: [facet] }) - .then(response => - response.facets[facet] - .map(e => e.value) - .map(ref => { - const { kind, name, namespace } = parseEntityRef(ref); - return { - apiVersion: 'backstage.io/v1beta1', - kind, - metadata: { name, namespace }, - }; - }) + .getEntitiesByRefs({ entityRefs }) + .then(resp => + resp.items + .filter(entity => entity !== undefined) + .map(entity => entity as Entity) .sort( (a, b) => (a.metadata.namespace || '').localeCompare( b.metadata.namespace || '', 'en-US', ) || - a.metadata.name.localeCompare(b.metadata.name, 'en-US') || + ( + get(a, 'spec.profile.displayName') || + a.metadata.title || + a.metadata.name + ).localeCompare( + get(b, 'spec.profile.displayName') || + b.metadata.title || + b.metadata.name, + 'en-US', + ) || a.kind.localeCompare(b.kind, 'en-US'), ), ) + .then(entities => entities) .catch(() => []); }); @@ -149,6 +158,10 @@ function filterEntity(text: string, entity: Entity) { return ( entity.kind.includes(normalizedText) || entity.metadata.namespace?.includes(normalizedText) || - entity.metadata.name.includes(normalizedText) + entity.metadata.name.includes(normalizedText) || + entity.metadata.title?.includes(normalizedText) || + (get(entity, 'spec.profile.displayName') as unknown as string)?.includes( + normalizedText, + ) ); } diff --git a/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx b/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx index 8caaef3563..23c681a145 100644 --- a/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx +++ b/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx @@ -121,6 +121,26 @@ describe('DefaultCatalogPage', () => { ], }, })), + getEntitiesByRefs: jest.fn().mockImplementation(async () => ({ + items: [ + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Group', + metadata: { + name: 'not-tools', + namespace: 'default', + }, + }, + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Group', + metadata: { + name: 'tools', + namespace: 'default', + }, + }, + ], + })), queryEntities: jest .fn() .mockImplementation(async (request: QueryEntitiesInitialRequest) => {