[HOLD #301281] [$1000] Web - Missing Default Merchant Statement in Split Bill Details

izarutskaya commented 1 year 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!

Action Performed:

Click on the plus sign and select "Split Bill."
Enter the desired numbers and add an email address.
Click on "See More" and observe the not-updated default statement
Open the split bill details and click on "See More" again.
Notice that the default merchant statement is not present.

Expected Result:

The default merchant statement should be present both in the split bill details and when splitting the bill.

Actual Result:

The default merchant statement is present when splitting the bill but is not displayed in the split bill details.

Workaround:

Unknown

Platforms:

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

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: v1.3.57-2

Reproducible in staging?: Y

Reproducible in production?: N

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

https://github.com/Expensify/App/assets/115492554/891a6b5a-ef2b-425d-b00f-043bccff7509

https://github.com/Expensify/App/assets/115492554/ed37e909-73a4-440a-b849-ac03b0e73993

Expensify/Expensify Issue URL:

Issue reported by: @TewodrosGirmaA

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692855560586859

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015da00ce15854b03b
Upwork Job ID: 1694858757030412288
Last Price Increase: 2023-09-13

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year 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

OSBotify commented 1 year ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

melvin-bot[bot] commented 1 year ago

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

alphaboss1104 commented 1 year ago

Proposal

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

Missing Default Merchant Statement in Split Bill Details

What is the root cause of that problem?

We displayed the merchant statement of split bill details here:

https://github.com/Expensify/App/blob/b38181a2d567cc3cbb5b426f0585db4286747fa6/src/components/MoneyRequestConfirmationList.js#L399-L407

But we didn't passed the props.iouMerchant from the parent.

https://github.com/Expensify/App/blob/b38181a2d567cc3cbb5b426f0585db4286747fa6/src/pages/iou/SplitBillDetailsPage.js#L85-L97

This is the root cause of this issue.

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

First of all we should get the merchant statement of split bill details here:

const {amount: splitAmount, currency: splitCurrency, comment: splitComment, merchant: splitMerchant} = ReportUtils.getTransactionDetails(transaction);

and then We should add iouMerchant as a prop to MoneyRequestConfirmationList component here:

                  <MoneyRequestConfirmationList
                      hasMultipleParticipants
                      payeePersonalDetails={payeePersonalDetails}
                      selectedParticipants={participantsExcludingPayee}
                      iouAmount={splitAmount}
                      iouComment={splitComment}
+                     iouMerchant={splitMerchant}
                      iouCurrencyCode={splitCurrency}
                      iouType={CONST.IOU.MONEY_REQUEST_TYPE.SPLIT}
                      isReadOnly
                      shouldShowFooter={false}
                  />

What alternative solutions did you explore? (Optional)

pecanoro commented 1 year ago

Trying to figure out from which PR is coming

pecanoro commented 1 year ago

@mountiny @luacmartins This deploy blocker is coming from this PR https://github.com/Expensify/App/pull/25794

luacmartins commented 1 year ago

I don't think this is a blocker. It's a really small issue and we're still making several changes to this flow.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~015da00ce15854b03b

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@pecanoro, @muttmuure, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

pecanoro commented 1 year ago

@situchan Could you review the proposals when you have a chance? Thank you!

pecanoro commented 1 year ago

@situchan Another friendly bump!

situchan commented 1 year ago

reviewing in a few hrs

situchan commented 1 year ago

@alphaboss1104's proposal looks good. However, 2 questions before move forward:

merchant is inconsistent between 2 pages. Is it expected?
if merchant is long, fully display in multiline or current behavior is fine which is single line?

cc: @luacmartins @pecanoro

alphaboss1104 commented 1 year ago

Question 1: It seems to me that it is expected behaviour for merchant to be inconsistent between the two pages. And Should we display the email or displayName if the user has the displayName? Question 2: In my opinion, if the merchant is long, display the merchant in single line is better.

pecanoro commented 1 year ago

merchant is inconsistent between 2 pages. Is it expected?

What do you mean by this? Different types of requests?

if merchant is long, fully display in multiline or current behavior is fine which is single line?

Yeah, one line is fine

situchan commented 1 year ago

What do you mean by this? Different types of requests?

Before split: Request After split: Bill split with xxx, xxx

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@pecanoro, @muttmuure, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

pecanoro commented 1 year ago

Before split: Request After split: Bill split with xxx, xxx

@mountiny @luacmartins Is this expected? I know we are still refactoring some of this stuff so I am not sure if it's expected or not.

mountiny commented 1 year ago

I would probably wait with the split bill after the conference, I dont think this was properly scoped in terms of how the merchant and fields should behave for split so this should be brought to slack first

pecanoro commented 1 year ago

Gotcha! Going to put a HOLD then and move it to weekly

pecanoro commented 1 year ago

Bringing the discussion to Slack

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@pecanoro @muttmuure @situchan this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

pecanoro commented 1 year ago

HOLDING on this: https://github.com/Expensify/Expensify/issues/301281

pecanoro commented 1 year ago

Still HOLDING

pecanoro commented 11 months ago

Still HOLDING

Expensify / App