Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

[HOLD for payment 2023-10-23][$1000] IOU - When performing an invoice division in a group, the amount of this is set to zero #26020

Closed lanitochka17 closed 11 months ago

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

  1. First login to different accounts which two different users, one as user A and the other as user B
  2. Create a group that includes user A and user B.
  3. [user A] Start chat in the group.
  4. [user B] Enter in the group
  5. [user A] Click plus sign and select Split bill.
  6. [user A] Write a number, click Next button and Split button

Expected Result:

Group members should see the split bill amount

Actual Result:

Split invoice amount is set to zero

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.57-5

Reproducible in staging?: Yes

Reproducible in production?: Yes

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/78819774/44fb2977-3da5-4490-bc6b-6db38461a5b3

https://github.com/Expensify/App/assets/78819774/015e82ed-86d1-4b0c-a2e4-dd668d3a64b2

Expensify/Expensify Issue URL:

Issue reported by: @Mauri-salazar)

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018591b88c96d0a9bb
  • Upwork Job ID: 1696254007472058368
  • Last Price Increase: 2023-09-18
  • Automatic offers:
    • aimane-chnaif | Reviewer | 27183589
    • paultsimura | Contributor | 27183592
    • Mauri-salazar | Reporter | 27183593
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @lschurr (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)

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~018591b88c96d0a9bb

melvin-bot[bot] commented 1 year ago

Current assignee @lschurr 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 - @aimane-chnaif (External)

lschurr commented 1 year ago

Waiting for proposals.

paultsimura commented 1 year ago

Proposal

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

While sending a "Split bill" request in a group, other participants see the MoneyRequestPreview with $0

What is the root cause of that problem?

First of all, I'd like to mention that on the current main branch, the $0 is not visible anymore. However, the issue is still there, just the $0 issue is replaced with the skeleton view being is stuck forever (until the next Report.openReport) is triggered.

When the Report is rendered on the User B side, the transaction is not yet processed, so it's not present in the BE response, and therefore – in Onyx.

This leads to the skeleton view being displayed without any report-related actions.

https://github.com/Expensify/App/blob/1bad06126b80fd2b436e870320ed04cc5b34e08a/src/components/ReportActionItem/MoneyRequestPreview.js#L240-L242

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

We should add the following hook inside the MoneyRequestPreview component:

    const isTransactionPresent = !_.isEmpty(props.transaction);
    useEffect(() => {
        if (isTransactionPresent) {
            return;
        }

        Report.openReport(props.chatReportID);
    }, [isTransactionPresent, props.chatReportID]);

This will cause report re-fetch if a transaction is missing for the IOU Item. The dependencies of the hook will ensure it's triggered only once.

Such approach will lead to the skeleton view being visible for a few seconds only with the report being re-fetched immediately.

Result:

https://github.com/Expensify/App/assets/12595293/a6f4562d-3dac-427a-99d6-37e182fd3f58

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 year ago

📣 @paultsimura! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
paultsimura commented 1 year ago
Contributor details
Your Expensify account email: paultsimura@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01998fcb3ca1e361ed
melvin-bot[bot] commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

lschurr commented 1 year ago

Could you review the proposals here @aimane-chnaif?

paultsimura commented 1 year ago

@lschurr even though the idea of getting paid for resolving this issue seems appealing, I think it will be mostly handled now in https://github.com/Expensify/App/issues/26270

However, the skeleton will still be visible for about a minute (until the next Ping request comes), as the inexistent transaction issue will still be in place. Taking that into consideration, I would modify my proposal by only keeping the part about calling Report.openReport() if we receive such a request with a not-yet-existing transaction ID.

JmillsExpensify commented 1 year ago

I think I'm missing something. Why does this matter when we have skeleton UI?

napster125 commented 1 year ago

https://github.com/Expensify/App/issues/26518#issuecomment-1703287836

Perhaps, the team don't want to add skeleton UI.

paultsimura commented 1 year ago

Huh, I was unaware of that. In this case I'm keeping my initial proposal🙂

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? 💸

lschurr commented 1 year ago

@aimane-chnaif could you please review?

lschurr commented 1 year ago

Bump @aimane-chnaif

aimane-chnaif commented 1 year ago

I don't see any solid proposal yet.

paultsimura commented 1 year ago

Proposal

Updated proposal https://github.com/Expensify/App/issues/26020#issuecomment-1698344130

paultsimura commented 1 year ago

@lschurr @aimane-chnaif I've updated my initial proposal. However, if you think that "skeleton view is stuck" is a different issue now, I have reported it here: https://expensify.slack.com/archives/C049HHMV9SM/p1694182124123539

