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 2024-08-14] [$250] Track expense - App returns to confirmation page when tapping back button on category page #41783

Closed
4 of 6 tasks
izarutskaya opened this issue May 7, 2024 · 51 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

@izarutskaya
Copy link

izarutskaya commented May 7, 2024

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.71-0
Reproducible in staging?: Y
Reproducible in production?: Y
Found when validating PR : #41465
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Precondition:

  • Account has no workspace.
  1. Launch New Expensify app.
  2. Go to FAB > Track expense.
  3. Track a manual expense.
  4. In self DM, tap Categorize it.
  5. Select a category.
  6. On confirmation page, tap on the back button.
  7. On category list page, tap on the back button again.

Expected Result:

App will return to self DM.

Actual Result:

App returns to confirmation page.

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

Bug6473887_1715095933705.RPReplay_Final1715095326.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01824a5f82b4736dcc
  • Upwork Job ID: 1792915953607385088
  • Last Price Increase: 2024-05-28
  • Automatic offers:
    • tienifr | Contributor | 102600461
Issue OwnerCurrent Issue Owner: @anmurali
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 7, 2024
Copy link

melvin-bot bot commented May 7, 2024

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

@izarutskaya
Copy link
Author

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

@gijoe0295
Copy link
Contributor

gijoe0295 commented May 7, 2024

Proposal

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

App returns to confirmation page when going back in categorize's category page.

What is the root cause of that problem?

We don't need to navigate to confirmation page to reproduce this bug.

Root cause is here where we set backTo as IOU confirmation page when categorizing:

App/src/libs/ReportUtils.ts

Lines 6462 to 6463 in 7e82fe5

if (isCategorizing) {
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CATEGORY.getRoute(actionName, CONST.IOU.TYPE.SUBMIT, transactionID, expenseChatReportID, iouConfirmationPageRoute));

Additional issue

There's another issue that:

  1. Press Share it with my accountant
  2. Press Back
  3. Verify app navigates to Participants page

Expected: App navigates back to the self-DM

The root cause here is that we used participantsAutoAssigned to determine whether to navigate back to Participants page. However, this is an optimistic property and only exists during creation flow (in transactionDraft). As a result, the participantsAutoAssigned here is undefined as we used the "real" transaction in categorize flow.

