[$500] Attachment - 'New message' button appears when deleting attachment

izarutskaya commented 1 month 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.59-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4468722 Email or phone of affected tester (no customers): natnael.expensify+3@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Navigate to a report has multiple chat
Send attachment
Delete the attachment
Click on 'New messages' button

Expected Result:

'New messages' button shouldn't appear, and attachment should remain deleted

Actual Result:

'New messages' button appears, and attachment reappear when clicking 'New messages' button

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/6830f6a6-bef6-4715-bdaf-8fde3bd6ceb7

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~010fcec62910c073e7
Upwork Job ID: 1775234083431460864
Last Price Increase: 2024-04-30
Automatic offers:

jjcoffee | Reviewer | 0
c3024 | Contributor | 0

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @deetergp (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] commented 1 month ago

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

github-actions[bot] commented 1 month ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

izarutskaya commented 1 month ago

@garrettmknight 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.

izarutskaya commented 1 month ago

We think this issue might be related to the #vip-vsb.

izarutskaya commented 1 month ago

Production

https://github.com/Expensify/App/assets/115492554/ac5be676-7aac-43b6-8b72-31c8abac07ba

deetergp commented 1 month ago

I was able to reproduce this locally. Also navigating away from the chat & back will cause the supposedly deleted attachment to reappear.

garrettmknight commented 1 month ago

Rerpo'd it as well. I think this could be External so I'll start there.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~010fcec62910c073e7

melvin-bot[bot] commented 1 month ago

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

jasperhuangg commented 1 month ago

I just reproduced this on prod so it doesn't need to be a blocker

kaushiktd commented 1 month ago

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

Attachment - 'New message' button appears when deleting attachment

What is the root cause of that problem?

The fundamental issue stems from the inaccurate calculation of the canScrollToNewerComments variable, which governs the display of the "New Messages" button. Currently, certain conditions, including !isLoadingInitialReportActions, !hasNewestReportAction, and !isLastPendingActionIsDelete, are incorrectly inverted, resulting in the button being erroneously shown even when there are no unread messages.

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

to solve this problem change here https://github.com/Expensify/App/blob/b0c41789d5def5f928a0338c3e4b39e5ba96ae34/src/pages/home/report/ReportActionsList.tsx#L614

const canScrollToNewerComments = isLoadingInitialReportActions && hasNewestReportAction && sortedReportActions.length > 25 && isLastPendingActionIsDelete;

canScrollToNewerComments: This variable determines whether the "scroll to newer comments" button should be displayed. It evaluates to true if the following conditions are met:

isLoadingInitialReportActions: Indicates whether the report actions are still loading initially.
hasNewestReportAction: Indicates whether there is a newest report action available.
sortedReportActions.length > 25: Ensures that there are more than 25 sorted report actions in the list.
isLastPendingActionIsDelete: Indicates whether the last pending action is a delete action.

The proposed fix adjusts the canScrollToNewerComments variable to ensure that it evaluates to false when it should not override the other conditions. By modifying the condition to include the correct logic without the use of the logical NOT operator, we ensure that the button is displayed only when necessary. By accurately evaluating the conditions and addressing the root cause of the issue, we ensure that the button displays correctly in all relevant scenarios

video:-https://drive.google.com/file/d/1dBX9XXeHCps6-OU2qmPJvhpcDts-9p5j/view?usp=sharing

jjcoffee commented 1 month ago

Reviewing Monday!

deetergp commented 1 month ago

Looking forward to it 👍

jjcoffee commented 1 month ago

Apologies I had higher priority work to get to today, will get back to this tomorrow! :bow:

jjcoffee commented 1 month ago

@kaushiktd Thanks for your proposal! It's lacking somewhat in details, however. Could I ask you to expand on where the code changes in your solution would go, as well as more detail in your RCA, i.e. what "deletion logic" are you referring to?

kaushiktd commented 1 month ago

in this file change in function deleteReportComment() https://github.com/Expensify/App/blob/b0c41789d5def5f928a0338c3e4b39e5ba96ae34/src/libs/actions/Report.ts#L1122

