Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2025-01-02] [$250] Search - App redirects to DM when submitting expense in Search after deleting expense #52477

Closed
2 of 8 tasks
IuliiaHerets opened this issue Nov 13, 2024 · 49 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 13, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


**Version Number:**9.0.61-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Open DM with any user.
  3. Submit an expense to the user.
  4. Go to Search.
  5. Select the expense via checkbox.
  6. Click on the dropdown > Delete expense.
  7. Delete the expense.
  8. Open FAB while stay on Search page.
  9. Click Submit expense under Quick action.
  10. Enter amount and submit it.
  11. Note that app redirects to DM with the user.
  12. Go to Search page.
  13. Open FAB > Click Submit expense under Quick action.
  14. Enter amount and submit it.
  15. Note that app stays in Search page in this case (expected result).

Expected Result:

In Step 11, app should remain in Search page when submitting expense to user after deleting an expense in Search.

Actual Result:

In Step 11, app redirects to DM when submitting expense to user after deleting an expense in Search.

This issue also happens with Submit expense option in FAB, and only happens when an expense is deleted in Search and submitting another expense.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6663547_1731490307345.20241113_172050.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021861529064896018849
  • Upwork Job ID: 1861529064896018849
  • Last Price Increase: 2024-11-26
  • Automatic offers:
    • DylanDylann | Reviewer | 105296071
    • FitseTLT | Contributor | 105296073
Issue OwnerCurrent Issue Owner: @abekkala
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 13, 2024
Copy link

melvin-bot bot commented Nov 13, 2024

Triggered auto assignment to @abekkala (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.

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 13, 2024

Edited by proposal-police: This proposal was edited at 2024-11-13 14:46:43 UTC.

Proposal

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

Search - App redirects to DM when submitting expense in Search after deleting expense
To reproduce:
Make sure you have an empty chat with someone (by creating an expense and deleting it so that it will appear in quick action menu in FAB)
then open search and create the expense from Quick action menu

What is the root cause of that problem?

When we notifyNewAction when creating the expense scrollToBottomForCurrentUserAction is called and if the lastVisibleActionCreated timestamp differs from the lastAction.created, which would mean the report action list doesn't have newest report action, we navigate to the report so as to fetch the newest report action by triggering openReport

Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));

Although we correctly update the lastVisibleActionCreated and lastAction this code is executed too quickly and by that time we don't have the correct updated values for both. So if we had any report action in the report then both values would be the same old values but as they will be equal this navigate code is not called (that's why we don't reproduce it on non-empty chats) but if we have a empty report before creating the expense the lastVisibleActionCreated will have a different value than the lastAction.created which is the created action.
BTW this navigate code is unnecessarily being called even when you submit expense from the report screen, the effect only became visible for this current issue because we were in search page.

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

We need to give it a time before scrollToBottomForCurrentUserAction is called so that we calculate according to the correct values of lastVisibleActionCreated and lastAction are populated by onyx. This will fix the problem from the root cause.

const unsubscribe = Report.subscribeToNewActionEvent(report.reportID, scrollToBottomForCurrentUserAction);

        const unsubscribe = Report.subscribeToNewActionEvent(report.reportID, () => InteractionManager.runAfterInteractions(scrollToBottomForCurrentUserAction));

_This root cause should be fixed but in parallel to that we can avoid notifying new Action when isSearchTopmostCentralPane() just the same as we are avoiding navigating to the report for the case search page is the active central pane

App/src/libs/actions/IOU.ts

Lines 3684 to 3685 in 4d9be4e

