[HOLD for payment 2024-09-10] [$250] IOU - Message and Notification Swapping in IOU When Transitioning from Offline to Online

[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.13-3
Reproducible in staging?: Y
Reproducible in production?: Y
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): betlihemasfaw14@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to Workspace Chat > Plus Sign > Submit Expense
2. Add amount and merchant, then confirm
3. Go to IOU, switch to offline mode, and generate a notification by changing the date
4. Write a message, then switch back to online mode. Notice the message and notification swap

Expected Result:

Messages and notifications should remain in place

Actual Result:

Messages and notifications swap in IOU when translated from offline to online

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

Bug6551162_1721788179924.Screen_Recording_2024-07-23_at_7.11.05_PM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017fdfd49bd9346b4c
• Upwork Job ID: 1818663278050494650
• Last Price Increase: 2024-08-14
• Automatic offers:
• nkdengineer | Contributor | 103525574

Issue Owner

Current Issue Owner: @greg-schroeder

[melvin-bot]

Triggered auto assignment to @greg-schroeder (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]

@greg-schroeder 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

[nyomanjyotisa]

Proposal

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

Message and Notification Swapping in IOU When Transitioning from Offline to Online

What is the root cause of that problem?

The root cause of the problem is that the API call to update the date modifies the created value, which subsequently rearranges the positions of report actions. This occurs because the current sorting mechanism does not prioritize report actions with pendingAction equal to add.

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

Updating the sorting logic to ensure that report actions with pendingAction equals to 'add' are always positioned in the first of the list. This change will prevent the reordering of these actions due to API updates on the created value if there are some pending add actions left

Add the following code here

        if (first.pendingAction === 'add' && second.pendingAction !== 'add') {
            return 1  * invertedMultiplier;
        } else if (first.pendingAction !== 'add' && second.pendingAction === 'add') {
            return -1  * invertedMultiplier;
        }

What alternative solutions did you explore? (Optional)

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-08-08 10:23:22 UTC.

Proposal

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

Messages and notifications swap in IOU when translated from offline to online

What is the root cause of that problem?

After we update the money request, we call saveUpdateInformation here making the queue paused here

App/src/libs/Middleware/SaveResponseInOnyx.ts

Line 38 in 00d2c42

OnyxUpdates.saveUpdateInformation(responseToApply);

Then when the queue is unpaused, we merge the pending Onyx data from the response API to Onyx meanwhile other write APIs are not complete. created from BE is larger than other report actions makes the order change.

App/src/libs/Network/SequentialQueue.ts

Line 179 in 00d2c42

flushOnyxUpdatesQueue();

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

We can remove the flushOnyxUpdatesQueue here then we should update here to only update the Onyx data if the queue is empty because when the queue is paused, the process promise will be resolved immediately

if (PersistedRequests.getAll().length === 0) {
  flushOnyxUpdatesQueue();
}

App/src/libs/Network/SequentialQueue.ts

Line 161 in 9c7c4a1

flushOnyxUpdatesQueue();

What alternative solutions did you explore? (Optional)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017fdfd49bd9346b4c

[melvin-bot]

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

[Ollyws]

@nkdengineer

We should check if the numberOfPersistedRequests is not 0 we will not merge the Onyx update to Onyx DB when the queue is unpaused

Hmm won't this have the effect of no Onyx updates being applied until ALL of the API requests are finished?
I imagine that could have alot of potential negative consequences as delay in a single request would stop all Onyx updates from other pending requests being applied.

[nkdengineer]

Hmm won't this have the effect of no Onyx updates being applied until ALL of the API requests are finished?

@Ollyws Yes, currently if the queue isn't unpause, we still wait until ALL of write API are finished to merge the Onyx update here.

App/src/libs/Network/SequentialQueue.ts

Line 161 in 207365c

flushOnyxUpdatesQueue();

[Ollyws]

@nkdengineer It seems that after adding if (numberOfPersistedRequests === 0) { that flushOnyxUpdatesQueue() will never be called in any circumstances, or am I missing something?

[nkdengineer]

@Ollyws it will be called here after all write APIs are complete.

App/src/libs/Network/SequentialQueue.ts

Line 161 in 207365c

flushOnyxUpdatesQueue();

[Ollyws]

Yes so it's effectively the same as removing flushOnyxUpdatesQueue() from the unpause() function, no?

[nkdengineer]

@Ollyws Yeah, I think it's the same.

[Ollyws]

Hmm, it was specifically added this way in #25455, although I'm not exactly sure WHY we're running flushOnyxUpdatesQueue() before flush()...
It seems a little redundant but perhaps I'm missing something.

[nkdengineer]

Let me check this again.

[nkdengineer]

@Ollyws Confirmed in Slack here. We can move forward with this solution.

[melvin-bot]

@Ollyws, @greg-schroeder Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Ollyws]

Thanks for opening a discussion, I'm just considering any adverse effects and will make a decision.

[melvin-bot]

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

[Ollyws]

I'm still getting essentially the same problem even with flushOnyxUpdatesQueue() removed:

Screen.Recording.2024-08-08.at.11.59.09.mov

[Ollyws]

Actually I think it was a problem on my end, seems to be working fine now.

[Ollyws]

Let's move forward with @nkdengineer's proposal.
🎀👀🎀 C+ reviewed

[melvin-bot]

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

[Beamanator]

Huh, @tgolen since you added some comments and concerns about the proposal in this issue, did you want to be assigned to review as well? 🤷 I'm happy to, just checking before I move forward here

[tgolen]

Sure, I wouldn't mind keeping an eye on this one. I'll swap you out

[greg-schroeder]

Okay, thanks @tgolen! Are you able to confirm the contributor assignment? #46393 (comment)

[melvin-bot]

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

[melvin-bot]

📣 @nkdengineer 🎉 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 📖

[tgolen]

OK, yeah. Getting it assigned now. Let's not forget to test this with an ad-hoc build.

[greg-schroeder]

PR is still in review - seems QA identified several issues that'll need to be addressed

[tgolen]

I'm checking with Applause to have those found issues verified against staging to see if they are happening in our main branch of code. If they are reproducible in staging, we can go ahead and merge the PR. If they are not, then @nkdengineer will need to investigate them more.

[tgolen]

It looks like the PR has been merged and deployed to production, so I think this is just waiting on the regression period and payments @greg-schroeder.

[greg-schroeder]

Ah, I guess it didn't get picked up by the automation. 🤔

[greg-schroeder]

Regression period end 2024-09-10

[greg-schroeder]

Payment summary:

Contributor: @nkdengineer - $250 - Paid via Upwork
C+: @Ollyws - $250 - You can make a manual request via ND for the reviewer role

[Ollyws]

Requested in ND.

[garrettmknight]

$250 approved for @Ollyws