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
4.03k stars 3.03k forks source link

[$250] Netsuite - App crashes when tapping on export settings after disconnecting with another device #55723

Open vincdargento opened 2 weeks ago

vincdargento commented 2 weeks 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.89-2 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both 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): htad26+ri@gmail.com Issue reported by: Applause Internal Team Device used: mWeb/Chrome, iphone 13/iOS 18.2.1 App Component: Other

Action Performed:

Prerequisite

  1. Sign in to the account with both web and iOS hybrid app
  2. On iOS app, open Accounting > NetSuite > Export (Stay in this page)
  3. On web navigate to Accounting > NetSuite > Three dot option > Disconnect
  4. On iOS app, tap on preferred exporter

Expected Result:

Not here page is displayed and the app doesn't crash

Actual Result:

App crashes

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/d2236e4d-91cb-4add-85ef-758e2a78d510

bug.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021882822298698496668
  • Upwork Job ID: 1882822298698496668
  • Last Price Increase: 2025-01-24
Issue OwnerCurrent Issue Owner: @ahmedGaber93
melvin-bot[bot] commented 2 weeks ago

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

dominictb commented 2 weeks ago

🚨 Edited by proposal-police: This proposal was edited at 2025-01-24 15:53:46 UTC.

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?

After disconnecting, policy.connections.netsuite is null causing the above error.

https://github.com/Expensify/App/blob/0de8f42ed5080ac1ef0acbc3a0d5cb5edc60dc6a/src/pages/workspace/accounting/netsuite/export/NetSuitePreferredExporterSelectPage.tsx#L26

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

Add optional chaining ? to netsuite above.

Optionally, we can mark each connection within the Connections type here as optional:

https://github.com/Expensify/App/blob/f2fc96d0b475f10c017687998eb18c3f353ce44c/src/types/onyx/Policy.ts#L1361

So we can easily check all the places missing optional chaining like the above.

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

N/A

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

hoangzinh commented 2 weeks ago

Proposal

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

After disconnecting with another device:

What is the root cause of that problem?

Based on expected behavior, there are 2 issues here: 1. App crashes when tapping on export settings: Image

According the log, it has error in this line https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/pages/workspace/accounting/netsuite/export/NetSuitePreferredExporterSelectPage.tsx#L26

Because when we disconnect Netsuite in 2nd device, policy?.connections?.netsuite will be undefined, therefore it causes "cannot read properties" error

2. App doesn't show "Not here" page in the export settings page When user in the Export settings page in the 1st device, then disconnect Netsuite in 2nd device, it should show "Not here" page to prevent the user continue access Netsuite setting pages. But it still shows Export settings page.

The reason is in NetSuiteExportConfigurationPage component, we use ConnectionLayout to handle policy connect, and it will show "Not here" page if the connection is null

https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/components/ConnectionLayout.tsx#L110

However, we don't subscribe to Onyx of policy, but we get the policy in memory here https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/components/ConnectionLayout.tsx#L109

Therefore, even when the Netsuite connect is cleared in Onyx, it still shows Export page normally

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

1. App crashes when tapping on export settings: To solve this issue, we should use optional chaining for netsuite?.options here:

https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/pages/workspace/accounting/netsuite/export/NetSuitePreferredExporterSelectPage.tsx#L26

We should fix it for other pages as well

2. App doesn't show "Not here" page in the export settings page In ConnectionLayout component (and SelectionScreen component), we should replace this line by getting policy from Onyx like this

https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/pages/workspace/withPolicy.tsx#L82

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

N/A

What alternative solutions did you explore? (Optional)

ahmedGaber93 commented 2 weeks ago

Thanks all for the proposals.

Expected result:

Not here page is displayed and the app doesn't crash

@dominictb your proposal can fix the crash, but you missed fixing "Not here page is not displayed".

@hoangzinh your proposal cover the two issues and it should fix them. But after going back from "Not here page", the "Acounting" page re-display the "NetSuite" as a connected. Could you please check it?

https://github.com/user-attachments/assets/cc426012-877e-4f1c-a8a0-4b122b3cb4e2

hoangzinh commented 2 weeks ago

@ahmedGaber93 could you test that issue again? I'm unable to reproduce it. I guess the remove connection failed or was unsuccessful.

https://github.com/user-attachments/assets/ea90f07c-3fa9-4c03-a31d-d097a93ace51

ahmedGaber93 commented 2 weeks ago

could you test that issue again? I'm unable to reproduce it. I guess the remove connection failed or was unsuccessful.

Hmm! I am also not able to reproduce, let's move forward and retest it on the PR.

https://github.com/user-attachments/assets/6299d832-79fb-4ccc-8b57-e9da1fd8a00a

ahmedGaber93 commented 2 weeks ago

@hoangzinh's proposal LGTM!

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 weeks ago

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

dangrous commented 2 weeks ago

Looks good to me too!