[PAID] [$1000] App crashes when splitting a bill offline #14355

kavimuru · 2023-01-17T14:27:18Z

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:

Go offline
Open the global create menu (big green ➕)
Select Split Bill
Enter an amount
Enter the email of exactly one other user who you have never chatted with before
Add a memo to the request
Split the bill to finalize the request

Expected Result:

You should be taken to a 1:1 chat with that user
You should a normal message that says Split $X with for
You should see another IOU action that says: Requested $(X/2) from for

Actual Result:

The app crashes and you land on the error boundary

Workaround:

unknown

Platforms:

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

Version Number: 1.2.55-0
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
Notes/Photos/Videos:

Recording.1313.mp4

Expensify/Expensify Issue URL:
Issue reported by: @roryabraham
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673903364260589

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013e5bf073f9b98c1b
Upwork Job ID: 1615404129941577728
Last Price Increase: 2023-01-17

melvin-bot · 2023-01-17T14:27:23Z

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

melvin-bot · 2023-01-17T17:42:14Z

Job added to Upwork: https://www.upwork.com/jobs/~013e5bf073f9b98c1b

melvin-bot · 2023-01-17T17:42:16Z

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

melvin-bot · 2023-01-17T17:42:23Z

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

melvin-bot · 2023-01-17T17:42:27Z

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

Pujan92 · 2023-01-17T17:48:56Z

Proposal

We can validate first the IOUReportID to avoid the error.

diff --git a/src/libs/IOUUtils.js b/src/libs/IOUUtils.js
index 5c1538682..681ada7c6 100644
--- a/src/libs/IOUUtils.js
+++ b/src/libs/IOUUtils.js
@@ -82,7 +82,7 @@ function getIOUReportActions(reportActions, iouReport, type = '', pendingAction
     return _.chain(reportActions)
         .filter(action => action.originalMessage
             && action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU
-            && action.originalMessage.IOUReportID.toString() === iouReport.reportID.toString())
+            && action.originalMessage.IOUReportID && action.originalMessage.IOUReportID.toString() === iouReport.reportID.toSt
ring())
         .filter(action => (!_.isEmpty(type) ? action.originalMessage.type === type : true))
         .filter(action => (!_.isEmpty(pendingAction) ? action.pendingAction === pendingAction : true))
         .filter(action => (filterRequestsInDifferentCurrency ? action.originalMessage.currency !== iouReport.currency : true))

tienifr · 2023-01-17T17:55:08Z

Proposal

We should check that both action.originalMessage.IOUReportID and iouReport.IOUReportID are not nil to avoid such crashes. Simply checking && action.originalMessage.IOUReportID is not completely correct to me since action.originalMessage.IOUReportID and iouReport.reportID can both be empty, it's fine to use toString() with an empty string. So using && action.originalMessage.IOUReportID will slightly modify the comparison logic (in case of empty string), rather than just fixing the crash.

Checking that they are not nil will preserve the previous logic without any potential regression that differs from existing logic.

(Below I use both isUndefined and isNull but we can consider creating an isNil util for it as well, the current version of underscoreJs we used does not have isNil)

