Fix the iou request for "unknown" users #24348

ShogunFire · 2023-08-10T01:51:09Z

Details

Due to the privacy changes, the login is null for users that haven't sent message to us so when we made an iou request the payerEmail sent to the api was null which was creating an error. Since we are actively creating a request or creating a chat with this user using his email/phone, we can use that as the payerEmail when there is no login

Fixed Issues

$ #22884
PROPOSAL: #22884 (comment)

Tests

Create a chat with a user that you have never sent a message to
In this chat, click on + and make an iou request
Send some message.
Request money again.
Verify that there is only one IOU Preview and that it is the last message, also verify that there are two replies below this preview

Verify that no errors appear in the JS console

Offline tests

While offline create a chat with a user that you have never sent a message to
In this chat, click on + and make an iou request
Send some message.
Request money again.
Verify that there is only one IOU Preview and that it is the last message
Come back online and verify that the iou preview now has 2 replies

QA Steps

Create a chat with a user that you have never sent a message to
In this chat, click on + and make an iou request
Send some message.
Request money again.
Verify that there is only one IOU Preview and that it is the last message, also verify that there are two replies below this preview

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

2023-08-09.19-14-26.mp4

Mobile Web - Chrome

2023-08-09.19-23-44.mp4

Mobile Web - Safari

2023-08-09.21-07-38.mp4

Desktop

2023-08-09.21-02-32.mp4

iOS

2023-08-09.20-57-26.mp4

Android

2023-08-10.11-05-47.mp4

melvin-bot · 2023-08-10T01:51:14Z

@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

ShogunFire · 2023-08-10T05:39:41Z

I think it's already working for split which for some reasons still has the email in the logins, buildOptimisticIOUReport is also used for sendMoney but if I am not mistaken this functionality is disabled.

ShogunFire · 2023-08-10T05:40:20Z

PR ready for review

melvin-bot · 2023-08-15T19:54:49Z

@parasharrajat Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

parasharrajat · 2023-08-16T12:48:23Z

Reviewed the issue. Reviewing PR now.

parasharrajat · 2023-08-16T15:32:08Z

src/libs/actions/IOU.js

