fix: adding value length check for full name field #1242

DmytroAlipov · 2024-04-23T12:52:21Z

Description

Fixing an error when a new user tries registering on the platform with a full name longer than 255 characters.

Now the request to the backend will not be sent because validation fails:

I also created a MR#34577 to Platform so that the backend would not miss such errors. But validation on the backend is just a fuse. It’s better not to send an invalid request to the backend.

openedx-webhooks · 2024-04-23T12:52:25Z

Thanks for the pull request, @DmytroAlipov!

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-infinity. 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.

mphilbrick211 · 2024-04-30T11:45:20Z

Hi @openedx/2u-vanguards! This is all set for review.

mphilbrick211 · 2024-07-17T13:45:34Z

Hi @openedx/2u-vanguards! This is all set for review.

Friendly follow-up on this, @openedx/2u-vanguards!

codecov · 2024-08-13T06:53:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.70%. Comparing base (52c6efc) to head (ae32491).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1242      +/-   ##
==========================================
- Coverage   87.94%   87.70%   -0.25%     
==========================================
  Files         139      124      -15     
  Lines        2812     2301     -511     
  Branches      839      643     -196     
==========================================
- Hits         2473     2018     -455     
+ Misses        324      274      -50     
+ Partials       15        9       -6

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

mphilbrick211 · 2024-08-26T19:58:10Z

Hi @openedx/vanguards @openedx/2u-vanguards - checking in on this.

syedsajjadkazmii

Changes look good. Thanks

syedsajjadkazmii · 2024-09-02T12:51:43Z

src/register/messages.jsx

