Skip to content

Commit ed98edb

Browse files
Leo6Leologonoff
andcommitted
OCPBUGS-56892: Console can only show user name instead of full name as the display name (#15522)
* fix: Update CoreState type to include userResource for k8s API integration * feat: Add userResource management to core actions and reducer * feat: Introduce useUser hook for centralized user data management * refactor: Replace direct user data fetching with centralized useUser hook in telemetry and masthead components * test: auto generated the unit tests for useUser hook to validate user data retrieval and dispatch behavior * feat: Enhance useUser hook to provide robust display name handling with fallbacks and add corresponding unit tests * fix: apply the review feedback * Apply suggestions from code review Co-authored-by: logonoff <[email protected]> * feat: run i18n * feat: Add UserKind type import * Apply suggestions from code review Co-authored-by: logonoff <[email protected]> * Update frontend/public/locales/en/public.json Co-authored-by: logonoff <[email protected]> * Update frontend/packages/console-dynamic-plugin-sdk/src/app/core/actions/core.ts Co-authored-by: logonoff <[email protected]> * feat: Import UserKind type in core actions * refactor: Update GetUserResource type to use UserKind for improved type safety * test: Update useUser.spec.ts to mock setUserResource and correct displayName expectation * test: fixing the failing CI issue --------- Co-authored-by: logonoff <[email protected]>
1 parent a13a3f6 commit ed98edb

File tree

12 files changed

+249
-25
lines changed

12 files changed

+249
-25
lines changed

frontend/packages/console-dynamic-plugin-sdk/src/app/core/actions/core.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
import { action, ActionType as Action } from 'typesafe-actions';
2+
import type { UserKind } from '@console/internal/module/k8s/types';
23
import { UserInfo } from '../../../extensions';
34
import { AdmissionWebhookWarning } from '../../redux-types';
45

56
export enum ActionType {
67
SetUser = 'setUser',
8+
SetUserResource = 'setUserResource',
79
BeginImpersonate = 'beginImpersonate',
810
EndImpersonate = 'endImpersonate',
911
SetActiveCluster = 'setActiveCluster',
@@ -12,6 +14,8 @@ export enum ActionType {
1214
}
1315

1416
export const setUser = (userInfo: UserInfo) => action(ActionType.SetUser, { userInfo });
17+
export const setUserResource = (userResource: UserKind) =>
18+
action(ActionType.SetUserResource, { userResource });
1519
export const beginImpersonate = (kind: string, name: string, subprotocols: string[]) =>
1620
action(ActionType.BeginImpersonate, { kind, name, subprotocols });
1721
export const endImpersonate = () => action(ActionType.EndImpersonate);
@@ -21,6 +25,7 @@ export const removeAdmissionWebhookWarning = (id) =>
2125
action(ActionType.RemoveAdmissionWebhookWarning, { id });
2226
const coreActions = {
2327
setUser,
28+
setUserResource,
2429
beginImpersonate,
2530
endImpersonate,
2631
setAdmissionWebhookWarning,

frontend/packages/console-dynamic-plugin-sdk/src/app/core/reducers/core.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { ActionType, CoreAction } from '../actions/core';
1414
export const coreReducer = (
1515
state: CoreState = {
1616
user: {},
17+
userResource: null,
1718
admissionWebhookWarnings: ImmutableMap<string, AdmissionWebhookWarning>(),
1819
},
1920
action: CoreAction,
@@ -47,6 +48,12 @@ export const coreReducer = (
4748
user: action.payload.userInfo,
4849
};
4950

51+
case ActionType.SetUserResource:
52+
return {
53+
...state,
54+
userResource: action.payload.userResource,
55+
};
56+
5057
case ActionType.SetAdmissionWebhookWarning:
5158
return {
5259
...state,

frontend/packages/console-dynamic-plugin-sdk/src/app/core/reducers/coreSelectors.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
import { Map as ImmutableMap } from 'immutable';
2+
import type { UserKind } from '@console/internal/module/k8s/types';
23
import { UserInfo } from '../../../extensions';
34
import { ImpersonateKind, SDKStoreState, AdmissionWebhookWarning } from '../../redux-types';
45

56
type GetImpersonate = (state: SDKStoreState) => ImpersonateKind;
67
type GetUser = (state: SDKStoreState) => UserInfo;
8+
type GetUserResource = (state: SDKStoreState) => UserKind;
79
type GetAdmissionWebhookWarnings = (
810
state: SDKStoreState,
911
) => ImmutableMap<string, AdmissionWebhookWarning>;
@@ -31,6 +33,13 @@ export const impersonateStateToProps = (state: SDKStoreState) => {
3133
*/
3234
export const getUser: GetUser = (state) => state.sdkCore.user;
3335

36+
/**
37+
* It provides user resource details from the redux store.
38+
* @param state the root state
39+
* @returns The user resource state.
40+
*/
41+
export const getUserResource: GetUserResource = (state) => state.sdkCore.userResource;
42+
3443
/**
3544
* It provides admission webhook warning data from the redux store.
3645
* @param state the root state

frontend/packages/console-dynamic-plugin-sdk/src/app/redux-types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { Map as ImmutableMap } from 'immutable';
2+
import type { UserKind } from '@console/internal/module/k8s/types';
23
import { UserInfo } from '../extensions/console-types';
34

45
export type K8sState = ImmutableMap<string, any>;
@@ -16,6 +17,7 @@ export type ImpersonateKind = {
1617

1718
export type CoreState = {
1819
user?: UserInfo;
20+
userResource?: UserKind;
1921
impersonate?: ImpersonateKind;
2022
admissionWebhookWarnings?: ImmutableMap<string, AdmissionWebhookWarning>;
2123
};

frontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,18 @@ jest.mock('@console/shared/src/hooks/useUserSettings', () => ({
2424
useUserSettings: jest.fn(),
2525
}));
2626

27+
jest.mock('@console/shared/src/hooks/useUser', () => ({
28+
useUser: jest.fn(() => ({
29+
user: {},
30+
userResource: {},
31+
userResourceLoaded: true,
32+
userResourceError: null,
33+
username: 'testuser',
34+
fullName: 'Test User',
35+
displayName: 'Test User',
36+
})),
37+
}));
38+
2739
const mockUserResource = {};
2840

2941
const exampleReturnValue = {
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
import { useSelector, useDispatch } from 'react-redux';
2+
import { useK8sGet } from '@console/internal/components/utils/k8s-get-hook';
3+
import { testHook } from '@console/shared/src/test-utils/hooks-utils';
4+
import { useUser } from '../useUser';
5+
6+
jest.mock('react-i18next', () => ({
7+
useTranslation: () => ({
8+
t: (key: string) => key,
9+
}),
10+
}));
11+
12+
jest.mock('react-redux', () => ({
13+
...jest.requireActual('react-redux'),
14+
useSelector: jest.fn(),
15+
useDispatch: jest.fn(),
16+
}));
17+
18+
jest.mock('@console/internal/components/utils/k8s-get-hook', () => ({
19+
useK8sGet: jest.fn(),
20+
}));
21+
22+
const mockSetUserResource = jest.fn((userResource) => ({
23+
type: 'setUserResource',
24+
payload: { userResource },
25+
}));
26+
27+
jest.mock('@console/dynamic-plugin-sdk', () => ({
28+
...jest.requireActual('@console/dynamic-plugin-sdk'),
29+
getUser: jest.fn(),
30+
getUserResource: jest.fn(),
31+
setUserResource: (...args) => mockSetUserResource(...args),
32+
}));
33+
34+
const mockDispatch = jest.fn();
35+
const mockUseSelector = useSelector as jest.Mock;
36+
const mockUseK8sGet = useK8sGet as jest.Mock;
37+
const mockUseDispatch = useDispatch as jest.Mock;
38+
39+
describe('useUser', () => {
40+
beforeEach(() => {
41+
jest.clearAllMocks();
42+
mockUseDispatch.mockReturnValue(mockDispatch);
43+
});
44+
45+
it('should return user data with displayName from fullName when available', () => {
46+
const mockUser = { username: '[email protected]', uid: '123' };
47+
const mockUserResource = { fullName: 'Test User', identities: ['testuser'] };
48+
49+
mockUseSelector
50+
.mockReturnValueOnce(mockUser) // for getUser
51+
.mockReturnValueOnce(mockUserResource); // for getUserResource
52+
53+
mockUseK8sGet.mockReturnValue([mockUserResource, true, null]);
54+
55+
const { result } = testHook(() => useUser());
56+
57+
expect(result.current.user).toEqual(mockUser);
58+
expect(result.current.userResource).toEqual(mockUserResource);
59+
expect(result.current.username).toBe('[email protected]');
60+
expect(result.current.fullName).toBe('Test User');
61+
expect(result.current.displayName).toBe('Test User'); // Should prefer fullName
62+
});
63+
64+
it('should fallback to username when fullName is not available', () => {
65+
const mockUser = { username: '[email protected]', uid: '123' };
66+
const mockUserResource = { identities: ['testuser'] }; // No fullName
67+
68+
mockUseSelector.mockReturnValueOnce(mockUser).mockReturnValueOnce(mockUserResource);
69+
70+
mockUseK8sGet.mockReturnValue([mockUserResource, true, null]);
71+
72+
const { result } = testHook(() => useUser());
73+
74+
expect(result.current.displayName).toBe('[email protected]'); // Should fallback to username
75+
expect(result.current.fullName).toBeUndefined();
76+
});
77+
78+
it('should dispatch setUserResource when user resource is loaded', () => {
79+
const mockUser = { username: '[email protected]' };
80+
const mockUserResource = { fullName: 'Test User' };
81+
82+
mockUseSelector.mockReturnValueOnce(mockUser).mockReturnValueOnce(null); // No userResource in Redux yet
83+
84+
mockUseK8sGet.mockReturnValue([mockUserResource, true, null]);
85+
86+
testHook(() => useUser());
87+
88+
expect(mockDispatch).toHaveBeenCalledWith({
89+
type: 'setUserResource',
90+
payload: { userResource: mockUserResource },
91+
});
92+
});
93+
94+
it('should handle edge cases with empty strings and fallback to "Unknown user"', () => {
95+
const mockUser = { username: '' }; // Empty username
96+
const mockUserResource = { fullName: ' ' }; // Whitespace-only fullName
97+
98+
mockUseSelector.mockReturnValueOnce(mockUser).mockReturnValueOnce(mockUserResource);
99+
100+
mockUseK8sGet.mockReturnValue([mockUserResource, true, null]);
101+
102+
const { result } = testHook(() => useUser());
103+
104+
expect(result.current.displayName).toBe('Unknown user'); // Should fallback to translated "Unknown user"
105+
});
106+
107+
it('should trim whitespace from fullName and username', () => {
108+
const mockUser = { username: ' [email protected] ' };
109+
const mockUserResource = { fullName: ' Test User ' };
110+
111+
mockUseSelector.mockReturnValueOnce(mockUser).mockReturnValueOnce(mockUserResource);
112+
113+
mockUseK8sGet.mockReturnValue([mockUserResource, true, null]);
114+
115+
const { result } = testHook(() => useUser());
116+
117+
expect(result.current.displayName).toBe('Test User'); // Should be trimmed
118+
});
119+
});

frontend/packages/console-shared/src/hooks/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,3 +37,4 @@ export * from './usePrometheusGate';
3737
export * from './useCopyCodeModal';
3838
export * from './useCopyLoginCommands';
3939
export * from './useQuickStartContext';
40+
export * from './useUser';

frontend/packages/console-shared/src/hooks/useTelemetry.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,13 @@ import {
66
TelemetryEventListener,
77
UserInfo,
88
} from '@console/dynamic-plugin-sdk';
9-
import { useK8sGet } from '@console/internal/components/utils/k8s-get-hook';
10-
import { UserModel } from '@console/internal/models';
119
import type { UserKind } from '@console/internal/module/k8s/types';
1210
import {
1311
CLUSTER_TELEMETRY_ANALYTICS,
1412
PREFERRED_TELEMETRY_USER_SETTING_KEY,
1513
USER_TELEMETRY_ANALYTICS,
1614
} from '../constants';
15+
import { useUser } from './useUser';
1716
import { useUserSettings } from './useUserSettings';
1817

1918
export interface ClusterProperties {
@@ -81,7 +80,8 @@ export const useTelemetry = () => {
8180
true,
8281
);
8382

84-
const [userResource, userResourceIsLoaded] = useK8sGet<UserKind>(UserModel, '~');
83+
// Use centralized user data instead of fetching directly
84+
const { userResource, userResourceLoaded: userResourceIsLoaded } = useUser();
8585

8686
const [extensions] = useResolvedExtensions<TelemetryListener>(isTelemetryListener);
8787

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
import { useEffect } from 'react';
2+
import { useTranslation } from 'react-i18next';
3+
import { useDispatch, useSelector } from 'react-redux';
4+
import { getUser, getUserResource, setUserResource } from '@console/dynamic-plugin-sdk';
5+
import { useK8sGet } from '@console/internal/components/utils/k8s-get-hook';
6+
import { UserModel } from '@console/internal/models';
7+
import type { UserKind } from '@console/internal/module/k8s/types';
8+
9+
/**
10+
* Custom hook that provides centralized user data fetching and management.
11+
* This hook fetches both the UserInfo (from authentication) and UserKind (from k8s API)
12+
* and stores them in Redux for use throughout the application.
13+
*
14+
* @returns Object containing user info, user resource, and loading states
15+
*/
16+
export const useUser = () => {
17+
const { t } = useTranslation('public');
18+
const dispatch = useDispatch();
19+
20+
// Get current user info from Redux (username, groups, etc.)
21+
const user = useSelector(getUser);
22+
23+
// Get current user resource from Redux (fullName, identities, etc.)
24+
const userResource = useSelector(getUserResource);
25+
26+
// Fetch user resource from k8s API
27+
const [userResourceData, userResourceLoaded, userResourceError] = useK8sGet<UserKind>(
28+
UserModel,
29+
'~',
30+
);
31+
32+
// Update Redux when user resource is loaded
33+
useEffect(() => {
34+
if (userResourceLoaded && userResourceData && !userResourceError) {
35+
dispatch(setUserResource(userResourceData));
36+
}
37+
}, [dispatch, userResourceData, userResourceLoaded, userResourceError]);
38+
39+
const currentUserResource = userResource || userResourceData;
40+
const currentUsername = user?.username;
41+
const currentFullName = currentUserResource?.fullName;
42+
43+
// Create a robust display name that always has a fallback
44+
const getDisplayName = () => {
45+
// Prefer fullName if it exists and is not empty
46+
if (currentFullName && currentFullName.trim()) {
47+
return currentFullName.trim();
48+
}
49+
// Fallback to username if it exists and is not empty
50+
if (currentUsername && currentUsername.trim()) {
51+
return currentUsername.trim();
52+
}
53+
// Final fallback for edge cases
54+
return t('Unknown user');
55+
};
56+
57+
return {
58+
user,
59+
userResource: currentUserResource,
60+
userResourceLoaded,
61+
userResourceError,
62+
// Computed properties for convenience
63+
username: currentUsername,
64+
fullName: currentFullName,
65+
displayName: getDisplayName(),
66+
};
67+
};

frontend/packages/topology/src/components/export-app/__tests__/ExportApplicationModal.spec.tsx

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { screen, render, configure, fireEvent } from '@testing-library/react';
1+
import { screen, configure, fireEvent } from '@testing-library/react';
22
import * as _ from 'lodash';
33
import { act } from 'react-dom/test-utils';
44
import * as k8sResourceModule from '@console/dynamic-plugin-sdk/src/utils/k8s/k8s-resource';
@@ -35,7 +35,7 @@ describe('ExportApplicationModal', () => {
3535
});
3636

3737
it('should show cancel and ok buttons when export app resource is not found', async () => {
38-
render(<ExportApplicationModal name="my-export" namespace="my-app" />);
38+
renderWithProviders(<ExportApplicationModal name="my-export" namespace="my-app" />);
3939
expect(screen.getByTestId('cancel-btn')).toBeInTheDocument();
4040
expect(screen.getByTestId('close-btn')).toBeInTheDocument();
4141
});
@@ -61,7 +61,7 @@ describe('ExportApplicationModal', () => {
6161
});
6262

6363
it('should show cancel and ok buttons when export app resource is created', async () => {
64-
render(
64+
renderWithProviders(
6565
<ExportApplicationModal
6666
name="my-export"
6767
namespace="my-app"
@@ -74,7 +74,9 @@ describe('ExportApplicationModal', () => {
7474

7575
it('should call k8sCreate with correct data on click of Ok button when the export resource is not created', async () => {
7676
const spyk8sCreate = jest.spyOn(k8sResourceModule, 'k8sCreate');
77-
render(<ExportApplicationModal namespace="my-app" name="my-export" cancel={jest.fn()} />);
77+
renderWithProviders(
78+
<ExportApplicationModal namespace="my-app" name="my-export" cancel={jest.fn()} />,
79+
);
7880
await act(async () => {
7981
fireEvent.click(screen.getByTestId('close-btn'));
8082
});
@@ -87,7 +89,7 @@ describe('ExportApplicationModal', () => {
8789
const spyk8sKill = jest.spyOn(k8sResourceModule, 'k8sKill');
8890
const spyk8sCreate = jest.spyOn(k8sResourceModule, 'k8sCreate');
8991

90-
render(
92+
renderWithProviders(
9193
<ExportApplicationModal
9294
name="my-export"
9395
namespace="my-app"

0 commit comments

Comments
 (0)