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

[HOLD for payment 2023-10-30] [HOLD for payment 2023-09-15] [$1000] HIGH [W4 Scan Receipts] LHN shows user paid $0.00 when sending first money request via Scan #25778

Closed
6 tasks done
luacmartins opened this issue Aug 23, 2023 · 55 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@luacmartins
Copy link
Contributor

luacmartins commented Aug 23, 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!


Action Performed:

  1. As userA, request money from userB via Scan
  2. Notice that the LHN shows <user> paid $0.00

Expected Result:

The LHN should show Scan in progress

Actual Result:

LHN says <user> paid $0.00

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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:
Reproducible in staging?:
Reproducible in production?:
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
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub
Screenshot 2023-08-23 at 9 46 40 AM

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015c1a629214ef23ce
  • Upwork Job ID: 1694442535880183808
  • Last Price Increase: 2023-08-23
@luacmartins luacmartins added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 23, 2023
@luacmartins luacmartins self-assigned this Aug 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 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

@maddylewis maddylewis added the External Added to denote the issue can be worked on by a contributor label Aug 23, 2023
@melvin-bot melvin-bot bot changed the title LOW [W4 Scan Receipts] LHN shows user paid $0.00 when sending first money request via Scan [$1000] LOW [W4 Scan Receipts] LHN shows user paid $0.00 when sending first money request via Scan Aug 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

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

@ygshbht
Copy link
Contributor

ygshbht commented Aug 23, 2023

Proposal

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

LHN shows user paid $0.00 when sending first money request via Scan

What is the root cause of that problem?

getMoneyRequestReportName function doesn't have functionality to handle this logic

App/src/libs/ReportUtils.js

Lines 1208 to 1225 in f0eeac6

function getMoneyRequestReportName(report, policy = undefined) {
const formattedAmount = CurrencyUtils.convertToDisplayString(getMoneyRequestTotal(report), report.currency);
const payerName = isExpenseReport(report) ? getPolicyName(report, false, policy) : getDisplayNameForParticipant(report.managerID);
const payerPaidAmountMesssage = Localize.translateLocal('iou.payerPaidAmount', {
payer: payerName,
amount: formattedAmount,
});
if (report.isWaitingOnBankAccount) {
return `${payerPaidAmountMesssage}${Localize.translateLocal('iou.pending')}`;
}
if (report.hasOutstandingIOU) {
return Localize.translateLocal('iou.payerOwesAmount', {payer: payerName, amount: formattedAmount});
}
return payerPaidAmountMesssage;
}

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

Add the below code to show the correct status.

// eslint-disable-next-line no-use-before-define
const transactionsWithReceipts = getTransactionsWithReceipts(report.reportID);
if (_.some(transactionsWithReceipts, (transaction) => TransactionUtils.isReceiptBeingScanned(transaction))) {
    return Localize.translate(undefined, 'iou.receiptScanning');
}

The above will show the message if any receipt in the Report is being scanned. To show only for the last message, we can only check with the lastReportAction. We can get that by passing it as argument or use methods such as getLastVisibleMessage (have further checks to see if its not a pending action like delete).

IOUReceipt_trimmed.mp4

Though it's not main issue listed here, i'd like to mention that if such message has to be shown somehwere else, like in the report where the IOU was created, similar logic can be used in functions like getLastMessageTextForReport.

@luacmartins
Copy link
Contributor Author

@ygshbht proposal looks mostly good to me, but what is the first undefined param here Localize.translate(undefined, 'iou.receiptScanning');?

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 24, 2023

📣 @thesahindia Please request via NewDot manual requests for the Reviewer role ($1000)

@melvin-bot
Copy link

melvin-bot bot commented Aug 24, 2023

📣 @ygshbht You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@luacmartins
Copy link
Contributor Author

We can sort that out in the PR review. Assigning you to the issue.

@ygshbht
Copy link
Contributor

ygshbht commented Aug 24, 2023

@luacmartins Thanks. I'll have the PR ready within a day. The undefined needs to be replaced with current language

@ygshbht proposal looks mostly good to me, but what is the first undefined param here Localize.translate(undefined, 'iou.receiptScanning');?

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 24, 2023
@ygshbht
Copy link
Contributor

ygshbht commented Aug 24, 2023

@luacmartins @thesahindia The PR is ready for review!

@allroundexperts
Copy link
Contributor

I'll be taking this one over as requested by @thesahindia.

@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 2023

📣 @allroundexperts Please request via NewDot manual requests for the Contributor role ($1000)

