[CHECKLIST] [$250] NetSuite - "Subsidiary" shows while the connection is syncs for the first time

[lanitochka17]

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: 9.0.26-1
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4901046&group_by=cases:section_id&group_order=asc&group_id=314239
Issue reported by: Applause - Internal Team

Action Performed:

1. Open the app
2. Log in with a new Gmail account
3. Click on FAB - New workspace
4. Enable "Accounting" in the "More features" page.
5. Navigate to "Accounting"
6. Connect to NetSuite and upgrade the workspace to Control when asked
7. Wait for the sync to finish

Expected Result:

"Subsidiary" should only show after the sync finished

Actual Result:

"Subsidiary" shows while the connection syncs for the first time

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

Bug6586205_1724916783236.bandicam_2024-08-29_09-23-05-717.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a057b4870b92dd2e
• Upwork Job ID: 1829528097305498004
• Last Price Increase: 2024-08-30
• Automatic offers:
• ikevin127 | Reviewer | 103803952
• nyomanjyotisa | Contributor | 103803953

Issue Owner

Current Issue Owner: @garrettmknight

[melvin-bot]

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

[lanitochka17]

@garrettmknight 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

[lanitochka17]

We think that this bug might be related to #wave-control

[Nodebrute]

Proposal

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

"Subsidiary" shows while the connection is syncs for the first time

What is the root cause of that problem?

When the sync is in progress we don't hide integrationSpecificMenuItems

App/src/pages/workspace/accounting/PolicyAccountingPage.tsx

Line 319 in e77f204

...(isEmptyObject(integrationSpecificMenuItems) || shouldShowSynchronizationError || isEmptyObject(policy?.connections) ? [] : [integrationSpecificMenuItems]),

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

We should hide menuItems when sync is in progress. Add isSyncInProgress here

App/src/pages/workspace/accounting/PolicyAccountingPage.tsx

Line 319 in e77f204

...(isEmptyObject(integrationSpecificMenuItems) || shouldShowSynchronizationError || isEmptyObject(policy?.connections) ? [] : [integrationSpecificMenuItems]),

(isEmptyObject(integrationSpecificMenuItems) || shouldShowSynchronizationError || isEmptyObject(policy?.connections) || isSyncInProgress ? [] : [integrationSpecificMenuItems])

What alternative solutions did you explore? (Optional)

[nyomanjyotisa]

Proposal

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

NetSuite - "Subsidiary" shows while the connection is syncs for the first time

What is the root cause of that problem?

We display the "Subsidiary" although there is no value to display for the Subsidiary

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

We should return empty object if there is no value for Subsidiary, like we do for the sage intact entity

App/src/pages/workspace/accounting/PolicyAccountingPage.tsx

Lines 174 to 178 in e77f204

	case CONST.POLICY.CONNECTIONS.NAME.SAGE_INTACCT:
	return !sageIntacctEntityList.length
	? {}
	: {
	description: translate('workspace.intacct.entity'),

Update this code to the following

return !policy?.connections?.netsuite?.options?.config?.subsidiary 
  ? {} 
  : {
      description: translate('workspace.netsuite.subsidiary'),
      ...
  };

additionally, we can do the same for Xero Organization Name

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

@Nodebrute Thank you for the proposal. While the observation mentioned in your proposal's RCA is not wrong, when it comes to the solution I think it's best to only hide integrationSpecificMenuItems specific sections if there's no information to be displayed for a specific section instead of hiding all sections based on isSyncInProgress.

The reasoning would be that if let's say isSyncInProgress is false (sync finished) but there's no data returned for that specific section -> then the UI will still show the empty Subsidiary section in our case.

[ikevin127]

@nyomanjyotisa's proposal looks good to me. The RCA is correct and the proposed solution fixes the issue as per the Expected result, following the code pattern we already use in the SageIntacct case.

PR Note: We should keep this within scope and only fix the NetSuite - Subsidiary issue by using the !netSuiteSubsidiaryList?.length ? {} : {... check, similar (but reverse) to what is used in the onPress to return early (see SageIntacct case model).

additionally, we can do the same for Xero Organization Name

cc @Beamanator What do you think about the above PR Note regarding keeping the fix for this issue within scope, do you agree or think we should extend the fix to handle the Xero - Organization case as well ?

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

@garrettmknight, @Beamanator, @ikevin127 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[ikevin127]

Currently discussing (scroll down from this comment) what to do with NetSuite related issues when it comes to people who don't have credentials, this includes all Contributors and all but one C+.

Quoting:

I could argue that the change / fix is obvious to the point where one wouldn't even need the credentials to test - but I understand that that's not not how we do things.

If I don't get access to the credentials soon (this week), I guess there's no other option then to have you take over.

C+ w/ Netsuite credentials said:
And contributors will not have access to these credentials anytime soon

^ Not sure what happens to the contributors which posted good proposals on these issues given they will never have the credentials to test the changes if they are assigned and have to complete their PR Author Checklist :think0:

cc @Beamanator for context.

[melvin-bot]

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @nyomanjyotisa 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[Beamanator]

@ikevin127 - honestly i think the solution is straightforward enough to also apply the Xero fix

[ikevin127]

⚠️ Automation failed here -> this should be on [HOLD for Payment 2024-09-20] according to 5 days ago's production deploy from #48519 (comment).

cc @garrettmknight

[garrettmknight]

Payment Summary:

• Contributor: @nyomanjyotisa $250 via upwork
• Reveiwer: @ikevin127 $250 via upwork

@ikevin127 can you complete the checklist?

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: [#Wave-Control: Add NetSuite]: Top-level Subsidiary selection #44038.
• [@ikevin127] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/44038/files#r1772405331.
• [@ikevin127] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. Enable "Accounting" in the "More features" page of a Workspace.
2. Navigate to the "Accounting" page.
3. Connect to NetSuite and upgrade the workspace to Control when asked.
4. Verify that 'Subsidiary' does not show while the connection is synchronizing for the first time, but only after the connection has successfully synced.

Do we agree 👍 or 👎.

[melvin-bot]

@garrettmknight @Beamanator @ikevin127 @nyomanjyotisa this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[ikevin127]

BZ Checklist completed, awaiting for BZ team member to close the issue.

[melvin-bot]

@garrettmknight, @Beamanator, @ikevin127, @nyomanjyotisa Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Beamanator]

Assigning back to @garrettmknight for payment 🙏