search

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-08-08] Split bill - App crashed when delete split bill #23824

Closed kbecciv closed 1 year ago

kbecciv 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. Launch App
  2. Create split bill
  3. Long press on request
  4. Tap on 'Delete request"

Expected Result:

The request should be delete.

Actual Result:

App crashed when delete split bill.

Workaround:

Unknown

Platforms:

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

Version Number: v1.3.47-3 Reproducible in staging?: y Reproducible in production?: new feature 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/93399543/10872bea-2bd9-40ca-b653-599153b94a9a

Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

View all open jobs on GitHub

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:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 1 year ago

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

marcaaron commented 1 year ago

Seems related -> https://console.firebase.google.com/u/0/project/expensify-chat/crashlytics/app/android:com.expensify.chat/issues/e8357e4aff4d3d4b1bf39478a981997d?time=last-seven-days&versions=1.3.45-8%20(1001034508);1.3.46-0%20(1001034600);1.3.46-1%20(1001034601)&sessionEventKey=64C1D1B6003700015C31403A37B96EE1_1838760527596780186

Beamanator commented 1 year ago

May be related to: https://github.com/Expensify/Expensify/issues/304096

mountiny commented 1 year ago

cc @luacmartins I am not sure if we have tested this flow well

Beamanator commented 1 year ago

I'm seeing this in web when I try to delete a split:

Screenshot 2023-07-31 at 2 05 00 PM
Beamanator commented 1 year ago

@mountiny can you find the PR that may have caused this?

mountiny commented 1 year ago

https://github.com/Expensify/App/pull/19095

I dont think we should revert though, I think we should just disable the option on split action before we figure this out

Beamanator commented 1 year ago

Ok can you take over finding a good solution for this one? 🙏

Beamanator commented 1 year ago

Idk why melvin thought this should be weekly lol

situchan commented 1 year ago

https://github.com/Expensify/App/blob/8cdb506e97e8382f4bc55b0f48784f576fbb2789/src/libs/actions/IOU.js#L775 The crash happens on this line because originalMessage.IOUReportID doesn't exist in split report action Based on https://github.com/Expensify/App/pull/19095/files#diff-16a590a45460a9abf1e402441b50e3cb2659ee3da203ca30a2fa4ce3f9411957L253-R253, I think the issue existed before https://github.com/Expensify/App/pull/19095. And enabling delete money request feature just revealed this.

mountiny commented 1 year ago

correct, I think we just forgot about this case.

mountiny commented 1 year ago

@Beamanator thats an automation for when PR is raised and marked as ready for a review.

Beamanator commented 1 year ago

Thanks @mountiny :D

So do we think this can be easily "fixed"? Or should we just disabled deleting splits for now

mountiny commented 1 year ago

disabled for now, I am not sure the auth command is build for this

Beamanator commented 1 year ago

Perfecto - PR merged, CPing to staging

Beamanator commented 1 year ago

Confirmed working in web & iOS staging - marking not a blocker!

Also assigning Bug to get C+ paid!

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @greg-schroeder (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

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

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2023-08-07. :confetti_ball:

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.

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

melvin-bot[bot] commented 1 year ago

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:

mountiny commented 1 year ago

$1000 to @Ollyws for internal review

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2023-08-08. :confetti_ball:

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.

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:

melvin-bot[bot] commented 1 year ago

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:

mountiny commented 1 year ago

$1000 for @Ollyws for internal review.

Ollyws commented 1 year ago

Is the BZ checklist required for a temporary fix like this?

greg-schroeder commented 1 year ago

Offer sent to @Ollyws

greg-schroeder commented 1 year ago

Don't think the checklist will be super necessary here personally. Going to close this now that offer is sent! will pay as soon as accepted

Ollyws commented 1 year ago

@greg-schroeder Friendly bump for payment.

greg-schroeder commented 1 year ago

Oh, wow, sorry, sending now @Ollyws

tranvantoan-qn commented 1 year ago

@mountiny I apologize for bringing up this matter once more.

Did we solely disable the "Delete" option on the main Request?

I'm still experiencing the same crash, but in a slightly different manner.

Whenever a split request is initiated, it also generates a sub-report for each participant, allowing me to delete each request. Unfortunately, this leads to a crash when in Offline mode.

TypeError: Cannot read properties of null (reading 'reportID')
    at main-426150f61736a9bd57d7.bundle.js:1:3040782
    at vendors-3db7fcdc0b1c4dcfe55f.bundle.js:1:16283362
    at Xt (vendors-3db7fcdc0b1c4dcfe55f.bundle.js:1:16282859)
    at Function.qt (vendors-3db7fcdc0b1c4dcfe55f.bundle.js:1:16283340)
    at Object.P (main-426150f61736a9bd57d7.bundle.js:1:3040751)
    at Object.m (main-426150f61736a9bd57d7.bundle.js:1:1269288)
    at jn (main-426150f61736a9bd57d7.bundle.js:1:1470777)
    at fA (vendors-3db7fcdc0b1c4dcfe55f.bundle.js:1:9865482)
    at Bs (vendors-3db7fcdc0b1c4dcfe55f.bundle.js:1:9922584)
    at bl (vendors-3db7fcdc0b1c4dcfe55f.bundle.js:1:9911745)