feat(search): Debounce and phone number validation #3124

pranshuchittora · 2021-05-25T18:01:29Z

Details

Fixed Issues

Closes #2936
Closes #2934

Tests

QA Steps

Tested On

Screenshots

Web

Peek.2021-05-25.23-26.mp4

Mobile Web

Desktop

iOS

Android

HorusGoul

There are many unnecessary changes related to the code style that shouldn't be in this PR. Please undo these and try to make sure the PR looks clean.

Also, please fill the PR template. It would be nice to indicate some QA steps so we can make sure everything works flawlessly.

src/components/OptionsList.js

src/pages/SearchPage.js

HorusGoul · 2021-06-07T09:48:33Z

@pranshuchittora Can you fix the conflicts and run npm run lint -- --fix? Thanks!

pranshuchittora · 2021-06-07T19:17:31Z

I have updated the PR

Beamanator

Thanks for updating, lots of little comments here, please ask if you have any questions!

Beamanator · 2021-06-09T13:42:11Z

src/components/OptionsList.js

+                <View>
+                    {this.props.headerMessage ? (
+                        <View style={[styles.ph5]}>
+                            <Text style={[styles.textLabel, styles.colorMuted]}>
+                                {this.props.headerMessage}
+                            </Text>
+                        </View>
+                    ) : null}
+                </View>


Was this change necessary? If yes, can you explain? I tested without the new View you added and it seems to be working fine

Removing this

src/styles/utilities/display.js

Beamanator · 2021-06-09T13:46:29Z

src/styles/styles.js

+    textMicroGTA: {
+        fontSize: variables.fontSizeSmall,
+        fontFamily: fontFamily.GTA,
+    },
+


Please delete if you're not using anymore

Beamanator · 2021-06-09T13:51:41Z

src/components/OptionsSelector.js

