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.32k stars 2.75k forks source link

[$250] QBO - The toggles are enabled and disabled twice in Advanced settings #45201

Closed lanitochka17 closed 2 weeks ago

lanitochka17 commented 2 months 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.6-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4706857 Issue reported by: Applause - Internal Team

Action Performed:

Precondition QBO connection is established in the workspace

  1. Navigate to workspace settings > Accounting
  2. Click on Advanced
  3. Enable and disable the toggles of the items a few times
  4. Observe how the toggles are behaving after enabling and disabling a few times

Expected Result:

Enabling and disabling occurs only once

Actual Result:

Toggles are enabled and disabled twice when the action is performed only once

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/1fc98280-3c82-4971-a2b4-748c23dd6281

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01123cce366aa8f37d
  • Upwork Job ID: 1811816997002609504
  • Last Price Increase: 2024-08-08
  • Automatic offers:
    • ahmedGaber93 | Reviewer | 103330245
Issue OwnerCurrent Issue Owner: @aldo-expensify
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @zanyrenney (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 2 months ago

@zanyrenney 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 2 months ago

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

zanyrenney commented 2 months ago

I am OOO until Tuesday 16th. This was assigned after my working day, I am readding the bug label.

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @puneetlath (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 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

ahmedGaber93 commented 1 month ago

Waiting on proposals

zanyrenney commented 1 month ago

I'm back @puneetlath so taking this one back! thanks

ahmedGaber93 commented 1 month ago

No proposals yet

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

@ahmedGaber93, @zanyrenney Huh... This is 4 days overdue. Who can take care of this?

ahmedGaber93 commented 1 month ago

No proposals yet

melvin-bot[bot] commented 1 month ago

@ahmedGaber93 @zanyrenney this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

dangrous commented 1 month ago

Question about this - do the toggles eventually end up in the right position? Like, even if they're sort of moving on their own at the end, if the last thing the user tried to do was turn them off, they end up off, and vice versa?

I'm asking because my guess is that each time we update from the backend we send the full settings object and so if we're trying to update a lot of things at once, it might not be fully updated on QBO's side yet, but eventually will get there... but if it's not ending up in the preferred state then there might be something else going on.

This will definitely be affected by us refactoring into more individual commands, but I'm not sure how haha

ahmedGaber93 commented 1 month ago

do the toggles eventually end up in the right position?

@dangrous Not always.

Steps I tested it.

  1. enable "Invite employees"
  2. enable "Auto-create entities"
  3. disable "Invite employees"
  4. disable "Auto-create entities"
  5. enable "Invite employees"

The final result is different every time I tested it.

videos https://github.com/user-attachments/assets/94cbb600-43d7-4c5c-b494-0798b0bb6179 https://github.com/user-attachments/assets/8ef40339-da64-46b6-b963-b2c03d360972
melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

rezkiy37 commented 1 month ago

Hi, I’m Michael (Mykhailo) from Callstack and I would like to work on this issue.

ahmedGaber93 commented 1 month ago

@dangrous do you work on fix for this from BE side? or we can move forward with @rezkiy37?

rezkiy37 commented 1 month ago

I need to investigate the update's optimistic, success, and failure data. I guess it could conflict with the backend response.

ahmedGaber93 commented 1 month ago

Yeah, Also you can check sync request behavior, do we send sync request after every change?

rezkiy37 commented 1 month ago

Guys, how to connect the debug QBO connection properly? Does it work on the dev?

cc @lanitochka17 @ahmedGaber93 @zanyrenney

ahmedGaber93 commented 1 month ago

@rezkiy37 Yest, it works on dev. You just need to remove .dev from URL after click connect and complete the steps

https://github.com/user-attachments/assets/80e6450e-d196-4849-ad5f-716520205896

rezkiy37 commented 1 month ago

@ahmedGaber93, do you have testing accounts in QBO?

It is not free for me

Screenshot 2024-07-29 at 16 47 07

ahmedGaber93 commented 1 month ago

@rezkiy37 Go to https://quickbooks.intuit.com/global/pricing/# and use start free trial, then follow https://github.com/Expensify/App/issues/45201#issuecomment-2256100944 to connect

Screenshot 2024-07-29 at 8 24 24 AM

rezkiy37 commented 1 month ago

Actively working on the issue. I am trying to figure out what invokes those updates. I compare it with the "More features" page, which works properly.

dangrous commented 1 month ago

Hm yeah let us know what you figure out @rezkiy37 - it feels like this will likely have a backend component as well but I think sorting out the front end will help us confirm that. I'll assign you!

melvin-bot[bot] commented 1 month ago

📣 @ahmedGaber93 🎉 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

rezkiy37 commented 1 month ago

Actively working on the issue. I have 3 places to investigate why the bug happens:

  1. The difference for those toggles is invoking GetMissingOnyxMessages once Pusher sends updates regarding changes. OnyxUpdates.apply() handles GetMissingOnyxMessages's response . Looks like right after this response the toggles are going crazy.
  2. The OnyxUpdates.applyPusherOnyxUpdates() updates Onyx state because Pusher sends progress status messages.
  3. withPolicyConnections.tsx is not complex, but probably it has extra rerenders.
rezkiy37 commented 1 month ago

Other tasks occupied me today. Will continue tomorrow.

rezkiy37 commented 1 month ago

I've investigated the problem. So the bug happens because of GetMissingOnyxMessages. This request calls after each click on the toggles. It has huge responses sometimes and invokes redundant intermediate updates. I am attaching a video example of skipping the command's responses and ignoring their updates. So definitely we need a backend engineer to reduce the amount of calls and probably batch into one instead of ten updates.

Details https://github.com/user-attachments/assets/8b424368-d5c7-42e4-95a3-0d1acc3c6c8a

cc @dangrous @ahmedGaber93

zanyrenney commented 1 month ago

Do you mean one of the internal Expensify team?

rezkiy37 commented 1 month ago

@zanyrenney Yes, I do.

rezkiy37 commented 1 month ago

There is this note https://github.com/Expensify/App/issues/45201#issuecomment-2258938420 as well.

ahmedGaber93 commented 1 month ago

Not overdue, we still discuss the Potential solution.

dangrous commented 1 month ago

So we're working on refactoring the QBO API commands here (internal only link sorry!) into individual commands - e.g. instead of UpdatePolicyConnectionConfiguration we'll call UpdateQuickbooksOnlineSyncCustomers or UpdateQuickbooksOnlineEnableNewCategories or UpdateXeroAutoSync or whatever. Do we think that will help at all?

And to clarify, too - when you say "we need a backend engineer to reduce the amount of calls and probably batch into one instead of ten updates" - are you referring to the number of calls inside GetMissingOnyxMessages? or inside the QBO commands?

Lastly, cc @aldo-expensify since you did some more of the actual backend implementation here and might have some ideas.

aldo-expensify commented 1 month ago

ahhhhhhh, I wonder if the multiple GetMissingOnyxMessages we see is because of:

Would have to investigate if this is true ^

rezkiy37 commented 1 month ago

And to clarify, too - when you say "we need a backend engineer to reduce the amount of calls and probably batch into one instead of ten updates" - are you referring to the number of calls inside GetMissingOnyxMessages? or inside the QBO commands?

I mean the number of calls inside GetMissingOnyxMessages.

rezkiy37 commented 1 month ago

...and the front end goes a bit crazy with the onyx updates arriving not in perfect order.

Exactly!

ahmedGaber93 commented 1 month ago

Not overdue, we still discuss the Potential solution.

melvin-bot[bot] commented 1 month ago

@ahmedGaber93 @zanyrenney @rezkiy37 this issue is now 4 weeks old, please consider:

Thanks!

ahmedGaber93 commented 1 month ago

Not overdue, we still discuss the Potential solution.

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

ahmedGaber93 commented 4 weeks ago

Would have to investigate if this is true ^

@aldo-expensify related to frontend part, me and @rezkiy37 agree with that. What is the potential solution then?

and the front end goes a bit crazy with the onyx updates arriving not in perfect order.

aldo-expensify commented 4 weeks ago

I tested switching fast the toggles and I confirmed that the sync IS jobs are running in parallel, so I think this guess is correct

What is the potential solution then?

I'm not sure. I imagine that the best would be something like:

Prevent running multiple sync jobs in parallel by having a logic like: If a configuration changes while a sync job is already running, a new sync job should be queued to run after, but if there is already a sync job queued, don't queue a second one

I'm not sure if this is feasible or not. @francoisl do you know if we can achieve something like this?

cc @dangrous @lakchote since this affects all our integrations (not only QBO)

aldo-expensify commented 4 weeks ago

Removing External for now, since this seems like something we need to fix in the backend

francoisl commented 4 weeks ago

Yeah this is something I pointed out was going to happen in the NetSuite design doc, but it was decided to continue with the current design.

I think what you'd need here is to queue a non-repeating, unique:true bedrock job that takes care of starting the IS accounting sync.

francoisl commented 4 weeks ago

Also to clarify:

If a configuration changes while a sync job is already running, a new sync job should be queued to run after, but if there is already a sync job queued, don't queue a second one

Were you referring to bedrock jobs? As far as I can tell, the API commands to modify QBO settings don't queue bedrock jobs at the moment, right? I also can't think of a way to do this without bedrock jobs with our current architecture, because 2 API requests to update the QBO configuration could be processed by different webservers & IS servers, that don't know anything about each other.

aldo-expensify commented 4 weeks ago

Were you referring to bedrock jobs?

Nop, I was just talking about IS jobs, and I didn't know if it was possible to "queue" or avoid duplication. What you suggested about using bedrock jobs makes sense to me!

aldo-expensify commented 4 weeks ago

I'll try to tackle this implementing a bedrock job.