[$500] Split - The order of split details and request preview is different before & after opening report

kbecciv commented 10 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: v1.4.19-1 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Go to workspace chat > + > Split bill.
Create a bill split.
Note the order of split preview and request preview (the one with pay button).
Click on the split preview.
Return to the main chat.

Expected Result:

The order of split preview and request preview should be consistent.

Actual Result:

Before opening the expense report from the preview (with green button), the order is the expense preview (with green button), followed by the split preview. After opening the expense report, the order is interchanged. Now the split preview appears first, followed by the expense preview (with green button).

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[ ] Android: mWeb Chrome
[x] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/6b2827f2-9b31-427e-84ff-97b6f69f2a84

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01151f83b81fcfc0b1
Upwork Job ID: 1740793866618548224
Last Price Increase: 2024-01-12

melvin-bot[bot] commented 10 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01151f83b81fcfc0b1

melvin-bot[bot] commented 10 months ago

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

@slafortune, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

brunovjk commented 10 months ago

Proposal

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

The issue revolves around the inconsistency in the order of 'split preview' and 'request preview' following the creation of a SplitBill in a workspace chat.

What is the root cause of that problem?

1 - Upon initiating the 'splitBill' action in a workspace chat through IOU.js > splitBill(){createSplitsAndOnyxData(...)}, the optimisticData sets a reportActions_.IOU.created. Upon receiving a response with successData, the reportActions_.IOU.created is then updated, and match the newly created reportActions_.IOU.lastModified.

2 - When displaying 'preview cards' on a chat using ReportActionsUtils.getSortedReportActions, we get the list of reports (reportActions) and subsequently sorting it. Initially, the sorting is based on the timestamp, and if timestamps are identical, further sorting is done by CREATED and REPORTPREVIEW.

3 - Immediately after creating a 'splitBill', both reportActions_.IOU.created values from 'split preview' and 'request preview' are nearly identical. Consequently, in getSortedReportActions, the system falls into one case. However, when successData.response is received, the reportActions_.IOU.created values diverge, leading to a different case in getSortedReportActions.

This inconsistency is at the core of the problem.

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

To address this issue, two potential solutions are proposed:

Backend Modification: Investigate the feasibility of preventing the backend from modifying the reportActions_.IOU.created property when 'SplitBill' is called. This modification could contribute to maintaining consistency.
Sorting Adjustment: Adjust the sorting logic in ReportActionsUtils.getSortedReportActions to prioritize sorting by CREATED and REPORTPREVIEW. This adjustment ensures a consistent ordering of 'split preview' and 'request preview', aligning with the desired arrangement of keeping CREATED at the top and REPORTPREVIEW at the bottom.

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 10 months ago

@slafortune, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

slafortune commented 10 months ago

@alitoshmatov will you be able to review this?

alitoshmatov commented 10 months ago

Working on it

alitoshmatov commented 10 months ago

Thank you @brunovjk for your proposal. I couldn't reproduce where split preview and request preview has identical created timestamp, in my case they are different by a smallest unit which is sufficient.

alitoshmatov commented 10 months ago

I think there is a problem with backend. Based on my research when split bill is made in workspace chat optimistic data for reportActions_{workspace chat} contains two entries one for split bill preview and one for request preview. (1st image)

(2nd image) But backend is sending only one entry for this reportActions_, it is sending only split bill preview. Thus we have 1) Request preview, which requires pay action - Completely optimistic data 2) Split bill iou preview - received from backend

But when we go to another report and comeback to workspace chat, openReport request is made and then backend is sending complete report actions in reportActions_{workspace chat} which includes split bill preview and Request preview (3rd image)

1st image

2nd image

3rd image

alitoshmatov commented 10 months ago

@slafortune I am suspecting the issue might be on backend side, can we get a internal engineer to look at my observations

melvin-bot[bot] commented 10 months 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 10 months ago

@slafortune, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

slafortune commented 10 months ago

@alitoshmatov sorry for the delay! Sure can, I'll get an engineer assigned to take a look.

melvin-bot[bot] commented 10 months ago

