Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Update error messages when adding user to library [FC-0062] #1543

Merged
merged 5 commits into from
Dec 6, 2024
Merged

fix: Update error messages when adding user to library [FC-0062] #1543

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f40d67
fix: Update error messages when adding user to library
ChrisChV Dec 4, 2024
d59f0a8
test: Add test to fix coverage
ChrisChV Dec 4, 2024
d1cedaf
feat: Add disableCapitalize in ProcessingNotification
ChrisChV Dec 4, 2024
d92ea86
refactor: Remove capitalize in ProcessingNotification
ChrisChV Dec 4, 2024
bb90f0b
style: Some nits on the code
ChrisChV Dec 4, 2024
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
57 changes: 54 additions & 3 deletions src/library-authoring/library-team/LibraryTeam.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -28,9 +29,11 @@ describe('<LibraryTeam />', () => {
const { libraryId } = mockContentLibrary;
const renderLibraryTeam = async () => {
render(
<LibraryProvider libraryId={libraryId}>
<LibraryTeam />
</LibraryProvider>,
<ToastProvider>
<LibraryProvider libraryId={libraryId}>
<LibraryTeam />
</LibraryProvider>
</ToastProvider>,
);

await waitFor(() => {
Expand Down Expand Up @@ -178,6 +181,54 @@ describe('<LibraryTeam />', () => {
});
});

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.',
Copy link
Contributor

@rpenido rpenido Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
'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.',

But this will only work if we fix the issue in the ProcessingNotification component.

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 think we should not do it. What do you think?

It is very restrictive and should allow placing a non-capitalized message, at least in the ToasContext.

I added that feature here: d1cedaf

Copy link
Contributor

@rpenido rpenido Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should make it simpler and just remove the capitalize call. If it is needed somewhere, we can call it outside the component.

Copy link
Contributor

@bradenmacdonald bradenmacdonald Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think our Toast/ToastContext/showToast should ever be changing the capitalization of the message. Why is it doing that now?

Copy link
Contributor

@bradenmacdonald bradenmacdonald Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess we maybe? need to leave the behavior as an option in ProcessingNotification but for ToastContext it should not be an option. Just always disable capitalization, and remove the new parameter.

Or if you think we can remove it altogether as @rpenido suggested, that's even better!

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 removed the capitalization at all! I think it doesn't break anything: d92ea86

)).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);
Expand Down
9 changes: 7 additions & 2 deletions src/library-authoring/library-team/LibraryTeam.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,13 @@ const LibraryTeam: React.FC<Record<never, never>> = () => {
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is fragile, if a non-Axios error is raised:
Screenshot 2024-12-04 at 10 12 56 AM

Try something like:

Suggested change
const errorData = addMemberError.response.data;
if ('email' in errorData) {
const errorData = typeof addMemberError === 'object' ? addMemberError.response?.data : undefined;
if (errorData && 'email' in errorData) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated bb90f0b

showToast(intl.formatMessage(messages.addMemberEmailError));
} else {
showToast(intl.formatMessage(messages.addMemberError));
}
});
closeAddLibraryTeamMember();
},
Expand Down
5 changes: 5 additions & 0 deletions src/library-authoring/library-team/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Loading