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
3.34k stars 2.77k forks source link

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

Open lanitochka17 opened 3 weeks ago

lanitochka17 commented 3 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: 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?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/464139e1-70a0-4d78-85ec-f0570e962eab

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 OwnerCurrent Issue Owner: @ikevin127
melvin-bot[bot] commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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

Nodebrute commented 3 weeks ago

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 https://github.com/Expensify/App/blob/e77f20458ad13d94c7b0188acc2ed1a91bd8ccbd/src/pages/workspace/accounting/PolicyAccountingPage.tsx#L319

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 https://github.com/Expensify/App/blob/e77f20458ad13d94c7b0188acc2ed1a91bd8ccbd/src/pages/workspace/accounting/PolicyAccountingPage.tsx#L319

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

What alternative solutions did you explore? (Optional)

nyomanjyotisa commented 2 weeks ago

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 https://github.com/Expensify/App/blob/e77f20458ad13d94c7b0188acc2ed1a91bd8ccbd/src/pages/workspace/accounting/PolicyAccountingPage.tsx#L174-L178

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[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

ikevin127 commented 2 weeks ago

@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 commented 2 weeks ago

@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[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

ikevin127 commented 2 weeks ago

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[bot] commented 2 weeks ago

πŸ“£ @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[bot] commented 2 weeks ago

πŸ“£ @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 commented 2 weeks ago

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

ikevin127 commented 16 hours ago

⚠️ Automation failed here -> this should be on [HOLD for Payment 2024-09-20] according to 5 days ago's production deploy from https://github.com/Expensify/App/pull/48519#issuecomment-2350595614.

cc @garrettmknight