From 2f40d674f06dccbf6dc3eda437bd08dfa8f2ff63 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 3 Dec 2024 19:33:11 -0500 Subject: [PATCH 1/5] fix: Update error messages when adding user to library --- .../library-team/LibraryTeam.test.tsx | 34 +++++++++++++++++-- .../library-team/LibraryTeam.tsx | 9 +++-- .../library-team/messages.ts | 5 +++ 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/library-authoring/library-team/LibraryTeam.test.tsx b/src/library-authoring/library-team/LibraryTeam.test.tsx index 2ab5ec52ab..40bba45f83 100644 --- a/src/library-authoring/library-team/LibraryTeam.test.tsx +++ b/src/library-authoring/library-team/LibraryTeam.test.tsx @@ -15,6 +15,7 @@ import { getLibraryTeamMemberApiUrl, } from '../data/api'; import { LibraryProvider } from '../common/context'; +import { ToastProvider } from '../../generic/toast-context'; import LibraryTeam from './LibraryTeam'; mockContentLibrary.applyMock(); @@ -28,9 +29,11 @@ describe('', () => { const { libraryId } = mockContentLibrary; const renderLibraryTeam = async () => { render( - - - , + + + + + , ); await waitFor(() => { @@ -178,6 +181,31 @@ describe('', () => { }); }); + it('shows error when user do not exist', async () => { + const url = getLibraryTeamApiUrl(libraryId); + const axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + axiosMock.onPost(url).reply(400, { email: 'Error' }); + + await renderLibraryTeam(); + + const addButton = screen.getByRole('button', { name: 'New team member' }); + userEvent.click(addButton); + const emailInput = screen.getByRole('textbox', { name: 'User\'s email address' }); + userEvent.click(emailInput); + userEvent.type(emailInput, 'another@user.tld'); + + const saveButton = screen.getByRole('button', { name: /add member/i }); + userEvent.click(saveButton); + + await waitFor(() => { + expect(axiosMock.history.post.length).toEqual(1); + }); + + expect(await screen.findByText( + 'Error adding team member. please verify that the email is correct and belongs to a registered user.', + )).toBeInTheDocument(); + }); + it('allows library team member roles to be changed', async () => { const { username } = mockGetLibraryTeam.readerMember; const url = getLibraryTeamMemberApiUrl(libraryId, username); diff --git a/src/library-authoring/library-team/LibraryTeam.tsx b/src/library-authoring/library-team/LibraryTeam.tsx index b41be5acf2..dcb8820794 100644 --- a/src/library-authoring/library-team/LibraryTeam.tsx +++ b/src/library-authoring/library-team/LibraryTeam.tsx @@ -65,8 +65,13 @@ const LibraryTeam: React.FC> = () => { accessLevel: LibraryRole.Reader.toString() as LibraryAccessLevel, }).then(() => { showToast(intl.formatMessage(messages.addMemberSuccess)); - }).catch(() => { - showToast(intl.formatMessage(messages.addMemberError)); + }).catch((addMemberError) => { + const errorData = addMemberError.response.data; + if ('email' in errorData) { + showToast(intl.formatMessage(messages.addMemberEmailError)); + } else { + showToast(intl.formatMessage(messages.addMemberError)); + } }); closeAddLibraryTeamMember(); }, diff --git a/src/library-authoring/library-team/messages.ts b/src/library-authoring/library-team/messages.ts index 6bf6a8c363..3b3ea23a37 100644 --- a/src/library-authoring/library-team/messages.ts +++ b/src/library-authoring/library-team/messages.ts @@ -124,6 +124,11 @@ const messages = defineMessages({ defaultMessage: 'Error adding Team Member', description: 'Message shown when an error occurs while adding a Library Team member', }, + addMemberEmailError: { + id: 'course-authoring.library-authoring.library-team.add-member-email-error', + defaultMessage: 'Error adding team member. Please verify that the email is correct and belongs to a registered user.', + description: 'Message shown when an error occurs with email while adding a Library Team member.', + }, deleteMemberSuccess: { id: 'course-authoring.library-authoring.library-team.delete-member-success', defaultMessage: 'Team Member deleted', From d59f0a81744825de18971829bd4e1a309869b120 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 3 Dec 2024 19:50:26 -0500 Subject: [PATCH 2/5] test: Add test to fix coverage --- .../library-team/LibraryTeam.test.tsx | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/library-authoring/library-team/LibraryTeam.test.tsx b/src/library-authoring/library-team/LibraryTeam.test.tsx index 40bba45f83..62743270d0 100644 --- a/src/library-authoring/library-team/LibraryTeam.test.tsx +++ b/src/library-authoring/library-team/LibraryTeam.test.tsx @@ -206,6 +206,29 @@ describe('', () => { )).toBeInTheDocument(); }); + it('shows error', async () => { + const url = getLibraryTeamApiUrl(libraryId); + const axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + axiosMock.onPost(url).reply(400, { error: 'Error' }); + + await renderLibraryTeam(); + + const addButton = screen.getByRole('button', { name: 'New team member' }); + userEvent.click(addButton); + const emailInput = screen.getByRole('textbox', { name: 'User\'s email address' }); + userEvent.click(emailInput); + userEvent.type(emailInput, 'another@user.tld'); + + const saveButton = screen.getByRole('button', { name: /add member/i }); + userEvent.click(saveButton); + + await waitFor(() => { + expect(axiosMock.history.post.length).toEqual(1); + }); + + expect(await screen.findByText('Error adding team member')).toBeInTheDocument(); + }); + it('allows library team member roles to be changed', async () => { const { username } = mockGetLibraryTeam.readerMember; const url = getLibraryTeamMemberApiUrl(libraryId, username); From d1cedaffefbaa5b6c2d06e16a20b65a6d2f9a0bf Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 4 Dec 2024 13:08:59 -0500 Subject: [PATCH 3/5] feat: Add disableCapitalize in ProcessingNotification --- .../ProcessingNotification.test.jsx | 10 +++++++++- src/generic/processing-notification/index.jsx | 8 ++++++-- src/generic/toast-context/index.test.tsx | 18 ++++++++++++------ src/generic/toast-context/index.tsx | 15 +++++++++++---- .../library-team/LibraryTeam.test.tsx | 6 ++++-- src/library-authoring/library-team/messages.ts | 2 +- 6 files changed, 43 insertions(+), 16 deletions(-) diff --git a/src/generic/processing-notification/ProcessingNotification.test.jsx b/src/generic/processing-notification/ProcessingNotification.test.jsx index 97f57429bf..70eb4d96f5 100644 --- a/src/generic/processing-notification/ProcessingNotification.test.jsx +++ b/src/generic/processing-notification/ProcessingNotification.test.jsx @@ -26,7 +26,15 @@ describe('', () => { expect(screen.getByText('Undo')).toBeInTheDocument(); expect(screen.getByRole('alert').querySelector('.processing-notification-hide-close-button')).not.toBeInTheDocument(); userEvent.click(screen.getByText('Undo')); - expect(mockUndo).toBeCalled(); + expect(mockUndo).toHaveBeenCalled(); + }); + + it('renders with `disableCapitalize`', () => { + const title = 'ThIs IS a Test. OK?'; + render( {}} title={title} disableCapitalize />); + expect(screen.getByText(title)).toBeInTheDocument(); + expect(screen.getByText('Undo')).toBeInTheDocument(); + expect(screen.getByRole('alert').querySelector('.processing-notification-hide-close-button')).not.toBeInTheDocument(); }); it('add hide-close-button class if no close action is passed', () => { diff --git a/src/generic/processing-notification/index.jsx b/src/generic/processing-notification/index.jsx index b31150a957..d8ccbc9250 100644 --- a/src/generic/processing-notification/index.jsx +++ b/src/generic/processing-notification/index.jsx @@ -7,7 +7,7 @@ import { capitalize } from 'lodash'; import classNames from 'classnames'; const ProcessingNotification = ({ - isShow, title, action, close, + isShow, title, action, close, disableCapitalize, }) => ( - {capitalize(title)} + + {!disableCapitalize ? capitalize(title) : title} + ); ProcessingNotification.defaultProps = { close: null, + disableCapitalize: false, }; ProcessingNotification.propTypes = { @@ -35,6 +38,7 @@ ProcessingNotification.propTypes = { onClick: PropTypes.func, }), close: PropTypes.func, + disableCapitalize: PropTypes.bool, }; export default ProcessingNotification; diff --git a/src/generic/toast-context/index.test.tsx b/src/generic/toast-context/index.test.tsx index f7e0a2e4b0..cf6355eb65 100644 --- a/src/generic/toast-context/index.test.tsx +++ b/src/generic/toast-context/index.test.tsx @@ -9,11 +9,12 @@ export interface WraperProps { children: React.ReactNode; } -const TestComponentToShow = () => { +// eslint-disable-next-line react/prop-types +const TestComponentToShow = ({ capitilize = false }) => { const { showToast } = React.useContext(ToastContext); React.useEffect(() => { - showToast('This is the toast!'); + showToast('This is the Toast!', undefined, capitilize); }, [showToast]); return
Content
; @@ -23,7 +24,7 @@ const TestComponentToClose = () => { const { showToast, closeToast } = React.useContext(ToastContext); React.useEffect(() => { - showToast('This is the toast!'); + showToast('This is the Toast!'); closeToast(); }, [showToast]); @@ -59,19 +60,24 @@ describe('', () => { it('should show toast', async () => { render(); + expect(await screen.findByText('This is the Toast!')).toBeInTheDocument(); + }); + + it('should capitilize toast', async () => { + render(); expect(await screen.findByText('This is the toast!')).toBeInTheDocument(); }); it('should close toast after 5000ms', async () => { render(); - expect(await screen.findByText('This is the toast!')).toBeInTheDocument(); + expect(await screen.findByText('This is the Toast!')).toBeInTheDocument(); jest.advanceTimersByTime(6000); - expect(screen.queryByText('This is the toast!')).not.toBeInTheDocument(); + expect(screen.queryByText('This is the Toast!')).not.toBeInTheDocument(); }); it('should close toast', async () => { render(); expect(await screen.findByText('Content')).toBeInTheDocument(); - expect(screen.queryByText('This is the toast!')).not.toBeInTheDocument(); + expect(screen.queryByText('This is the Toast!')).not.toBeInTheDocument(); }); }); diff --git a/src/generic/toast-context/index.tsx b/src/generic/toast-context/index.tsx index ea47dce8d2..6a6c9761f9 100644 --- a/src/generic/toast-context/index.tsx +++ b/src/generic/toast-context/index.tsx @@ -10,7 +10,8 @@ export interface ToastActionData { export interface ToastContextData { toastMessage: string | null; toastAction?: ToastActionData; - showToast: (message: string, action?: ToastActionData) => void; + capitilize?: boolean; + showToast: (message: string, action?: ToastActionData, capitilize?: boolean) => void; closeToast: () => void; } @@ -25,6 +26,7 @@ export interface ToastProviderProps { export const ToastContext = React.createContext({ toastMessage: null, toastAction: undefined, + capitilize: false, showToast: () => {}, closeToast: () => {}, }); @@ -38,10 +40,12 @@ export const ToastProvider = (props: ToastProviderProps) => { const [toastMessage, setToastMessage] = React.useState(null); const [toastAction, setToastAction] = React.useState(undefined); + const [capitilize, setCapitilize] = React.useState(false); const resetState = React.useCallback(() => { setToastMessage(null); setToastAction(undefined); + setCapitilize(false); }, []); React.useEffect(() => () => { @@ -49,18 +53,20 @@ export const ToastProvider = (props: ToastProviderProps) => { resetState(); }, []); - const showToast = React.useCallback((message, action?: ToastActionData) => { + const showToast = React.useCallback((message, action?: ToastActionData, isCapitilize?: boolean) => { setToastMessage(message); setToastAction(action); + setCapitilize(isCapitilize ?? false); }, [setToastMessage, setToastAction]); - const closeToast = React.useCallback(() => resetState(), [setToastMessage, setToastAction]); + const closeToast = React.useCallback(() => resetState(), [setToastMessage, setToastAction, setCapitilize]); const context = React.useMemo(() => ({ toastMessage, toastAction, + capitilize, showToast, closeToast, - }), [toastMessage, toastAction, showToast, closeToast]); + }), [toastMessage, toastAction, capitilize, showToast, closeToast]); return ( @@ -71,6 +77,7 @@ export const ToastProvider = (props: ToastProviderProps) => { title={toastMessage} action={toastAction} close={closeToast} + disableCapitalize={!capitilize} /> )} diff --git a/src/library-authoring/library-team/LibraryTeam.test.tsx b/src/library-authoring/library-team/LibraryTeam.test.tsx index 62743270d0..fd50a09273 100644 --- a/src/library-authoring/library-team/LibraryTeam.test.tsx +++ b/src/library-authoring/library-team/LibraryTeam.test.tsx @@ -179,6 +179,8 @@ describe('', () => { `{"library_id":"${libraryId}","email":"another@user.tld","access_level":"read"}`, ); }); + + expect(await screen.findByText('Team Member added')).toBeInTheDocument(); }); it('shows error when user do not exist', async () => { @@ -202,7 +204,7 @@ describe('', () => { }); expect(await screen.findByText( - 'Error adding team member. please verify that the email is correct and belongs to a registered user.', + 'Error adding Team Member. Please verify that the email is correct and belongs to a registered user.', )).toBeInTheDocument(); }); @@ -226,7 +228,7 @@ describe('', () => { expect(axiosMock.history.post.length).toEqual(1); }); - expect(await screen.findByText('Error adding team member')).toBeInTheDocument(); + expect(await screen.findByText('Error adding Team Member')).toBeInTheDocument(); }); it('allows library team member roles to be changed', async () => { diff --git a/src/library-authoring/library-team/messages.ts b/src/library-authoring/library-team/messages.ts index 3b3ea23a37..d56d606153 100644 --- a/src/library-authoring/library-team/messages.ts +++ b/src/library-authoring/library-team/messages.ts @@ -126,7 +126,7 @@ const messages = defineMessages({ }, addMemberEmailError: { id: 'course-authoring.library-authoring.library-team.add-member-email-error', - defaultMessage: 'Error adding team member. Please verify that the email is correct and belongs to a registered user.', + defaultMessage: 'Error adding Team Member. Please verify that the email is correct and belongs to a registered user.', description: 'Message shown when an error occurs with email while adding a Library Team member.', }, deleteMemberSuccess: { From d92ea86e3abed5cab22b2c67ff222238241242de Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 4 Dec 2024 14:10:02 -0500 Subject: [PATCH 4/5] refactor: Remove capitalize in ProcessingNotification --- .../ProcessingNotification.test.jsx | 16 +++------------- src/generic/processing-notification/index.jsx | 9 ++------- src/generic/toast-context/index.test.tsx | 9 ++------- src/generic/toast-context/index.tsx | 15 ++++----------- 4 files changed, 11 insertions(+), 38 deletions(-) diff --git a/src/generic/processing-notification/ProcessingNotification.test.jsx b/src/generic/processing-notification/ProcessingNotification.test.jsx index 70eb4d96f5..d2bbdbeaca 100644 --- a/src/generic/processing-notification/ProcessingNotification.test.jsx +++ b/src/generic/processing-notification/ProcessingNotification.test.jsx @@ -1,13 +1,11 @@ -import { capitalize } from 'lodash'; import userEvent from '@testing-library/user-event'; import { initializeMocks, render, screen } from '../../testUtils'; -import { NOTIFICATION_MESSAGES } from '../../constants'; import ProcessingNotification from '.'; const mockUndo = jest.fn(); const props = { - title: NOTIFICATION_MESSAGES.saving, + title: 'ThIs IS a Test. OK?', isShow: true, action: { label: 'Undo', @@ -22,24 +20,16 @@ describe('', () => { it('renders successfully', () => { render( {}} />); - expect(screen.getByText(capitalize(props.title))).toBeInTheDocument(); + expect(screen.getByText(props.title)).toBeInTheDocument(); expect(screen.getByText('Undo')).toBeInTheDocument(); expect(screen.getByRole('alert').querySelector('.processing-notification-hide-close-button')).not.toBeInTheDocument(); userEvent.click(screen.getByText('Undo')); expect(mockUndo).toHaveBeenCalled(); }); - it('renders with `disableCapitalize`', () => { - const title = 'ThIs IS a Test. OK?'; - render( {}} title={title} disableCapitalize />); - expect(screen.getByText(title)).toBeInTheDocument(); - expect(screen.getByText('Undo')).toBeInTheDocument(); - expect(screen.getByRole('alert').querySelector('.processing-notification-hide-close-button')).not.toBeInTheDocument(); - }); - it('add hide-close-button class if no close action is passed', () => { render(); - expect(screen.getByText(capitalize(props.title))).toBeInTheDocument(); + expect(screen.getByText(props.title)).toBeInTheDocument(); expect(screen.getByRole('alert').querySelector('.processing-notification-hide-close-button')).toBeInTheDocument(); }); }); diff --git a/src/generic/processing-notification/index.jsx b/src/generic/processing-notification/index.jsx index d8ccbc9250..42dc95711f 100644 --- a/src/generic/processing-notification/index.jsx +++ b/src/generic/processing-notification/index.jsx @@ -3,11 +3,10 @@ import { Icon, Toast, } from '@openedx/paragon'; import { Settings as IconSettings } from '@openedx/paragon/icons'; -import { capitalize } from 'lodash'; import classNames from 'classnames'; const ProcessingNotification = ({ - isShow, title, action, close, disableCapitalize, + isShow, title, action, close, }) => ( - - {!disableCapitalize ? capitalize(title) : title} - + {title} ); ProcessingNotification.defaultProps = { close: null, - disableCapitalize: false, }; ProcessingNotification.propTypes = { @@ -38,7 +34,6 @@ ProcessingNotification.propTypes = { onClick: PropTypes.func, }), close: PropTypes.func, - disableCapitalize: PropTypes.bool, }; export default ProcessingNotification; diff --git a/src/generic/toast-context/index.test.tsx b/src/generic/toast-context/index.test.tsx index cf6355eb65..21fa781438 100644 --- a/src/generic/toast-context/index.test.tsx +++ b/src/generic/toast-context/index.test.tsx @@ -10,11 +10,11 @@ export interface WraperProps { } // eslint-disable-next-line react/prop-types -const TestComponentToShow = ({ capitilize = false }) => { +const TestComponentToShow = () => { const { showToast } = React.useContext(ToastContext); React.useEffect(() => { - showToast('This is the Toast!', undefined, capitilize); + showToast('This is the Toast!'); }, [showToast]); return
Content
; @@ -63,11 +63,6 @@ describe('', () => { expect(await screen.findByText('This is the Toast!')).toBeInTheDocument(); }); - it('should capitilize toast', async () => { - render(); - expect(await screen.findByText('This is the toast!')).toBeInTheDocument(); - }); - it('should close toast after 5000ms', async () => { render(); expect(await screen.findByText('This is the Toast!')).toBeInTheDocument(); diff --git a/src/generic/toast-context/index.tsx b/src/generic/toast-context/index.tsx index 6a6c9761f9..ea47dce8d2 100644 --- a/src/generic/toast-context/index.tsx +++ b/src/generic/toast-context/index.tsx @@ -10,8 +10,7 @@ export interface ToastActionData { export interface ToastContextData { toastMessage: string | null; toastAction?: ToastActionData; - capitilize?: boolean; - showToast: (message: string, action?: ToastActionData, capitilize?: boolean) => void; + showToast: (message: string, action?: ToastActionData) => void; closeToast: () => void; } @@ -26,7 +25,6 @@ export interface ToastProviderProps { export const ToastContext = React.createContext({ toastMessage: null, toastAction: undefined, - capitilize: false, showToast: () => {}, closeToast: () => {}, }); @@ -40,12 +38,10 @@ export const ToastProvider = (props: ToastProviderProps) => { const [toastMessage, setToastMessage] = React.useState(null); const [toastAction, setToastAction] = React.useState(undefined); - const [capitilize, setCapitilize] = React.useState(false); const resetState = React.useCallback(() => { setToastMessage(null); setToastAction(undefined); - setCapitilize(false); }, []); React.useEffect(() => () => { @@ -53,20 +49,18 @@ export const ToastProvider = (props: ToastProviderProps) => { resetState(); }, []); - const showToast = React.useCallback((message, action?: ToastActionData, isCapitilize?: boolean) => { + const showToast = React.useCallback((message, action?: ToastActionData) => { setToastMessage(message); setToastAction(action); - setCapitilize(isCapitilize ?? false); }, [setToastMessage, setToastAction]); - const closeToast = React.useCallback(() => resetState(), [setToastMessage, setToastAction, setCapitilize]); + const closeToast = React.useCallback(() => resetState(), [setToastMessage, setToastAction]); const context = React.useMemo(() => ({ toastMessage, toastAction, - capitilize, showToast, closeToast, - }), [toastMessage, toastAction, capitilize, showToast, closeToast]); + }), [toastMessage, toastAction, showToast, closeToast]); return ( @@ -77,7 +71,6 @@ export const ToastProvider = (props: ToastProviderProps) => { title={toastMessage} action={toastAction} close={closeToast} - disableCapitalize={!capitilize} /> )} From bb90f0bb4f22b16135fb36dbcd1184565e01517d Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 4 Dec 2024 14:34:32 -0500 Subject: [PATCH 5/5] style: Some nits on the code --- src/generic/toast-context/index.test.tsx | 1 - src/library-authoring/library-team/LibraryTeam.test.tsx | 2 +- src/library-authoring/library-team/LibraryTeam.tsx | 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/generic/toast-context/index.test.tsx b/src/generic/toast-context/index.test.tsx index 21fa781438..11294b0699 100644 --- a/src/generic/toast-context/index.test.tsx +++ b/src/generic/toast-context/index.test.tsx @@ -9,7 +9,6 @@ export interface WraperProps { children: React.ReactNode; } -// eslint-disable-next-line react/prop-types const TestComponentToShow = () => { const { showToast } = React.useContext(ToastContext); diff --git a/src/library-authoring/library-team/LibraryTeam.test.tsx b/src/library-authoring/library-team/LibraryTeam.test.tsx index fd50a09273..bdd424fc08 100644 --- a/src/library-authoring/library-team/LibraryTeam.test.tsx +++ b/src/library-authoring/library-team/LibraryTeam.test.tsx @@ -211,7 +211,7 @@ describe('', () => { it('shows error', async () => { const url = getLibraryTeamApiUrl(libraryId); const axiosMock = new MockAdapter(getAuthenticatedHttpClient()); - axiosMock.onPost(url).reply(400, { error: 'Error' }); + axiosMock.onPost(url).reply(400, {}); await renderLibraryTeam(); diff --git a/src/library-authoring/library-team/LibraryTeam.tsx b/src/library-authoring/library-team/LibraryTeam.tsx index dcb8820794..8bf4d2e31c 100644 --- a/src/library-authoring/library-team/LibraryTeam.tsx +++ b/src/library-authoring/library-team/LibraryTeam.tsx @@ -66,8 +66,8 @@ const LibraryTeam: React.FC> = () => { }).then(() => { showToast(intl.formatMessage(messages.addMemberSuccess)); }).catch((addMemberError) => { - const errorData = addMemberError.response.data; - if ('email' in errorData) { + const errorData = typeof addMemberError === 'object' ? addMemberError.response?.data : undefined; + if (errorData && 'email' in errorData) { showToast(intl.formatMessage(messages.addMemberEmailError)); } else { showToast(intl.formatMessage(messages.addMemberError));