[$1000] Web - Request Money: Hmm... it's not here when requesting money after updating a user's profile

[kbecciv]

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

---

Action Performed:

1. Open the website in two windows
2. create two new users
3. Login both users
4. click the + icon from LHN for user A
5. create a new chat from user A to user B
6. send a message from user A to user B
7. reply to a message from user B to user A
8. update a profile for both users
9. click the + icon from LHN
10. click money request
11. enter a value
12. select or enter user B's email
13. confirm the requested money
14. Observe the result

Expected Result:

A money request should be sent to user B

Actual Result:

Hmm... it's not here page appears

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.57-6
Reproducible in staging?: n/a
Reproducible in production?: n/a
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screenshare.-.2023-08-28.8_03_08.PM.mp4

Expensify/Expensify Issue URL:
Issue reported by: @misgana96
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692440705016829

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0199bcac5d2d2e5bbb
• Upwork Job ID: 1696546344484073472
• Last Price Increase: 2023-12-14

[melvin-bot]

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0199bcac5d2d2e5bbb

[melvin-bot]

Current assignee @lschurr is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @burczu (External)

[lschurr]

Hi @burczu - could you try reproducing this bug?

[binary3oul]

This issue is seen by only user A.
I see login property is missing.

[dantastisk]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The problem is that you can not request money from a user by using the FAB, if you have first created a chat with the user. It has nothing to do with changing settings and can be reproduced by just these simple steps:

1. Click the FAB, click "New chat" and type the email of a user that you have no chat history with.
2. Click the FAB again, click "Request money" and type the same email as in step 1
3. You will see a "Hmm... it's not here - You don't have access to this chat"-page

What is the root cause of that problem?

The problem is caused by a combination of multiple factors. What happens is that the FE is not able to find the existing chat when creating the IOU, assumes that no chat exists between the 2 users and tries to create a new one. The BE is aware of the existing chat and returns the following error:

{
...,
jsonCode: 666,
message: "There is a previously existing chat between these users.",
...

The way the FE checks if there is an existing chat/report when "Request money" is used from the FAB, is by comparing the account ID of the participant selected with the participants of all of the users existing chats/reports.

This happens here:

App/src/libs/ReportUtils.js

Lines 2820 to 2839 in c20c387

	function getChatByParticipants(newParticipantList) {
	newParticipantList.sort();
	return _.find(allReports, (report) => {
	// If the report has been deleted, or there are no participants (like an empty #admins room) then skip it
	if (
	!report ||
	_.isEmpty(report.participantAccountIDs) ||
	isChatThread(report) ||
	isTaskReport(report) ||
	isMoneyRequestReport(report) ||
	isChatRoom(report) ||
	isPolicyExpenseChat(report)
	) {
	return false;
	}
	// Only return the chat if it has all the participants
	return _.isEqual(newParticipantList, _.sortBy(report.participantAccountIDs));
	});
	}

So why is the FE not able the existing chat/report?

To explain this, let me first summarize what happens when a new chat is created from the FAB:

1. If the recipient participant is unknown, an optimistic personal detail with a temporary, locally generated accountID is created and stored in Onyx. This happens here:

App/src/libs/actions/Report.js

Lines 491 to 521 in c20c387

	// Add optimistic personal details for new participants
	const optimisticPersonalDetails = {};
	const settledPersonalDetails = {};
	_.map(participantLoginList, (login, index) => {
	const accountID = newReportObject.participantAccountIDs[index];
	optimisticPersonalDetails[accountID] = allPersonalDetails[accountID] || {
	login,
	accountID,
	avatar: UserUtils.getDefaultAvatarURL(accountID),
	displayName: login,
	isOptimisticPersonalDetail: true,
	};
	settledPersonalDetails[accountID] = allPersonalDetails[accountID] || null;
	});
	onyxData.optimisticData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.PERSONAL_DETAILS_LIST,
	value: optimisticPersonalDetails,
	});
	onyxData.successData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.PERSONAL_DETAILS_LIST,
	value: settledPersonalDetails,
	});
	onyxData.failureData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.PERSONAL_DETAILS_LIST,
	value: settledPersonalDetails,
	});

Notice how this also pushes an object like the following to onyxData.successData:

{onyxMethod: 'merge',
key: 'personalDetailsList',
value: {
  [ACCOUNT_ID]: null
}}

The purpose of this is to make sure that the optimistic personal detail is cleared from Onyx once the server responds with the actual personal detail with the correct accountID. However, the above code does not work. It is not possible to clear an object from Onyx by merging it with null. You can easily test this for yourself by opening the console of your browser and calling Onyx.merge with an object like the above wrapped in an array and [ACCOUNT_ID] substituted for one of the accountID's found under the personalDetailList of your Onyx store.