Triggered auto assignment to @joelbettner (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

melvin-bot[bot] commented 10 months 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 10 months ago

@slafortune @joelbettner @alitoshmatov 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!

joelbettner commented 10 months ago

I have been testing to try and re-create this. I haven't been able to.

@kbecciv can you please try to re-create this again and let me know if you're able to?

joelbettner commented 10 months ago

I just manually tested this locally again. I checked it with timestamps that were the same and timestamps that were different by manually updating the timestamps in my local database. In neither case did the order that the report actions are displayed change. I think this can be closed.

How it displayed with different timestamps:

Manually updating the timestamps of the report actions:

sqlite> SELECT * FROM reportActions WHERE reportActionID = 1695655901254895829;
created                  reportID          accountID  action  message                                                                                                                                                                        reportActionID     
-----------------------  ----------------  ---------  ------  -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------  -------------------
2024-01-12 19:17:37.834  5873902278986266  15         IOU     {"IOUTransactionID":"3064946919122085017","amount":"1100","comment":"","currency":"USD","lastModified":"2024-01-12 19:17:37.834","participantAccountIDs":[15],"type":"split"}  1695655901254895829
sqlite> SELECT * FROM reportActions WHERE reportActionID = 5013158385792148183;
created                  reportID          accountID  action         message                                                                       reportActionID     
-----------------------  ----------------  ---------  -------------  ----------------------------------------------------------------------------  -------------------
2024-01-12 19:17:37.838  5873902278986266  0          REPORTPREVIEW  {"lastModified":"2024-01-12 19:17:37.838","linkedReportID":2595405387559353}  5013158385792148183
sqlite> UPDATE reportActions SET created = '2024-01-12 19:17:37.834' WHERE reportActionID = 5013158385792148183;
sqlite> SELECT * FROM reportActions WHERE reportActionID = 5013158385792148183;
created                  reportID          accountID  action         message                                                                       reportActionID     
-----------------------  ----------------  ---------  -------------  ----------------------------------------------------------------------------  -------------------
2024-01-12 19:17:37.834  5873902278986266  0          REPORTPREVIEW  {"lastModified":"2024-01-12 19:17:37.838","linkedReportID":2595405387559353}  5013158385792148183

How it displayed with identical timestamps:

roryabraham commented 10 months ago

I was able to reproduce this:

It took two tries. I think there's just a race condition because there's nothing in the sorting order to ensure that the "IOU preview" appears after the "split preview" (apologies for the ambiguous language)

roryabraham commented 10 months ago

It's a bit hard to get clear on exactly what is supposed to happen optimistically with a bill split, because the existing tests don't cover REPORTPREVIEW actions, only IOU actions.

So whatever the outcome for this particular issue, I hope that we at least have an external contributor update all scenarios in IOUTest.js to cover the optimistic, success, and failure data for the REPORTPREVIEW actions

cc @cristipaval

brunovjk commented 10 months ago

I think there's just a race condition because there's nothing in the sorting order to ensure

Exactly, I believe it is worth reviewing ReportActionsUtils.getSortedReportActions.

update all scenarios in IOUTest.js to cover the optimistic, success, and failure data for the REPORTPREVIEW actions

@roryabraham If we do it, can I work on it?

roryabraham commented 10 months ago

Exactly, I believe it is worth reviewing ReportActionsUtils.getSortedReportActions.

I'm not sure yet. For sure the canonical order of reportActions on a report comes from the server. I think we need to have a wider conversation about this because the REPORTPREVIEW action is the only reportAction whose created value ever changes in the database, and that's a bit strange.

One solution could be to update the back-end with a 1ms sleep in The Right Place™ to ensure that the REPORTPREVIEW action appears after the IOU action in the report. I don't think it's likely that we want to change the primary sorting criteria to be something other than created timestamp... 🤔

Sorry, not sure the best solution here and need to spend some more time discussing it before we greenlight a solution.

brunovjk commented 10 months ago

Totally get the need for more discussion before giving the green light. Happy to jump in if need any help.

kbecciv commented 10 months ago

Issue is reproducible v.1.4.24-7

https://github.com/Expensify/App/assets/93399543/fa5e815f-c29a-4db1-9567-2b8af26f355b

joelbettner commented 10 months ago

I was able to reproduce this, but I had to have the timestamp for the REPORTPREVIEW action be BEFORE (not the same as) the timestamp for the IOU action.

So, I think this can easily be sorted in the front end. IMO, sorting it in the backend will add unnecessary load to our servers when this is easy to process on the client.

roryabraham commented 10 months ago

Mentioned this over DMs, but it's critical that the sorting order of reportActions (or reports, for that matter) to be the same in both the front-end and the back-end. Currently they're actually out-of-sync and we might need to update this query to account for this piece of front-end logic

Failure to do this might lead to the previousReportActionID essentially being "wrong" from the front-end's perspective, which after comment linking is merged could lead to rendering issues and potentially an infinite loop of API calls trying to "fill the gap" from the "missing" reportActions.

This issue is not that urgent, so I want to make sure we take our time to get this right, as the potential downside is pretty bad

roryabraham commented 10 months ago

@joelbettner and I are a bit confused about this, but have some general thoughts:

It seems like REPORTPREVIEW actions are not generated optimistically.
I further feel that they are weird in that they're the only actions that can move their position in the front-end. It also seems like they're a duplicated source of truth about another report – might it be simpler to just generate report previews on the fly in the front-end only?
The requirement to sync reportAction sorting between the front-end and the back-end kind of sucks. @perunt might it be possible to just update the reportAction sorting in the front-end to just trust the previousReportActionID from the back-end? Then a "gap" is detected anywhere where a previousReportActionID references a reportAction that we just don't have?
It also seems like, at the time when this bug occurs, the timestamps of the IOU and REPORTPREVIEW actions are correct in the database, but incorrect in Onyx. So there's some place in the back-end where we're forgetting to update one or both of those actions in Onyx via Pusher or reliable updates.
- Note: I don't see any reference to a REPORTPREVIEW action in the response to SplitBill

roryabraham commented 10 months ago

Pretty confused because if the REPORTPREVIEW action isn't being generated optimistically, and it isn't coming from the SplitBill API response, then where is it coming from? The REPORTPREVIEW action also appears to not show up at all if you're offline. 😕

roryabraham commented 10 months ago

cc @cristipaval any insights here?

roryabraham commented 10 months ago

It seems like REPORTPREVIEW actions are not generated optimistically.

This does seem like it might be missing from the IOU.splitBill action, but I did find ReportUtils.buildOptimisticReportPreview

cristipaval commented 10 months ago

@joelbettner and I are a bit confused about this, but have some general thoughts:

It seems like REPORTPREVIEW actions are not generated optimistically.

Report previews have changed a lot in the front-end since they were created. Tagging @youssef-lr as I know he's done a decent amount of work around SplitBill

I further feel that they are weird in that they're the only actions that can move their position in the front-end. It also seems like they're a duplicated source of truth about another report – might it be simpler to just generate report previews on the fly in the front-end only?

We discussed this idea when we created report previews. It was decided to have a report action in the database for the report previews because we want them to behave like a proper report action, with the ability to react to, reply-in-thread, etc. The information displayed on the report previews is gathered on the fly. In the database, we store minimal info like linkedReportID, which is the ID of the report that is previewed. I don't think we have duplicated sources of truth.

The requirement to sync reportAction sorting between the front-end and the back-end kind of sucks. @perunt might it be possible to just update the reportAction sorting in the front-end to just trust the previousReportActionID from the back-end? Then a "gap" is detected anywhere where a previousReportActionID references a reportAction that we just don't have?

I do agree with this. If we agree on another approach, we would also have to refactor the way we sort CREATED report actions. They are always lower than any other report actions, exactly as REPORTPREVIEWS should always be greater when the timestamp is the same.

It also seems like, at the time when this bug occurs, the timestamps of the IOU and REPORTPREVIEW actions are correct in the database, but incorrect in Onyx. So there's some place in the back-end where we're forgetting to update one or both of those actions in Onyx via Pusher or reliable updates.

Note: I don't see any reference to a REPORTPREVIEW action in the response to SplitBill

cc'ing @youssef-lr for the last statement here, let me know if you're the wrong person to tag about SplitBill

joelbettner commented 10 months ago

I've been doing some digging.

I found that this query does not run when we create a split. Shouldn't it be? Since that query is "the source of truth" for the order of the report actions, when we are creating new report actions by adding splits, I think we should be running that query...no?

I am getting the report actions out of order when I create a new split or add to an existing split:

But as soon as I refresh the page, the query runs and the report actions are back in the correct order:

joelbettner commented 10 months ago

I also did this...

I added some console logging in the getSortedReportActions function here:

        // First sort by timestamp
        if (first.created !== second.created) {
            if (first.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW) {
                console.log('report preview created: ', first.created);
            }
            return (first.created < second.created ? -1 : 1) * invertedMultiplier;
        }

Then I started adding splits.

Initial creation of the split bill --> the created time on the front end does not match the created time saved in the db:

After a refresh --> the created still does not match the time saved in the db but everything displays in the correct order because this query ran which returned all the entries in the correct order regardless of their timestamp:

I added another split to the report --> the created time on the front end still does not update for the report preview and things display in the incorrect order again:

I refreshed the page --> things displayed in the correct order again despite having the incorrect timestamp in the front end:

cc: @roryabraham @cristipaval @youssef-lr It looks like MAYBE the report preview action is being optimistically created with a timestamp, but that timestamp never gets updated after the report preview is successfully created?? What do you think?

youssef-lr commented 10 months ago

@roryabraham we do build the optimistic report preview here. I was also able to reproduce this, I can see that the REPORTPREVIEW action has the optimistic created timestamp, but the IOU action (split action) has the server's timestamp (which can be newer than the optimistic timestamp of the report preview)

I think the issue here is that we're not pushing the updated REPORTPREVIEW back to the frontend like @joelbettner mentioned. So I think this is a backend issue, as when you log out and log back in, the actions get sorted properly. I verified the code and we don't really send it back, so it remains having the optimistic timestamp only. For comparison, here's CompleteSplitBill which sends back the updated REPORTPREVIEW.

roryabraham commented 10 months ago

Great, thanks for all the investigation and discussion everyone. It looks like this PR will most likely fix this issue. Thanks @youssef-lr!

We're further discussing REPORTPREVIEW actions and previousReportActionID here

joelbettner commented 9 months ago

Not sure why this didn't get closed when the PR was deployed. I'm going to close it.

Expensify / App