diff --git a/src/libs/IOUUtils.js b/src/libs/IOUUtils.js
index 5c1538682c..aa8ecb6139 100644
--- a/src/libs/IOUUtils.js
+++ b/src/libs/IOUUtils.js
@@ -81,6 +81,8 @@ function updateIOUOwnerAndTotal(iouReport, actorEmail, amount, currency, type =
 function getIOUReportActions(reportActions, iouReport, type = '', pendingAction = '', filterRequestsInDifferentCurrency = false) {
     return _.chain(reportActions)
         .filter(action => action.originalMessage
+            && !_.isUndefined(action.originalMessage.IOUReportID) && !_.isNull(action.originalMessage.IOUReportID)
+            && !_.isUndefined(iouReport.reportID) && !_.isNull(iouReport.reportID)
             && action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU
             && action.originalMessage.IOUReportID.toString() === iouReport.reportID.toString())
         .filter(action => (!_.isEmpty(type) ? action.originalMessage.type === type : true))

Pujan92 · 2023-01-17T19:10:37Z

I don't think we need to validate iouReport. reportID too as it will be available due to the fact we are generating it.

App/src/libs/ReportUtils.js

Line 725 in 3636f1f

reportID: generateReportID(),

fedirjh · 2023-01-17T21:09:49Z

Proposal

Root Issue

This is a regression from 966e018 , removing this line && action.originalMessage.type === type introduced an error in filtering reportActions

This is an example of IOU report action that causes this issue , this reportAction does not have IOUReportID ( it's undefined ) hence converting it's value .toString() will cause an error

Solution

Rollback old code that validates action.originalMessage.type by it's type

diff --git a/src/libs/IOUUtils.js b/src/libs/IOUUtils.js
index 5c1538682c..0e06b5a16e 100644
--- a/src/libs/IOUUtils.js
+++ b/src/libs/IOUUtils.js
@@ -82,6 +82,7 @@ function getIOUReportActions(reportActions, iouReport, type = '', pendingAction
     return _.chain(reportActions)
         .filter(action => action.originalMessage
             && action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU
+            && action.originalMessage.type === type
             && action.originalMessage.IOUReportID.toString() === iouReport.reportID.toString())
         .filter(action => (!_.isEmpty(type) ? action.originalMessage.type === type : true))
         .filter(action => (!_.isEmpty(pendingAction) ? action.pendingAction === pendingAction : true))

Ollyws · 2023-01-17T22:50:53Z

Proposal

It's the IOU reports without the specified type (CREATE) that don't contain the report ID, causing the error when using toString().
It seems the author already accounted for this here, it just needs re-ordering to resolve this error:

diff --git a/src/libs/IOUUtils.js b/src/libs/IOUUtils.js
index 5c1538682..a8cb9a6c7 100644
--- a/src/libs/IOUUtils.js
+++ b/src/libs/IOUUtils.js
@@ -81,9 +81,9 @@ function updateIOUOwnerAndTotal(iouReport, actorEmail, amount, currency, type =
 function getIOUReportActions(reportActions, iouReport, type = '', pendingAction = '', filterRequestsInDifferentCurrency = false) {
     return _.chain(reportActions)
         .filter(action => action.originalMessage
-            && action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU
+            && (!_.isEmpty(type) ? action.originalMessage.type === type : true))
+        .filter(action => action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU
             && action.originalMessage.IOUReportID.toString() === iouReport.reportID.toString())
-        .filter(action => (!_.isEmpty(type) ? action.originalMessage.type === type : true))
         .filter(action => (!_.isEmpty(pendingAction) ? action.pendingAction === pendingAction : true))
         .filter(action => (filterRequestsInDifferentCurrency ? action.originalMessage.currency !== iouReport.currency : true))
         .value();

strepanier03 · 2023-01-18T00:59:27Z

@sobitneupane - A few proposals coming through, thanks for taking a look. I'll circle back tomorrow to check in for any updates.

sobitneupane · 2023-01-18T04:23:28Z

@Ollyws Can you please elaborate how re-ordering will solve the issue?

Ollyws · 2023-01-18T11:15:53Z

@sobitneupane
We specify the report action type to filter here, this type always contains action.originalMessage.IOUReportID in this case.
The problem is we are filtering by type after attempting to filter by action.originalMessage.IOUReportID, leading to the error on .toString() when it doesn't exist.
If we filter by type beforehand we will only be left with reports that contain action.originalMessage.IOUReportID when attempting the comparison.

sobitneupane · 2023-01-18T16:29:09Z

@Ollyws Thanks for the clarification.

@Ollyws's proposal looks good to me.

I will like to suggest a minor change in the proposal if it doesn't change the expected output.

function getIOUReportActions(reportActions, iouReport, type = '', pendingAction = '', filterRequestsInDifferentCurrency = false) {
    return _.chain(reportActions)
        .filter(action => action.originalMessage
            && action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU
-            && action.originalMessage.IOUReportID.toString() === iouReport.reportID.toString())
-        .filter(action => (!_.isEmpty(type) ? action.originalMessage.type === type : true))
+            && (!_.isEmpty(type) ? action.originalMessage.type === type : true))
+        .filter(action => action.originalMessage.IOUReportID.toString() === iouReport.reportID.toString())
        .filter(action => (!_.isEmpty(pendingAction) ? action.pendingAction === pendingAction : true))
        .filter(action => (filterRequestsInDifferentCurrency ? action.originalMessage.currency !== iouReport.currency : true))
        .value();
}

🎀👀🎀 C+ reviewed

cc: @yuwenmemon

yuwenmemon · 2023-01-18T20:45:53Z

Sounds good thanks! @Ollyws Please submit a PR at your earliest convenience.

melvin-bot · 2023-01-18T20:46:08Z

📣 @Ollyws You have been assigned to this job by @yuwenmemon!
Please apply to this job in Upwork 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 📖

Ollyws · 2023-01-18T21:56:41Z

All yours @sobitneupane !

strepanier03 · 2023-01-19T01:49:52Z

Thank you everyone for moving this along!

strepanier03 · 2023-01-20T22:59:02Z

PR review is waiting on @yuwenmemon but he's OoO today. I'll take this into consideration when assessing the bonus for merge during payout.

strepanier03 · 2023-01-24T23:06:35Z

Most recent update

PR is merged and awaiting deploy to production, all good here for now.

melvin-bot · 2023-01-26T19:25:15Z

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

melvin-bot · 2023-01-26T19:25:16Z

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

Fix for: App crashes when splitting a bill offline #14400

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

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

External issue reporter - @roryabraham - Internal report not available for reporting payment.
Contributor that fixed the issue - $1000+$500 bonus - @Ollyws
Contributor+ that helped on the issue and/or PR - $1000+$500 bonus - @sobitneupane

Timeline assessment for bonus:

PR was submitted on January 18 @1:55pm
PR was merged on January 24 @10:21 am

That's technically 4 business days but Yuwen was OoO and reviewed promptly as soon as he got back. This did push it out at least one extra business day so I'm going to apply the speed bonus.

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 · 2023-01-26T19:25:19Z

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:

[@sobitneupane / @yuwenmemon] The PR that introduced the bug has been identified. Link to the PR: Improve cancelling money in a different currency offline #13329
[@sobitneupane / @yuwenmemon] 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: Improve cancelling money in a different currency offline #13329 (comment)
[@sobitneupane / @yuwenmemon] 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: Offline tests are already included in the PR review checklist. So, I don't think any update in the PR review checklist will be necessary.
[@strepanier03] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

strepanier03 · 2023-01-30T22:24:28Z

@sobitneupane and @Ollyws - Please feel free to apply for the job here and I'll keep an eye open for your application so I can send you offers. If we get this taken care of now I will be able to pay more promptly on 2023-02-02. Thanks!

Will work on reg test now.

strepanier03 · 2023-01-31T23:23:01Z

@sobitneupane and @Ollyws - I've accepted your proposals and extended the offer, thank you for taking care of it so quickly.

strepanier03 · 2023-02-01T23:36:23Z

@sobitneupane or @yuwenmemon - Can one of you please finish the three steps in this checklist?

strepanier03 · 2023-02-01T23:52:54Z

Reg test buddy check here.

sobitneupane · 2023-02-02T04:28:13Z

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:

The PR that introduced the bug has been identified. Link to the PR:

#13329

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:

#13329 (comment)

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:

Offline tests are already included in the PR review checklist. So, I don't think any update in the PR review checklist will be necessary.

strepanier03 · 2023-02-02T22:41:07Z

@sobitneupane and @Ollyws have been paid and contracts closed in UpWork, thank you both for your hard work.

This GH will stay open until the reg test is resolved.

strepanier03 · 2023-02-03T23:56:29Z

Bumped reg test buddy-check.

yuwenmemon · 2023-02-06T21:57:41Z

@strepanier03 Are we good to close this out?

strepanier03 · 2023-02-07T16:01:28Z

Nope, not ready to close yet, the reg test isn't added.

strepanier03 · 2023-02-07T16:12:08Z

Waiting for reg test to be added here.

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 17, 2023

melvin-bot bot assigned strepanier03 Jan 17, 2023

melvin-bot bot locked and limited conversation to collaborators Jan 17, 2023

roryabraham added the External Added to denote the issue can be worked on by a contributor label Jan 17, 2023

melvin-bot bot unlocked this conversation Jan 17, 2023

melvin-bot bot changed the title ~~App crashes when splitting a bill offline~~ [$1000] App crashes when splitting a bill offline Jan 17, 2023

melvin-bot bot assigned sobitneupane Jan 17, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 17, 2023

melvin-bot bot assigned yuwenmemon Jan 17, 2023

yuwenmemon assigned Ollyws Jan 18, 2023

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 18, 2023

Ollyws mentioned this issue Jan 18, 2023

Fix for: App crashes when splitting a bill offline #14400

Merged

54 tasks

melvin-bot bot added the Reviewing Has a PR in review label Jan 18, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jan 26, 2023

melvin-bot bot changed the title ~~[$1000] App crashes when splitting a bill offline~~ [HOLD for payment 2023-02-02] [$1000] App crashes when splitting a bill offline Jan 26, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Jan 26, 2023

strepanier03 added Daily KSv2 and removed Weekly KSv2 labels Jan 30, 2023

sobitneupane mentioned this issue Feb 2, 2023

Improve cancelling money in a different currency offline #13329

Merged

53 tasks

sobitneupane mentioned this issue Feb 2, 2023

Request/Split bill - Page crash after money request/Split bill while offline #14136

Closed

4 tasks

strepanier03 changed the title ~~[HOLD for payment 2023-02-02] [$1000] App crashes when splitting a bill offline~~ [PAID] [$1000] App crashes when splitting a bill offline Feb 2, 2023

melvin-bot bot added the Overdue label Feb 6, 2023

melvin-bot bot removed the Overdue label Feb 6, 2023

strepanier03 closed this as completed Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAID] [$1000] App crashes when splitting a bill offline #14355

[PAID] [$1000] App crashes when splitting a bill offline #14355

kavimuru commented Jan 17, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jan 17, 2023

melvin-bot bot commented Jan 17, 2023

melvin-bot bot commented Jan 17, 2023

melvin-bot bot commented Jan 17, 2023

melvin-bot bot commented Jan 17, 2023

Pujan92 commented Jan 17, 2023

tienifr commented Jan 17, 2023 •

edited

Loading

Pujan92 commented Jan 17, 2023

fedirjh commented Jan 17, 2023

Ollyws commented Jan 17, 2023

strepanier03 commented Jan 18, 2023

sobitneupane commented Jan 18, 2023

Ollyws commented Jan 18, 2023

sobitneupane commented Jan 18, 2023

yuwenmemon commented Jan 18, 2023

melvin-bot bot commented Jan 18, 2023

Ollyws commented Jan 18, 2023

strepanier03 commented Jan 19, 2023

strepanier03 commented Jan 20, 2023

strepanier03 commented Jan 24, 2023 •

edited

Loading

melvin-bot bot commented Jan 26, 2023

melvin-bot bot commented Jan 26, 2023 •

edited by strepanier03

Loading

melvin-bot bot commented Jan 26, 2023 •

edited by strepanier03

Loading

strepanier03 commented Jan 30, 2023

strepanier03 commented Jan 31, 2023

strepanier03 commented Feb 1, 2023

strepanier03 commented Feb 1, 2023

sobitneupane commented Feb 2, 2023 •

edited

Loading

strepanier03 commented Feb 2, 2023

strepanier03 commented Feb 3, 2023

yuwenmemon commented Feb 6, 2023

strepanier03 commented Feb 7, 2023

strepanier03 commented Feb 7, 2023

[PAID] [$1000] App crashes when splitting a bill offline #14355

[PAID] [$1000] App crashes when splitting a bill offline #14355

Comments

kavimuru commented Jan 17, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Jan 17, 2023

melvin-bot bot commented Jan 17, 2023

melvin-bot bot commented Jan 17, 2023

melvin-bot bot commented Jan 17, 2023

melvin-bot bot commented Jan 17, 2023

Pujan92 commented Jan 17, 2023

tienifr commented Jan 17, 2023 • edited Loading

Proposal

Pujan92 commented Jan 17, 2023

fedirjh commented Jan 17, 2023

Proposal

Root Issue

Solution

Ollyws commented Jan 17, 2023

strepanier03 commented Jan 18, 2023

sobitneupane commented Jan 18, 2023

Ollyws commented Jan 18, 2023

sobitneupane commented Jan 18, 2023

yuwenmemon commented Jan 18, 2023

melvin-bot bot commented Jan 18, 2023

Ollyws commented Jan 18, 2023

strepanier03 commented Jan 19, 2023

strepanier03 commented Jan 20, 2023

strepanier03 commented Jan 24, 2023 • edited Loading

Most recent update

melvin-bot bot commented Jan 26, 2023

melvin-bot bot commented Jan 26, 2023 • edited by strepanier03 Loading

melvin-bot bot commented Jan 26, 2023 • edited by strepanier03 Loading

strepanier03 commented Jan 30, 2023

strepanier03 commented Jan 31, 2023

strepanier03 commented Feb 1, 2023

strepanier03 commented Feb 1, 2023

sobitneupane commented Feb 2, 2023 • edited Loading

strepanier03 commented Feb 2, 2023

strepanier03 commented Feb 3, 2023

yuwenmemon commented Feb 6, 2023

strepanier03 commented Feb 7, 2023

strepanier03 commented Feb 7, 2023

kavimuru commented Jan 17, 2023 •

edited by melvin-bot bot

Loading

tienifr commented Jan 17, 2023 •

edited

Loading

strepanier03 commented Jan 24, 2023 •

edited

Loading

melvin-bot bot commented Jan 26, 2023 •

edited by strepanier03

Loading

melvin-bot bot commented Jan 26, 2023 •

edited by strepanier03

Loading

sobitneupane commented Feb 2, 2023 •

edited

Loading