i introduce a new variable hasChildReport to identify if a report action has a child report. add here https://github.com/Expensify/App/blob/b0c41789d5def5f928a0338c3e4b39e5ba96ae34/src/libs/actions/Report.ts#L1129

const hasChildReport = reportAction.childReportID !== null && reportAction.childReportID !== undefined;

and also Adjust the condition for deletion in the deleteReportComment function to consider both cases: whether the report action is a parent action of a thread or if it lacks a child report. https://github.com/Expensify/App/blob/b0c41789d5def5f928a0338c3e4b39e5ba96ae34/src/libs/actions/Report.ts#L1242

const conflictingCommands = (
    (!hasChildReport || isDeletedParentAction)
        ? [WRITE_COMMANDS.UPDATE_COMMENT]
        : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT, WRITE_COMMANDS.DELETE_COMMENT]
) as string[];

This modification ensures that the report action will be deleted if it's a parent action of a thread or if it lacks a child report.

jjcoffee commented 1 month ago

@kaushiktd Sorry, the RCA is still unclear here. Can you update your proposal with a detailed RCA that explains what the issue with conflictingCommands as implemented is?

kaushiktd commented 1 month ago

The issue with conflictingCommands lies in its failure to accurately identify which report actions should be deleted. At present, the condition for deletion relies on the variable isDeletedParentAction, which indicates whether the report action being considered is a parent action of a thread.

const conflictingCommands = (
    (!hasChildReport || isDeletedParentAction)
        ? [WRITE_COMMANDS.UPDATE_COMMENT]
        : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT, WRITE_COMMANDS.DELETE_COMMENT]
) as string[];

The intent behind this condition is to delete a report action if either of the following conditions is met:

The report action does not have a child report (!hasChildReport).
The report action is a parent action of a thread (isDeletedParentAction).

However, the logic fails to account for cases where a report action does not have a child report but is still a parent action. This leads to inconsistencies in deletion behavior, where report actions without child reports remain undeleted despite being parent action

jjcoffee commented 1 month ago

@kaushiktd conflictingCommands was introduced in https://github.com/Expensify/App/pull/39024 to deal with conflicting requests, so it's not a condition for deletion, per se. It is clear that there's an issue with it, but I'm not convinced that you understand what your change does or why it works, because you've not identified a clear RCA here. We do require this clarity of RCA and a clear link to the solution in proposals, so that we can be sure the fix is not just a workaround.

kaushiktd commented 1 month ago

@jjcoffee the current implementation focuses on deleting report actions only if they are parent actions of threads, which essentially means they have child reports. However, the oversight lies in the fact that the system doesn't consider that a report action without a child report should still be treated as a parent action.

Thus, it's essential to address this gap in logic by ensuring that report actions are deleted if they are parent actions or if they simply lack child reports. This refinement in the deletion logic will lead to more accurate handling of report actions and prevent inconsistencies in deletion behavior.

bernhardoj commented 1 month ago

jjcoffee commented 1 month ago

Cheers for the heads up @bernhardoj! It does look very likely that your PR will resolve this issue, so @deetergp can we HOLD this issue for https://github.com/Expensify/App/issues/39432?

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? 💸

deetergp commented 1 month ago

Still holding. Going to make this a weekly for the time being.

garrettmknight commented 3 weeks ago

Other issue is deployed, moving to daily to test in the morning.

garrettmknight commented 3 weeks ago

I was able to repro this issue. Opening up for help.

jjcoffee commented 3 weeks ago

Pinging @bernhardoj and @kaushiktd in case you want to update/add proposals based on the latest changes.

kaushiktd commented 3 weeks ago

@jjcoffee i updated my proposal

melvin-bot[bot] commented 3 weeks 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 3 weeks ago

Will try to review tomorrow!

jjcoffee commented 3 weeks ago

Hmm I'm not able to repro on latest staging. @kaushiktd or @garrettmknight were there any special steps other than those in the issue description?

kaushiktd commented 3 weeks ago

@jjcoffee When you delete an attachment, it will no longer reappear. This is a solved issue. However, a new message box still pops up. I have updated my proposal to address this problem.

jjcoffee commented 3 weeks ago

@kaushiktd Hmm I'm not seeing that on latest staging. What version of the app are you seeing this behaviour on?

