Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
6 tasks done
izarutskaya opened this issue Oct 18, 2023 · 33 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@izarutskaya
Copy link

izarutskaya commented Oct 18, 2023

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: #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:

  1. Go to staging.new.expensify.com
  2. Go to + > Request money > Manual.
  3. Select Split with a few participants with display name.
  4. Create the bill split.
  5. 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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Bug6241512_1697620965636.20231018_111025.mp4
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
@izarutskaya izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

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

@melvin-bot melvin-bot bot changed the title Split - Contact method of the participants is shown in Merchant instead of display name [$500] Split - Contact method of the participants is shown in Merchant instead of display name Oct 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

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 melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

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

@shubham1206agra
Copy link
Contributor

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

App/src/libs/actions/IOU.js

Lines 910 to 912 in a24e6a0

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

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
Copy link
Contributor

ayazalavi commented Oct 18, 2023

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.

App/src/libs/actions/IOU.js

Lines 910 to 912 in a24e6a0

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

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:

App/src/libs/actions/IOU.js

Lines 914 to 927 in a24e6a0

const splitTransaction = TransactionUtils.buildOptimisticTransaction(
amount,
currency,
CONST.REPORT.SPLIT_REPORTID,
comment,
'',
'',
'',
`${Localize.translateLocal('iou.splitBill')} ${Localize.translateLocal('common.with')} ${formattedParticipants} [${DateUtils.getDBTime().slice(0, 10)}]`,
undefined,
undefined,
undefined,
category,
);

@zanyrenney
Copy link
Contributor

Nope, this is not reproducible, I suspect it has been fixed elsewhere or is an older bug:
2023-10-20_11-30-03

@shubham1206agra
Copy link
Contributor

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

Screenshot 2023-10-20 at 6 08 28 PM

@lanitochka17
Copy link

Issue is still reproducible on build 1.3.88-3

Screen_Recording_20231021_235952_Chrome.mp4

@lanitochka17 lanitochka17 reopened this Oct 21, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 24, 2023
@zanyrenney
Copy link
Contributor

Thanks for the additional step @shubham1206agra

@melvin-bot melvin-bot bot removed the Overdue label Oct 24, 2023
@zanyrenney
Copy link
Contributor

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

@ntdiary
Copy link
Contributor

ntdiary commented Oct 24, 2023

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
Copy link
Contributor

ntdiary commented Oct 25, 2023

under review

@ntdiary
Copy link
Contributor

ntdiary commented Oct 25, 2023

@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.

demo.mp4

@melvin-bot
Copy link

melvin-bot bot commented Oct 25, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Oct 27, 2023
@zanyrenney
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 27, 2023
@zanyrenney
Copy link
Contributor

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2023
@situchan
Copy link
Contributor

We had same discussion - #25885 (comment)
Maybe related to https://github.com/Expensify/Expensify/issues/301281

@DylanDylann
Copy link
Contributor

DylanDylann commented Oct 31, 2023

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.

  1. Issue 2: The displayed merchant data and the actual merchant data sent are different, that can make user confuse.
  2. 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?

  1. Issue 1:When calling API 'SplitBill', we are just creating the optimisticData.merchant, but does not pass this merchant data to API.

  2. 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 :

    `${Localize.translateLocal('iou.splitBill')} ${Localize.translateLocal('common.with')} ${formattedParticipants} [${DateUtils.getDBTime().slice(0, 10)}]`,

  3. Issue 3: We are using the below logic to generate merchant with string, that uses participant.login:

    App/src/libs/actions/IOU.js

    Lines 915 to 917 in 5b5d465

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

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

  1. Issue 1: We should pass the merchant data to 'SplitBill' API, this one is straightforward to implement.
  2. 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:
    const navigateToConfirmationStep = (moneyRequestType) => {
    IOU.setMoneyRequestId(moneyRequestType);
    Navigation.navigate(ROUTES.MONEY_REQUEST_CONFIRMATION.getRoute(moneyRequestType, reportID));
    };

    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:
    `${Localize.translateLocal('iou.splitBill')} ${Localize.translateLocal('common.with')} ${formattedParticipants} [${DateUtils.getDBTime().slice(0, 10)}]`,

    to
    merchant with merchant is from iou rather than using the default merchant data.
  1. Issue 3: Using the above getSplitBillMerchant fixed this issue as well

What alternative solutions did you explore? (Optional)

  • NA

@aimane-chnaif
Copy link
Contributor

Dupe of #29691

@shubham1206agra
Copy link
Contributor

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

@DylanDylann
Copy link
Contributor

@aimane-chnaif My proposal mention 3 related issue #29858 (comment) So should we need to fix each issue in separate PR or wrap these into one?

Copy link

melvin-bot bot commented Nov 1, 2023

@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!

Copy link

melvin-bot bot commented Nov 1, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Nov 2, 2023
@zanyrenney
Copy link
Contributor

Hmm, I think this is a dupe of #29691

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 2, 2023
@DylanDylann
Copy link
Contributor

DylanDylann commented Nov 2, 2023

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

@ntdiary Can you help confirm.

@situchan
Copy link
Contributor

situchan commented Nov 2, 2023

@DylanDylann please check #29858 (comment).
This will be handled internally.
cc: @youssef-lr

@DylanDylann
Copy link
Contributor

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

@situchan
Copy link
Contributor

situchan commented Nov 2, 2023

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

@DylanDylann
Copy link
Contributor

DylanDylann commented Nov 2, 2023

@situchan Yeah. Many thanks. But I have a minor question, In the link you mentioned, I just can access the #25885, and the issue mentioned in that does not appear anymore and does not relate to this issue.

@youssef-lr
Copy link
Contributor

@DylanDylann this is getting fixed here #30721

@ntdiary
Copy link
Contributor

ntdiary commented Nov 3, 2023

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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

10 participants