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] IOU - Incorrect total amount shown when deleting money request in offline mode #26054

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 28, 2023 · 45 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 28, 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. Login to staging new dot
  2. Turn offline mode on
  3. Click on the plus sign and select "Request Money".
  4. Enter the desired amount and email address for the money request
  5. Open IOU report details and edit the amount to a smaller amount than the one in step 4
  6. Click on the three dots and delete the request
  7. Go back to IOU report and observe that the difference of the two amounts entered is shown in total rather than 0

Expected Result:

0 should be shown in total amount

Actual Result:

A difference of the initial amount and edited amount is shown as total

Workaround:

Unknown

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

2023_08_27_10_25_34.mp4
Record_2023-08-27-23-54-30.mp4

Expensify/Expensify Issue URL:

Issue reported by: @SofoniasN

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0140d719da85b979b3
  • Upwork Job ID: 1697022453349801984
  • Last Price Increase: 2023-08-30
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 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 Overdue label Aug 30, 2023
@adelekennedy adelekennedy added the External Added to denote the issue can be worked on by a contributor label Aug 30, 2023
@melvin-bot melvin-bot bot changed the title IOU - Incorrect total amount shown when deleting money request in offline mode [$500] IOU - Incorrect total amount shown when deleting money request in offline mode Aug 30, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0140d719da85b979b3

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

melvin-bot bot commented Aug 30, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2023

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

@adelekennedy
Copy link

🐛 let's make it external

@melvin-bot melvin-bot bot removed the Overdue label Aug 30, 2023
@im-adithya
Copy link

Proposal

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

Incorrect total amounts shown after editing invoices

What is the root cause of that problem?

Logic is missing in IOU due to which the total amount of iouReport is not changing

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

Add the required code here to fill step 3

What alternative solutions did you explore? (Optional)

--

Working Demo (Implemented already, waiting for assignment)

Screen.Recording.2023-08-31.at.8.51.01.AM.mov

@tienifr
Copy link
Contributor

tienifr commented Aug 31, 2023

Proposal

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

IOU - Incorrect total amount shown when deleting money request in offline mode

What is the root cause of that problem?

When edit the money request we lack of re-calculating the amount, so after deleting the request, we use the outdated amount

// STEP 3: Compute the IOU total and update the report preview message so LHN amount owed is correct

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

We should add the logic to re-calculate the amount in editMoneyRequest function

