-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Due for payment 2025-03-05] [$250] Netsuite - App crashes when tapping on export settings after disconnecting with another device #55723
Comments
Triggered auto assignment to @michaelkwardrop ( |
🚨 Edited by proposal-police: This proposal was edited at 2025-01-24 15:53:46 UTC. ProposalPlease 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, App/src/pages/workspace/accounting/netsuite/export/NetSuitePreferredExporterSelectPage.tsx Line 26 in 0de8f42
What changes do you think we should make in order to solve the problem?Add optional chaining Optionally, we can mark each connection within the Line 1361 in f2fc96d
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 |
Job added to Upwork: https://www.upwork.com/jobs/~021882822298698496668 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ahmedGaber93 ( |
ProposalPlease 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: According the log, it has error in this line App/src/pages/workspace/accounting/netsuite/export/NetSuitePreferredExporterSelectPage.tsx Line 26 in b59f23b
Because when we disconnect Netsuite in 2nd device, 2. App doesn't show "Not here" page in the export settings page The reason is in App/src/components/ConnectionLayout.tsx Line 110 in b59f23b
However, we don't subscribe to Onyx of policy, but we get the policy in memory here App/src/components/ConnectionLayout.tsx Line 109 in b59f23b
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: App/src/pages/workspace/accounting/netsuite/export/NetSuitePreferredExporterSelectPage.tsx Line 26 in b59f23b
We should fix it for other pages as well 2. App doesn't show "Not here" page in the export settings page App/src/pages/workspace/withPolicy.tsx Line 82 in b59f23b
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) |
Thanks all for the proposals. Expected result:
@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? 20250126165838387.mp4 |
@ahmedGaber93 could you test that issue again? I'm unable to reproduce it. I guess the remove connection failed or was unsuccessful. Screen.Recording.2025-01-27.at.22.22.38.mov |
Hmm! I am also not able to reproduce, let's move forward and retest it on the PR. 20250127204437078.mp4 |
@hoangzinh's proposal LGTM! 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Looks good to me too! |
This issue has not been updated in over 15 days. @dangrous, @hoangzinh, @ahmedGaber93, @michaelkwardrop eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.6-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2025-03-05. 🎊 For reference, here are some details about the assignees on this issue:
|
@hoangzinh / @ahmedGaber93 @michaelkwardrop @hoangzinh / @ahmedGaber93 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button] |
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: 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): [email protected]
Issue reported by: Applause Internal Team
Device used: mWeb/Chrome, iphone 13/iOS 18.2.1
App Component: Other
Action Performed:
Prerequisite
Expected Result:
Not here page is displayed and the app doesn't crash
Actual Result:
App crashes
Workaround:
Unknown
Platforms:
Screenshots/Videos
bug.mp4
bug.txt
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @michaelkwardropThe text was updated successfully, but these errors were encountered: