[$500] Expense - LHN does not show expense preview when there is a thread with deleted root message

lanitochka17 commented 6 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: 1.4.47-2 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 staging.new.expensify.com
Go to workspace chat that has no pending request
Create a manual expense
Send a message to the main chat
Create another manual expense
Note that LHN shows the expense preview
Send a message to the main chat
Send a reply to the message in Step 6
Delete the parent message in Step 6
Create a manual expense

Expected Result:

LHN will show the expense preview whenever a request is made, just like Step 6

Actual Result:

LHN does not show the expense preview when the latest message is a thread with deleted parent message

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/f95d9015-3f60-474c-82cc-9989d6a6579c

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~012901a33cf428c32c
Upwork Job ID: 1765486641961807872
Last Price Increase: 2024-04-24

melvin-bot[bot] commented 6 months ago

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

lanitochka17 commented 6 months ago

@abekkala 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 6 months ago

We think that this bug might be related to #wave5 CC @dylanexpensify

nkdengineer commented 6 months ago

Proposal

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

Expense - LHN does not show expense preview when there is a thread with deleted root message
What is the root cause of that problem?
Before step 8, the LHN show the correct last message text. In this case, the last message text is from report.lastMessageText
But after step 10, the LHN show the incorrect last message text. It is because the lastReportAction in here is a deleted message. So the last message text is from the below: https://github.com/Expensify/App/blob/042bdcc33b7f7527b68238ad5941864781ed063b/src/libs/OptionsListUtils.ts#L578-L579
What changes do you think we should make in order to solve the problem?
When we get the lastReportAction, we should use both the current report's actions and the child money request actions in each report preview in the current report's actions

So, the logic to get the lastReportAction will become to:


const allReportPreviews = allSortedReportActions[report?.reportID ?? '']?.filter((reportAction) => ReportActionUtils.isReportPreviewAction(reportAction));
let allMoneyRequestActions = [];
forEach(allReportPreviews, (reportPreview) => {
    const allActions = allSortedReportActions[reportPreview?.childReportID];
    forEach(allActions, (action) => {
        if (ReportActionUtils.isMoneyRequestAction(action)) {
            allMoneyRequestActions.push(action);
        }
    });
});

const lastReportAction =
    ReportActionUtils.getSortedReportActions([...Object.values(allSortedReportActions[report?.reportID ?? '']), ...allMoneyRequestActions], true)?.find((reportAction) =>
        ReportActionUtils.shouldReportActionBeVisibleAsLastAction(reportAction),
    ) ?? null;

- Then, if ```lastReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU```, so we can return last message text by using the ```getReportPreviewMessage``` - that we use to get the report preview copy messge:

if (lastReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU) {
    const iouReportID = lastReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ? lastReportAction.originalMessage.IOUReportID : '';
    const iouReport = ReportUtils.getReport(iouReportID);
    const reportPreviewAction = ReportActionsUtils.getReportPreviewAction(iouReport?.chatReportID ?? '', iouReport?.reportID ?? '');
    const displayMessage = ReportUtils.getReportPreviewMessage(iouReport, reportPreviewAction);
    return displayMessage;
}


### What alternative solutions did you explore? (Optional)
- NA

melvin-bot[bot] commented 6 months ago

Job added to Upwork: https://www.upwork.com/jobs/~012901a33cf428c32c

melvin-bot[bot] commented 6 months ago

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

wildan-m commented 6 months ago

Proposal

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

Expense - LHN does not show expense preview when there is a thread with deleted root message

What is the root cause of that problem?

If the previous message is a deleted parent chat that contains a thread, the LHN will check and display the deleted message instead of report?.lastMessageText.

https://github.com/Expensify/App/blob/2979e3c709f9e0d8cfe3b25dc132fea03d046a5a/src/libs/OptionsListUtils.ts#L577-L579

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

If we want the previously deleted message to behave the same as other messages after creating a new money request, we can avoid entering the isDeletedParentAction check by comparing lastReportAction?.created and report?.lastVisibleActionCreated. Change:

https://github.com/Expensify/App/blob/2979e3c709f9e0d8cfe3b25dc132fea03d046a5a/src/libs/OptionsListUtils.ts#L577