lschurr commented 1 year ago

Could you take another look @aimane-chnaif?

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? 💸

lschurr commented 1 year ago

Bump @aimane-chnaif

melvin-bot[bot] commented 1 year ago

@lschurr @aimane-chnaif 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!

lschurr commented 1 year ago

Bumped in Slack as well - https://expensify.slack.com/archives/C01GTK53T8Q/p1694947661478389

Pluto0104 commented 1 year ago

Proposal

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

When users split a bill in a group, the amount of the split is set to zero.

What is the root cause of that problem?

The root cause of this problem can be traced to specific lines of code: https://github.com/Expensify/App/blob/bb432aa6d787c6ba95faed521286cd169cbcade6/src/components/ReportActionItem/MoneyRequestPreview.js#L151 https://github.com/Expensify/App/blob/bb432aa6d787c6ba95faed521286cd169cbcade6/src/components/ReportActionItem/MoneyRequestPreview.js#L344-L346 In these code snippets, action.originalMessage.IOUTransactionID contains the correct value, but the transaction data isn't present in the OnyxDB. This is due to the fact that only the report and reportAction are being updated, while the transaction is not. As a result, when props.transaction is accessed, it returns an empty object ({ }), leading to the displayed transaction amount being zero.

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

To resolve this issue, I suggest the following approach: https://github.com/Expensify/App/blob/bb432aa6d787c6ba95faed521286cd169cbcade6/src/pages/home/ReportScreen.js#L279-L283 https://github.com/Expensify/App/blob/6e17b64be0e571773bed42446346045c659640d6/src/pages/home/ReportScreen.js#L208-L224

In this code snippet, the changes involve:

if (onyxReportID === prevReport.reportID && (!onyxReportID || onyxReportID === routeReportID) && !ReportUtils.isUnread(report)) {
  return;
}

fetchReportIfNeeded(true);

The function ReportUtils.isUnread determines whether a report has been read or remains unread.

The function fetchReportIfNeeded should also be updated as follows:

const fetchReportIfNeeded = useCallback((shouldFetch = false) => {
    const reportIDFromPath = getReportID(route);
    if (!reportIDFromPath) {
        return;
    }
    if (!shouldFetch && report.reportID && report.reportID === getReportID(route)) {
        return;
    }
    Report.openReport(reportIDFromPath);
}, [report.reportID, route]);

In this updated function, the shouldFetch parameter enables a forced fetch without meeting the usual conditions.

Additionally, we should modify the code as follows:

https://github.com/Expensify/App/blob/bb432aa6d787c6ba95faed521286cd169cbcade6/src/components/ReportActionItem/MoneyRequestPreview.js#L151

const {
        amount: requestAmount,
        currency: requestCurrency,
        comment: requestComment,
        merchant: requestMerchant,
    } = _.isEmpty(props.transaction) ? lodashGet(props.action, 'originalMessage', {}) : ReportUtils.getTransactionDetails(props.transaction);

This modification temporarily uses props.action.originalMessage when props.transaction is empty, providing a workaround to populate the transaction data. get_temp_transaction_data

Lastly, in the section of code related to handling the report action item press, adjustments are necessary to ensure that the component is unavailable to press when props.transaction is empty: https://github.com/Expensify/App/blob/bb432aa6d787c6ba95faed521286cd169cbcade6/src/components/ReportActionItem/MoneyRequestPreview.js#L307-L309

if (!props.onPreviewPressed || _.isEmpty(props.transaction)) {
  return childContainer;
}

Result

https://github.com/Expensify/App/assets/136437976/6ff6c830-aac7-4e20-97a3-82a05850f0a2

What alternative solutions did you explore? (Optional)

N/A

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? 💸

aimane-chnaif commented 1 year ago

updating today

melvin-bot[bot] commented 1 year ago

@lschurr @aimane-chnaif this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.

lschurr commented 1 year ago

Any update @aimane-chnaif?

lschurr commented 1 year ago

Bumped in Slack - https://expensify.slack.com/archives/C01GTK53T8Q/p1695790942443119

aimane-chnaif commented 1 year ago

There's another bug related to split bill. It says owes $0.00 in LHN last message. It might be merged into this issue.

@paultsimura @Pluto0104 can you please find the root cause?

https://github.com/Expensify/App/assets/96077027/a7f73221-9336-449b-9173-6216d54888c7

paultsimura commented 1 year ago

