Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 34 additions & 7 deletions src/ProgramEnrollments/ProgramInspector/ProgramInspector.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import {
Alert, Col, Row, Button, Input,
} from '@openedx/paragon';
import { getSsoRecords } from '../../users/data/api';
import { getSsoRecords, getUser } from '../../users/data/api';
import EnrollmentDetails from './EnrollmentDetails';
import SingleSignOnRecordCard from '../../users/SingleSignOnRecordCard';
import {
Expand All @@ -26,6 +26,8 @@
const [externalUserKey, setExternalUserKey] = useState(params.get('external_user_key'));
const [clickEventCall, setClickEventCall] = useState(false);

const [query, setQuery] = useState(null);

const getOrgKeyList = () => (orgKeyList
? orgKeyList.map((data) => ({
value: data,
Expand All @@ -41,13 +43,13 @@
setSsoRecords([]);
navigate('/programs');
} else {
const newLink = `/programs?edx_user=${
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we remove /programs again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier, we would navigate to this url. Now, we are only using it to store the "query" (backend api call) that we should make. I have refactored it a little. Hopefully it's clearer now. I also removed the ClickEvent state, as it wasn't being used anywhere.

const newLink = `?edx_user=${
username || ''
}&org_key=${activeOrgKey}&external_user_key=${externalUserKey || ''}`;
if (newLink === location.pathname + location.search) {
if (newLink === location.search) {
setClickEventCall(!clickEventCall);
} else {
navigate(newLink);
setQuery(newLink);
}
}
};
Expand All @@ -66,13 +68,38 @@
setError(response.error);
setActiveOrgKey(response.org_keys);
setLearnerProgramEnrollment(response.learner_program_enrollments);
});
const name = response?.learner_program_enrollments?.user?.username;
return name;
}).then((name) => getUser(name)).then((res) => {
navigate(`?edx_user_id=${res.id}`);
})
.catch(err => {
console.error(err);

Check warning on line 77 in src/ProgramEnrollments/ProgramInspector/ProgramInspector.jsx

View workflow job for this annotation

GitHub Actions / tests

Unexpected console statement
setError('An error occured while fetching user id');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spell: occurred

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update in all places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

navigate('/programs');
});
}
};

useEffect(() => {
fetchInspectorData(location.search);
}, [location.search, clickEventCall]);
if (query) {
fetchInspectorData(query);
}
}, [query]);

useEffect(() => {
const userId = new URLSearchParams(location.search).get('edx_user_id');
if (userId) {
getUser(userId).then(res => {
setUsername(res.username);
setQuery(`?edx_user=${res.username}&org_key=${activeOrgKey}&external_user_key=${externalUserKey}`);
}).catch(err => {
console.error(err);

Check warning on line 97 in src/ProgramEnrollments/ProgramInspector/ProgramInspector.jsx

View workflow job for this annotation

GitHub Actions / tests

Unexpected console statement
setError('An error occured while fetching user id');
navigate('/programs');
});
}
}, []);

useEffect(() => {
if (!orgKeyList) {
Expand Down
76 changes: 60 additions & 16 deletions src/ProgramEnrollments/ProgramInspector/ProgramInspector.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,17 @@ import {
programInspectorErrorResponse,
} from './data/test/programInspector';
import ssoRecordsData from '../../users/data/test/ssoRecords';
import * as ssoApi from '../../users/data/api';
import * as ssoAndUserApi from '../../users/data/api';
import samlProvidersResponseValues from './data/test/samlProviders';
import verifiedNameHistory from '../../users/data/test/verifiedNameHistory';
import UserSummaryData from '../../users/data/test/userSummary';

const mockedNavigator = jest.fn();

jest.mock('react-router-dom', () => ({
...jest.requireActual('react-router-dom'),
useNavigate: () => mockedNavigator,
useLocation: () => ({ pathname: '/programs', search: '?edx_user_id=123' }),
}));

const ProgramEnrollmentsWrapper = () => (
Expand All @@ -38,23 +40,32 @@ describe('Program Inspector', () => {
let samlMock;
let ssoMock;
let verifiedNameMock;
let getUserMock;

const data = {
username: 'verified',
orgKey: samlProvidersResponseValues[0],
externalKey: 'testuser',
};

const updateWrapperState = async (w) => {
await new Promise((res) => { setTimeout(() => res(), 0); });
w.update();
};

beforeEach(() => {
ssoMock = jest
.spyOn(ssoApi, 'getSsoRecords')
.spyOn(ssoAndUserApi, 'getSsoRecords')
.mockImplementationOnce(() => Promise.resolve(ssoRecordsData));
samlMock = jest
.spyOn(api, 'getSAMLProviderList')
.mockImplementationOnce(() => Promise.resolve(samlProvidersResponseValues));
verifiedNameMock = jest
.spyOn(ssoApi, 'getVerifiedNameHistory')
.spyOn(ssoAndUserApi, 'getVerifiedNameHistory')
.mockImplementationOnce(() => Promise.resolve(verifiedNameHistory));
getUserMock = jest
.spyOn(ssoAndUserApi, 'getUser')
.mockImplementation(() => Promise.resolve(UserSummaryData.userData));
jest.clearAllMocks();
});

Expand All @@ -68,24 +79,26 @@ describe('Program Inspector', () => {
samlMock.mockReset();
ssoMock.mockReset();
verifiedNameMock.mockReset();
getUserMock.mockReset();
});

it('default render', async () => {
wrapper = mount(<ProgramEnrollmentsWrapper />);
apiMock = jest
.spyOn(api, 'getProgramEnrollmentsInspector')
.mockImplementationOnce(() => Promise.resolve(programInspectorErrorResponse));
.mockImplementation(() => Promise.resolve(programInspectorErrorResponse));

const usernameInput = wrapper.find("input[name='username']");
const externalKeyInput = wrapper.find("input[name='externalKey']");
expect(usernameInput.prop('defaultValue')).toEqual('');
expect(externalKeyInput.prop('defaultValue')).toEqual('');
expect(usernameInput.prop('defaultValue')).toEqual(undefined);
expect(externalKeyInput.prop('defaultValue')).toEqual(undefined);
});

it('render when username', async () => {
apiMock = jest
.spyOn(api, 'getProgramEnrollmentsInspector')
.mockImplementationOnce(() => Promise.resolve(programInspectorSuccessResponse));
.mockImplementation(() => Promise.resolve(programInspectorSuccessResponse));

wrapper = mount(<ProgramEnrollmentsWrapper />);

wrapper.find("input[name='username']").simulate(
Expand All @@ -98,9 +111,12 @@ describe('Program Inspector', () => {
);
wrapper.find('button.btn-primary').simulate('click');

expect(mockedNavigator).toHaveBeenCalledWith(
`/programs?edx_user=${data.username}&org_key=${data.orgKey}&external_user_key=`,
);
await waitFor(() => {
expect(mockedNavigator).toHaveBeenCalledWith(
`?edx_user_id=${UserSummaryData.userData.id}`,
);
});

waitFor(() => {
expect(wrapper.find('.inspector-name-row p.h5').at(0).text()).toEqual(
'Username',
Expand All @@ -120,7 +136,7 @@ describe('Program Inspector', () => {
it('render when external_user_key', async () => {
apiMock = jest
.spyOn(api, 'getProgramEnrollmentsInspector')
.mockImplementationOnce(() => Promise.resolve(programInspectorSuccessResponse));
.mockImplementation(() => Promise.resolve(programInspectorSuccessResponse));
wrapper = mount(<ProgramEnrollmentsWrapper />);

wrapper.find(
Expand All @@ -137,9 +153,12 @@ describe('Program Inspector', () => {
);
wrapper.find('button.btn-primary').simulate('click');

expect(mockedNavigator).toHaveBeenCalledWith(
`/programs?edx_user=&org_key=${data.orgKey}&external_user_key=${data.externalKey}`,
);
await waitFor(() => {
expect(mockedNavigator).toHaveBeenCalledWith(
`?edx_user_id=${UserSummaryData.userData.id}`,
);
});

waitFor(() => {
expect(wrapper.find('.inspector-name-row p.h5').at(0).text()).toEqual(
'Username',
Expand All @@ -159,7 +178,7 @@ describe('Program Inspector', () => {
it('render nothing when no username or external_user_key', async () => {
apiMock = jest
.spyOn(api, 'getProgramEnrollmentsInspector')
.mockImplementationOnce(() => Promise.resolve(programInspectorSuccessResponse));
.mockImplementation(() => Promise.resolve(programInspectorSuccessResponse));
wrapper = mount(<ProgramEnrollmentsWrapper />);

wrapper.find(
Expand Down Expand Up @@ -188,10 +207,35 @@ describe('Program Inspector', () => {
expect(wrapper.find('.inspector-name-row').exists()).toBeFalsy();
});

it('render when getUser fails', async () => {
apiMock = jest
.spyOn(api, 'getProgramEnrollmentsInspector')
.mockImplementation(() => Promise.resolve(programInspectorSuccessResponse));

getUserMock = jest
.spyOn(ssoAndUserApi, 'getUser')
.mockImplementation(() => Promise.reject(new Error('Error fetching User Info')));

wrapper = mount(<ProgramEnrollmentsWrapper />);

await updateWrapperState(wrapper);
expect(wrapper.find('Alert').at(0).text()).toEqual('An error occured while fetching user id');

wrapper.find(
"input[name='username']",
).simulate(
'change',
{ target: { value: 'AnonyMouse' } },
);
wrapper.find('button.btn-primary').simulate('click');
await updateWrapperState(wrapper);
expect(wrapper.find('Alert').at(0).text()).toEqual('An error occured while fetching user id');
});

it('check if SSO is present', async () => {
apiMock = jest
.spyOn(api, 'getProgramEnrollmentsInspector')
.mockImplementationOnce(() => Promise.resolve(programInspectorSuccessResponse));
.mockImplementation(() => Promise.resolve(programInspectorSuccessResponse));
wrapper = mount(<ProgramEnrollmentsWrapper />);

wrapper.find(
Expand Down
33 changes: 9 additions & 24 deletions src/users/UserPage.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { camelCaseObject } from '@edx/frontend-platform';
import React, {
useCallback, useContext, useEffect, useState, useLayoutEffect,
useCallback, useContext, useEffect, useState,
} from 'react';
import { useLocation, useNavigate } from 'react-router-dom';
import PageLoading from '../components/common/PageLoading';
Expand All @@ -11,7 +11,7 @@
import { getAllUserData } from './data/api';
import UserSearch from './UserSearch';
import LearnerInformation from './LearnerInformation';
import { LEARNER_INFO_TAB, TAB_PATH_MAP } from '../SupportToolsTab/constants';
import { TAB_PATH_MAP } from '../SupportToolsTab/constants';
import CancelRetirement from './account-actions/CancelRetirement';

// Supports urls such as /users/?username={username}, /users/?email={email} and /users/?lms_user_id={lms_user_id}
Expand Down Expand Up @@ -45,19 +45,13 @@
}
}

function getUpdatedURL(value) {
const updatedHistory = `${TAB_PATH_MAP['learner-information']}/?PARAM_NAME=${value}`;
let identifierType = '';
function getUpdatedURL(result) {
const lmsId = result?.user?.id;

if (isEmail(value)) {
identifierType = 'email';
} else if (isValidLMSUserID(value)) {
identifierType = 'lms_user_id';
} else if (isValidUsername(value)) {
identifierType = 'username';
if (lmsId) {
return `${TAB_PATH_MAP['learner-information']}/?lms_user_id=${lmsId}`;
}

return updatedHistory.replace('PARAM_NAME', identifierType);
return `${TAB_PATH_MAP['learner-information']}`;

Check warning on line 54 in src/users/UserPage.jsx

View check run for this annotation

Codecov / codecov/patch

src/users/UserPage.jsx#L54

Added line #L54 was not covered by tests
}

function processSearchResult(searchValue, result) {
Expand All @@ -69,7 +63,7 @@
navigate(`${TAB_PATH_MAP['learner-information']}`, { replace: true });
document.title = 'Support Tools | edX';
} else {
pushHistoryIfChanged(getUpdatedURL(searchValue));
pushHistoryIfChanged(getUpdatedURL(result));
document.title = `Support Tools | edX | ${searchValue}`;
}

Expand Down Expand Up @@ -137,16 +131,7 @@
} else if (params.get('lms_user_id') && params.get('lms_user_id') !== userIdentifier) {
handleFetchSearchResults(params.get('lms_user_id'));
}
}, [params.get('username'), params.get('email'), params.get('lms_user_id')]);

// To change the url with appropriate query param if query param info is not present in URL
useLayoutEffect(() => {
if (userIdentifier
&& location.pathname.indexOf(TAB_PATH_MAP[LEARNER_INFO_TAB]) !== -1
&& !(params.get('email') || params.get('username') || params.get('lms_user_id'))) {
pushHistoryIfChanged(getUpdatedURL(userIdentifier));
}
});
}, []);

return (
<main className="mt-3 mb-5">
Expand Down
65 changes: 65 additions & 0 deletions src/users/UserPage.test.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { mount } from 'enzyme';
import React from 'react';
import { MemoryRouter } from 'react-router-dom';
import { IntlProvider } from '@edx/frontend-platform/i18n';
import { waitFor } from '@testing-library/react';
import UserMessagesProvider from '../userMessages/UserMessagesProvider';
import UserPage from './UserPage';
import * as ssoAndUserApi from './data/api';
import UserSummaryData from './data/test/userSummary';
import verifiedNameHistoryData from './data/test/verifiedNameHistory';
import onboardingStatusData from './data/test/onboardingStatus';
import { entitlementsData } from './data/test/entitlements';
import enterpriseCustomerUsersData from './data/test/enterpriseCustomerUsers';
import { enrollmentsData } from './data/test/enrollments';
import ssoRecordsData from './data/test/ssoRecords';
import licensesData from './data/test/licenses';

const mockedNavigator = jest.fn();

jest.mock('react-router-dom', () => ({
...jest.requireActual('react-router-dom'),
useNavigate: () => mockedNavigator,
}));

const UserPageWrapper = () => (
<MemoryRouter>
<IntlProvider locale="en">
<UserMessagesProvider>
<UserPage />
</UserMessagesProvider>
</IntlProvider>
</MemoryRouter>
);

describe('User Page', () => {
let wrapper;

beforeEach(() => {
jest.spyOn(ssoAndUserApi, 'getAllUserData').mockImplementation(() => Promise.resolve({ user: UserSummaryData.userData, errors: [] }));
jest.spyOn(ssoAndUserApi, 'getVerifiedNameHistory').mockImplementation(() => Promise.resolve(verifiedNameHistoryData));
jest.spyOn(ssoAndUserApi, 'getEnrollments').mockImplementation(() => Promise.resolve(enrollmentsData));
jest.spyOn(ssoAndUserApi, 'getOnboardingStatus').mockImplementation(() => Promise.resolve(onboardingStatusData));
jest.spyOn(ssoAndUserApi, 'getSsoRecords').mockImplementation(() => Promise.resolve(ssoRecordsData));
jest.spyOn(ssoAndUserApi, 'getLicense').mockImplementation(() => Promise.resolve(licensesData));
jest.spyOn(ssoAndUserApi, 'getEntitlements').mockImplementation(() => Promise.resolve(entitlementsData));
jest.spyOn(ssoAndUserApi, 'getEnrollments').mockImplementation(() => Promise.resolve(enrollmentsData));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We got a duplicate line here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is written twice.

jest.spyOn(ssoAndUserApi, 'getEnrollments').mockImplementation(() => Promise.resolve(enrollmentsData));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

jest.spyOn(ssoAndUserApi, 'getEnterpriseCustomerUsers').mockImplementation(() => Promise.resolve(enterpriseCustomerUsersData));

jest.clearAllMocks();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we have this in afterEach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it is fine in both afterEach and beforeEach. Most tests add it in beforeEach too.

});

it('default render', async () => {
wrapper = mount(<UserPageWrapper />);
wrapper.find(
"input[name='userIdentifier']",
).instance().value = 'ANonyMouse';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not trigger input change if I'm not wrong... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, yes. But we don't need a change event anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, when I tried a simulate('onchange'...), the value of the input wasn't changing for some reason, and hence the test was failing. Thus, I took this approach.

wrapper.find('.btn.btn-primary').simulate('click');

await waitFor(() => {
expect(mockedNavigator).toHaveBeenCalledWith(
`/learner_information/?lms_user_id=${UserSummaryData.userData.id}`,
);
Comment on lines +58 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some assertions to check if the API was called?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should call getAllUserData if I'm not wrong 😵‍💫

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

});
});
});