kaushiktd commented 3 weeks ago

@jjcoffee "name": "new.expensify", "version": "1.4.65-5",

I'm checking in "version": "1.4.66-2",

kaushiktd commented 3 weeks ago

@jjcoffee New pop up shows in latest version

I made a video: https://drive.google.com/file/d/12WxR3rgjeT3Y7V8ACUbLgML6B09wHh-r/view?usp=sharing

jjcoffee commented 2 weeks ago

@kaushiktd Thanks for retesting! I've managed to reproduce; it seems like it's a requirement to fill the chat with a certain number of messages before it is triggered.

Your proposal is lacking in detail. Can you please explain the root cause and how/why the solution handles it? Thanks!

kaushiktd commented 2 weeks ago

@jjcoffee please have a look

Root Cause Analysis:

The root cause of the issue lies in the incorrect evaluation of the canScrollToNewerComments variable, which determines whether the "scroll to newer comments" button should be displayed. Currently, the condition used to calculate this variable includes checks for isLoadingInitialReportActions, hasNewestReportAction, and isLastPendingActionIsDelete. However, due to the use of the logical NOT operator (!) in front of isLoadingInitialReportActions, hasNewestReportAction, and isLastPendingActionIsDelete, the variable canScrollToNewerComments ends up being true when it should not be.

Proposed Solution:

To address this issue and ensure that the "scroll to newer comments" button is displayed only when necessary, we need to adjust the condition used to calculate the canScrollToNewerComments variable. Instead of using the logical NOT operator, we should directly evaluate the conditions based on their intended behavior.

We can achieve this by updating the condition to the following:

const canScrollToNewerComments = isLoadingInitialReportActions && hasNewestReportAction && sortedReportActions.length > 25 && isLastPendingActionIsDelete;

With this change, the canScrollToNewerComments variable will only be true when all conditions are met, including being in the initial loading state, having a newest report action, having more than 25 sorted report actions, and the last pending action not being a delete action.

By accurately evaluating these conditions, we ensure that the "scroll to newer comments" button is displayed appropriately, addressing the issue and providing the expected behavior for users.

jjcoffee commented 2 weeks ago

@kaushiktd Thanks, could you explain though what condition (or combination) is causing the button to display in this particular case? I think it would be reckless to flip the logic on all the conditions without understanding why :smile:

I feel like your fix probably only works because your changes evaluate canScrollToNewerComments as false and so it no longer overrides the other conditions here:

https://github.com/Expensify/App/blob/b0c41789d5def5f928a0338c3e4b39e5ba96ae34/src/pages/home/report/ReportActionsList.tsx#L618-L618

kaushiktd commented 2 weeks ago

@jjcoffee

canScrollToNewerComments: This variable determines whether the "scroll to newer comments" button should be displayed. It evaluates to true if the following conditions are met:

isLoadingInitialReportActions: Indicates whether the report actions are still loading initially.
hasNewestReportAction: Indicates whether there is a newest report action available.
sortedReportActions.length > 25: Ensures that there are more than 25 sorted report actions in the list.
isLastPendingActionIsDelete: Indicates whether the last pending action is a delete action. The proposed fix adjusts the canScrollToNewerComments variable to ensure that it evaluates to false when it should not override the other conditions. By modifying the condition to include the correct logic without the use of the logical NOT operator, we ensure that the button is displayed only when necessary.

By accurately evaluating the conditions and addressing the root cause of the issue, we ensure that the button displays correctly in all relevant scenarios

jjcoffee commented 2 weeks ago

@kaushiktd Flipping the conditions will likely break the expected behaviour from the comment above the code you're proposing to change:

https://github.com/Expensify/App/blob/b0c41789d5def5f928a0338c3e4b39e5ba96ae34/src/pages/home/report/ReportActionsList.tsx#L612-L614

jjcoffee commented 2 weeks ago

Friendly bump @bernhardoj in case you're available and want to take a look at this, since you worked on a related PR recently.

kaushiktd commented 2 weeks ago

@kaushiktd Flipping the conditions will likely break the expected behaviour from the comment above the code you're proposing to change:

