Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

[HOLD #21518][$1000] Web - Blank Pages Displayed After Splitting Bill and Returning from Concierge Page #23895

Closed kbecciv closed 12 months ago

kbecciv commented 1 year 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!


Action Performed:

  1. Click on the plus sign, select "Split Bill", add the desired number, click "Next", choose email, and click "Split Bill".
  2. The split money appears in the composer. Click on the money.
  3. Click on the three dots in the top left corner, then select "Delete" The app redirects you to the Concierge page.
  4. Go back, and observe that a blank page is displayed.

Expected Result:

The app should not show blank pages after returning from the Concierge page following a split bill.

Actual Result:

After returning from Concierge following a split bill, the app displays blank pages.

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.47-3 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/579295f0-23ef-4a90-8a18-e226adcf5a3d

https://github.com/Expensify/App/assets/93399543/ca75e456-f3f5-4a67-ac49-2d4c25b28ab5

Expensify/Expensify Issue URL: Issue reported by: @tewodrosGirmaA Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690622954068189

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c5608dd0a0136fdb
  • Upwork Job ID: 1689058798459588608
  • Last Price Increase: 2023-08-08
  • Automatic offers:
    • tienifr | Contributor | 26076517
    • tewodrosGirmaA | Reporter | 26076519
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

s-alves10 commented 1 year ago

Proposal

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

Blank page displayed when splitting bill, deleting the money request, and going back

What is the root cause of that problem?

When deleting a money request, we call DeleteMoneyRequest API call and navigate to the iou or chat report here https://github.com/Expensify/App/blob/024d210bc0b5a0454b62a72de60057af67cdd045/src/libs/actions/IOU.js#L965-L977

In the case shouldDeleteIOUReport is true, we only go back once and navigate to chat report. But if we create split bill and go to the transaction report, the navigation looks like

    chat report -> iou report -> money request report

Now that we go back and navigate to chat report, the current navigation stack would look like

    chat report -> iou report -> chat report

If we go back, navigation would go to iou report, but this was already deleted and empty page appears This is the root cause

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

In the case shouldDeleteIOUReport is true, we have to go back to the chat report and navigate In order to do this, create an utility function getReportIdInStack(depth = 0) in Navigation.js

const getReportIdInStack = (depth = 0) => {
    const state = navigationRef.getState();
    console.log(state.routes);
    let reportCount = 0;
    for (let i = state.routes.length - 1; i >= 0; i --) {
        const route = state.routes[i];
        if (route.name === 'CentralPaneNavigator') {
            if (depth === reportCount) {
                return route.params.params.reportID;
            }
            reportCount ++;
        } else {
            return '';
        }
    }

    return '';
}

And go back twice if we need to go back once more in deleteMoneyRequest function

    if (shouldDeleteIOUReport) {
        // Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
        const previousReportId = Navigation.getReportIdInStack(1);
        Navigation.goBack();
        if (previousReportId === reportAction.originalMessage.IOUReportID.toString()) {
            Navigation.goBack();
        }
        Navigation.navigate(ROUTES.getReportRoute(iouReport.chatReportID));
    }

This works perfect

Result https://github.com/Expensify/App/assets/126839717/e0af4ac3-4eab-417e-9dcb-454fd022cdb9

What alternative solutions did you explore? (Optional)

alexpensify commented 1 year ago

I'll test soon

alexpensify commented 1 year ago

@allroundexperts - can I get your feedback here? https://github.com/Expensify/App/issues/22964 is about when you click into the Split Bill. Will the fix for that one address the Delete button which also appears to be taking you to the Concierge chat? Thanks for checking if the upcoming fix address this one too.

allroundexperts commented 1 year ago

Hi @alexpensify! I don't think that the two are related. When a request is deleted, it should take the user to the concierge chat as far as I know.

alexpensify commented 1 year ago

Got it, thank you for confirming this isn't a duplicate! I'll test to confirm if I get the blank page then.

alexpensify commented 1 year ago

Still on my test list

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @alexpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

alexpensify commented 1 year ago

I was able to replicate it, and I marked it as External. I did notice a JS snippet in the process.

image

tienifr commented 1 year ago

Proposal

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

After returning from Concierge following a split bill, the app displays blank pages.

What is the root cause of that problem?

In here https://github.com/Expensify/App/blob/024d210bc0b5a0454b62a72de60057af67cdd045/src/libs/actions/IOU.js#L972, when deleting a money request, we're going back to the previous page, then navigate to the chat report.

But in this case, the IOU report is also deleted (since it's the only money request in that IOU report), so when we go back to that page we navigate to the Concierge page (as per the logic in here)

The way we're handling that Concierge navigation is not ideal, it causes us to add workarounds all around the places like here and here where to avoid unexpected Concierge navigation when we optimistically delete a report.

We shouldn't have to add those workarounds.

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

We need to have a better design for the Concierge navigation when the report is deleted.

  1. When we optimistically delete a report, we should set it to

    {
    pendingAction: 'delete'
    }

    instead of null.

  2. Then in successData, we can set the report to null in Onyx.

  3. In here, we check if we're optimistically deleting the report, we'll not do the Concierge navigation because we already handle the navigation in the optimistic delete logic itself.

  4. We can remove the Navigation.goBack workaround in here and other similar places since it will not navigate to Concierge unexpectedly now

  5. There might be a few other places that will need to change to filter out the pendingAction: 'delete'. But overall this is better design. We already did this for the reportActions (it also has pendingAction) so we should do it for the report too.

What alternative solutions did you explore? (Optional)

We can add another Navigation.goBack here for it to go back all the way to the chat report (avoiding the deleted iou report), https://github.com/Expensify/App/blob/024d210bc0b5a0454b62a72de60057af67cdd045/src/libs/actions/IOU.js#L974

alexpensify commented 1 year ago

@mananjadhav - when you get a chance, can you please review the recent proposal? Thanks!

mananjadhav commented 1 year ago

Thanks for the bump @alexpensify. @tienifr's proposal looks good to me.

I think setting optimistic data is a better design, but I will let @deetergp confirm if we want to take Navigation approach.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed.

melvin-bot[bot] commented 1 year ago

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

deetergp commented 1 year ago

If that's what we are already doing with reportAction, let's go ahead and do the same with IOUs. @tienifr's solution looks good to me too.

melvin-bot[bot] commented 1 year ago

πŸ“£ @mananjadhav Please request via NewDot manual requests for the Reviewer role ($1000)

melvin-bot[bot] commented 1 year 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 1 year ago

πŸ“£ @tewodrosGirmaA πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reporter role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

tienifr commented 1 year ago

@mananjadhav @deetergp I can't reproduce this bug because of the PR. What should we do next?

tienifr commented 1 year ago

I think we should hold this GH util the issue is merged

tienifr commented 1 year ago

@deetergp @mananjadhav any updates?

deetergp commented 1 year ago

Sorry, @tienifr I agreed with you yesterday β€” in my head β€” where it does you absolutely no good. πŸ˜… I'll put a hold on this one till #21518 is merged.

melvin-bot[bot] commented 1 year ago

@deetergp, @mananjadhav, @alexpensify, @tienifr Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 1 year ago

@deetergp, @mananjadhav, @alexpensify, @tienifr Eep! 4 days overdue now. Issues have feelings too...

alexpensify commented 1 year ago

Weekly Update: Still on hold for https://github.com/Expensify/App/issues/21518

alexpensify commented 1 year ago

Weekly Update: On Hold

alexpensify commented 1 year ago

Weekly Update: On Hold

alexpensify commented 1 year ago

Also, moving this to a weekly.

alexpensify commented 1 year ago

Weekly Update: On Hold

alexpensify commented 1 year ago

Weekly update: Should be in production soon.

alexpensify commented 1 year ago

Weekly Update: some conflicts are being reviewed

alexpensify commented 1 year ago

Weekly Update: still waiting for a conflict update

alexpensify commented 1 year ago

Weekly Update: By next week, we should be clear to move forward here!

alexpensify commented 12 months ago

@tienifr and @mananjadhav - can one of y'all test to identify if this is still an issue? The other PR was just moved to payment stages, so all clear here.

tienifr commented 12 months ago

@alexpensify I can't reproduce this GH anymore

alexpensify commented 12 months ago

Ok with this news, I'm going to close it out.

alexpensify commented 12 months ago

Thank you!

tienifr commented 11 months ago

hi @alexpensify, I think contributors are eligible for compensation here since the solution was valid at the point of assignment and the PR process already started, just that due to some other changes, the solution became out of date.

Some other similar cases are this or this

TIA!

mananjadhav commented 11 months ago

Agreed with @tienifr here. Bump @alexpensify @deetergp.

alexpensify commented 11 months ago

There have been a few updates to the partial payment process. I need to ask a few questions and would like your opinions - @tienifr and @mananjadhav :

  1. What compensation do you think is the percent due here for the work input here - 0, 25, 50, OR 100%?
  2. Reasoning why has been shared https://github.com/Expensify/App/issues/23895#issuecomment-1751144412

Thanks!

tienifr commented 11 months ago

What compensation do you think is the percent due here for the work input here - 0, 25, 50, OR 100%?

@alexpensify I believe that should be 100% based on the precedents mentioned in here, thanks!

alexpensify commented 11 months ago

@mananjadhav we need your feedback here, please reply with the percent. Thanks!

tienifr commented 11 months ago

@mananjadhav we need your feedback here, please reply with the percent. Thanks!

@mananjadhav gentle bump on this, thanks!

mananjadhav commented 11 months ago

Thanks for the tag @tienifr. I missed this because the issue was closed. Based on the linked discussions I also feel it should be 100%.

alexpensify commented 11 months ago

Thanks, I will work on the process this week.

alexpensify commented 11 months ago

Here is the payment summary:

Upwork Job: https://www.upwork.com/jobs/~01c5608dd0a0136fdb

*If applicable, the bonuses will be applied on the final payment

Extra Notes regarding payment: There is no urgency bonus. Payments have been sent out to those who receive payments there.

JmillsExpensify commented 11 months ago

$1,000 payment approved for @mananjadhav based on comment above.