@@ -126,6 +126,11 @@ const messages = defineMessages({
    defaultMessage: 'Enter a valid name',
    description: 'Validation message that appears when fullname contain URL',
  },
+  'name.validation.length.message': {
+    id: 'name.validation.message',
+    defaultMessage: 'Full name can not be longer than 255 symbols',


Do we need product approval on this message copy? If we don't, then maybe we can have an error message as for username field.
Full name must be between 1 and 255 characters?

Not sure but please let me know if my comment makes sense

syedsajjadkazmii · 2024-09-02T12:52:53Z

src/register/RegistrationFields/NameField/NameField.test.jsx

@@ -94,6 +94,25 @@ describe('NameField', () => {
      );
    });

+    it('should check for long full name error', () => {


Suggested change

it('should check for long full name error', () => {

it('should validate for full name length', () => {

farhaanbukhsh · 2024-10-29T11:54:23Z

@DmytroAlipov I think the changes here are obsolete since we have limited the username to 30 characters already:

Do you think this is still relevant?

DmytroAlipov · 2024-10-30T08:09:22Z

Hi @farhaanbukhsh !
Yes, that's what I wanted to fix! Thanks!
But there's also a fix I made to the platform openedx/edx-platform#34577

farhaanbukhsh · 2024-10-31T06:08:44Z

yes @DmytroAlipov !

The validation is already being done using

frontend-app-authn/src/register/RegistrationFields/NameField/NameField.jsx

Line 38 in 046fbea

dispatch(fetchRealtimeValidations({ name: value }));

This is a server side call which calls /v1/validation/registration so either we should remove this in favor of client side validation or maybe have an action of disabling the submit button till all validation pass through.

Possibly 🤔

The real bug 🐛 is not disabling the submit button on validation issues. What do you say?

DmytroAlipov · 2024-11-03T20:10:13Z

@farhaanbukhsh yes, I agree.
We can either disable the button on the frontend until validation is passed or fix the validation on the backend. However, handling everything on the frontend seems more logical. This approach would make the system's behavior more expected and predictable for the user.

awais-ansari

Small nits.

awais-ansari · 2024-12-02T08:56:37Z

src/register/messages.jsx

@@ -126,6 +126,11 @@ const messages = defineMessages({
    defaultMessage: 'Enter a valid name',
    description: 'Validation message that appears when fullname contain URL',
  },
+  'name.validation.length.message': {
+    id: 'name.validation.message',
+    defaultMessage: 'Full name can not be longer than 255 symbols',


Hi @DmytroAlipov, I coordinated with the product. Product suggestion is to use Full name cannot be longer than 255 characters Message.

@awais-ansari
✅ done

@awais-ansari I don't understand why the codecov failed

DmytroAlipov · 2024-12-03T18:25:17Z

I can't figure out why my coverage has decreased.

mphilbrick211 · 2024-12-09T20:57:26Z

Hi @openedx/2u-infinity! Can someone please help out?

awais-ansari · 2024-12-10T07:27:00Z

src/register/messages.jsx

+  'name.validation.length.message': {
+    id: 'name.validation.message',
+    defaultMessage: 'Full name cannot be longer than 255 characters',
+    description: 'Validation message that appears when fullname contain URL',


description is not correct.

DmytroAlipov requested a review from a team as a code owner April 23, 2024 12:52

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

DmytroAlipov added the bug Report of or fix for something that isn't working as intended label Apr 23, 2024

DmytroAlipov mentioned this pull request Apr 23, 2024

Fix: registering a new user with a full name longer than 255 characters openedx/edx-platform#34577

Merged

DmytroAlipov force-pushed the fix-name-max-lenght branch from 92b5a9e to 4b2eb25 Compare August 13, 2024 06:47

DmytroAlipov force-pushed the fix-name-max-lenght branch from 4b2eb25 to 076b078 Compare August 27, 2024 06:16

syedsajjadkazmii reviewed Sep 2, 2024

View reviewed changes

DmytroAlipov force-pushed the fix-name-max-lenght branch from 076b078 to c4f3824 Compare October 14, 2024 05:50

DmytroAlipov requested a review from a team as a code owner October 14, 2024 05:50

awais-ansari requested changes Dec 2, 2024

View reviewed changes

DmytroAlipov force-pushed the fix-name-max-lenght branch 5 times, most recently from 724049a to 4b7c089 Compare December 3, 2024 08:49

fix: adding value length check for full name field

ae32491

DmytroAlipov force-pushed the fix-name-max-lenght branch from 4b7c089 to ae32491 Compare December 3, 2024 08:57

mphilbrick211 requested a review from a team December 9, 2024 20:57

awais-ansari requested changes Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: adding value length check for full name field #1242

fix: adding value length check for full name field #1242

DmytroAlipov commented Apr 23, 2024 •

edited

Loading

openedx-webhooks commented Apr 23, 2024 •

edited

Loading

mphilbrick211 commented Apr 30, 2024

mphilbrick211 commented Jul 17, 2024

codecov bot commented Aug 13, 2024 •

edited

Loading

mphilbrick211 commented Aug 26, 2024

syedsajjadkazmii left a comment

syedsajjadkazmii Sep 2, 2024

syedsajjadkazmii Sep 2, 2024

farhaanbukhsh commented Oct 29, 2024

DmytroAlipov commented Oct 30, 2024

farhaanbukhsh commented Oct 31, 2024

DmytroAlipov commented Nov 3, 2024

awais-ansari left a comment

awais-ansari Dec 2, 2024

DmytroAlipov Dec 3, 2024 •

edited

Loading

DmytroAlipov Dec 3, 2024

DmytroAlipov commented Dec 3, 2024

mphilbrick211 commented Dec 9, 2024

awais-ansari Dec 10, 2024

	it('should check for long full name error', () => {
	it('should validate for full name length', () => {

fix: adding value length check for full name field #1242

Are you sure you want to change the base?

fix: adding value length check for full name field #1242

Conversation

DmytroAlipov commented Apr 23, 2024 • edited Loading

Description

openedx-webhooks commented Apr 23, 2024 • edited Loading

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?

mphilbrick211 commented Apr 30, 2024

mphilbrick211 commented Jul 17, 2024

codecov bot commented Aug 13, 2024 • edited Loading

Codecov Report

mphilbrick211 commented Aug 26, 2024

syedsajjadkazmii left a comment

Choose a reason for hiding this comment

syedsajjadkazmii Sep 2, 2024

Choose a reason for hiding this comment

syedsajjadkazmii Sep 2, 2024

Choose a reason for hiding this comment

farhaanbukhsh commented Oct 29, 2024

DmytroAlipov commented Oct 30, 2024

farhaanbukhsh commented Oct 31, 2024

DmytroAlipov commented Nov 3, 2024

awais-ansari left a comment

Choose a reason for hiding this comment

awais-ansari Dec 2, 2024

Choose a reason for hiding this comment

DmytroAlipov Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

DmytroAlipov Dec 3, 2024

Choose a reason for hiding this comment

DmytroAlipov commented Dec 3, 2024

mphilbrick211 commented Dec 9, 2024

awais-ansari Dec 10, 2024

Choose a reason for hiding this comment

DmytroAlipov commented Apr 23, 2024 •

edited

Loading

openedx-webhooks commented Apr 23, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

DmytroAlipov Dec 3, 2024 •

edited

Loading