[$500] Split - Contact method of the participants is shown in Merchant instead of display name

izarutskaya commented 11 months ago

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

Issue found when executing PR: https://github.com/Expensify/App/pull/29449

Version Number: v1.3.86-1 Reproducible in staging?: Y Reproducible in production?: Y 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 Expensify/Expensify Issue URL: Issue reported by: Applause-Internal Team Slack conversation: @

Action Performed:

Go to staging.new.expensify.com
Go to + > Request money > Manual.
Select Split with a few participants with display name.
Create the bill split.
Click on the bill split preview > Show more.

Expected Result:

Display name of the participants will be displayed in the Merchant.

Actual Result:

Contact method of the participants is displayed in the Merchant.

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

https://github.com/Expensify/App/assets/115492554/3cd10f75-d226-4752-9e43-0076d61c9712

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~018d28ed12ed77a99a
Upwork Job ID: 1714607183948734464
Last Price Increase: 2023-11-01

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

Job added to Upwork: https://www.upwork.com/jobs/~018d28ed12ed77a99a

melvin-bot[bot] commented 11 months ago

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[bot] commented 11 months ago

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

shubham1206agra commented 11 months ago

Proposal

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

Split - Contact method of the participants is shown in Merchant instead of display name

What is the root cause of that problem?

In

https://github.com/Expensify/App/blob/a24e6a09b86f703f8ccf2e8f4bb29a4aabd6c761/src/libs/actions/IOU.js#L910-L912

We are using participant.login instead of participant.text, which is displaying contact method.

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

Use participant.text instead of participant.text in the given line. And maybe somehow use current user display name instead of currentUserLogin.

What alternative solutions did you explore? (Optional)

ayazalavi commented 11 months ago

Proposal

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

When adding a money request manually and selecting participants, the report details show the participants' email addresses in the merchant field.

What is the root cause of that problem?

Root cause is that when adding transaction for split bill then following line of code uses user's login for generating Bill Split with string.

https://github.com/Expensify/App/blob/a24e6a09b86f703f8ccf2e8f4bb29a4aabd6c761/src/libs/actions/IOU.js#L910-L912

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

Use getDisplayName

so instead of currentUserLogin we need to use getDisplayName(currentUserLogin, currentUserPersonalDetails)

and instead of participant.login we need to use getDisplayName(participant.login, participant)

    const formattedParticipants = isOwnPolicyExpenseChat
        ? [currentUserLogin, ReportUtils.getReportName(splitChatReport)]
        : Localize.arrayToString([getDisplayName(currentUserLogin, currentUserPersonalDetails), ..._.map(participants, (participant) => getDisplayName(participant.login, participant) || '')]);

What alternative solutions did you explore? (Optional)

We also need to ensure that if a user updates their display name from their profile, the transaction gets updated with the new display name. To achieve this, we should generate the formatted string as mentioned above before displaying the data OR update transactions merchant field as soon as user updates his or her profile, other than storing it in Onyx as follows:

https://github.com/Expensify/App/blob/a24e6a09b86f703f8ccf2e8f4bb29a4aabd6c761/src/libs/actions/IOU.js#L914-L927

zanyrenney commented 11 months ago

Nope, this is not reproducible, I suspect it has been fixed elsewhere or is an older bug:

shubham1206agra commented 11 months ago

@zanyrenney This is still reproducible You have to complete the split request first Then we have to check in preview

lanitochka17 commented 11 months ago

Issue is still reproducible on build 1.3.88-3

https://github.com/Expensify/App/assets/78819774/db3397f3-8c18-4341-a44d-3c2b72857b22

zanyrenney commented 11 months ago

Thanks for the additional step @shubham1206agra

zanyrenney commented 11 months ago

@ntdiary please can you review the initial proposals we have received?

ntdiary commented 11 months ago

Hey, @zanyrenney, sorry for the delay, there are two other issues I need to handle. I estimate I'll have time to review this issue in 24 hours, so If it's urgent, please feel free to reassign another C+. 😂

ntdiary commented 11 months ago

under review

ntdiary commented 11 months ago

@zanyrenney, I feel the expected behavior in the OP may not be what we want. In my testing, I saw different behavior: when I reopen the split report and detail page, the Merchant field always shows Request, even if I enter demo when creating it. So this feature may still be unstable, it would be better to confirm the expected behavior with our internal engineers first.

https://github.com/Expensify/App/assets/8579651/0d7774c1-0228-4e81-b133-588ae8ea93c5

melvin-bot[bot] commented 11 months ago

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

zanyrenney commented 11 months ago

Thanks @ntdiary I will ask for a second opinion here!

zanyrenney commented 11 months ago

Checking here - https://expensify.slack.com/archives/C01GTK53T8Q/p1698691907199119

situchan commented 11 months ago

We had same discussion - https://github.com/Expensify/App/issues/25885#issuecomment-1699762925 Maybe related to https://github.com/Expensify/Expensify/issues/301281

DylanDylann commented 11 months ago

Proposal

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

There are 3 issues that I found related to this split bill:
1. Issue 1: The behavior that is mentioned by @ntdiary above:
  
  when I reopen the split report and detail page, the Merchant field always shows Request, even if I enter demo when creating it.
