Requesting payments and Splitting bills with self causes indefinite loading

[Santhosh-Sellavel]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Expected Result:

Requesting payments and splitting bills with self should not be allowed.

Actual Result:

Requesting payments and splitting bills with self results in infinite loading.

Action Performed:

Splitting bills:

• New
• Split Bill
• Enter your own email in search
• Select your email from result
• Enter amount & what is it for and tap split.

Requesting money:

• New
• Request Money
• Enter your own email in search
• Select your email from result
• Enter amount & reason and send

Workaround:

Need close and reopen app. Refresh in web.
We can restrict the user to pick own email from search result. As we do for new chat with self.

Or if team we have other ideas, I'll do that.

I'll work on my proposal, based on your inputs.

Platform:

Where is this issue occurring?
I personal able to reproduce on android & web, I sure it will occur on all platform.

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:1.0.65
Notes/Photos/Videos:

Splitting bills:
https://user-images.githubusercontent.com/85645967/121790974-99f67980-cc02-11eb-969c-c355961de0c3.mp4

Requesting money:
https://user-images.githubusercontent.com/85645967/121790494-872d7600-cbfd-11eb-964a-1bd8fe0eaa1a.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[Christinadobrzyn]

Should we combine this GH with this GH into one issue?

[MelvinBot]

Triggered auto assignment to @Dal-Papa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[quinthar]

Trying to split a bill with yourself rips open SpaceTime and causes a singularity. Please don't do it.

…

On Sun, Jun 13, 2021, 6:54 PM MelvinBot ***@***.***> wrote: Triggered auto assignment to @Dal-Papa <https://github.com/Dal-Papa> ( Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3572 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEMNUTWPEAJG3A7QQ6ICRLTSVONVANCNFSM46TFKXTA> .

[jasperhuangg]

Hey @Santhosh-Sellavel!

Do you have a proposal for how you plan to prevent this from happening? I'm thinking it would be something along the lines of filtering out the user both here and here.

What are your thoughts?

[jasperhuangg]

@Christinadobrzyn @Santhosh-Sellavel I've closed the other, related issue out and updated this issue to include both Splitting Bills and Requesting Money 👍

[Santhosh-Sellavel]

Hey @Santhosh-Sellavel!

Do you have a proposal for how you plan to prevent this from happening? I'm thinking it would be something along the lines of filtering out the user both here and here.

What are your thoughts?

Hey @jasperhuangg ,
Do we need to show, user email in search?

If yes
we need to just need to restrict action on tap.
else
we can filter out the current user email from search.

Let me know how you need to handle, I'll propose with technical solution.

[Santhosh-Sellavel]

Here is my proposal,
@jasperhuangg

First in both files will add the following lines to retrieve myPersonalDetails as a prop.
myPersonalDetails:{ key: ONYXKEYS.MY_PERSONAL_DETAILS, },

Then,

https://github.com/Expensify/Expensify.cash/blob/3ebf6136517de6f9ad101037392e2a0a5954347c/src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsRequest.js#L93

https://github.com/Expensify/Expensify.cash/blob/3ebf6136517de6f9ad101037392e2a0a5954347c/src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsSplit.js#L135

Change the above line in respective files to:

if(this.state.userToInvite && this.state.userToInvite.login.toLowerCase() != this.props.myPersonalDetails.login.toLowerCase())

this will filter out the logged-in user

[Santhosh-Sellavel]

Hey @jasperhuangg ,

Instead of duplicating logic, will add a function in OptionUtils and invoke from both files.

I proposed with myPersonalDetails prop.
May be I should use something else?

Please share your thoughts.

[jasperhuangg]

Hey @Santhosh-Sellavel,

That looks good, although you'll probably wanna look into using ONYXKEYS.SESSION to get the current user's login. Quick question, why do you feel that it's necessary to convert to lowercase?

Also, there's a separate issue where we'll still move forward with the IOU request if we've typed in an email address that doesn't appear as a search result. Just wanted to clarify with you that this issue is unrelated to the task at hand.

[Santhosh-Sellavel]

Hey @Santhosh-Sellavel,

That looks good, although you'll probably wanna look into using ONYXKEYS.SESSION to get the current user's login. Quick question, why do you feel that it's necessary to convert to lowercase?

Also, there's a separate issue where we'll still move forward with the IOU request if we've typed in an email address that doesn't appear as a search result. Just wanted to clarify with you that this issue is unrelated to the task at hand.

Hey @jasperhuangg ,
In userToInvite.login email value starts with upperCase.

I don't know exactly why, But I am assume it may due to user typed with upper case.

In my phone when I initiate search first letter is always in upperCase, unless I manually switch it lower (Which I feel unnecessary).

So for the safer side I used toLowerCase().

ONYXKEYS.SESSION - I will check this.

Thanks for suggestion, will send a updated proposal soon.

[Santhosh-Sellavel]

