[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

https://github.com/user-attachments/assets/94716cba-2baf-4928-b738-f1aa03d54c9f

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[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

https://github.com/Expensify/App/blob/00d2c42b4a1d53696e4cb0808ba31fc10028ffcd/src/libs/Middleware/SaveResponseInOnyx.ts#L38

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. https://github.com/Expensify/App/blob/00d2c42b4a1d53696e4cb0808ba31fc10028ffcd/src/libs/Network/SequentialQueue.ts#L179

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();
}

https://github.com/Expensify/App/blob/9c7c4a1cb9405f64c31c03c4f1fa33c0c041dac0/src/libs/Network/SequentialQueue.ts#L161

What alternative solutions did you explore? (Optional)

[melvin-bot[bot]]

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

[melvin-bot[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.

https://github.com/Expensify/App/blob/207365c57858701f397c5aa334c175113a5b8da2/src/libs/Network/SequentialQueue.ts#L161

[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.

https://github.com/Expensify/App/blob/207365c57858701f397c5aa334c175113a5b8da2/src/libs/Network/SequentialQueue.ts#L161

[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 https://github.com/Expensify/App/pull/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[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[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:

https://github.com/user-attachments/assets/665864c6-564e-4f8e-b4ac-f820d609041c

[nkdengineer]

@Ollyws You're right. The problem is when the queue is paused, the process promise is also resolved immediately

https://github.com/Expensify/App/blob/9c7c4a1cb9405f64c31c03c4f1fa33c0c041dac0/src/libs/Network/SequentialQueue.ts#L66-L68

Then the onyx data is updated here while other write API areas remain in the queue. We should update to only update onyx data if the queue is empty. I tested locally with this change and it works well now. Correct me if I missed something.

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

https://github.com/Expensify/App/blob/9c7c4a1cb9405f64c31c03c4f1fa33c0c041dac0/src/libs/Network/SequentialQueue.ts#L161

[nkdengineer]

I updated my proposal

[trjExpensify]

From @tgolen's response here, I think we should close this issue. We have many instances of the replay effect in the app, and I'm hesitant of proceeding without a holistic solution to solve them all.

[nkdengineer]

My proposal can be a holistic solution to solve them all.

[tgolen]

I talked with Tom about this one, and I think we'd both like to have the UX flashing solved, but we are definitely both concerned that this is a low-level change that carries a lot of risk. I would like to propose that in order to move forward with @nkdengineer's proposal, that we create an ad-hoc build for the PR and have QA do a full regression test on it to help ensure that everything is still working how it's expected to.

What do you think @nkdengineer and @Ollyws ?

[nkdengineer]

@tgolen Yeah, look good to me.

[Ollyws]

@nkdengineer To clarify, your proposal is to remove flushOnyxUpdatesQueue() from the unpause() function and wrap this call of flushOnyxUpdatesQueue() in if (PersistedRequests.getAll().length === 0) { ? Because that solution doesn't fix the issue for me.

[nkdengineer]

@Ollyws Yes, this is my solution. Can you please share the video about this case?

[Ollyws]

@nkdengineer Sure:

https://github.com/user-attachments/assets/459e74fa-6785-46ad-b7df-3149db498035

[melvin-bot[bot]]

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

[nkdengineer]

@Ollyws I tested and it works well. Am I missing something?

https://github.com/user-attachments/assets/4025cdc1-9a37-4432-b8f7-aa805c94b38b

[Ollyws]

@nkdengineer it's only occuring for me on a money request with a very large amount of messages, on a new request it works fine.

[nkdengineer]

it's only occuring for me on a money request with a very large amount of messages, on a new request it works fine.

@Ollyws Can you please enter @nkdengineer+s31@outlook.com in this report and then invite this account to the report then I can test with this, thanks.

[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[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? https://github.com/Expensify/App/issues/46393#issuecomment-2283330210

[melvin-bot[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[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