// STEP 3: Compute the IOU total and update the report preview message so LHN amount owed is correct
    let updatedIOUReport = {...iouReport};

    if (_.has(transactionChanges, 'amount') && transactionChanges.currency === iouReport.currency) {
        updatedIOUReport.total += updatedReportAction.originalMessage.amount - updatedReportAction.originalMessage.oldAmount;
    }

  // STEP 4: Compose the optimistic data
    const optimisticData = [
   ...
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`,
            value: updatedIOUReport,
        },
    ];

Result

Screen.Recording.2023-08-31.at.11.22.39.mp4

@mananjadhav
Copy link
Collaborator

@im-adithya Can you please explain/share some details on how are you planning to get the total? Which objects/properties will you use to update?

@im-adithya
Copy link

Hey @mananjadhav, I first added this utils function in IOUUtils.js

function editIOUTotal(iouReport, initialTransaction, updatedTransaction, isExpenseReport = false) {
  if (initialTransaction.currency !== updatedTransaction.currency) {
    return iouReport;
  }

  const initialAmount = TransactionUtils.getAmount(initialTransaction, false);
  const updatedAmount = TransactionUtils.getAmount(updatedTransaction, false);

  // Make a copy so we don't mutate the original object
  const iouReportUpdate = {...iouReport};

  iouReportUpdate.total += !isExpenseReport ? -initialAmount : initialAmount;
  iouReportUpdate.total += !isExpenseReport ? updatedAmount : -updatedAmount;

  iouReportUpdate.hasOutstandingIOU = iouReportUpdate.total !== 0;

  return iouReportUpdate;
}

And then used this to fill step 3 mentioned above to update the IOUReport and then also updated the message "xxx owes $yyy " using the reportPreviewAction (fetched from getReportPreviewAction using iouReport.chatReportID)

And finally update the optimistic, success and failure data which are used while making the API call.

That fixed the issue (both while request amounts from others (+) and while requesting in own workspace (-)) as in the demo.

Let me know if there are any changes, happy to tweak the solution as per the requirement!

@im-adithya
Copy link

Also, this error also persists in online mode too, not just offline!

Would like to request if the bounty can be raised as this is more critical than tooltips issue (an example) 🙏

@mananjadhav
Copy link
Collaborator

mananjadhav commented Sep 1, 2023

Thanks for the details @im-adithya. Few more clarifications.

Why do we need to const initialAmount = TransactionUtils.getAmount(initialTransaction, false); when we can get updatedReportAction.originalMessage.amount from @tienifr's proposal.

Also what does updatedTransaction mean?

Would like to request if the bounty can be raised as this is more critical than

I think it is a fair price, but I will still leave this for the internal team to decide.

@tienifr
Copy link
Contributor

tienifr commented Sep 1, 2023

@mananjadhav What do you think about my proposal? Thanks

@im-adithya
Copy link

Yes we can do both, I had the initialTransaction and updatedTransaction readily available in the above lines from Step 1 and 2 respectively so I thought of using them to keep it uniform like it's used here:

updatedIOUReport.total += TransactionUtils.getAmount(transaction, true);

Just to give more context on how the utils function is going to be used:

const updatedIOUReport = IOUUtils.editIOUTotal(iouReport, transaction, updatedTransaction, isFromExpenseReport);

Initial Tx:

const transaction = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];

Also what does updatedTransaction mean?

updated Tx:

const updatedTransaction = TransactionUtils.getUpdatedTransaction(transaction, transactionChanges, isFromExpenseReport);

and then we move forward to edit the message:

    const reportPreviewAction = ReportActionsUtils.getReportPreviewAction(iouReport.chatReportID, iouReport.reportID);
    const updatedReportPreviewAction = {...reportPreviewAction};
    const messageText = Localize.translateLocal('iou.payerOwesAmount', {
        payer: updatedIOUReport.managerEmail,
        amount: CurrencyUtils.convertToDisplayString(updatedIOUReport.total, updatedIOUReport.currency),
    });
    updatedReportPreviewAction.message[0].text = messageText;
    updatedReportPreviewAction.message[0].html = messageText;

and then finally add updatedIOUReport and updatedReportPreviewAction in the optimistic data and reportPreviewAction in failure data (others are already there in success and failure data)

@im-adithya
Copy link

I think it is a fair price, but I will still leave this for the internal team to decide.

That definitely is a fair price individually, just wanted to bring up the comparison based on issue criticality/difficulty.

@mananjadhav
Copy link
Collaborator

@im-adithya's proposal looks good to me.

🎀 👀 🎀 C+ reviewed.

@melvin-bot
Copy link

melvin-bot bot commented Sep 1, 2023

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

@mananjadhav
Copy link
Collaborator

Would like to request if the bounty can be raised as this is more critical than

And @adelekennedy @yuwenmemon your thoughts on this comment.

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Sep 1, 2023
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Sep 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

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

@yuwenmemon yuwenmemon added Weekly KSv2 Daily KSv2 and removed Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Sep 5, 2023
@yuwenmemon
Copy link
Contributor

yuwenmemon commented Sep 5, 2023

Yep, sorry for the delay I was out for a US holiday!

(also sorry for the label spam I clicked the wrong button 🤦 )

@im-adithya
Copy link

@mananjadhav not opening a new PR as there's #26486 already

@mananjadhav
Copy link
Collaborator

@yuwenmemon @adelekennedy The fix was merged with another PR, while I was OOO and this one got delayed.

IMO, @im-adithya is eligible for the full compensation on this one.

@yuwenmemon
Copy link
Contributor

Sounds good, thanks for letting us know @mananjadhav - I guess we'll just close this out then @adelekennedy?

@SofoniasN
Copy link

@adelekennedy @mananjadhav Hey guys even though this was fixed by another PR, as it was a legitimate bug shouldnt i be eligible for reporter bonus?

@mananjadhav
Copy link
Collaborator

@adelekennedy did you get to see my comment regarding compensation for @im-adithya?

@adelekennedy
Copy link

Yes!

Payouts due:

Issue Reporter: $250 @SofoniasN (as this was a legit bug) Upwork
Contributor: $500 @im-adithya Upwork
Contributor+: $500 @mananjadhav (proposal reviews) NewDot

Eligible for 50% #urgency bonus? N

Upwork job is here.

@mananjadhav
Copy link
Collaborator

Thanks @adelekennedy.

@adelekennedy
Copy link

@SofoniasN @im-adithya will you apply here

@im-adithya
Copy link

Thanks @adelekennedy, however it says Only invited users can find, view and apply to the job

@SofoniasN
Copy link

Hey @adelekennedy since the upwork job link is private, i couldnt apply.

@JmillsExpensify
Copy link

$500 payment approved for @mananjadhav based on BZ summary.

@adelekennedy
Copy link

@SofoniasN @im-adithya I swear this is this Upwork bug when we re-use postings, try this link?

@im-adithya
Copy link

Now it says Access denied. This job is private. Only freelancers invited by client can view this job.

@adelekennedy
Copy link

I have refreshed it again - edited it and republished. If this doesn't work I will open a new job:https://www.upwork.com/jobs/~0140d719da85b979b3

@SofoniasN
Copy link

Same here it still says privateScreenshot_2023-09-21-14-12-52-471.jpg

@adelekennedy
Copy link

Then this is an Upwork bug as I see it's public on my end 💀 I'm going to close this one and open a new one

@adelekennedy
Copy link

🤞 @SofoniasN @im-adithya - I closed the original and created a new job

@im-adithya
Copy link

Done now! Thanks again @adelekennedy!

@SofoniasN
Copy link

@adelekennedy works now. Offer accepted!

@adelekennedy
Copy link

hired!

@SofoniasN
Copy link

@adelekennedy offer accepted. Thank you!

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants