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.79k forks source link

[Payment Due 2024-08-20] [$250] App crashes when the user initiates the send invoice flow twice #46821

Closed rezkiy37 closed 1 month ago

rezkiy37 commented 1 month 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!

Version Number: v9.0.16-4 Reproducible in staging?: Yes Reproducible in production?: No Expensify/Expensify Issue URL: Issue reported by: @rezkiy37 Slack conversation: https://callstack-hq.slack.com/archives/C049HHMV9SM/p1722876158696939, https://callstack-hq.slack.com/archives/C06SPA2JTPC/p1722865278681679

Action Performed:

Break down in numbered steps

  1. Open an invoice room.
  2. Initiate the Send Invoice flow from the room.
  3. Close it.
  4. Initiate the Send Invoice flow one more time.

Expected Result:

The app initiates the Send invoice and does not crash.

Actual Result:

The app crashes.

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?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/6467e11d-2c18-4504-9275-be1e08d7066f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011b5e32288a6dbf2c
  • Upwork Job ID: 1820648846301758830
  • Last Price Increase: 2024-08-06
Issue OwnerCurrent Issue Owner: @brunovjk
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @VictoriaExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

daledah commented 1 month ago

Proposal

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

The app crashes.

What is the root cause of that problem?

Optional chaining wasn't used for transaction?.comment here:

https://github.com/Expensify/App/blob/8cfeb33a702c74241aeaec024d8e76776e544fab/src/pages/iou/request/step/IOURequestStepConfirmation.tsx#L611

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

Add optional chaining for transaction?.comment to prevent undefined error:

iouComment={transaction?.comment?.comment ?? ''}

What alternative solutions did you explore? (Optional)

VictoriaExpensify commented 1 month ago

Agree this is a bug that should be fixed

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~011b5e32288a6dbf2c

melvin-bot[bot] commented 1 month ago

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

rezkiy37 commented 1 month ago

@daledah I am developing the invoicing feature and I've found this bug. So I will take care of the fix. Thank you for the proposal.

rezkiy37 commented 1 month ago

Hi, I’m Michael (Mykhailo) from Callstack and I would like to work on this issue.

rezkiy37 commented 1 month ago

Preparing the PR - https://github.com/Expensify/App/pull/46869.

rezkiy37 commented 1 month ago

The PR has been opened for review 🙂

brunovjk commented 1 month ago

Reviewing after lunch.

daledah commented 1 month ago

@rezkiy37 According to the Expensify Contributing Guideline, existing proposals should be prioritized, so I think your PR should be put on HOLD, or closed while proposals are being reviewed.

If there are existing proposals, BZ will put the issue on hold. Contributor+ will review the existing proposals. If a contributor’s proposal is accepted then the contributor will be assigned to the issue. If not the issue will be assigned to the agency-employee.

@brunovjk Could you help review my proposal here according to the guideline? Thanks!

tsa321 commented 1 month ago

@daledah I am developing the invoicing feature and I've found this bug. So I will take care of the fix. Thank you for the proposal.

@daledah @brunovjk I think @rezkiy37 is saying that while developing something, he found a bug and already found a solution. He created this issue to track payment for the agency and need C+ review.

Please take a look at the issue reporter. If it is from an agency employee, I believe it is assigned to them.

daledah commented 1 month ago

I think @rezkiy37 is saying that while developing something, he found a bug and already found a solution.

If this is the case a comment should be posted on the GH before proposals come in. Otherwise I think we should stick to the Contributing Guideline, if not it'd be unfair for contributors who follow the guideline and who post proposals to help with the issue.

brunovjk commented 1 month ago

Hi @VictoriaExpensify, Can you assist with the assignment for this issue? What should I review? Thank you.

davidcardoza commented 1 month ago

Hey everyone, after chatting with the internal Expensify team we are going to move forward with @rezkiy37's PR.

VictoriaExpensify commented 1 month ago

Sounds good, thanks @davidcardoza and @rezkiy37 !

rezkiy37 commented 1 month ago

The PR (https://github.com/Expensify/App/pull/46869) is ready for review.

melvin-bot[bot] commented 1 month ago

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

daledah commented 1 month ago

Hey everyone, after chatting with the internal Expensify team we are going to move forward with @rezkiy37's PR.

Hi @davidcardoza would appreciate if you can provide the reasoning for this decision? (As it's an exception to the contributor guideline)

brunovjk commented 1 month ago

I the PR has now been on prod for a week. Deployed on August 13th: https://github.com/Expensify/App/issues/47219#issuecomment-2287028910

cc: @VictoriaExpensify, @Beamanator

Beamanator commented 1 month ago

Agreed 👍

brunovjk commented 1 month ago

@Beamanator, please remove the Reviewing label and bump to Daily as payment is due. Thank you :D

Beamanator commented 1 month ago

Done 😅

mvtglobally commented 1 month ago

Issue not reproducible during KI retests. (First week)

brunovjk commented 1 month ago

Regression Test Proposal

Send invoice

Do we agree 👍 or 👎

brunovjk commented 1 month ago

Hey @VictoriaExpensify, just a heads up that the payment for this issue is still pending. When you have a moment, could you help process it so we can close this out? Appreciate it!

mallenexpensify commented 1 month ago

@brunovjk can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~011b5e32288a6dbf2c

brunovjk commented 1 month ago

@mallenexpensify Offer accepted, thanks.

mallenexpensify commented 1 month ago

Contributor+: @brunovjk paid $250 via Upwork

@brunovjk] 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. N/A

@brunovjk can you provide reasoning why we don't want a regression test here? Thx

brunovjk commented 1 month ago

@brunovjk can you provide reasoning why we don't want a regression test here? Thx

@mallenexpensify In light of the recent type-safe changes across multiple files and flows, I initially considered that a regression test might not be necessary. However, after reviewing, I don’t see why we shouldn’t create a test. I updated this comment with the test.

mallenexpensify commented 1 month ago

Thanks @brunovjk , when in doubt, let's always create the test cases cuz QA reviews them and they can decide what's best.

brunovjk commented 1 month ago

Good to know, thank you @mallenexpensify 😄