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] Taxes - App crashes when changing tax rate to 0% on native apps and it shows 00 on web #39600

Closed
6 tasks done
kbecciv opened this issue Apr 4, 2024 · 27 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Apr 4, 2024

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.60-1
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

  • User is an employee of Collect workspace that has taxes.
  • One of the tax rates is 0%.
  1. Go to staging.new.expensify.com
  2. Go to workspace chat.
  3. Create a manual request with tax that is not 0%.
  4. Go to transaction thread.
  5. Click Tax.
  6. Select the 0% tax rate.

Expected Result:

App does not crash

Actual Result:

App crashes. On web and desktop app, it shows 00 instead of crashing

Workaround:

n/a

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

Bug6437919_1712233908134.iOS.mp4
Bug6437919_1712233908103.tax_0__web.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01738457c70add314d
  • Upwork Job ID: 1776310057649451008
  • Last Price Increase: 2024-04-05
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Apr 4, 2024
Copy link

melvin-bot bot commented Apr 4, 2024

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

Copy link
Contributor

github-actions bot commented Apr 4, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Apr 4, 2024

We think that this bug might be related to #wave-collect - Release 1

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Apr 4, 2024

Proposal

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

Taxes - App crashes when changing tax rate to 0% on native apps and it shows 00 on web

What is the root cause of that problem?

The main problem with issue is that shouldShowTax can return number

const shouldShowTax = isTaxPolicyEnabled(isPolicyExpenseChat, policy) && transactionTaxCode && transactionTaxAmount;

As result we show 0 like text(For web) instead hide this element
For native we get crash

{shouldShowTax && (
<OfflineWithFeedback pendingAction={getPendingFieldAction('taxCode')}>
<MenuItemWithTopDescription
title={taxRateTitle ?? ''}
description={taxRatesDescription}
interactive={canEdit}
shouldShowRightIcon={canEdit}
titleStyle={styles.flex1}
onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.TAX_RATE))}
/>
</OfflineWithFeedback>
)}

{shouldShowTax && (
<OfflineWithFeedback pendingAction={getPendingFieldAction('taxAmount')}>
<MenuItemWithTopDescription
title={formattedTaxAmount ? formattedTaxAmount.toString() : ''}
description={translate('iou.taxAmount')}
interactive={canEdit}
shouldShowRightIcon={canEdit}
titleStyle={styles.flex1}
onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.TAX_AMOUNT))}
/>
</OfflineWithFeedback>
)}

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

To fix this issue we can we can convert this value to boolean (!!shouldShowTax)

{shouldShowTax && (
<OfflineWithFeedback pendingAction={getPendingFieldAction('taxCode')}>
<MenuItemWithTopDescription
title={taxRateTitle ?? ''}
description={taxRatesDescription}
interactive={canEdit}
shouldShowRightIcon={canEdit}
titleStyle={styles.flex1}
onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.TAX_RATE))}
/>
</OfflineWithFeedback>
)}

{shouldShowTax && (
<OfflineWithFeedback pendingAction={getPendingFieldAction('taxAmount')}>
<MenuItemWithTopDescription
title={formattedTaxAmount ? formattedTaxAmount.toString() : ''}
description={translate('iou.taxAmount')}
interactive={canEdit}
shouldShowRightIcon={canEdit}
titleStyle={styles.flex1}
onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.TAX_AMOUNT))}
/>
</OfflineWithFeedback>
)}

But it's a little unclear here
Should we hide the ability to select taxes when we have chosen the default value (0)?

Because in this case we have a slightly incorrect condition that hides the item menu when the tax value is 0

const shouldShowTax = isTaxPolicyEnabled(isPolicyExpenseChat, policy) && transactionTaxCode && transactionTaxAmount;

What alternative solutions did you explore? (Optional)

NA

@MonilBhavsar MonilBhavsar self-assigned this Apr 4, 2024
@MonilBhavsar
Copy link
Contributor

Looking

@jasperhuangg
Copy link
Contributor

PR merged ^ CPing now

@srikarparsi srikarparsi added the Bug Something is broken. Auto assigns a BugZero manager. label Apr 4, 2024
Copy link

melvin-bot bot commented Apr 4, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 4, 2024
@srikarparsi
Copy link
Contributor

Hey @johncschuster, do you think you can help with paying @allroundexperts for reviewing this PR?

@jasperhuangg
Copy link
Contributor

CP request here

@jasperhuangg
Copy link
Contributor

CP running

@jasperhuangg
Copy link
Contributor

Confirmed the crash was fixed on staging, nice work @MonilBhavsar!

@MonilBhavsar MonilBhavsar removed the DeployBlockerCash This issue or pull request should block deployment label Apr 5, 2024
@MonilBhavsar
Copy link
Contributor

Thanks Jasper! Removing blocker label

@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Apr 5, 2024
@melvin-bot melvin-bot bot changed the title Taxes - App crashes when changing tax rate to 0% on native apps and it shows 00 on web [$250] Taxes - App crashes when changing tax rate to 0% on native apps and it shows 00 on web Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01738457c70add314d

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

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

@johncschuster johncschuster removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 5, 2024
@johncschuster johncschuster changed the title [$250] Taxes - App crashes when changing tax rate to 0% on native apps and it shows 00 on web [$500] Taxes - App crashes when changing tax rate to 0% on native apps and it shows 00 on web Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

Upwork job price has been updated to $500

@johncschuster
Copy link
Contributor

Changing the bounty to $500, as it was issued prior to 2024-04-05.

@johncschuster
Copy link
Contributor

@allroundexperts I've invited you to the job, here. Can you accept that and ping me when you've done it so I can issue payment? Thank you!

@allroundexperts
Copy link
Contributor

Thanks @johncschuster, but I get paid through NewDot. Can you please write a payment summary instead? Thanks!

Copy link

melvin-bot bot commented Apr 19, 2024

@johncschuster, @allroundexperts, @MonilBhavsar, @srikarparsi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Apr 23, 2024

@johncschuster, @allroundexperts, @MonilBhavsar, @srikarparsi 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

Copy link

melvin-bot bot commented Apr 25, 2024

@johncschuster, @allroundexperts, @MonilBhavsar, @srikarparsi 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

Copy link

melvin-bot bot commented Apr 29, 2024

@johncschuster, @allroundexperts, @MonilBhavsar, @srikarparsi 12 days overdue now... This issue's end is nigh!

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels May 2, 2024
Copy link

melvin-bot bot commented May 2, 2024

This issue has not been updated in over 14 days. @johncschuster, @allroundexperts, @MonilBhavsar, @srikarparsi eroding to Weekly issue.

@johncschuster
Copy link
Contributor

Payment Summary

C+ Reviewer - @allroundexperts - $500 to be paid via Manual Request

@JmillsExpensify
Copy link

$500 approved for @allroundexperts

@srikarparsi
Copy link
Contributor

Is this good to close out @johncschuster?

@MonilBhavsar
Copy link
Contributor

Looks like everyone is paid out, so we can close this one.

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