Changes to how validation displays in UI #4354

rdjuric · 2021-08-01T15:59:58Z

Details

Fixed Issues

$ #2934

Tests

Open the New chat/New Group modal
Try to write an invalid phone number (0909 for example)
It should show an error message and no search results.

QA Steps

Tested On

Screenshots

Web

web.mov

Mobile Web

mWeb.mp4

Desktop

desktop.mov

iOS

iOS.mp4

Android

Android.mp4

rdjuric · 2021-08-02T14:07:05Z

@Beamanator I made a few changes to the original proposal to make it easier for us to apply the enhancements of #2936.

Modifies our getHeaderMessage to return an object with message and isValid properties. isValid helps us decide if we want to show the search results for a given searchValue or not.
In our OptionsList, check's if the searchValue is marked as isValid before showing the search results.

I think this is better than the original proposal because:

Uses the getHeaderMessage to show the error as we normally do (instead of rendering another component in OptionsSelector)
The logic can be reused for other search values that we might not want to show search results for
Since the check is done in OptionsList instead of OptionsSelector, we have more granular control of when we want to show the search results - e.g we can check if there's a match in the Recent section and decide to show results even if the isValid check returns false. (Looks important so we can execute your plan here)

rdjuric · 2021-08-02T14:17:01Z

I marked this as ready for review to get a bit of help 😅

This is working fine on Web/Desktop, but on mobile the SectionList doesn't display the search results when we pass the this.props.headerMessage.isValid ? test.

The OptionsList component is being re-rendered, but I guess SectionList isn't (since it's a PureComponent)? I tried adding this.props.headerMessage.isValid to our extraData props but that didn't help.

Weird that this only happens on Mobile. Do you have any idea why this might be happening?

ios.mp4

Just a note that this problem also happens if I try the original proposal.

thienlnam · 2021-08-02T17:28:56Z

src/libs/OptionsListUtils.js

- * Helper method that returns the text to be used for the header's message and title (if any)
- *
+ * Helper method that returns the text to be used for the header's message and title (if any), the isValid
+ * property decides if we should show the search results for the given search value.
 * @param {Boolean} hasSelectableOptions
 * @param {Boolean} hasUserToInvite
 * @param {String} searchValue
 * @param {Boolean} [maxParticipantsReached]
