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

[$250] Taxes - App crashes when trying to disable tax rate. #56158

Open
4 of 8 tasks
IuliiaHerets opened this issue Jan 31, 2025 · 8 comments
Open
4 of 8 tasks

[$250] Taxes - App crashes when trying to disable tax rate. #56158

IuliiaHerets opened this issue Jan 31, 2025 · 8 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Jan 31, 2025

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: 9.0.93-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, repro on both
If this was caught during regression testing, add the test name, ID and link from TestRail: #55192
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Android and IOS
App Component: Workspace Settings

Action Performed:

  1. Open the app
  2. Create a workspace
  3. Go to more features and enable taxes
  4. Go to taxes and disable Tax Rate 1

Expected Result:

App shouldn't crash

Actual Result:

App crashes

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/0d63e0eb-d327-432e-a895-46c35a23bcce
Bug6729510_1738324887963!log.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021889364840283872581
  • Upwork Job ID: 1889364840283872581
  • Last Price Increase: 2025-02-11
Issue OwnerCurrent Issue Owner: @mollfpr
@IuliiaHerets IuliiaHerets added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 31, 2025
Copy link

melvin-bot bot commented Jan 31, 2025

Triggered auto assignment to @abekkala (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.

@nkdengineer
Copy link
Contributor

Proposal

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

App crashes

What is the root cause of that problem?

In native, after we reload the app and the first time we call setPolicyTaxesEnabled, allPolicies is undefined because the callback here isn't triggered before. So originalTaxes is an empty object

const originalTaxes = {...policy?.taxRates?.taxes};

Then the app crashes here

isDisabled: !!originalTaxes[taxID].isDisabled,

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

We should get the isDisabled safer

isDisabled: !!originalTaxes[taxID]?.isDisabled,

isDisabled: !!originalTaxes[taxID].isDisabled,

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None

What alternative solutions did you explore? (Optional)

NA

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

Copy link

melvin-bot bot commented Feb 4, 2025

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

Copy link

melvin-bot bot commented Feb 6, 2025

@abekkala Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Feb 10, 2025

@abekkala Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@bernhardoj
Copy link
Contributor

Proposal

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

App crashes when trying to disable tax rate after creating a new workspace.

What is the root cause of that problem?

This only happens when we open the tax rate page for the first time. No need to create a new workspace. When we toggle the tax, the policy here is undefined, so the taxes is undefined too.

function setPolicyTaxesEnabled(policyID: string, taxesIDsToUpdate: string[], isEnabled: boolean) {
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`];
const originalTaxes = {...policy?.taxRates?.taxes};

So, when we try to access the tax isDisabled here, it will crash.

isDisabled: !!originalTaxes[taxID].isDisabled,

It's undefined because allPolicies is also undefined. This all happens after #53852. In #53852, we enable inlineRequires, so this top level code is not immediately evaluated when we named import it.

import {clearTaxRateError, deletePolicyTaxes, setPolicyTaxesEnabled} from '@libs/actions/TaxRate';

let allPolicies: OnyxCollection<Policy>;
Onyx.connect({
key: ONYXKEYS.COLLECTION.POLICY,
waitForCollectionCallback: true,
callback: (value) => (allPolicies = value),
});

It will only be evaluated when the imported function is called (doc). But when setPolicyTaxesEnabled is called, the onyx callback hasn't completed yet.

function setPolicyTaxesEnabled(policyID: string, taxesIDsToUpdate: string[], isEnabled: boolean) {
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`];

This is one of the pitfall described on the doc linked above.

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

We have a few options.

First, instead of relying the policy data from Onyx.connect, we can pass the policy from the component because it's already available anyway.

// policy instead of policyID
function setPolicyTaxesEnabled(policy: OnyxEntry<Policy>

// update all usages of policyID with policy.id

Then pass the policy from the component.

setPolicyTaxesEnabled(policyID, [taxID], value);

OR

We can disable inline requires in WorkspaceTaxesPage, but that means other import inline requires are also disabled.

OR

Import TaxRate without any module.

import '@libs/actions/TaxRate'

OR

Create a file that contains the common onyx connection such as policy, import it like above in App.tsx and TaxRate can get the all policies data from that file. or we can get the policy data from PolicyUtils

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

@abekkala abekkala added the External Added to denote the issue can be worked on by a contributor label Feb 11, 2025
@melvin-bot melvin-bot bot changed the title Taxes - App crashes when trying to disable tax rate. [$250] Taxes - App crashes when trying to disable tax rate. Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

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

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 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
Status: No status
Development

No branches or pull requests

5 participants