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

ChrisChV · 2024-12-04T00:33:43Z

Description

Updates the message error when the user doesn't exist when adding a new team member to a library.
Which edX user roles will this change impact? "Course Author"

Supporting information

Github issue: "Manage Users/Permissions" for v2 Libraries #1342 (comment)
Internal ticket: FAL-3978

Testing instructions

Select a library from the list of v2 Libraries, and open the Library Info sidebar. You need to use a user with Admin access.
Click on Manage Access, and add a user that doesn't exist.
Verify the new error message.

openedx-webhooks · 2024-12-04T00:33:48Z

Thanks for the pull request, @ChrisChV!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

codecov · 2024-12-04T00:44:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.94%. Comparing base (6e53e37) to head (bb90f0b).
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1543      +/-   ##
==========================================
+ Coverage   92.91%   92.94%   +0.03%     
==========================================
  Files        1071     1073       +2     
  Lines       21038    21095      +57     
  Branches     4538     4477      -61     
==========================================
+ Hits        19547    19607      +60     
- Misses       1418     1422       +4     
+ Partials       73       66       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rpenido

LGTM 👍
Thank you for your work, @ChrisChV!

I tested this using the instructions from the PR
I read through the code
I checked for accessibility issues
Includes documentation

A minor issue with capitalization (not directly related to this code).
We are changing the string case inside the Toast and rendering it wrong.

frontend-app-authoring/src/generic/processing-notification/index.jsx

Line 21 in e75ce15

    
           <span className="font-weight-bold h4 mb-0 text-white">{capitalize(title)}</span>

I think we should not do it. What do you think?

rpenido · 2024-12-04T12:57:41Z

src/library-authoring/library-team/LibraryTeam.test.tsx

+    });
+
+    expect(await screen.findByText(
+      'Error adding team member. please verify that the email is correct and belongs to a registered user.',


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.

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

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.

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

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!

I removed the capitalization at all! I think it doesn't break anything: d92ea86

bradenmacdonald · 2024-12-04T18:13:39Z

src/library-authoring/library-team/LibraryTeam.tsx

+        const errorData = addMemberError.response.data;
+        if ('email' in errorData) {


This code is fragile, if a non-Axios error is raised:

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) {

Updated bb90f0b

bradenmacdonald · 2024-12-04T18:16:53Z

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;


What is the purpose of this new parameter? I don't think "capitalization" should be a feature of our toasts. Do we really need it?

Also, the spelling is wrong in several places. It should be "Capitalize" with two As.

Removed here d92ea86

ChrisChV · 2024-12-04T19:42:55Z

@bradenmacdonald It's ready for another review

…nedx#1543) Updates the message error when the user doesn't exist when adding a new team member to a library.

) (#1550) Updates the message error when the user doesn't exist when adding a new team member to a library.

fix: Update error messages when adding user to library

2f40d67

ChrisChV requested a review from a team as a code owner December 4, 2024 00:33

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 4, 2024

ChrisChV changed the title ~~fix: Update error messages when adding user to library~~ fix: Update error messages when adding user to library [FC-0062] Dec 4, 2024

test: Add test to fix coverage

d59f0a8

rpenido approved these changes Dec 4, 2024

View reviewed changes

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Dec 4, 2024

feat: Add disableCapitalize in ProcessingNotification

d1cedaf

bradenmacdonald reviewed Dec 4, 2024

View reviewed changes

ChrisChV added 2 commits December 4, 2024 14:10

refactor: Remove capitalize in ProcessingNotification

d92ea86

style: Some nits on the code

bb90f0b

bradenmacdonald approved these changes Dec 6, 2024

View reviewed changes

ChrisChV merged commit d2d7532 into openedx:master Dec 6, 2024
7 checks passed

ChrisChV deleted the chris/FAL-3978-update-team-member-error-msg branch December 6, 2024 20:46

ChrisChV mentioned this pull request Dec 6, 2024

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

Merged

bradenmacdonald pushed a commit that referenced this pull request Dec 9, 2024

fix: Update error messages when adding user to library (backport) (#1543

4835f72

) (#1550) Updates the message error when the user doesn't exist when adding a new team member to a library.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ChrisChV commented Dec 4, 2024 •

edited

Loading

openedx-webhooks commented Dec 4, 2024

codecov bot commented Dec 4, 2024 •

edited

Loading

rpenido left a comment

rpenido Dec 4, 2024 •

edited

Loading

ChrisChV Dec 4, 2024

rpenido Dec 4, 2024 •

edited

Loading

bradenmacdonald Dec 4, 2024 •

edited

Loading

bradenmacdonald Dec 4, 2024 •

edited

Loading

ChrisChV Dec 4, 2024

bradenmacdonald Dec 4, 2024

ChrisChV Dec 4, 2024

bradenmacdonald Dec 4, 2024

ChrisChV Dec 4, 2024 •

edited

Loading

ChrisChV commented Dec 4, 2024

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

		const errorData = addMemberError.response.data;
		if ('email' in errorData) {

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

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

Conversation

ChrisChV commented Dec 4, 2024 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Dec 4, 2024

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

rpenido left a comment

Choose a reason for hiding this comment

rpenido Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

ChrisChV Dec 4, 2024

Choose a reason for hiding this comment

rpenido Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

bradenmacdonald Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

bradenmacdonald Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

ChrisChV Dec 4, 2024

Choose a reason for hiding this comment

bradenmacdonald Dec 4, 2024

Choose a reason for hiding this comment

ChrisChV Dec 4, 2024

Choose a reason for hiding this comment

bradenmacdonald Dec 4, 2024

Choose a reason for hiding this comment

ChrisChV Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

ChrisChV commented Dec 4, 2024

ChrisChV commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

rpenido Dec 4, 2024 •

edited

Loading

rpenido Dec 4, 2024 •

edited

Loading

bradenmacdonald Dec 4, 2024 •

edited

Loading

bradenmacdonald Dec 4, 2024 •

edited

Loading

ChrisChV Dec 4, 2024 •

edited

Loading