2. Issue 2: The displayed merchant data and the actual merchant data sent are different, that can make user confuse.
3. Issue 3: It is the issue that mentioned in this thread: Split - Contact method of the participants is shown in Merchant instead of display name.
  What is the root cause of that problem?
4. Issue 1:When calling API 'SplitBill', we are just creating the optimisticData.merchant, but does not pass this merchant data to API.

Issue 2: Suppose the 1st issue is fixed (just to reduce the complexity): When going to MoneyRequestConfirmPage, we are always displaying the merchant field as "Request", but then the user clicks on the submit button, the actual data is passed to API 'SplitBill' is : https://github.com/Expensify/App/blob/5b5d465f9d39e01468db731ad5d278b026a8a565/src/libs/actions/IOU.js#L927
Issue 3: We are using the below logic to generate merchant with string, that uses participant.login: https://github.com/Expensify/App/blob/5b5d465f9d39e01468db731ad5d278b026a8a565/src/libs/actions/IOU.js#L915-L917

What changes do you think we should make in order to solve the problem?
Issue 1: We should pass the merchant data to 'SplitBill' API, this one is straightforward to implement.
Issue 2: When clicking on "Add to split" button to navigate to MoneyRequestConfirmPage, we need to update the iou data by updating the below logic: https://github.com/Expensify/App/blob/5b5d465f9d39e01468db731ad5d278b026a8a565/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L77-L80 to
```
const navigateToConfirmationStep = (moneyRequestType) => {
    IOU.setMoneyRequestMerchant(getSplitBillMerchant(props.iou.participants))
    ...
};
```
- getSplitBillMerchant is util function that we use to get the merchant and based on what we did to get the LHNRow and Report`s header in case of group chat. It can be something like:
```
const getSplitBillMerchant = (participants, currentUserLogin, isOwnPolicyExpenseChat) => {
if (isOwnPolicyExpenseChat) return [currentUserLogin, ReportUtils.getReportName(splitChatReport)];
const participantPersonalDetails = OptionsListUtils.getPersonalDetailsForAccountIDs(participants, allPersonalDetails);
const displayNamesWithTooltips = getDisplayNamesWithTooltips(participantPersonalDetails, true);
return getDisplayNamesStringFromTooltips(displayNamesWithTooltips);
};
```
- And also we need to update: https://github.com/Expensify/App/blob/5b5d465f9d39e01468db731ad5d278b026a8a565/src/libs/actions/IOU.js#L927 to merchant with merchant is from iou rather than using the default merchant data.
Issue 3: Using the above getSplitBillMerchant fixed this issue as well

What alternative solutions did you explore? (Optional)

NA

aimane-chnaif commented 11 months ago

Dupe of https://github.com/Expensify/App/issues/29691

shubham1206agra commented 11 months ago

@aimane-chnaif I just checked This is not a dupe

DylanDylann commented 11 months ago

@aimane-chnaif My proposal mention 3 related issue https://github.com/Expensify/App/issues/29858#issuecomment-1786463262 So should we need to fix each issue in separate PR or wrap these into one?

melvin-bot[bot] commented 11 months ago

@ntdiary @zanyrenney this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 11 months ago

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

zanyrenney commented 11 months ago

Hmm, I think this is a dupe of https://github.com/Expensify/App/issues/29691

I see the comment above, but closing on these grounds.

DylanDylann commented 11 months ago

@zanyrenney it's not a dupe. Please help check my proposal for more detail.

The https://github.com/Expensify/App/issues/29691 is: When calling API 'SplitBill', we are just creating the optimisticData.merchant, but does not pass this merchant data to API.
The https://github.com/Expensify/App/issues/29858 is: Split - Contact method of the participants is shown in Merchant instead of display name

@ntdiary Can you help confirm.

situchan commented 11 months ago

@DylanDylann please check https://github.com/Expensify/App/issues/29858#issuecomment-1785886524. This will be handled internally. cc: @youssef-lr

DylanDylann commented 11 months ago

@situchan My proposal mentioned 3 related issues. What issue that you mentioned in "This will be handled internally"?

situchan commented 11 months ago

@youssef-lr will decide as he's leading split bill project. Please hold on and wait for update.

DylanDylann commented 11 months ago

@situchan Yeah. Many thanks. But I have a minor question, In the link you mentioned, I just can access the https://github.com/Expensify/App/issues/25885, and the issue mentioned in that does not appear anymore and does not relate to this issue.

youssef-lr commented 11 months ago

@DylanDylann this is getting fixed here https://github.com/Expensify/App/pull/30721

ntdiary commented 11 months ago

Expected Result:

Display name of the participants will be displayed in the Merchant.

I feel the expected behavior in the OP may not be what we want.

As I mentioned before, this is not so much a bug but rather a feature request, and likely one that would not get approved. 😂 So it's fine to just fix the merchant field saving problem.

ayazalavi commented 11 months ago

@ntdiary My proposal also mentioned an issue that if user updates his account details then merchant field does not gets updated with his new email address or display name what ever we need to show there.

Expensify / App