if (!transaction?.participantsAutoAssigned) {
Navigation.goBack(ROUTES.MONEY_REQUEST_STEP_PARTICIPANTS.getRoute(iouType, transactionID, reportID, undefined, action));

This issue happens to all actionable whisper actions.

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

Remove the iouConfirmationPageRoute param to let the categorize's category page navigate back to its previous page which is the report screen.

To fix the additional issue, we need to set participantsAutoAssigned: true to transaction in getTrackExpenseInformation here. By that we would navigate back to the self-DM:

App/src/libs/IOUUtils.ts

Lines 12 to 13 in e1e22f0

if (iouAction === CONST.IOU.ACTION.CATEGORIZE || iouAction === CONST.IOU.ACTION.SUBMIT || iouAction === CONST.IOU.ACTION.SHARE) {
Navigation.goBack();

What alternative solutions did you explore? (Optional)

NA

@melvin-bot melvin-bot bot added the Overdue label May 9, 2024
Copy link

melvin-bot bot commented May 10, 2024

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

@gijoe0295
Copy link
Contributor

Hi @anmurali did you have a chance to take a look at this?

Copy link

melvin-bot bot commented May 14, 2024

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

@anmurali
Copy link

I can't reproduce this.

Screen.Recording.2024-05-14.at.9.39.10.PM.mov

@melvin-bot melvin-bot bot removed the Overdue label May 15, 2024
@gijoe0295
Copy link
Contributor

@anmurali Hi the issue is still reproducible.

In your videos, when you pressed the category menu in confirmation page and press back, it navigated to participant selector while it should be the confirmation page.

But that's another issue, the issue in OP is different. You need to press back while you're in confirmation screen, you don't need to press the category menu.

@melvin-bot melvin-bot bot added the Overdue label May 17, 2024
Copy link

melvin-bot bot commented May 20, 2024

@anmurali Eep! 4 days overdue now. Issues have feelings too...

@anmurali
Copy link

Thank you for the clarification @gijoe0295 - I am able to reproduce it.

@melvin-bot melvin-bot bot removed the Overdue label May 21, 2024
@anmurali anmurali added the External Added to denote the issue can be worked on by a contributor label May 21, 2024
@melvin-bot melvin-bot bot changed the title Track expense - App returns to confirmation page when tapping back button on category page [$250] Track expense - App returns to confirmation page when tapping back button on category page May 21, 2024
Copy link

melvin-bot bot commented May 21, 2024

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

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

melvin-bot bot commented May 21, 2024

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

@tienifr
Copy link
Contributor

tienifr commented May 21, 2024

Proposal

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

  1. When pressing "Categorize it" and back, app returns to confirmation page.
  2. When press "Share it with my accountant" and back, app returns to the participants page.

What is the root cause of that problem?

  1. In here, we're passing iouConfirmationPageRoute as the backTo, so when going back from the categorize page, it will go to the confirmation page. This is not expected since we should just go back to the DM
  2. In here, we're missing the check that the user is using a draft workspace, if the user is using a draft workspace, that means they did not select the participants and when going back should just go back to the DM. We can see the logic here, we'll use a draft workspace only when there's no other option to be selected

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

  1. Remove iouConfirmationPageRoute from here
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CATEGORY.getRoute(actionName, CONST.IOU.TYPE.SUBMIT, transactionID, expenseChatReportID));
  1. We'll be using the alternative solution below

What alternative solutions did you explore? (Optional)

We should consider other places too to see if the same fix is needed, like in here

An alternative for 2:

  • When calling "createDraftWorkspaceAndNavigateToConfirmationScreen" from "Share it with my accountant" button (or similar buttons), pass a flag like participantsAutoAssigned=true and in confirmation screen, use it in here to check too (besides the existing transaction?.participantsAutoAssigned)

More details:

  • First add the participantsAutoAssigned param to the route here
getRoute: (action: IOUAction, iouType: IOUType, transactionID: string, reportID: string, participantsAutoAssigned?: boolean) =>
            `${action as string}/${iouType as string}/confirmation/${transactionID}/${reportID}${participantsAutoAssigned ? '?participantsAutoAssigned=true' : ''}` as const,
  • In here define the param
params: {iouType, reportID, transactionID, action, participantsAutoAssigned},
  • Then in here add the check for the participantsAutoAssigned in the param
if (!transaction?.participantsAutoAssigned && participantsAutoAssigned !== 'true') {
  • In createDraftWorkspaceAndNavigateToConfirmationScreen here, pass participantsAutoAssigned as true when getting the confirmation route
const iouConfirmationPageRoute = ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(actionName, CONST.IOU.TYPE.SUBMIT, transactionID, expenseChatReportID, true);

We can be even more specific by doing so only if we're from the Share it with my accountant flow.

The code can be further polished like parsing the participantsAutoAssigned in the route to boolean before passing to the component, ...

Copy link

melvin-bot bot commented May 21, 2024

@anmurali @sobitneupane 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!

@sobitneupane
Copy link
Contributor

Thanks for the proposal @gijoe0295 and @tienifr

I cannot reproduce the issue mentioned in the OP as well as the issue in Share it with my accountant. The only issue I can reproduce is 1. Categorize it > 2. Select workspace> 3. Select a Category > 4. Press on Category in Confirmation Page > Instead of opening a new Category Page, you will be send back to the Category Page from Step 3. I am not sure if it's an issue or intended behavior.

Can any of you reproduce any other issues? If yes, Can you please share reproductions steps and on which platform it can be reproduced?

@gijoe0295
Copy link
Contributor

gijoe0295 commented May 22, 2024

@sobitneupane I could reproduce the OP. Note that you need to press back twice: one in confirmation page and the other in category page.

Screen.Recording.2024-05-22.at.20.57.46-source.mov

The issue you mentioned also has the same root cause. Several bugs that I already mentioned in my proposal but rephrase here with a recording:

  1. Press Categorize it
  2. Press Back in Category page
  3. Observe app navigates to Confirmation page

  1. Press Share it with my accountant
  2. Press Back
  3. Verify app navigates to Participants page
Screen.Recording.2024-05-22.at.20.59.53-source.mov

@tienifr
Copy link
Contributor

tienifr commented May 22, 2024

The only issue I can reproduce is 1. Categorize it > 2. Select workspace> 3. Select a Category

@sobitneupane I think you might have missed the pre-condition in the OP.

Precondition:
Account has no workspace.

Could you make sure to use a new account (without any workspace) and try the OP steps again?

@sobitneupane
Copy link
Contributor

Thank you @gijoe0295 and @tienifr

I can reproduce the issue now.

I think you might have missed the pre-condition in the OP.

Yup. I missed the pre-condition.

@melvin-bot melvin-bot bot added the Awaiting Payment Auto-added when associated PR is deployed to production label Aug 7, 2024
@melvin-bot melvin-bot bot changed the title [$250] Track expense - App returns to confirmation page when tapping back button on category page [HOLD for payment 2024-08-14] [$250] Track expense - App returns to confirmation page when tapping back button on category page Aug 7, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Aug 7, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

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

Copy link

melvin-bot bot commented Aug 7, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.17-2 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 2024-08-14. 🎊

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

Copy link

melvin-bot bot commented Aug 7, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
  • [@sobitneupane] 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:
  • [@sobitneupane] A discussion in #expensify-bugs 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:
  • [@sobitneupane] Determine if we should create a regression test for this bug.
  • [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Aug 14, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

@anmurali, @sobitneupane, @srikarparsi, @tienifr Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Aug 20, 2024

@anmurali, @sobitneupane, @srikarparsi, @tienifr Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented Aug 22, 2024

@anmurali, @sobitneupane, @srikarparsi, @tienifr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@srikarparsi
Copy link
Contributor

@sobitneupane can you complete the checklist whenever you have a chance please

@anmurali
Copy link

@tienifr the offer extended expired. So I resent a new one - https://www.upwork.com/nx/wm/offer/103712326

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 28, 2024
@sobitneupane
Copy link
Contributor

Regression Test Proposal:

  1. Log in with an account with no workspace
  2. Create a track expense
  3. Press Categorize it
  4. Press Back
  5. Verify app dismisses the RHP
  6. Press Share it with my accountant
  7. Press Back
  8. Verify app dismisses the RHP

Do we agree 👍 or 👎

@sobitneupane
Copy link
Contributor

sobitneupane commented Sep 2, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

This issue likely arose when the feature to categorize tracked expenses was added.

  • [@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

  • [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

#41783 (comment)

Copy link

melvin-bot bot commented Sep 2, 2024

@anmurali, @sobitneupane, @srikarparsi, @tienifr Eep! 4 days overdue now. Issues have feelings too...

@tienifr
Copy link
Contributor

tienifr commented Sep 4, 2024

@tienifr the offer extended expired. So I resent a new one - https://www.upwork.com/nx/wm/offer/103712326

@anmurali Hi I receive payment via ND, I'll submit there.

Thanks 🙏

@garrettmknight
Copy link
Contributor

@anmurali please post a payment summary for @tienifr when you get a chance.

Copy link

melvin-bot bot commented Sep 4, 2024

@anmurali, @sobitneupane, @srikarparsi, @tienifr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

Copy link

melvin-bot bot commented Sep 6, 2024

@anmurali, @sobitneupane, @srikarparsi, @tienifr Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@anmurali
Copy link

Payment Summary

  • $250 to @tienifr approved, please request on ND

@melvin-bot melvin-bot bot removed the Overdue label Sep 10, 2024
@sobitneupane
Copy link
Contributor

@anmurali I will also require the payment summary to request money in newdot.

@garrettmknight
Copy link
Contributor

$250 approved for @tienifr

@sobitneupane
Copy link
Contributor

@anmurali Bump for the payment summary.

@sobitneupane
Copy link
Contributor

Bump @anmurali

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
No open projects
Status: No status
Development

No branches or pull requests

7 participants