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

Split scan - Empty tax row in split confirmation page which leads to crash #38260

Closed
6 tasks done
m-natarajan opened this issue Mar 13, 2024 · 15 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause

Comments

@m-natarajan
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.52-0
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: n/a
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: applause internal team
Slack conversation:

Action Performed:

Precondition:

  • User is an employee of Collect workspace.
  • Tax is enabled by admin end via Workspace features > Taxes on ND.
  • There is no any tax rate in the workspace.
  1. Go to staging.new.expensify.com.
  2. Go to workspace chat.
  3. Go to + > Split bill > Scan.
  4. Upload a receipt.
  5. Click on the empty row.

Expected Result:

There will be no empty row in confirmation page.

Actual Result:

There is an empty row in confirmation page, which crashes the app when clicked on it.

Workaround:

unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6412858_1710369778260.bandicam_2024-03-14_03-58-42-377.mp4

View all open jobs on GitHub

@m-natarajan m-natarajan added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 13, 2024
Copy link

melvin-bot bot commented Mar 13, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Mar 13, 2024
Copy link
Contributor

👋 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.

Copy link

melvin-bot bot commented Mar 13, 2024

Triggered auto assignment to @marcaaron (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@m-natarajan
Copy link
Author

@stephanieelliott FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@abzokhattab
Copy link
Contributor

abzokhattab commented Mar 13, 2024

Proposal

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

Empty tax row in split confirmation page which leads to crash

What is the root cause of that problem?

the tax row is shown even though the tax rate is empty

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

we need to update the shouldShowTax so that it returns false if the taxRates is empty

const shouldShowTax = isPolicyExpenseChat && policy && lodashGet(policy, 'tax.trackingEnabled', policy.isTaxTrackingEnabled) && !_.isEmpty(taxRates);

@marcaaron
Copy link
Contributor

Seems possibly related to one of these:

#38111
#37870

@mountiny or @luacmartins probably knows

@luacmartins
Copy link
Contributor

This seems more related to #38111. @yuwenmemon @dukenv0307 @mananjadhav could you please check?

@dukenv0307
Copy link
Contributor

dukenv0307 commented Mar 14, 2024

  • I try reverting PR Fix/37289: Incorrect tag in split bill view #38111 but still can reproduce the bug.

  • RCA: When we enable the tax feature in ND, there is no default tax is created like in OD. It is known issue mentioned in here. That leads to when we click on the tax option (the empty row), the TaxPicker is open. In this page, the below logic is called:

    const taxRatesCount = TransactionUtils.getEnabledTaxRateCount(taxRates.taxes);

    with taxRates.taxes: undefined that leads to the bug

  • Solution: Update the shouldShowTax here. It will be:

 const shouldShowTax = isPolicyExpenseChat && (policy?.tax?.trackingEnabled ?? policy?.isTaxTrackingEnabled) && policy.taxRates;

Also, we need to keep the consistency in the type of the taxRates. Object or Array?

@luacmartins
Copy link
Contributor

Nice find @dukenv0307. If this is related to enabling the tax feature in NewDot, this is not a blocker then since that feature is still WIP and only internal people have access to that. It should also be solved here

@luacmartins luacmartins removed the DeployBlockerCash This issue or pull request should block deployment label Mar 14, 2024
@luacmartins luacmartins assigned luacmartins and unassigned marcaaron Mar 14, 2024
@luacmartins luacmartins added Weekly KSv2 and removed Hourly KSv2 labels Mar 14, 2024
@luacmartins luacmartins changed the title Split scan - Empty tax row in split confirmation page which leads to crash [HOLD #38234] Split scan - Empty tax row in split confirmation page which leads to crash Mar 14, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 25, 2024
@stephanieelliott
Copy link
Contributor

Still held on #38234

@melvin-bot melvin-bot bot removed the Overdue label Mar 26, 2024
@trjExpensify
Copy link
Contributor

trjExpensify commented Apr 4, 2024

This can come off hold. I think it can be retested now!

@melvin-bot melvin-bot bot added the Overdue label Apr 4, 2024
@trjExpensify trjExpensify changed the title [HOLD #38234] Split scan - Empty tax row in split confirmation page which leads to crash Split scan - Empty tax row in split confirmation page which leads to crash Apr 4, 2024
@stephanieelliott
Copy link
Contributor

Just tested and I think this is not happening anymore? The conditions to repro are:

There is no any tax rate in the workspace,

and

Click on the empty row.

Now, when tax is enabled it's not possible to have no tax rate because the rateTax Exempt auto populates and can't be deleted. With this being the case, you will never have an empty row.

image

@melvin-bot melvin-bot bot removed the Overdue label Apr 4, 2024
@stephanieelliott
Copy link
Contributor

Hey @m-natarajan can you check me on the above? It seems like this is either no longer happening, or we need to add more details to Action Performed and include steps for configuring Tax on the workspace so that this is reproducible

@stephanieelliott stephanieelliott added Daily KSv2 retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause and removed Weekly KSv2 labels Apr 4, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

@luacmartins, @stephanieelliott Whoops! This issue is 2 days overdue. Let's get this updated quick!

@luacmartins
Copy link
Contributor

I think this has been fixed by #38234. We're good to close.

@melvin-bot melvin-bot bot removed the Overdue label Apr 8, 2024
@github-project-automation github-project-automation bot moved this from Polish to Done in [#whatsnext] #wave-collect Apr 8, 2024
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. Daily KSv2 Engineering retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause
Projects
No open projects
Archived in project
Development

No branches or pull requests

7 participants