- * @return {String}
+ * @return {Object}
 */
 function getHeaderMessage(hasSelectableOptions, hasUserToInvite, searchValue, maxParticipantsReached = false) {
    if (maxParticipantsReached) {
-        return translate(preferredLocale, 'messages.maxParticipantsReached');
+        return {message: translate(preferredLocale, 'messages.maxParticipantsReached'), isValid: true};
    }

-    if (!hasSelectableOptions && !hasUserToInvite) {
-        if (/^\d+$/.test(searchValue)) {
-            return translate(preferredLocale, 'messages.noPhoneNumber');
-        }
+    if (isSearchingInvalidPhone(searchValue)) {
+        return {message: translate(preferredLocale, 'messages.noPhoneNumber'), isValid: false};
+    }

-        return searchValue;
+    if (!hasSelectableOptions && !hasUserToInvite) {
+        return {message: searchValue, isValid: true};
    }

-    return '';
+    return {message: '', isValid: true};


We should probably decouple the header message from the rendering of the SectionList. If needed, we can just add another prop that is along the lines of shouldShowSectionList

thienlnam · 2021-08-02T17:30:36Z

src/libs/OptionsListUtils.js

+function isSearchingInvalidPhone(searchValue) {
+    return searchValue && CONST.REGEX.DIGITS_AND_PLUS.test(searchValue) && !Str.isValidPhone(searchValue);
+}


Doesn't seem like we need a whole method for this and can just use it below

thienlnam · 2021-08-02T17:31:37Z

src/components/OptionsList.js

+    // eslint-disable-next-line react/forbid-prop-types
+    headerMessage: PropTypes.object,


Mentioned above I don't think we want to couple the headerMessage and rendering the SectionList but if so, you should shape the expected object instead of disabling the lint

Beamanator · 2021-08-02T18:53:25Z

@rdjuric Hmm I tried reproducing the error you saw on iOS and I wasn't getting any weird behavior like you 😅 Did you check the debugger logs? (Flipper or remote debugging with Chrome?)

rdjuric · 2021-08-02T19:46:46Z

@Beamanator Yes, checked it and both Flipper and Chrome debugging shows nothing weird. They even show the SectionList being rendered correctly 😅

Just to confirm the steps, using the last commit from this branch:

Open the New chat/New group modal
Start typing a invalid number (like +5)
The error message shows up, with no results
Add another digit to make it a valid number (like +55)
The error message doesn't show up, but neither does the results

Did this happen in your tests @thienlnam?

thienlnam · 2021-08-03T00:56:17Z

I run into the same thing, but isn't that just because you don't have any contacts with that number so there are no results?

Beamanator · 2021-08-03T07:47:12Z

I think the main point of this specific issue is to make sure the "i couldn't validate the phone number" error doesn't just get displayed in the console for nobody to see. So either:

We tighten up the regex or use the API call so it's much less likely that an invalid phone number can be used to start a new chat, or
If a new chat with an invalid phone number is clicked, the error message should show below the search box so users know the number is invalid and they should try a new number

What do y'all think?

rdjuric · 2021-08-03T15:16:06Z

@thienlnam If the number isn't in the contacts but is valid, we add them as a userToInvite and display it on the third section of the SectionList so It should have shown up.

@Beamanator

We tighten up the regex or use the API call so it's much less likely that an invalid phone number can be used to start a new chat

This looks like a must-do as our regex lets numbers like +55 (just a country code) pass as valid. But I think this was an improvement to be made in #2936?

If a new chat with an invalid phone number is clicked, the error message should show below the search box so users know the number is invalid and they should try a new number

Hmm. I understand the point but I think that this is worst UX than not showing the result at all.

rdjuric · 2021-08-03T15:21:56Z

Took me a while but I realized that we already filter results for invalid numbers here (see code), so conditionally rendering the SectionList based on the Str.isValidPhone is pointless 😅

App/src/libs/OptionsListUtils.js

Lines 466 to 472 in d15beee

    
           if (searchValue 
        
               && recentReportOptions.length === 0 
        
               && personalDetailsOptions.length === 0 
        
               && _.every(selectedOptions, option => option.login !== searchValue) 
        
               && ((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)) || Str.isValidPhone(searchValue)) 
        
               && (searchValue !== CONST.EMAIL.CHRONOS || Permissions.canUseChronos(betas)) 
        
           ) {

I think all that is left is making sure that

the "i couldn't validate the phone number" error doesn't just get displayed in the console for nobody to see.

by doing the equivalent test in our getHeaderMessage. This way if we change our idea of what a valid number is (by updating the regex or doing an API call) we just have to replace our uses of Str.isValidPhone.

Here's a vid comparing the behavior on this PR and on Staging. I think this is what we're looking for in this issue?

PRxStaging.mov

OSBotify · 2021-08-04T22:24:02Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Beamanator · 2021-08-05T09:00:19Z

Hmm I do like your changes @rdjuric but I was also hoping we could avoid having this situation happen (sorry if it wasn't clear in the issue):

Screen.Recording.2021-08-05.at.10.59.23.AM.mov

Beamanator · 2021-08-05T09:01:15Z

A.k.a. if a user clicks the "new chat" but the phone number isn't 100% valid (a.k.a. our API returns an error) it would be best to show this error to the end user instead of just silently throw the console error and a normal user won't know what happened

rdjuric · 2021-08-05T09:35:43Z

Ahh, I see @Beamanator! Thanks for the video.

Do we want to show a Growl to the user in this scenario? Let me know and I'll open a follow-up PR for this 😃

Beamanator · 2021-08-05T10:12:47Z

NP! Ooh I think a Growl could be great, I'd say go for it!

OSBotify · 2021-08-06T03:17:04Z

🚀 Deployed to staging in version: 1.0.82-8🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-08-11T22:17:30Z

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

rdjuric marked this pull request as ready for review August 2, 2021 13:58

rdjuric requested a review from a team as a code owner August 2, 2021 13:58

MelvinBot requested review from thienlnam and removed request for a team August 2, 2021 13:58

Beamanator assigned rdjuric Aug 2, 2021

thienlnam requested changes Aug 2, 2021

View reviewed changes

remove changes to OptionsList

0f26898

rdjuric requested a review from thienlnam August 3, 2021 16:30

thienlnam approved these changes Aug 4, 2021

View reviewed changes

thienlnam merged commit 9b1ea35 into Expensify:main Aug 4, 2021

This was referenced Aug 5, 2021

Deploy Checklist: Expensify.cash 2021-07-30 #4333

Closed

Deploy Checklist: Expensify.cash 2021-08-06 #4447

Closed

Beamanator mentioned this pull request Aug 9, 2021

[PAY 8/19] Fix how validation displays in UI #2934

Closed

5 tasks

Santhosh-Sellavel mentioned this pull request Aug 13, 2021

Phone Number Validation Message Issue in Login Page #4415

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to how validation displays in UI #4354

Changes to how validation displays in UI #4354

rdjuric commented Aug 1, 2021 •

edited

Loading

rdjuric commented Aug 2, 2021 •

edited

Loading

rdjuric commented Aug 2, 2021 •

edited

Loading

thienlnam Aug 2, 2021

thienlnam Aug 2, 2021

thienlnam Aug 2, 2021 •

edited

Loading

Beamanator commented Aug 2, 2021

rdjuric commented Aug 2, 2021 •

edited

Loading

thienlnam commented Aug 3, 2021

Beamanator commented Aug 3, 2021

rdjuric commented Aug 3, 2021

rdjuric commented Aug 3, 2021 •

edited

Loading

OSBotify commented Aug 4, 2021

Beamanator commented Aug 5, 2021

Beamanator commented Aug 5, 2021

rdjuric commented Aug 5, 2021

Beamanator commented Aug 5, 2021

OSBotify commented Aug 6, 2021

OSBotify commented Aug 11, 2021

		// eslint-disable-next-line react/forbid-prop-types
		headerMessage: PropTypes.object,

Changes to how validation displays in UI #4354

Changes to how validation displays in UI #4354

Conversation

rdjuric commented Aug 1, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

rdjuric commented Aug 2, 2021 • edited Loading

rdjuric commented Aug 2, 2021 • edited Loading

thienlnam Aug 2, 2021

Choose a reason for hiding this comment

thienlnam Aug 2, 2021

Choose a reason for hiding this comment

thienlnam Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Beamanator commented Aug 2, 2021

rdjuric commented Aug 2, 2021 • edited Loading

thienlnam commented Aug 3, 2021

Beamanator commented Aug 3, 2021

rdjuric commented Aug 3, 2021

rdjuric commented Aug 3, 2021 • edited Loading

OSBotify commented Aug 4, 2021

Beamanator commented Aug 5, 2021

Beamanator commented Aug 5, 2021

rdjuric commented Aug 5, 2021

Beamanator commented Aug 5, 2021

OSBotify commented Aug 6, 2021

OSBotify commented Aug 11, 2021

rdjuric commented Aug 1, 2021 •

edited

Loading

rdjuric commented Aug 2, 2021 •

edited

Loading

rdjuric commented Aug 2, 2021 •

edited

Loading

thienlnam Aug 2, 2021 •

edited

Loading

rdjuric commented Aug 2, 2021 •

edited

Loading

rdjuric commented Aug 3, 2021 •

edited

Loading