@aimane-chnaif the root cause is different, it's a regression from https://github.com/Expensify/App/pull/25629 But in order to propose a solution I need to understand, what is the expected text to be displayed for such scenario?

aimane-chnaif commented 1 year ago

@aimane-chnaif the root cause is different, it's a regression from #25629 But in order to propose a solution I need to understand, what is the expected text to be displayed for such scenario?

split xxx for yyy. It's from api response

paultsimura commented 1 year ago

Proposal

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

After a split bill request is created in a group, the group's text in LHN shows "owes $0.00"

What is the root cause of that problem?

The root cause lies here:

https://github.com/Expensify/App/blob/0c49b1198357f33f443543d6873badafd009b093/src/libs/OptionsListUtils.js#L392-L393

We are checking if the last report action is a money request action. However, we do not check if the report itself is an IOU report.

The group chat is not an IOU report, but the split action is the MoneyRequest action, therefore we proceed to calculate the last message as if the report was an IOU one.

We calculate its amount here as 0.00 because the report is not IOU:

https://github.com/Expensify/App/blob/0c49b1198357f33f443543d6873badafd009b093/src/libs/ReportUtils.js#L1508

And proceed all the way to the final return statement:

https://github.com/Expensify/App/blob/0c49b1198357f33f443543d6873badafd009b093/src/libs/ReportUtils.js#L1541

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

If we want to have this message localizable, we should dive deeper into the ReportUtils.getReportPreviewMessage logic and add a separate return for our case:

if (!isIOUReport(report) && ReportActionsUtils.isSplitBillAction(reportAction)) {
    const linkedTransaction = TransactionUtils.getLinkedTransaction(reportAction);
    const {amount, comment} = getTransactionDetails(linkedTransaction);
    return Localize.translateLocal('iou.splitAmount', {amount, comment});
}

And slightly modify the copies:

-splitAmount: ({amount}: SplitAmountParams) => `split ${amount}`,
+splitAmount: ({amount, comment}: SplitAmountParams) => `split ${amount}${comment ? ` for ${comment}` : ''}`,

For this, however, we might want to explore some different cases:

https://www.loom.com/share/a2dedc1cfcc6433c887d2a00b7697ae1?sid=01b8e496-4cad-49a2-85a6-3f0f89d7a8df

What alternative solutions did you explore? (Optional)

We can simply check if the report is an IOU report here:

https://github.com/Expensify/App/blob/0c49b1198357f33f443543d6873badafd009b093/src/libs/OptionsListUtils.js#L392-L393

+} else if (ReportUtils.isIOUReport(report) && ReportActionUtils.isMoneyRequestAction(lastReportAction)) {
-} else if (ReportActionUtils.isMoneyRequestAction(lastReportAction)) {

This will lead to displaying the message from the original API response ("split xxx for yyy").

Please note: this message will not be localized since it's a message that persists from the API response, so this might not be a preferred solution.

Result:

https://www.loom.com/share/8983a6d7e7104bb8afeb00f35d4955ad?sid=c35ea9d4-61fb-4fd7-b27c-125b37acdcb6

paultsimura commented 1 year ago

@aimane-chnaif please see the proposal above and share your thoughts on the mentioned localization matters.

lschurr commented 12 months ago

Bump @aimane-chnaif

lschurr commented 12 months ago

Bumped in Slack as well - https://expensify.slack.com/archives/C01GTK53T8Q/p1696520302027759

aimane-chnaif commented 11 months ago

I think we should fix backend to send transaction data in pusher. Using originalMessage when transaction is empty is workaround. Also, no need to call openReport again because of this. The only fix in frontend here is LHN last message, which is @paultsimura's 2nd proposal. 🎀 👀 🎀 C+ reviewed

Before fix:

Screenshot 2023-10-06 at 4 12 52 PM

After fix:

Screenshot 2023-10-06 at 4 11 51 PM
melvin-bot[bot] commented 11 months ago

Triggered auto assignment to @robertjchen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

robertjchen commented 11 months ago

Would the LNH last message fix still be necessary if the backend is fixed? 🤔

aimane-chnaif commented 11 months ago

Would the LNH last message fix still be necessary if the backend is fixed? 🤔

yes, it's frontend bug

robertjchen commented 11 months ago

Got it, thanks for the review- let's move forward with @paultsimura's second proposal

paultsimura commented 11 months ago

@robertjchen for some reason, the automation was not triggered after the assignment – could you please trigger it?

melvin-bot[bot] commented 11 months ago

Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 11 months ago

📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 11 months ago

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 11 months ago

📣 @Mauri-salazar 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job