So, the first step to fixing the issue is to fix this code to properly clear the optimistic personal detail.

This doesn't fully solve the problem, however, as there is more to it.

The personal detail returned from the BE and stored in Onyx when a new chat is created with a previously unknown participant looks like this:

"15542922": {
  "accountID": 15542922,
  "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/default-avatar_15.png",
  "displayName": "daniel.s.brinkmann+change4@gmail.com",
  "firstName": "",
  "lastName": "",
  "status": null
}

If you compare this to the optimistic personal detail from Report.js, you will notice that it has a few extra properties, but more importantly that the login-property is missing.

This is important because the code responsible for displaying the possible recipients when requesting money from the FAB includes these lines:

App/src/libs/OptionsListUtils.js

Lines 715 to 719 in c20c387

	// We're only picking personal details that have logins set
	// This is a temporary fix for all the logic that's been breaking because of the new privacy changes
	// See https://github.com/Expensify/Expensify/issues/293465 for more context
	// Moreover, we should not override the personalDetails object, otherwise the createOption util won't work properly, it returns incorrect tooltipText
	const havingLoginPersonalDetails = !includeP2P ? {} : _.pick(personalDetails, (detail) => Boolean(detail.login));

What changes do you think we should make in order to solve the problem?

To solve the problem, we will have to:

1. Fix the faulty onyxData.successData-code by either merging with an object that sets all the required keys to null, or (probably the better solution) uses Onyx.METHOD.SET to properly clean up.
2. Update the BE so that the login property gets sets on the returned personal detail

I have tested this by manually simulating the BE change and it perfectly resolves the issue.

[burczu]

Hey @Ischurr! Sorry for the delay - I've missed this one. I'll take a look today!

[binary3oul]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When we attempt to request money using the "New chat" feature, we encounter the message "Hmm... it's not here when requesting money."
In other words, we are unable to request money through the "New chat" .

To begin with:

1. Open the website in two windows
2. create two new users
3. Login both users
4. click the + icon from LHN for user A
5. create a new chat from user A to user B
6. send a message from user A to user B
7. reply to a message from user B to user A
8. update a profile for both users
9. click the + icon from LHN
10. click money request
11. enter a value
12. select or enter user B's email
13. confirm the requested money
    Observe the result.

What is the root cause of that problem?

Final cause for this problem is debtorEmail property is missing (assigned null exactly) in the payload towards API when we request through the New Chat

It brings the following response.

Therefore, when user confirms requested money, 403 error encountered and shows Hmm... It's not here error.

Let me explain why.

To begin with:

1. Press F12 and click Application tab, then select IndexedDB\OnyxDB\keyvaluepairs.
2. Please take a look at the row with the key personalDetailsList.
   (especially, please pay attention to the objects with the keys "15544627" and "189181028" )

To perform this operation successfully, we need login property that is associated with this operation.
Therefore, we have to update the merge operation of Onyx with correct data so that enables to insert login propery into personalDetailsList property.

This login porperty is none other than debtorEmail I mentioned above.

What changes do you think we should make in order to solve the problem?

We have to update these lines as following:

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

[binary3oul]

@burczu Please check my proposal.
I was able to resolve this issue quite easily by making a simple update to just one line of code.

[dantastisk]

#26114 (comment) This does not fix the issue. The code you linked is not even executed when requesting money by clicking the FAB, only when doing it from within a report.

Like mentioned in my proposal, the cause of the issue is Onyx having multiple personal details stored for the same participant because the optimistic data is not cleared properly upon retrieving the correct data from the BE
AND
the BE not setting the login property

I wrote in my original proposal that a possible solution could be to remove the filter that requires the login property to be set. However, after further testing, I can now conclude that this is not the way to go, as it will leave "ghost" contacts in the suggestions like this:

I will update my proposal to reflect this.

I also forgot to mention in my explanation that a new optimistic personal detail is created here when no existing is found (due to the missing login property). This will have the same, temporary and wrong accountID as the one created when opening the chat report which is why it cannot find the existing chat report to create the IOU within.

So to summarize, the solution is to:

1. Properly clear the optimistic personal detail
2. Have the BE set the login property of the personal detail created when opening a report with a new/unknown participant

If you want to confirm my solution you can:

1. Change this line

   App/src/libs/actions/Report.js

   Line 504 in 54e6904

   settledPersonalDetails[accountID] = allPersonalDetails[accountID] || null;

   
   to