@trjExpensify
Copy link
Contributor

trjExpensify commented Oct 13, 2023

Can you guys help look into this please? I'm going to track this now in wave7. CC: @youssef-lr

@ygshbht
Copy link
Contributor

ygshbht commented Oct 13, 2023

@trjExpensify I think that was the inital job but had to revert due to change in requirements. You can follow the dicussion in the PR or read only from here. Unfortunately, there might have been confusions
image

@trjExpensify
Copy link
Contributor

The discussion seems to point out the expectation to me. Especially as you go further down to say here. Why did we revert it? It shouldn't be reading user paid $0.00.

@ygshbht
Copy link
Contributor

ygshbht commented Oct 13, 2023

@trjExpensify Sorry, I don't understand.
Here is the expectation and I asking for clarificaiton
image
Here's the response #25906 (comment)
image

@trjExpensify
Copy link
Contributor

Right, and prior to the receipt scan completing then the title of the expenseReport (providing this is the only request on the report) is %workspaceName% owes $0.00?

@ygshbht
Copy link
Contributor

ygshbht commented Oct 13, 2023

That's what you see, don't you?
From what i infer, you probably want to show the Scan in progress.. message for LHN title if it's the first reportAction as the job title says?
Again, this was the basis on which I did/ undid the changes
image

#25906 (comment)
image
Minor issues here and there happen. I apologize if its a fault on my side.
If you want, I am more than willing to implement your request through a new job.

@trjExpensify
Copy link
Contributor

From what i infer, you probably want to show the Scan in progress.. message for LHN title if it's the first reportAction as the job title says?

Ah, so that's the diff. I'm not saying that. For the expenseReport...
The LHN title is: %workspaceName% owes $0.00
The LHN description is: Receipt scan in progress...

Right now the LHN title is user paid $0.00 which is incorrect. The LHN description is correct.

@ygshbht
Copy link
Contributor

ygshbht commented Oct 13, 2023

So instead of user paid $0.00, you want to show %workspaceName% owes $0.00? Owe instead of Pay?

@trjExpensify
Copy link
Contributor

Correct, owes, and more specifically given we have two types of reports in question here:

iouReport
%displayName% owes $0.00
Receipt scan in progress...

expenseReport
%workspaceName% owes $0.00
Receipt scan in progress...

@ygshbht
Copy link
Contributor

ygshbht commented Oct 13, 2023

Sorry, whats the iouReport from the below three? Is it the request view?
image

@trjExpensify
Copy link
Contributor

It's the expenseReport one.

If I request money from you, the expense goes on a report of type iou and therefore you owe me.
If I request money from my company on a workspace, the expense goes on a report of type expense therefore the workspace owes me.

@ygshbht
Copy link
Contributor

ygshbht commented Oct 13, 2023

Ok, so in iou, user owes and in expense workspace owes.
I can make that change and create a small PR with the commit or push it to this PR.
However, if you want a full PR with testing on all paltforms and attached screenshots/ videos. I'd say we create a new job

@melvin-bot melvin-bot bot added the Overdue label Oct 16, 2023
@trjExpensify
Copy link
Contributor

Ok, so in iou, user owes and in expense workspace owes.

That's correct!

I can make that change and create a small PR with the commit or push it to this PR.

I don't understand the latter as an option because this PR has been deployed already. I also don't think this needs a new issue, so can we please create a follow-up PR linked to this issue to fix it?

@ygshbht
Copy link
Contributor

ygshbht commented Oct 16, 2023

@trjExpensify Created PR

@melvin-bot
Copy link

melvin-bot bot commented Oct 19, 2023

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

  • when @ygshbht got assigned: 2023-08-24 15:33:05 Z
  • when the PR got merged: 2023-10-19 16:48:23 UTC
  • days elapsed: 40

On to the next one 🚀

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 23, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-09-15] [$1000] HIGH [W4 Scan Receipts] LHN shows user paid $0.00 when sending first money request via Scan [HOLD for payment 2023-10-30] [HOLD for payment 2023-09-15] [$1000] HIGH [W4 Scan Receipts] LHN shows user paid $0.00 when sending first money request via Scan Oct 23, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.88-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-30. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
  • [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@allroundexperts] Determine if we should create a regression test for this bug.
  • [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@trjExpensify
Copy link
Contributor

Okay, so with that now taken care of I think we can proceed to close this out for good now. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
No open projects
Development

No branches or pull requests

7 participants