if (activeReportID) {
Report.notifyNewAction(activeReportID, payeeAccountID);

But we should do the same change in other cases such as sendInvoice trackExpense splitBill here createDistanceRequest ... cases in IOU.ts too. Or centralize the check inside Report.notifyNewAction

What alternative solutions did you explore? (Optional)

As need to check if the current report screen is focused before executing it

if (!hasNewestReportActionRef.current) {

            if (!hasNewestReportActionRef.current || !isFocused) {

but if we don't want the scrollToBottom to be called we can add it here

if (!isFromCurrentUser) {

            if (!isFromCurrentUser || !isFocused) {

@gijoe0295
Copy link
Contributor

Proposal

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

In Step 11, app redirects to DM when submitting expense to user after deleting an expense in Search.

What is the root cause of that problem?

We call notifyNewAction when completing the request:

Report.notifyNewAction(activeReportID, payeeAccountID);

In the new action event listener, we navigate to the DM report

Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));

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

If the topmost central pane screen is SearchCentralPane, we wouldn't need to call notifyNewAction.

@FitseTLT
Copy link
Contributor

Updated

@melvin-bot melvin-bot bot added the Overdue label Nov 18, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

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

Copy link

melvin-bot bot commented Nov 21, 2024

@abekkala Still overdue 6 days?! Let's take care of this!

Copy link

melvin-bot bot commented Nov 25, 2024

@abekkala 10 days overdue. Is anyone even seeing these? Hello?

@abekkala abekkala added the External Added to denote the issue can be worked on by a contributor label Nov 26, 2024
@melvin-bot melvin-bot bot changed the title Search - App redirects to DM when submitting expense in Search after deleting expense [$250] Search - App redirects to DM when submitting expense in Search after deleting expense Nov 26, 2024
Copy link

melvin-bot bot commented Nov 26, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 26, 2024
Copy link

melvin-bot bot commented Nov 26, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 26, 2024
@abekkala
Copy link
Contributor

@DylanDylann we do have some proposals above 😄

Copy link

melvin-bot bot commented Nov 27, 2024

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

@DylanDylann
Copy link
Contributor

I am reviewing

@DylanDylann

This comment was marked as outdated.

@DylanDylann
Copy link
Contributor

DylanDylann commented Nov 29, 2024

@FitseTLT After checking I don't think your RCA is correct in this case

We need to give it a time before scrollToBottomForCurrentUserAction is called so that we calculate according to the correct values of lastVisibleActionCreated and lastAction are populated by onyx. This will fix the problem from the root cause.

We should go with @gijoe0295 proposal. If we are on Search page, we don't need to notify new action, it is only necessary if we are in chat screen

🎀 👀 🎀 C+ Reviewed

Copy link

melvin-bot bot commented Nov 29, 2024

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 29, 2024

You mean that we need to wait for BE response before calling scrollToBottomForCurrentUserAction

Nope not for the BE but we need to wait the addition of the report action in onyx takes effect on our report action list and lastVisibleAction of the report

@FitseTLT After checking I don't think your RCA is correct in this case

@DylanDylann My RCA is correct and the problem is only being visible here because we are being navigated away from search page but the problem also happens for report screen on an empty chat (I have explained why it happens for empty chats in my RCA) and we are unnecessarily navigating to the same report here

Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));

                console.log('It is navigating');
                Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));

Here is the debugging demo of navigating happening even in report screen and you can easily debug it. Please re-review it we should solve the problem from the root cause because as I have explain in my proposal it is not happening for most cases because both values we are comparing lastVisibleActionCreated and the lastAction created timestamp are in most cases compared with their old values so the problem only gets visible in empty chats where the lastVisibleActionCreated will be different. Thx

2024-11-29.12-11-46.mp4

@DylanDylann
Copy link
Contributor

but the problem also happens for report screen on an empty chat

@FitseTLT Could you please detail this additional bug including steps, actual behavior, and expected behavior?

@DylanDylann
Copy link
Contributor

Nope not for the BE but we need to wait the addition of the report action in onyx takes effect on our report action list and lastVisibleAction of the report

As I understand, lastVisibleActionCreated shouldn't be updated in this case because the user created the request on the search page, the lastVisibleActionCreated only should be updated if the user comes back to the chat screen. So using runAfterInteractions as in your proposal seems like a workaround to me. In the scope of this issue, I think the selected proposal is enough to fix this issue

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 1, 2024

As I understand, lastVisibleActionCreated shouldn't be updated in this case because the user created the request on the search page, the lastVisibleActionCreated only should be updated if the user comes back to the chat screen.