settledPersonalDetails[accountID] = allPersonalDetails[accountID] || {
  login: null,
  accountID: null,
  avatar: null,
  displayName: null,
   isOptimisticPersonalDetail: null
};

(for the actual PR this can be done cleaner, this is just for the POC)

2. Create a chat report from the FAB
3. Simulate the BE setting the login property by executing this in your browser console:

Onyx.update([{
  onyxMethod: 'merge',
  key: 'personalDetailsList',
  value: {
    12345678: {
      login: "example@mail.com"
     }
  }
}])

(replace "example@mail.com" with the correct email and 12345678 with the account ID returned by the BE in step 2. This can be extracted from the response of the OpenReport request in the network tab or by finding the correct object in Onyx, under the "personalDetailsList"-key).
4. Click "Request money" from the FAB, select the user and it should work.

@burczu This can be a confusing issue to wrap your head around, so lmk if you have any questions or need further explanation :-)

[binary3oul]

Thank you @dantastisk

In that case, we can see the following message from API.

I believe that the backend is functioning properly, so now we need to proceed with the redirect operation to the chat channel.
I have outlined the details of this operation in my proposal.

[misgana96]

@binary3oul @dantastisk This report is not about requesting money from an unknown user or having no chat history. If you check the video these two users had a chat history

[burczu]

Thanks for all the proposals - I'll be reviewing them soon.

[dantastisk]

@misgana96 I did some further testing and you are correct. The solution is still the same, however, as the problem is still, that the BE does not set the login property of the personal detail when responding to the OpenReport-request.

Because of this the correct personal detail gets filtered out here:

App/src/libs/OptionsListUtils.js

Lines 715 to 719 in c20c387

	// We're only picking personal details that have logins set
	// This is a temporary fix for all the logic that's been breaking because of the new privacy changes
	// See https://github.com/Expensify/Expensify/issues/293465 for more context
	// Moreover, we should not override the personalDetails object, otherwise the createOption util won't work properly, it returns incorrect tooltipText
	const havingLoginPersonalDetails = !includeP2P ? {} : _.pick(personalDetails, (detail) => Boolean(detail.login));

[burczu]

@lschurr - I've just tested the proposal from @dantastisk and it seem to work. Unfortunately it requires some backend changes so I think we should switch to Internal for now to get an opinion about it from backend engineer.

I've also tested the solution from @binary3oul but it doesn't fix the issue.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[suneox]

This issue still happens on the latest staging

I have found the root cause and it can be resolved on the FE side, I'd like to test on other cases and provide my proposal end of the day

[burczu]

@lschurr As I wrote in this comment: #26114 (comment) - I think we should wait for @dantastisk to fix the FE part of the issue, as they provided the solution that we originally followed (please see this comment: #26114 (comment)). Maybe we should just assign them?

[dantastisk]

Sorry, I have been mia, have been ooo for a couple weeks.

I will take a look this week.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[burczu]

@dantastisk Any update here?

[dantastisk]

Sorry for the delay. I just checked on staging and it seems like BE is still not sending a login property.

[lschurr]

@amyevans are you able to help us with this one?

[burczu]

Yeah, @amyevans, could you confirm? After your comment: #26114 (comment) you have removed the HOLD mark from the title so I think @lschurr and I assumed that the BE side is done, but it seems it's not yet?

[amyevans]

Sorry this wasn't clearer! I haven't done any backend work for this yet, it doesn't feel like it should be prioritized over wave6 work.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[lschurr]

Should this one still be on hold @amyevans?

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[amyevans]

Should this one still be on hold @amyevans?

Probably not HOLD since we don't have any other tracking issue for it.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[lschurr]

Based on Matt's update, should we be closing this one @amyevans? https://expensify.slack.com/archives/C01SKUP7QR0/p1702341728013009

[amyevans]

Yeah that makes sense to me, thanks @lschurr!

[dantastisk]

I am really curious as to what was in that update. Can you share?

[lschurr]

The update was crossposted here as well - https://expensify.slack.com/archives/C01GTK53T8Q/p1702496349813859

[dantastisk]

Thanks! Does that mean that I get no compensation? Because that would be quite disappointing I must say.

#26114 (comment)

[lschurr]

Yes, unfortunately since we won't be fixing this bug right now, we won't be issuing compensation. It's possible that this will get reopened in the future, at which point you could be hired for the job and compensated once a fix is put in place. Thanks @dantastisk!

[misgana96]

@lschurr What about the reporter bonus? Am I eligiable for a compensation?