To:

    } else if (ReportActionUtils.isDeletedParentAction(lastReportAction) && ReportUtils.isChatReport(report) && lastReportAction?.created === report?.lastVisibleActionCreated) {

Or enter the condition only if report?.lastVisibleActionCreated is the most recent or equal to lastReportAction?.created.

Please note that whether the previous message is a deleted chat or other chat types, the text preview for Request Money will be removed after page refresh, because we are not modifying the sorting algorithm. In my opinion, the ideal solution is to actually move the ReportActionPreview as described in my previous suggestion (see alternative solution)

What alternative solutions did you explore? (Optional)

I believe the problem is linked to the reportpreview position. It may be best to postpone addressing this as it could be resolved in https://github.com/Expensify/App/issues/37245.

If deemed separate, I suggest the same solution for this issue.

Proposal link.

jjcoffee commented 6 months ago

@wildan-m Thanks for the heads up! If you have time and don't mind, it'd be great if you could post a proposal specific to this issue as it does sound different to me.

wildan-m commented 6 months ago

@jjcoffee, please check the position of the report preview as it is currently stationary due to an active report preview. The last message in the LHN is determined by this sorting algorithm.

https://github.com/Expensify/App/blob/a0e444ed80c1642b78e973ec179568b2a785f66c/src/libs/ReportActionsUtils.ts#L220-L225

Please test this branch in offline mode, as it requires backend adjustments.

jjcoffee commented 6 months ago

Hmm on retesting this, now I'm getting that step 6 also doesn't show the expense preview in LHN, i.e. after an initial request, send a message, then make another request, and LHN still just shows the message. Weirdly, I seem to get the expense message in the LHN "after a while" or after signing out/in.

jjcoffee commented 6 months ago

@wildan-m The other issue you've proposed for is quite different, so a proposal directed specifically to this issue would be more appropriate here.

@nkdengineer Thanks for the proposal, and sorry I think I must have overlooked this previously! :bow: For your RCA I'd like to hear some more detail around why this is specifically happening after a deleted thread parent message, rather than any time a further money request is made.

nkdengineer commented 6 months ago

@jjcoffee

why this is specifically happening after a deleted thread parent message

It is because after step 10, the lastReportAction in here is a deleted message. So the last message text is from the below: https://github.com/Expensify/App/blob/042bdcc33b7f7527b68238ad5941864781ed063b/src/libs/OptionsListUtils.ts#L578-L579

And as you can see, getDeletedParentActionMessageForChatReport just return Deleted message or Deleted task

jjcoffee commented 6 months ago

@nkdengineer Sorry, what I'm getting at is that presumably the lastReportAction is correctly the (new) expense action in step 6, so why does it become incorrect after the deleted message? I think that's missing from your RCA.

nkdengineer commented 6 months ago

The last message to display after step 6 and after step 10 are from getLastMessageTextForReport
In case after step 6, the logic to get the last message is: https://github.com/Expensify/App/blob/3d4504e54154540629477cfc6ae0e897a09e8a5d/src/libs/OptionsListUtils.ts#L594 because the lastReportAction in this case is text message
In case after step 10, the logic to get last message is: https://github.com/Expensify/App/blob/3d4504e54154540629477cfc6ae0e897a09e8a5d/src/libs/OptionsListUtils.ts#L576-L577 because the lastReportAction is deleted parent message

andrey010 commented 6 months ago

Proposal

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

LHN does not show the expense preview when the latest message is a thread with deleted parent message

What is the root cause of that problem?

OptionsListUtils.getLastMessageTextForReport return as lastMesasgeText [Deleted message], it happens because lastReportAction https://github.com/Expensify/App/blob/4239b37f279ae1f2f6e338134dff926fdc441912/src/libs/OptionsListUtils.ts#L531 isDeletedParentAction but not report preview actions and function execute next line with condition https://github.com/Expensify/App/blob/4239b37f279ae1f2f6e338134dff926fdc441912/src/libs/OptionsListUtils.ts#L576

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

I think should add additional condition and set lastReportAction = previewAction

    const isDeletedParentAction = ReportActionUtils.isDeletedParentAction(lastReportAction)
    const reportPreview = allSortedReportActions[report?.reportID ?? '']?.find(reportAction => ReportActionUtils.isReportPreviewAction(reportAction))

    if (isDeletedParentAction && reportPreview) lastReportAction = reportPreview

in this way getLastMessageTextForReport function will execute next condition https://github.com/Expensify/App/blob/4239b37f279ae1f2f6e338134dff926fdc441912/src/libs/OptionsListUtils.ts#L556

What alternative solutions did you explore? (Optional)

Also reportPreviewAction has not updated created, lastModified values after request additional money, so I think if also if update create value, sortedActions sort callback will return reportPreview as last action after deleting parent message or maybe should update lastModified and add additional condition into sortedActions sort callback

jjcoffee commented 6 months ago

The missing piece here is that the lastReportAction won't update with new money requests, so following the steps here, the lastReportAction will always be the message (ADDCOMMENT report action), even after subsequent money requests. The report's lastMessageText (from the BE) is correct though, and we display it directly if we have no overrides:

https://github.com/Expensify/App/blob/3d4504e54154540629477cfc6ae0e897a09e8a5d/src/libs/OptionsListUtils.ts#L594

This is how initially the new money request preview is correctly shown in the LHN. However, once we delete that message, we override the report's lastMessageText because of this condition:

https://github.com/Expensify/App/blob/4239b37f279ae1f2f6e338134dff926fdc441912/src/libs/OptionsListUtils.ts#L576

I think I'm therefore most interested in a solution that just continues to use the report's lastMessageText in this situation, rather than the current proposals which seem more like workarounds when we already have the correct value to display.

andrey010 commented 6 months ago

@jjcoffee what if update created in the report preview action after a new money request ? in this way after sorting, report preview action will be as lastReportAction

jjcoffee commented 6 months ago

@andrey010 I think that would cause a regression as it would also move the money request report preview in the chat report itself.

melvin-bot[bot] commented 6 months ago

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

andrey010 commented 6 months ago

I cant reproduce now this issue

kbecciv commented 6 months ago

Issue is reproducible in QA side.

https://github.com/Expensify/App/assets/93399543/b1750c56-37aa-48ed-b3d7-3f83daf8df02

abekkala commented 6 months ago

I agree that we should have the most recent request show in the LHN regardless if there was a previous parent thread removed within the conversation

wildan-m commented 6 months ago

@jjcoffee I have made updates to my proposal. My main solution will function similarly to the current behavior, although I believe my previous suggestion may be more effective.

jjcoffee commented 6 months ago

the text preview for Request Money will be removed after page refresh, because we are not modifying the sorting algorithm

@wildan-m Thanks for the updated proposal! I'm not sure I understand why this part would be true though. Do the created and lastVisibleActionCreated values change on refresh?

wildan-m commented 6 months ago

@jjcoffee when a report containing a thread is deleted, the following occurred:

A money request was pending, with a created date of 2024-03-12 03:10:38.566 in reportpreview.
The parent report was deleted, which had a created date of 2024-03-16 04:00:45.797.
Subsequently, a new money request was created, and the backend updated the report's lastVisibleActionCreated to match the created date of the pending reportreview, which was 2024-03-12 03:10:38.566.
Upon page refresh, the backend restored the lastVisibleActionCreated to the last report action in the chat (the deleted parent report / 2024-03-16 04:00:45.797), likely utilizing the getSortedReportActions sorting algorithm.

This recent adjustment caused the LHNOptionRow preview to revert.

Given the current circumstances, the expected behavior needs to be altered. To ensure consistency, the backend should update report.lastVisibleActionCreated with [Deleted Message] instead of the request money's message.

However, since this is not the desired outcome, I suggest relocating the reportpreview below, incorporating the lastModified property when the reportpreview is updated. This will improve visibility and maintain consistent sorting.

melvin-bot[bot] commented 6 months ago

@abekkala @jjcoffee 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!

jjcoffee commented 6 months ago

Reviewing today!

melvin-bot[bot] commented 6 months ago

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

jjcoffee commented 5 months ago

Looks like this would require a BE fix too, as calling OpenReport (either on refresh or navigating to another chat and back) sets the lastMessageText to the last ADDCOMMENT action in the report, whereas when we call RequestMoney the lastMessageText is correctly set to the REPORTPREVIEW for the newly created money request. We would need to fix that so that the lastMessageText remains consistent (presumably RequestMoney has some special logic that OpenReport doesn't).

I think we could go with @wildan-m's proposal here (along with the BE fix above), but it does feel a bit hacky to rely on lastReportAction?.created === report?.lastVisibleActionCreated. I wonder if there is a better way to get the corect lastModified date in the IOU report preview (maybe as a separate field?). We would also probably want to optimistically set the correct lastMessageText as part of the FE changes.

I have some new repro steps that demonstrate the more general issue (i.e. not only involving deleting a parent thread message):

Request money from a workspace
Add a message
Verify the LHN shows the message
Request money again
Verify the LHN shows the money request (xxx owes $x.xx)
Refresh
The LHN now shows the last message from (2), even though nothing has changed

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

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

abekkala commented 5 months ago

@bfitzexpensify I'm ooo until April 08 - I'll be taking back any issues still open once I return

CURRENT STATUS

@jjcoffee has chosen a proposal by @wildan-m. Waiting for confirmation from @Beamanator Once he assigns @wildan-m they will work on the PR

wildan-m commented 5 months ago

@jjcoffee @Beamanator before proceeding with the lastModified approach, it's crucial to grasp the rationale behind selecting the current report preview behavior (to maintain the report preview position even after updates). My linked post was closed without a satisfactory explanation. If you have access to internal links, could you please share the decision summary?

bfitzexpensify commented 5 months ago

Hm @miljakljajic mind sharing a little more context on the close over in https://github.com/Expensify/App/issues/37245? I couldn't find anything in Slack talking about why it was closed

melvin-bot[bot] commented 5 months ago

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

bfitzexpensify commented 5 months ago

Bump on https://github.com/Expensify/App/issues/37775#issuecomment-2021655735 @miljakljajic when you're back tomorrow - thank you!

miljakljajic commented 5 months ago

Apologies - I've reopened the original issue, I misunderstood this comment as consensus that the issue should be closed!

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

@Beamanator, @abekkala, @jjcoffee, @bfitzexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

bfitzexpensify commented 5 months ago

Waiting for confirmation of the expected behaviour via the question in https://github.com/Expensify/App/issues/37245#issuecomment-2038556064 - @Beamanator, don't suppose you know the answer?

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

@Beamanator, @abekkala, @jjcoffee Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Beamanator commented 5 months ago

Oofda sorry for my delay here!

I have some new repro steps that demonstrate the more general issue (i.e. not only involving deleting a parent thread message):

Request money from a workspace

Add a message

Verify the LHN shows the message

Request money again

Verify the LHN shows the money request (xxx owes $x.xx)

Refresh

The LHN now shows the last message from (2), even though nothing has changed

I like @jjcoffee 's simplified steps here which definitely show a more general problem. @bfitzexpensify @miljakljajic I feel like we need a slack discussion to resolve the "desired behavior" for this issue & for this other issue in 1 place. Do either of you mind starting that convo in #expensify-open-source?

mvtglobally commented 5 months ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 5 months ago

@Beamanator, @abekkala, @jjcoffee Huh... This is 4 days overdue. Who can take care of this?

jjcoffee commented 5 months ago

Issue not reproducible during KI retests. (First week)

On v1.4.62-13, after step 9

Delete the parent message in Step 6

I'm still seeing an incorrect message in the LHN, but now it's No activity yet. The rest seems to be fixed.

https://github.com/Expensify/App/assets/27287420/a9cc9e18-52a3-4a1e-9220-d0909bfae895

melvin-bot[bot] commented 5 months ago

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

mvtglobally commented 5 months ago

Issue not reproducible during KI retests. (Second week)

jjcoffee commented 4 months ago

@abekkala Should we handle this related issue here?

abekkala commented 4 months ago

ah, if there is still an issue occurring with the same flow, then yes

Expensify / App