@@ -315,7 +315,7 @@ function buildOnyxDataForMoneyRequest(
 *
 */
 function requestMoney(report, amount, currency, payeeEmail, payeeAccountID, participant, comment, receipt = undefined) {
-    const payerEmail = OptionsListUtils.addSMSDomainIfPhoneNumber(participant.login);
+    const payerEmail = OptionsListUtils.addSMSDomainIfPhoneNumber(participant.login || participant.text);


I am not 100% into this solution. Although it solves this issue it does not solve the root cause. There will be more cases where the same root cause can become another issue.

IMO, we should set the login to the default email for optimistic accounts.

I agree with you, I had sent this on slack for another issue: https://expensify.slack.com/archives/C01GTK53T8Q/p1690214768266839

I was saying if we create the chat with another user, we know his login already so there is no point in hiding it. It looks like this weird behaviour was intended so I don't know...

What is the default email that you are referring to?

The one that we invited.

So what do we decide ? Or who could help deciding ?

I would love to see an analysis of alternatives if any. I will give it a thought over weekend and then share update on Monday.

Honestly I don't see any alternative if we keep the privacy change as it is

parasharrajat · 2023-08-16T15:32:58Z

src/libs/ReportUtils.js

    const formattedTotal = CurrencyUtils.convertToDisplayString(total, currency);
    const personalDetails = getPersonalDetailsForAccountID(payerAccountID);
-    const payerEmail = personalDetails.login;
+    const payerEmail = personalDetails.login || unknownPayerEmail;


Again, the concept of unknownPayerEmail does not make sense. You are building an iou and the payer is unknown....

mountiny · 2023-08-21T22:44:40Z

Yeah I can see why this does not look ideal, I think we however need to make a change like this right now and document it better to make it clear why we are doing this. It might be temporary but worth it.

mountiny

@ShogunFire @parasharrajat we also need the same fix for Split bill right?

mountiny · 2023-08-21T22:45:17Z

src/libs/ReportUtils.js

 * @returns {Object}
 */
-function buildOptimisticIOUReport(payeeAccountID, payerAccountID, total, chatReportID, currency, isSendingMoney = false) {
+function buildOptimisticIOUReport(payeeAccountID, payerAccountID, total, chatReportID, currency, isSendingMoney = false, unknownPayerEmail = '') {


Lets just call it payerEmail, but in the docs explain the situation

Suggested change

function buildOptimisticIOUReport(payeeAccountID, payerAccountID, total, chatReportID, currency, isSendingMoney = false, unknownPayerEmail = '') {

function buildOptimisticIOUReport(payeeAccountID, payerAccountID, total, chatReportID, currency, isSendingMoney = false, payerEmail = '') {

There is already a payerEmail variable in the function, checking if we need it now

ShogunFire · 2023-08-21T23:01:52Z

@ShogunFire @parasharrajat we also need the same fix for Split bill right?

̶I̶ ̶t̶h̶i̶n̶k̶ ̶y̶e̶s̶.̶ ̶A̶c̶t̶u̶a̶l̶l̶y̶ ̶I̶ ̶w̶a̶n̶t̶e̶d̶ ̶t̶o̶ ̶r̶e̶v̶e̶r̶i̶f̶y̶ ̶t̶h̶i̶s̶ ̶b̶u̶t̶ ̶t̶h̶e̶r̶e̶ ̶s̶e̶e̶m̶s̶ ̶t̶o̶ ̶b̶e̶ ̶a̶n̶o̶t̶h̶e̶r̶ ̶e̶r̶r̶o̶r̶ ̶r̶i̶g̶h̶t̶ ̶n̶o̶w̶ ̶w̶h̶e̶n̶ ̶w̶e̶ ̶s̶p̶l̶i̶t̶ ̶b̶i̶l̶l̶ ̶w̶i̶t̶h̶ ̶u̶n̶k̶n̶o̶w̶n̶ ̶u̶s̶e̶r̶s̶ ̶d̶u̶e̶ ̶t̶o̶ ̶f̶o̶r̶m̶a̶t̶i̶n̶g̶ ̶"̶u̶s̶e̶r̶1̶ ̶a̶n̶d̶ ̶u̶s̶e̶r̶ ̶2̶"̶ ̶w̶h̶e̶n̶ ̶t̶h̶e̶y̶ ̶d̶o̶n̶'̶t̶ ̶h̶a̶v̶e̶ ̶l̶o̶g̶i̶n̶ ̶h̶e̶r̶e̶:̶ ̶h̶t̶t̶p̶s̶:̶/̶/̶g̶i̶t̶h̶u̶b̶.̶c̶o̶m̶/̶E̶x̶p̶e̶n̶s̶i̶f̶y̶/̶A̶p̶p̶/̶b̶l̶o̶b̶/̶e̶3̶0̶7̶4̶c̶4̶9̶a̶7̶9̶3̶8̶b̶9̶d̶9̶7̶8̶b̶4̶8̶7̶2̶1̶f̶d̶b̶a̶d̶d̶8̶9̶8̶0̶b̶f̶6̶4̶4̶/̶s̶r̶c̶/̶l̶i̶b̶s̶/̶a̶c̶t̶i̶o̶n̶s̶/̶I̶O̶U̶.̶j̶s̶#̶L̶6̶1̶5̶-̶L̶6̶1̶7̶ ̶

EDIT: I was looking at prod but this has already been fixed in main

Anyways I will Make the change because yes I think it's necessary

mountiny · 2023-08-21T23:04:01Z

Yep I think thats highly possible and we should try to make it safely accessed so it looks fine at the same time

ShogunFire · 2023-08-22T00:21:58Z

Ok I made some changes, I made payerEmail required parameter and removed the one that was in the function, tell me what you think.

Also I am not sure why this PR still thinks there is conflict, I think I resolved them already

ShogunFire · 2023-08-22T04:33:36Z

@dukenv0307 has a good point here: #23038 (comment) That method doesn't work if the user changed his display name.

I have another sketchy method that seem to work for all users,
When the user change his display name the only place where we can find his email seem to be in report.participants, unfortunely this is just a list of all emails of the report without any way to search a specific user, but the order of report.participants seem to be the same than the order of report.participantAccountIDs. So in navigateToNextPage here

App/src/libs/actions/IOU.js

Lines 1789 to 1794 in 5a373bf

    
           const participants = ReportUtils.isPolicyExpenseChat(report) 
        
               ? [{reportID: report.reportID, isPolicyExpenseChat: true, selected: true}] 
        
               : _.chain(report.participantAccountIDs) 
        
                     .filter((accountID) => currentUserAccountID !== accountID) 
        
                     .map((accountID) => ({accountID, selected: true})) 
        
                     .value();

, we can set the login of the participants like this:

var participants = [];

           
for (let i = 0; i < report.participantAccountIDs.length; i++) {
  const accountID = report.participantAccountIDs[i];
  if (currentUserAccountID !== accountID){
      participants.push({accountID, selected: true, login: report.participants[i]})
  }
}

If we don't want to put it in the login field (to not have to display it) we will have to create another field
If we go with that method, the other changes are unnecessary

ShogunFire · 2023-08-22T11:56:05Z

Otherwise I recommend this but it would take way more effort and time:

"I have a proposition about the privacy change that is causing so many issues. Why don't we create another field email or privateLogin that we will never display and if needed we can even encrypted it so that it doesn't appear in the network. And use that field to do all the things that we are currently doing with the login field"

mountiny · 2023-08-22T12:35:44Z

Argh I hate this thing, I think we discussed this couple of times but it was deprioritized.

could we maybe just Blcok requesting money from chat with user where we dont have email, ie you can request from Global create from anyone but you cannot keep requesting until that perosn responds to you within the chat

…ave login because we would not be able to send payerEmail easily Signed-off-by: Pierre Michel <[email protected]>

ShogunFire · 2023-08-22T13:35:42Z

I made the change

ShogunFire · 2023-08-23T02:55:28Z

There seems to have an issue, when we create a chat with a unknown user the money request option is showing for a few seconds (probably until the server response) I will try to fix that

ShogunFire · 2023-08-23T13:08:34Z

:( so in getOptions we add the login to the userToInviteOption, I don't think it is safe to remove that

App/src/libs/OptionsListUtils.js

Line 833 in 9a0dd42

userToInvite.login = searchValue;

So maybe we can also prevent money request if the report is optimistic, is there a reason we don't add isOptimisticReport: true in this method:

App/src/libs/ReportUtils.js

Line 2199 in 9a0dd42

function buildOptimisticChatReport(

mountiny · 2023-08-23T14:26:28Z

@ShogunFire thanks for trying

I think we can hold on this one until we have a clearer plan as it does not seem as ultimately the best direction to head into.

ShogunFire · 2023-08-23T14:49:29Z

Can I try one last thing ? I can start implementing this

Why don't we create another field email or privateLogin that we will never display and if needed we can even encrypted it so that it doesn't appear in the network. And use that field to do all the things that we are currently doing with the login field

I don't think I need back end changes, I can just fill the new field optimistically with the login here:

App/src/libs/actions/Report.js

Lines 496 to 502 in 867d61e

    
           optimisticPersonalDetails[accountID] = allPersonalDetails[accountID] || { 
        
               login, 
        
               accountID, 
        
               avatar: UserUtils.getDefaultAvatarURL(accountID), 
        
               displayName: login, 
        
               isOptimisticPersonalDetail: true, 
        
           };

Then for the payerEmail I can use that field instead of login or text here:

App/src/libs/actions/IOU.js

Line 308 in 40f354b

    
           const payerEmail = OptionsListUtils.addSMSDomainIfPhoneNumber(participant.login);

ShogunFire · 2023-08-23T14:58:58Z

That would not work if you change platforms though but I think that's a good beginning

mountiny · 2023-08-24T00:29:53Z

the best solution so far is to update those flows to work with accountIDs as we have those, I think any other proposed solution so far is quite "dirty"

ShogunFire · 2023-08-24T00:58:47Z

The accountIDs created optimistically and returned by the backend can be different right ? Then I don't know how making iou request offline based on something that can change would work. Anyways, seems like there is not much I can do right now if we go with that option

mountiny · 2023-08-24T01:14:06Z

@ShogunFire you are right, that is one of the challenges there

mountiny · 2023-08-24T01:14:41Z

This is the issue #22480

mountiny · 2023-08-24T01:16:49Z

The accountIDs created optimistically and returned by the backend can be different right ? Then I don't know how making iou request offline based on something that can change would work. Anyways, seems like there is not much I can do right now if we go with that option

I think we would have to use both email and accountID. AccountID would be a fallback, when you are creating this offline you know the email so you can use that, then once you sign out and sign back in you had to be online and you had to get the correct accountID

ShogunFire · 2023-08-24T05:39:00Z

Honestly it doesn't seem like a pretty solution. I feel like the privacy change should have just been a display issue. Not displaying the login, but still using it.

I would personally prefer to have an email field that we would never display or a boolean shouldDisplayLogin field. It would require way less changes and seems like an easier solution.

mountiny · 2023-08-24T14:15:22Z

Not displaying the login, but still using it.

this would not prevent people from just scraping the contact details from the public chat rooms, we cannot return this data from BE

ShogunFire · 2023-08-24T14:19:12Z

Even if we encrypt it ?

mountiny · 2023-08-24T14:24:16Z

I think that was one of the options when the project was planned but it was not chosen for reasons I am not familiar with

mountiny · 2023-08-24T15:22:54Z

Lets close this PR for now @ShogunFire thanks for your work, we can discuss a partial reward in the issue

ShogunFire requested a review from a team as a code owner August 10, 2023 01:51

melvin-bot bot requested review from thesahindia and removed request for a team August 10, 2023 01:51

jasperhuangg requested review from a team and removed request for thesahindia August 15, 2023 19:46

melvin-bot bot requested review from parasharrajat and removed request for a team August 15, 2023 19:54

parasharrajat reviewed Aug 16, 2023

View reviewed changes

ShogunFire mentioned this pull request Aug 21, 2023

[LOW] [Splits] Web - Request Money Action Item changes to $0.00 when requesting money from an account that is not on Expensify. #23038

Closed

6 tasks

mountiny reviewed Aug 21, 2023

View reviewed changes

mountiny assigned ShogunFire Aug 22, 2023

Disable the request money options if one of the participant doesn't h…

b657c8c

…ave login because we would not be able to send payerEmail easily Signed-off-by: Pierre Michel <[email protected]>

ShogunFire force-pushed the fixIOURequestForUnknownUsers branch from 441b829 to b657c8c Compare August 22, 2023 13:34

mountiny closed this Aug 24, 2023

	function buildOptimisticIOUReport(payeeAccountID, payerAccountID, total, chatReportID, currency, isSendingMoney = false, unknownPayerEmail = '') {
	function buildOptimisticIOUReport(payeeAccountID, payerAccountID, total, chatReportID, currency, isSendingMoney = false, payerEmail = '') {

Fix the iou request for "unknown" users #24348

Fix the iou request for "unknown" users #24348

Conversation

ShogunFire commented Aug 10, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Aug 10, 2023

ShogunFire commented Aug 10, 2023

ShogunFire commented Aug 10, 2023 • edited Loading

melvin-bot bot commented Aug 15, 2023

parasharrajat commented Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

ShogunFire Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShogunFire Aug 18, 2023 • 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

mountiny commented Aug 21, 2023

mountiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShogunFire commented Aug 21, 2023 • edited Loading

mountiny commented Aug 21, 2023

ShogunFire commented Aug 22, 2023

ShogunFire commented Aug 22, 2023 • edited Loading

ShogunFire commented Aug 22, 2023

mountiny commented Aug 22, 2023

ShogunFire commented Aug 22, 2023

ShogunFire commented Aug 23, 2023

ShogunFire commented Aug 23, 2023

mountiny commented Aug 23, 2023

ShogunFire commented Aug 23, 2023

ShogunFire commented Aug 23, 2023

mountiny commented Aug 24, 2023

ShogunFire commented Aug 24, 2023

mountiny commented Aug 24, 2023

mountiny commented Aug 24, 2023

mountiny commented Aug 24, 2023

ShogunFire commented Aug 24, 2023

mountiny commented Aug 24, 2023

ShogunFire commented Aug 24, 2023

mountiny commented Aug 24, 2023

mountiny commented Aug 24, 2023

ShogunFire commented Aug 10, 2023 •

edited

Loading

ShogunFire commented Aug 10, 2023 •

edited

Loading

parasharrajat commented Aug 16, 2023 •

edited

Loading

ShogunFire Aug 16, 2023 •

edited

Loading

ShogunFire Aug 18, 2023 •

edited

Loading

ShogunFire commented Aug 21, 2023 •

edited

Loading

ShogunFire commented Aug 22, 2023 •

edited

Loading