-                <View style={[styles.ph5, styles.pv3]}>
+                <View style={[
+                    styles.ph5,
+                    styles.pt3,


I think none of the changes in this file are needed, the only difference I can see that your changes make is they reduce the padding below the following TextInputWithFocusStyles, but I think it looks find with the original padding. Is there any other change you were trying to make with these style updates?

Reverting back

Beamanator · 2021-06-09T13:57:09Z

src/pages/SearchPage.js

-            userToInvite,
-        } = getSearchOptions(
+        this.validateInput = _.debounce(this.validateInput.bind(this), 300);
+        const {recentReports, personalDetails, userToInvite} = getSearchOptions(


This change isn't necessary - please try to keep lines the same as before, unless you're changing something in the line(s).

Beamanator · 2021-06-10T10:47:38Z

src/pages/SearchPage.js

+        const headerMessage = getHeaderMessage(
+            this.state.personalDetails.length !== 0,
+            false,
+            searchValue,
+        );


In this function, it looks like the only time we're using this headerMessage is if the the phone number is invalid, so I think it would be better if we just do headerMessage: translate(preferredLocale, 'messages.noPhoneNumber') directly in the else statement below - since this will only be displayed if the modifiedSearchValue is an invalid phone number - we shouldn't need to call getHeaderMessage right?

I am keeping the call to getHeaderMessage because if we want to make any change in the underlying messaging part then we have only one place to change. Or a single source of truth.

Beamanator · 2021-06-10T10:48:05Z

src/pages/SearchPage.js

+                        this.setState({
+                            recentReports: [],
+                            userToInvite: null,
+                            headerMessage,


As mentioned above, let's do something like headerMessage: translate(preferredLocale, 'messages.noPhoneNumber') here

src/pages/SearchPage.js

Beamanator · 2021-06-10T10:57:59Z

src/pages/SearchPage.js

+                userToInvite,
+                recentReports,
+                personalDetails,
+                headerMessage: recentReports.length + personalDetails.length


I think it could be cleaner to just make this else an else if (Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)), where headerMessage should always be '', then add an else which displays the this.props.translate('messages.noEmailOrPhone') error message, what do you think?

This validation is not required as it is done internally by getSearchOptions

src/pages/SearchPage.js

pranshuchittora · 2021-06-11T17:23:37Z

@Beamanator I have updated the PR

Beamanator

The work you're doing so far is great for the Search Page, but we also need it to be used in all pages that have account-search functionality (+ new chat, new group, some IOU pages)

Beamanator · 2021-06-12T13:20:02Z

src/pages/SearchPage.js

+     * @param {String} searchValue
+     * @returns {void}
+     */
+    validateInput(searchValue) {


I believe we're going to have to move this into OptionsListUtils since the validation you're adding is going to apply to all pages that have account-search functionality. Can you figure out how we can reuse getSearchOptions here (by editing getOptions)? I'm thinking we can have getOptions also return headerMessage

I discussed this with you earlier. That my initial approach was to add the changes in getOptions only but it's not flexible enough and might require good amount of refactoring.

Because there are a lot of things happening internally and the code is a bit messy and adding more code will increase the complexity and maintainability efforts.

Woah you did? I'm sorry I don't remember you bringing up that approach, maybe I misunderstood :O

Hmm I don't see why we can't refactor getHeaderMessage just a bit, and make getOptions also return something like headerMessage: getMeaderMessage(...). Here's why I'm thinking this will work (please let me know if you disagree on these points):

getHeaderMessage takes only a few parameters, and all of the params should be able to be calculated from other variables inside getOptions

getOptions is only used in OptionsListUtils, which has get...Options functions for specific pages, all of those pages will need this type of functionality (the same type of logic for headerMessage)

Hi @Beamanator
I tried merging these changes into utils. So getSearchOptions & getHeaderMessage are independent of each other. They both take the searchValue as one of the input. With this, we might end up hitting the phone number validation API twice.

Hmm once you push your latest changes I can check and see if I can help come up with a way to only hit that validation API once 👍

Beamanator · 2021-06-24T09:17:19Z

@pranshuchittora Can you give an update when you think you'll be able to get to the requested changes above?

mallenexpensify · 2021-06-28T16:57:45Z

@pranshuchittora a requirement that you promptly reply to comments on issues and PRs in order to avoid receiving negative reviews internally which could affect the future hiring. Can you please respond to @Beamanator above and also check all other issues and PRs that you're working on to make sure you're caught up?

pranshuchittora · 2021-06-28T17:01:10Z

Pardon the delayed reply, I have completed this task and able to get the required output but need to migrate the logic inside utils. Will update the PR as soon as possible.

pranshuchittora · 2021-07-01T21:09:51Z

@Beamanator here's the demo for the integration of search validation API inside utils.

Peek.2021-07-02.02-31.mp4

Here are the changes -> pranshuchittora@86d635e
(These changes are just a proof of concept)

Few insights while implementing these changes.

If we add the API call inside utils then we need to make all the consumers thenable, which itself is a huge effort. Because we are calling APIs (Promise).
Multiple API calls for the same payload, as getHeaderMessage and getOptions are independent and merging them is also a huge effort.

Correct me if I am wrong or missing anything here.
(P.S. you have more content then me)

Beamanator · 2021-07-05T15:09:58Z

@pranshuchittora Ok i see that the API is being called twice - once in getHeaderMessage and once in getOptions

If we add the API call inside utils then we need to make all the consumers thenable, which itself is a huge effort. Because we are calling APIs (Promise).

You're right that we'd have to asynchronous chain (promises) down a bit, but I don't see this being a huge effort - maybe that's because I have lots of experience doing that. Are you comfortable making those promise chains? Also if you don't feel this is a good implementation can you ask for recommendations in #expensify-open-source?

Multiple API calls for the same payload, as getHeaderMessage and getOptions are independent and merging them is also a huge effort.

True that if you keep them separate we could have the api called twice when we only want to call it once. However, I don't see how combining them will be difficult since getHeaderMessage is a pretty small function, and the parameters it takes are already available inside getOptions. Am I overlooking something?

HorusGoul · 2021-07-05T16:10:08Z

@pranshuchittora The solution you're proposing is breaking the React rule related to triggering side-effects in the render() function (the this.getHeader() call).

Also, I don't see the need for calling the IsValidPhoneNumber function inside the other API call, we could show the translate(preferredLocale, 'messages.noPhoneNumber'); text if this condition is true: Str.isValidPhone(searchValue) && this.state.userToInvite === null.

What do you think?

pranshuchittora · 2021-07-07T21:12:21Z

React rule related to triggering side-effects in the render() function (the this.getHeader() call).

I agree. I have done this to reduce the possibility of any breaking change. Because right now getHeaderMessage gives pretty deterministic output for the given input (search value etc). But introducing the validation API for showing the header message is a breaking change.

Either we call the API and wait for the response (blocking render ❌ )
Or we keep the message empty and wait for the APIs response and then show a relevant message (bad UX)

Also, I don't see the need for calling the IsValidPhoneNumber function inside the other API call, we could show the translate(preferredLocale, 'messages.noPhoneNumber'); text if this condition is true: Str.isValidPhone(searchValue) && this.state.userToInvite === null.

We need to call the API that the goal of this ticket. To disable creating chats with invalid phone numbers.
@HorusGoul Correct me if I am wrong 🤔

HorusGoul · 2021-07-08T08:37:58Z

We can't block rendering because the validation is asynchronous. If there are no valid phone numbers, the user won't be able to select anything.
This is what's happening in your demo. We could improve the UX by adding a spinner or something on top of the field, but let's focus on getting the other things right first, as adding a spinner later is easier.

We need to call the API that the goal of this ticket. To disable creating chats with invalid phone numbers.

Of course, it's just that we would only do it in one place. The getSearchOptions function calls that API command and returns the state that we need. Then, using that state, we can derive from it if we need to show the invalid message or the list of results, which can be done with code similar to this:

const apiSaysPhoneIsInvalid = Str.isValidPhone(searchValue) && this.state.userToInvite === null;

pranshuchittora · 2021-07-08T09:45:59Z

Right now the getSearchOptions is being called inside the constructor and does not have any promise based call inside it. Adding API call inside the getSearchOptions will be a breaking change and requires all the consuming entities to wait for the promise resolution (.then) with this we can no longer call it inside the constructor

HorusGoul · 2021-07-08T11:50:01Z

You added the API call inside getOptions, and getSearchOptions calls getOptions.
https://github.com/pranshuchittora/Expensify.cash/blob/86d635eafbde5aea7c46c466a20cae657a313624/src/libs/OptionsListUtils.js#L413-L471

Also, you're already treating getSearchOptions like an asynchronous function in your code:
https://github.com/pranshuchittora/Expensify.cash/blob/86d635eafbde5aea7c46c466a20cae657a313624/src/pages/SearchPage.js#L152-L171

To be clear: All I'm saying is related to the changes you linked in this comment: #3124 (comment)

Could you upload those changes to this branch? It would be easier for us to review.

pranshuchittora · 2021-07-13T17:31:38Z

Could you upload those changes to this branch? It would be easier for us to review.

I haven't uploaded them to this branch because I am not sure of them as they are breaking changes. As the methods have async call then we need to change the consumption everywhere which is out of scope for this ticket.

HorusGoul · 2021-07-21T15:01:18Z

Could you upload those changes to this branch? It would be easier for us to review.

I haven't uploaded them to this branch because I am not sure of them as they are breaking changes. As the methods have async call then we need to change the consumption everywhere which is out of scope for this ticket.

Could we move the asynchronous code inside the getSearchOptions function so the getOptions doesn't need to return a promise and we only need to worry about the usages of getSearchOptions in the codebase?

trjExpensify · 2021-07-26T09:12:34Z

Hi @pranshuchittora, can we get an update here please?

pranshuchittora · 2021-07-26T19:12:04Z

Could we move the asynchronous code inside the getSearchOptions function so the getOptions doesn't need to return a promise and we only need to worry about the usages of getSearchOptions in the codebase?

No, moving the async code from getOptions would require copying/migrating the validation logic from getOptions to getSearchOptions as well.

Anyway, this will come with a breaking change because

getSearchOptions is being called inside the constructor due to which calling any async code is not possible.
Validation logic is inside getOptions.

HorusGoul · 2021-07-27T11:40:36Z

Remove that call. Let's call this.updateOptions() inside the componentDidMount.
Duplicating the validation logic in the getSearchOptions could be a great start to get this to work without breaking any rule. We can worry about that duplication later.

pranshuchittora · 2021-08-02T15:01:25Z

Migrating the logic inside utils will be a breaking change considering consumption (i.e. inside constructor) therefore some good amount of refactoring is required.

No worries, it was really nice working with you all. Looking forward to working collaborating with you in future :)

pranshuchittora requested a review from a team as a code owner May 25, 2021 18:01

MelvinBot requested review from HorusGoul and removed request for a team May 25, 2021 18:01

pranshuchittora added 4 commits May 25, 2021 23:35

feat: Debouncing of search with phone number validation

74f0c59

feat: Loader on search page

f1ee5ff

feat: Design changes

f9f000a

feat: No email message

3effa64

pranshuchittora force-pushed the pranshuchittora/fix/2936 branch from 82699ae to 3effa64 Compare May 25, 2021 18:05

pranshuchittora mentioned this pull request May 25, 2021

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

Closed

5 tasks

Beamanator assigned pranshuchittora May 26, 2021

HorusGoul suggested changes May 27, 2021

View reviewed changes

src/components/OptionsList.js Outdated Show resolved Hide resolved

src/pages/SearchPage.js Outdated Show resolved Hide resolved

HorusGoul requested a review from Beamanator May 27, 2021 12:11

feat: Err msg or results not both

cf25b91

pranshuchittora added 2 commits June 8, 2021 00:35

Merge remote-tracking branch 'origin/main' into pranshuchittora/fix/2936

2000719

feat: Liniting changes

8a8b1cf

pranshuchittora added 3 commits June 8, 2021 00:49

fix: Minor linitng changes

4c41ecc

fix: Minor styling fixes

39581a4

Merge remote-tracking branch 'origin/main' into pranshuchittora/fix/2936

429d9aa

Beamanator requested changes Jun 10, 2021

View reviewed changes

feat: PR review changes

cfb09ce

pranshuchittora requested a review from Beamanator June 11, 2021 17:23

Beamanator requested changes Jun 12, 2021

View reviewed changes

Beamanator mentioned this pull request Jun 18, 2021

Fix IOU request sent to new phone number account #3651

Merged

5 tasks

Beamanator closed this Aug 2, 2021

feat(search): Debounce and phone number validation #3124

feat(search): Debounce and phone number validation #3124

Conversation

pranshuchittora commented May 25, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

HorusGoul left a comment

Choose a reason for hiding this comment

HorusGoul commented Jun 7, 2021

pranshuchittora commented Jun 7, 2021

Beamanator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora commented Jun 11, 2021

Beamanator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beamanator commented Jun 24, 2021

mallenexpensify commented Jun 28, 2021

pranshuchittora commented Jun 28, 2021

pranshuchittora commented Jul 1, 2021 • edited Loading

Beamanator commented Jul 5, 2021

HorusGoul commented Jul 5, 2021

pranshuchittora commented Jul 7, 2021 • edited Loading

HorusGoul commented Jul 8, 2021

pranshuchittora commented Jul 8, 2021

HorusGoul commented Jul 8, 2021

pranshuchittora commented Jul 13, 2021 • edited Loading

HorusGoul commented Jul 21, 2021

trjExpensify commented Jul 26, 2021

pranshuchittora commented Jul 26, 2021

HorusGoul commented Jul 27, 2021

pranshuchittora commented Aug 2, 2021

pranshuchittora commented May 25, 2021 •

edited

Loading

pranshuchittora Jun 11, 2021 •

edited

Loading

pranshuchittora Jun 14, 2021 •

edited

Loading

pranshuchittora Jun 28, 2021 •

edited

Loading

pranshuchittora commented Jul 1, 2021 •

edited

Loading

pranshuchittora commented Jul 7, 2021 •

edited

Loading

pranshuchittora commented Jul 13, 2021 •

edited

Loading