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

QBO - Connection options are missing in QBO connection #46445

Open lanitochka17 opened 1 month ago

lanitochka17 commented 1 month 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.14-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: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Log in to the app with a new expensifail account
  2. Create a new workspace
  3. Go to more features > enable accounting
  4. Navigate to the accountings page and connect with QBO
  5. When you are navigated to the QBO page enter the credentials and finish the connection
  6. After connecting with QBO, you will be navigated to the app
  7. Wait for the connection to sync

Expected Result:

QBO options (Import, Export, Card reconciliation and Advanced options) are displayed

Actual Result:

QBO options (Import, Export, Card reconciliation, and Advanced options) are not displayed

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/68e150fa-1694-4b1f-b1b5-1345f6db5416

View all open jobs on GitHub

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @tgolen (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 1 month ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
lanitochka17 commented 1 month ago

We think that this bug might be related to #wave-collect - Release 1

tgolen commented 1 month ago

@kosmydel @hungvu193 @arosiclair Is it possible this is related to https://github.com/Expensify/App/pull/44733 ?

arosiclair commented 1 month ago

I don't think so. Those changes should only affect report exports.

chiragsalian commented 1 month ago

Not a web-e blocker as discussed here

tgolen commented 1 month ago

Sounds like @yuwenmemon might know what's happened here and he will be looking into it.

yuwenmemon commented 1 month ago

Weird, not able to reproduce this in Dev

yuwenmemon commented 1 month ago

Oh I see it now. @NikkiWines it looks like this is because we initially set the lastSync isConnected property to false: https://github.com/Expensify/Integration-Server/commit/a4cca1bb4863dfcbe9c566cbeeaa47d5676c8c72#diff-49d9c0cf9775a8c0d9890e99f5085ddbed36dbe92d1d8ec3a3956e50d5d9a555R27

yuwenmemon commented 1 month ago

I think this will need to be fixed via the IS.

melvin-bot[bot] commented 1 month ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

ShridharGoel commented 1 month ago

Proposal

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

Connection options are missing in QBO connection

What is the root cause of that problem?

There's a check to skip showing the options if a connection is unverified. For QBO, that will depend on lastSync?.isConnected.

https://github.com/Expensify/App/blob/1a3c334fc4eafbc887807035a7ac4dc9461d7348/src/libs/actions/connections/index.ts#L374-L382

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

We can just check if lastSync object is not-null (or some other field).

yuwenmemon commented 1 month ago

@roryabraham This should be fixed when https://github.com/Expensify/Integration-Server/pull/8025 is deployed to prod.

roryabraham commented 1 month ago

tester was no longer able to reproduce, so I'm going to close this and check it off

lanitochka17 commented 1 week ago

Issue is still reproducible on the latest build 9.0.29-9

https://github.com/user-attachments/assets/6496f751-a381-4ee1-a15a-31b4f482bf74

twisterdotcom commented 1 week ago

I still cannot recreate this, I'm not clear how to follow the steps in the video without always having to configure the company you choose in QBO when you authenticate. It seems you never had to do that @lanitochka17?

Also this bug states that it occurs on MacOS: Chrome / Safari and Android: mWeb Chrome, but your video seems to show Desktop. Does this only happen for you on Desktop?

https://github.com/user-attachments/assets/5a415aef-1020-45c9-a61a-525b5b18fa4b

I'm going to downgrade this to Daily as I can't reliably reproduce and the mainline case of connecting in Chrome works fine.

melvin-bot[bot] commented 1 week ago

Current assignee @twisterdotcom is eligible for the Bug assigner, not assigning anyone new.

yuwenmemon commented 1 week ago

I wasn't able to reproduce this exactly in Desktop, but I was able to see a split second here where we do see this behavior, but eventually, the menu options load.

https://github.com/user-attachments/assets/f4192109-c7e0-47e0-b25f-f67338df00ad

yuwenmemon commented 1 week ago

What's tipping me off is that we see Last synced in both videos where this happens, which means the lastSync object or something we're reading off of it is off.

yuwenmemon commented 1 week ago

So yeah for when we see Last synced all I see in the connection object is this: Screenshot 2024-09-06 at 2 05 21 PM

However, a few seconds later an onyxUpdate is pushed updating the connection and showing the full connection: Screenshot 2024-09-06 at 2 04 12 PM

What looks to be happening is that the jobDone event is getting sent before the policy update that updates the lastSync value. cc @aldo-expensify

yuwenmemon commented 1 week ago

Yup, that's exactly it. If I play around with the debugger to delay the jobDone notification this is what I see:

https://github.com/user-attachments/assets/a9f93430-01c1-49b7-bc93-5fb4f21547b3

yuwenmemon commented 1 week ago

@francoisl @aldo-expensify Not sure what the best way to fix this is 🤔

It's kind of a race condition and sits at the heart of how the new connection with Onyx updates works. A few ideas

  1. Delay the jobDone notification by 1 second - kind of hacky, doesn't solve the root of the problem
  2. Somehow, redundantly push the entire connectionData with the jobDone event. This way even if the onyx update from setting the connection data via the API doesn't come through, we send it directly from the IS.
  3. Do something in the front-end where we show "Loading" state or something like that if we don't have full connectionData for a connection yet.
aldo-expensify commented 1 week ago

hmm what about passing some flag from the IS to the last SavePolicy so that last SavePolicy pushes the jobDone instead of the IS?

This would be to ensure the right order of the events

yuwenmemon commented 1 week ago

Oooooooh, I like that 💡

@francoisl does that sound good to you?

francoisl commented 1 week ago

But even if we do that, can a command in either Auth or PHP know when the Onyx updates queued by Auth have been sent?

yuwenmemon commented 1 week ago

Hmmm.. yeah. In theory, you would want to do this when you save the connection data, so that there's no race condition between the connection object having a config/data and the spinner stopping (jobDone). However, there are other sync tasks that could still be running after that.

aldo-expensify commented 1 week ago

However, there are other sync tasks that could still be running after that.

but I think the user wouldn't be able to change any setting until the jobDone is received, right? If the user can't touch any setting until that, then there won't be more syncs running.