https://github.com/Expensify/App/blob/b0c41789d5def5f928a0338c3e4b39e5ba96ae34/src/pages/home/report/ReportActionsList.tsx#L612-L614

@jjcoffee As I've mentioned in my last response, by evacuating the conditions the behaviour is expected and will not break.🙂 Still if there is a way for you to test it, I'd love to see the results because it is working as expected for me.

melvin-bot[bot] commented 2 weeks 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 2 weeks ago

@deetergp @garrettmknight @jjcoffee this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

jjcoffee commented 2 weeks ago

@kaushiktd My problem with your proposal as it stands is that it lacks a clear RCA and the explanatory link to the solution, i.e. why does it work. If you can explain how flipping the isLoadingInitialReportActions and hasNewestReportAction conditions won't break the comment linking functionality, that would help.

In the meantime open to new proposals!

kaushiktd commented 2 weeks ago

@jjcoffee proposal updated

c3024 commented 2 weeks ago

Proposal

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

'New messages' floating button appears when the latest message is deleted after crossing a threshold.

What is the root cause of that problem?

We show the floating message counter when hasNewestReportAction is false among other things here https://github.com/Expensify/App/blob/573d9893ae2a8e6e4ebe64088cb2fbf62ce81804/src/pages/home/report/ReportActionsList.tsx#L616-L622 but hasNewestReportAction here compares the latest sortedReportAction https://github.com/Expensify/App/blob/573d9893ae2a8e6e4ebe64088cb2fbf62ce81804/src/pages/home/report/ReportActionsList.tsx#L203 but latest sortedReportActions include deleted actions and the check fails even though we are already at the latest message because the latest sortedReportAction created time is of the deleted reportAction's whereas the report's lastVisibleActionCreated is of the present latest message creation time. This check fails and the floating button appears

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

We have sortedVisibleReportActions here https://github.com/Expensify/App/blob/573d9893ae2a8e6e4ebe64088cb2fbf62ce81804/src/pages/home/report/ReportActionsList.tsx#L192-L199 and this omits the deleted action here https://github.com/Expensify/App/blob/573d9893ae2a8e6e4ebe64088cb2fbf62ce81804/src/libs/ReportActionsUtils.ts#L539 So, change the check for hasNewestReportAction above to use sortedVisibleReportActions?.[0].created

What alternative solutions did you explore? (Optional)

dragnoir commented 2 weeks ago

Proposal

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

'New message' button appears when deleting attachment

What is the root cause of that problem?

The logic behind canScrollToNewerComments is using a wrong conditions

https://github.com/Expensify/App/blob/573d9893ae2a8e6e4ebe64088cb2fbf62ce81804/src/pages/home/report/ReportActionsList.tsx#L618

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

I suggest implementing a new logic for the "New Message" floating button, similar to the functionality observed in Slack. This button should only appear under specific conditions:

If there are new unread messages.
If the user's scrolling position is above the first unread message.

This approach ensures that the button is both contextual and minimally intrusive, improving user experience by providing a quick way to navigate to the first unread message.

State Management:
- Maintain a state variable hasUnreadMessages to track if there are unread messages.
- Use another state variable firstUnreadMessageIndex to store the index of the first unread message in your messages array.
Scroll Position Detection:
- Attach an onScroll event handler to determine the scroll position. Use the event.nativeEvent.contentOffset.y to get the vertical scroll position.
Display Logic for Button:
- Determine the position of the first unread message relative to the scroll view.
- Compare the current scroll position with this calculated position. If the scroll position is above this point and hasUnreadMessages is true, set another state variable showFloatingButton to true.

By following these steps, we can create a user-friendly navigation feature that closely mirrors the convenient functionality seen in Slack.

jjcoffee commented 2 weeks ago

The approach in @c3024's proposal LGTM! It also explains why the scroll button appears after a short delay; report.lastVisibleActionCreated is populated once the BE request completes. We have to make sure that any changes to hasNewestReportAction don't have knock-on effects, or maybe introduce a new hasNewestVisibleReportAction to be safe :smile:

I don't think we want to rework the whole way this feature works, as @dragnoir suggests.

@kaushiktd's proposal lacks the detail for me to fully analyse it. I strongly doubt it's a workable fix.

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

Expensify / App