Hey @jasperhuangg,

In [OptionsListUtils.js]
(https://github.com/Expensify/Expensify.cash/blob/3ebf6136517de6f9ad101037392e2a0a5954347c/src/libs/OptionsListUtils.js)

currentUserLogin is already available here
https://github.com/Expensify/Expensify.cash/blob/3ebf6136517de6f9ad101037392e2a0a5954347c/src/libs/OptionsListUtils.js#L20
https://github.com/Expensify/Expensify.cash/blob/3ebf6136517de6f9ad101037392e2a0a5954347c/src/libs/OptionsListUtils.js#L153-L156

So will invoke the method by modifying the following line in respective files.
https://github.com/Expensify/Expensify.cash/blob/3ebf6136517de6f9ad101037392e2a0a5954347c/src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsRequest.js#L93

https://github.com/Expensify/Expensify.cash/blob/3ebf6136517de6f9ad101037392e2a0a5954347c/src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsSplit.js#L135

with if(this.state.userToInvite && isNotCurrentUser(this.state.userToInvite)).

Kindly share your thoughts on this.

[Santhosh-Sellavel]

Hey @jasperhuangg,

Another case for this issue,

When using email for sign in,
currentUserLogin had value: something@mail.com
and searchItem result Item userToInvite.login also had the same valuesomething@mail.com.

But for mobile,
currentUserLogin has value: +91XXXXXXX5X3@expensify.sms
and searchItem result Item userToInvite.login also had the value +91XXXXXXX5X3.

it doesn't work in this case.

So we should get appended result with '@expensify.sms' in the userToInvite.login, if login is a mobile number.

One last scenario,
If the user has a secondary login option email + mobile number.
Primary Login - Signed account this login
Secondary Login - Added after login

If the user requests to primary login, we have it almost handled with my proposal(Need to finalize on mobile number).

But to handle when the user requests to secondary login, we cannot handle it with currentUserLogin alone.

For handling the case we need to fetch user details using ONYXKEYS.USER.

User Object:

It has loginList has both logins.

So need to valid isNotCurrentUser using loginList to handle all cases in this scenario.

Share your thoughts & Let me know If I'm missing something.

[Santhosh-Sellavel]

Hey @jasperhuangg ,

Post your comments

[Santhosh-Sellavel]

Hey @jasperhuangg,

Another case for this issue,

When using email for sign in,
currentUserLogin had value: something@mail.com
and searchItem result Item userToInvite.login also had the same valuesomething@mail.com.

But for mobile,
currentUserLogin has value: +91XXXXXXX5X3@expensify.sms
and searchItem result Item userToInvite.login also had the value +91XXXXXXX5X3.

it doesn't work in this case.

So we should get appended result with '@expensify.sms' in the userToInvite.login, if login is a mobile number.

One last scenario,
If the user has a secondary login option email + mobile number.
Primary Login - Signed account this login
Secondary Login - Added after login

If the user requests to primary login, we have it almost handled with my proposal(Need to finalize on mobile number).

But to handle when the user requests to secondary login, we cannot handle it with currentUserLogin alone.

For handling the case we need to fetch user details using ONYXKEYS.USER.

User Object:

It has loginList has both logins.

So need to valid isNotCurrentUser using loginList to handle all cases in this scenario.

Share your thoughts & Let me know If I'm missing something.

Post your thoughts, @Dal-Papa

[Dal-Papa]

Hey @Santhosh-Sellavel sorry for the delayed response, I was trying to find a way for you to get your own accountID from your login but it doesn't seem to exist. I think your proposal is fine. Feel free to throw some ideas and we can chat internally if that sounds good.

[Santhosh-Sellavel]

@Dal-Papa

I couldn't able to find you in slack group. Ping me there, we can discuss more over there.

[Santhosh-Sellavel]

@Dal-Papa Upwork job for this ?

[MelvinBot]

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[stephanieelliott]

Job in Upwork is here: https://www.upwork.com/jobs/~0180725decbdd185e0

Heads up @Santhosh-Sellavel, we'll want to agree on a comprehensive proposal here in the GH issue before hiring in Upwork. Once you settle on the solution you'd plan to implement, please post it here in the issue as a comment.

[MelvinBot]

Triggered auto assignment to @deetergp (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Santhosh-Sellavel]

Already completed,

Proposals:
#3572 (comment)
#3572 (comment)

Proposal Acceptance
#3572 (comment)

Slack discussion
https://expensify.slack.com/archives/C01GTK53T8Q/p1625752160038700

Even PR is merged #4062

@stephanieelliott

[stephanieelliott]

Ah ok thanks for clarifying @Santhosh-Sellavel. Looks like #4063 was the PR merged for this fix. I hired you on Upwork, please accept the offer and we will pay up for the issue + reporting bonus ASAP!

[stephanieelliott]

7-day regression period was already completed, paid in Upwork!