[HOLD for payment 2024-08-14] [$250] Track expense - App returns to confirmation page when tapping back button on category page

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

Action Performed:

Precondition:

Account has no workspace.

Launch New Expensify app.
Go to FAB > Track expense.
Track a manual expense.
In self DM, tap Categorize it.
Select a category.
On confirmation page, tap on the back button.
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?

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/f686b437-8347-4bce-a700-2be2741f4642

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 Owner

Current Issue Owner: @anmurali

melvin-bot[bot] commented 5 months ago

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 commented 5 months ago

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

gijoe0295 commented 5 months ago

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:

https://github.com/Expensify/App/blob/7e82fe57e41fca5c71017a719f2e9fdd86f1cfcd/src/libs/ReportUtils.ts#L6462-L6463

Additional issue

There's another issue that:

Press Share it with my accountant
Press Back
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.

https://github.com/Expensify/App/blob/7e82fe57e41fca5c71017a719f2e9fdd86f1cfcd/src/pages/iou/request/step/IOURequestStepConfirmation.tsx#L188-L189

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:

https://github.com/Expensify/App/blob/e1e22f0fb9cf7d6a1ca5c9b3ddd243462b731234/src/libs/IOUUtils.ts#L12-L13

What alternative solutions did you explore? (Optional)

NA

melvin-bot[bot] commented 5 months ago

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

gijoe0295 commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

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

anmurali commented 5 months ago

I can't reproduce this.

https://github.com/Expensify/App/assets/14225673/f2796596-2f2c-4b51-bc09-b7cac59b95e8

gijoe0295 commented 5 months ago

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

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

anmurali commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

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

tienifr commented 5 months ago

Proposal

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

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

What is the root cause of that problem?

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

Remove iouConfirmationPageRoute from here

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

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, ...

melvin-bot[bot] commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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

https://github.com/Expensify/App/assets/153004152/8fec3e9a-c96d-4bcb-b780-c56b51e60cfb

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:

Press Categorize it
Press Back in Category page
Observe app navigates to Confirmation page

Press Share it with my accountant
Press Back
Verify app navigates to Participants page

https://github.com/Expensify/App/assets/153004152/b31629d3-8b87-4b91-88ac-eed983161b59

tienifr commented 5 months ago

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 commented 5 months ago

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.

sobitneupane commented 5 months ago

Thanks for your proposal @gijoe0295

FYI We cannot set the participantsAutoAssigned in getTrackExpenseInformation as https://github.com/Expensify/App/issues/41783#issuecomment-2098932814 because participantsAutoAssigned is a client-side only properly, once the user logged out and logged back in, it will not be available and the issue will happen again.

I agree with @tienifr that 2nd part of your proposal might not work once user log out and log back in.

gijoe0295 commented 5 months ago

@sobitneupane Did you have final decision on this?

sobitneupane commented 5 months ago

Thanks for the proposal @tienifr

Can you please add more details to the first alternative solution in your proposal. I want to see how it works.

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

tienifr commented 5 months ago

Can you please add more details to the first alternative solution in your proposal. I want to see how it works.

@sobitneupane I've done that! Proposal updated

Please kindly review again

sobitneupane commented 5 months ago

Thanks for the update @tienifr

Alternative solution 1 from @tienifr's propsoal looks good to me.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 4 months ago

@anmurali, @sobitneupane, @srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 4 months ago

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

anmurali commented 4 months ago

@srikarparsi - do you agree with https://github.com/Expensify/App/issues/41783#issuecomment-2141466185

srikarparsi commented 4 months ago

Sorry for missing this, was OOO last week. @tienifr's alternative looks good to me as well :)

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 3 months ago

This issue has not been updated in over 15 days. @anmurali, @sobitneupane, @srikarparsi, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

https://github.com/Expensify/App/pull/43163

If no regressions arise, payment will be issued on 2024-08-14. :confetti_ball:

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

@sobitneupane requires payment through NewDot Manual Requests
@tienifr requires payment automatic offer (Contributor)

melvin-bot[bot] commented 2 months ago

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

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

srikarparsi commented 2 months ago

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

anmurali commented 2 months ago

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

sobitneupane commented 1 month ago

Regression Test Proposal:

Log in with an account with no workspace
Create a track expense
Press Categorize it
Press Back
Verify app dismisses the RHP
Press Share it with my accountant
Press Back
Verify app dismisses the RHP

Do we agree 👍 or 👎

sobitneupane commented 1 month ago

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.

https://github.com/Expensify/App/issues/41783#issuecomment-2324103115

melvin-bot[bot] commented 1 month ago

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

tienifr commented 1 month ago

@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 commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@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 commented 1 month ago

Payment Summary

[ ] $250 to @tienifr approved, please request on ND

sobitneupane commented 1 month ago

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

garrettmknight commented 1 month ago

$250 approved for @tienifr

sobitneupane commented 1 week ago

@anmurali Bump for the payment summary.

Expensify / App