@DylanDylann You are mixing up lastVisibleActionCreated with lastVisitTime, lastVisibleActionCreated is updated whenever a new visible action is created with created timestamp of the action so it should be updated whether it is created from chat screen or not.
Let me explain the purpose of the navigate here

if (!hasNewestReportActionRef.current) {
if (isInNarrowPaneModal) {
return;
}
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));

When we are linked to an old reportAction (comment linking) in a chat with many report actions and the most recent actions haven't been fetched/available and scrollToBottom is called we will check if we have the newest report action and if we don't we will navigate to the chat (without the report action link) to fetch the most recent actions.
So now if you think runAfterInteraction is a workaround we can apply my alternative solution of only navigating when isFocused is true which will solve the problem from the root cause that will solve similar problems too because we only want to navigate (and even to scrollToBottom) to (fetch latest actions) when the screen is focused. Just like we are early returning when the action is not from the current user here
if (!isFromCurrentUser) {
return;

that will ensure similar problems will not occur for other cases like sendInvoice trackExpense splitBill here createDistanceRequest.

@DylanDylann
Copy link
Contributor

DylanDylann commented Dec 2, 2024

@FitseTLT I overlooked that 😄. Thanks for your explanation

@DylanDylann You are mixing up lastVisibleActionCreated with lastVisitTime, lastVisibleActionCreated is updated whenever a new visible action is created with created timestamp of the action so it should be updated whether it is created from chat screen or not.

Anyway, I still think the selected proposal is a good approach in this case. Let's leave the final decision to the internal engineer

Copy link

melvin-bot bot commented Dec 11, 2024

📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Dec 11, 2024

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

@DylanDylann
Copy link
Contributor

@FitseTLT Please prioritize creating PR before posting new proposal

5. Contributors should **not** submit proposals on issues when they have assigned issues or PRs that are awaiting an action from them. If so, they will be in violation of Rule #1 (Get Shit Done) in our [Code of Conduct](https://github.com/Expensify/App/blob/main/CODE_OF_CONDUCT.md) and will receive a warning. Multiple warnings can lead to removal from the program.

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

This comment was marked as off-topic.

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Dec 24, 2024

Hi @FitseTLT @DylanDylann can you tell if this blocker is from this PR

@FitseTLT
Copy link
Contributor

Hi @FitseTLT @DylanDylann can you tell if this blocker is from this PR

No @Christinadobrzyn It can not be related to this 👍

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 26, 2024
@melvin-bot melvin-bot bot changed the title [$250] Search - App redirects to DM when submitting expense in Search after deleting expense [HOLD for payment 2025-01-02] [$250] Search - App redirects to DM when submitting expense in Search after deleting expense Dec 26, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 26, 2024
Copy link

melvin-bot bot commented Dec 26, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Dec 26, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.78-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-01-02. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Dec 26, 2024

@DylanDylann @abekkala @DylanDylann The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@garrettmknight garrettmknight moved this from Bugs and Follow Up Issues to Hold for Payment in [#whatsnext] #expense Dec 31, 2024
@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Jan 2, 2025
@abekkala
Copy link
Contributor

abekkala commented Jan 6, 2025

@DylanDylann can you complete the checklist above?

@melvin-bot melvin-bot bot removed the Overdue label Jan 6, 2025
@DylanDylann
Copy link
Contributor

DylanDylann commented Jan 7, 2025

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment: NA

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion: NA

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Test:

  1. Open DM with any user.
  2. Submit an expense to the user.
  3. Go to Search.
  4. Select the expense via checkbox.
  5. Click on the dropdown > Delete expense.
  6. Delete the expense.
  7. Open FAB while stay on Search page.
  8. Click Submit expense under Quick action.
  9. Enter amount and submit it.
  10. Verify that app stays in Search page.

Do we agree 👍 or 👎

@abekkala
Copy link
Contributor

abekkala commented Jan 7, 2025

@FitseTLT and @DylanDylann payments sent and contracts ended - thank you! 🎉

@abekkala abekkala closed this as completed Jan 7, 2025
@github-project-automation github-project-automation bot moved this from Hold for Payment to Done in [#whatsnext] #expense Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Status: Done
Development

No branches or pull requests

9 participants