[HOLD for payment 2024-02-07] [$500] Web - Chat- Hmm it's not here page displayed on reloading page after selecting Request money

lanitochka17 commented 10 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.22-0 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Login with gmail account on ND site 2.Go to any 1:1 DM
Tap + FAB button
Select either Manual, Scan or Distance option
Reload page

Expected Result:

Previous opened chat history should display on reloading page after selecting Request money options

Actual Result:

Hmm... it's not here page displayed on reloading page after selecting Request money

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/9a3aea78-4dea-4ad4-8d4e-733ae38f3dc2

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01356e28bc3ed07d0f
Upwork Job ID: 1743071894141489152
Last Price Increase: 2024-01-19
Automatic offers:

cubuspl42 | Reviewer | 28120404
DylanDylann | Contributor | 28120405

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 10 months ago

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

unidev727 commented 10 months ago

Proposal

from: @unicorndev-727

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

Chat- Hmm it's not here page displayed on reloading page after selecting Request money.

What is the root cause of that problem?

The root cause is that we use new reportId(doesn't exist) when we create money request and CustomRouter returns random reportId for CENTRAL_PANE_NAVIGATOR here in this case without checking report.

create/request/start/1/8425039469826829/manual | scan | distance The route has reportId param so the getLastAccessedReportID doesn't work in these 3 routes.

https://github.com/Expensify/App/blob/dc3e19bdcc95023d74cd7fdd433f6208707eaaf8/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L31-L33

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

We should check if report exists regardless reportId is valid here and if report doesn't exist, we shouldn't return that reportId. https://github.com/Expensify/App/blob/dc3e19bdcc95023d74cd7fdd433f6208707eaaf8/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L30-L32

to

if (topmostRoute?.params && 'reportID' in topmostRoute.params && typeof topmostRoute.params.reportID === 'string' && topmostRoute.params.reportID && ReportUtils.getReport(topmostRoute.params.reportID)?.reportID) {
        return topmostRoute.params.reportID;
}

screen-capture (4).webm

What alternative solutions did you explore?

N/A

tienifr commented 10 months ago

Proposal

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

Hmm... it's not here page displayed on reloading page after selecting Request money

What is the root cause of that problem?

When creating money request via FAB, we'll generate a random reportID and put in the route. When reloading we're unable to find the report associated with that reportID so this issue happens.

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

If we're on the request money/split bill/related route, and we cannot find any report matching reportID in Onyx, in here fallback to the getLastAccessedReportID.

We can optionally optimistically create the report for those reportID as well, with the isOptimisticReport: true field, and when we find the request money/split bill report by the reportID, if what we found is an optimistic report, then fallback to using getLastAccessedReportID. We can then clear this optimistic report when we complete/get out of the money request flow.

What alternative solutions did you explore? (Optional)

NA

yh-0218 commented 10 months ago

Proposal

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

Hmm... it's not here page displayed on reloading page after selecting Request money

What is the root cause of that problem?

When we create money request via FAB, we generate a random reportID and put in the route. This random reportID make this issue.

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

We need to compare route?.params?.reportID and getLastAccessedReportID here

if (route?.params?.reportID === reportID) {

What alternative solutions did you explore? (Optional)

https://github.com/Expensify/App/assets/65789015/12117019-b8aa-434f-be80-9df79f9350e3

DylanDylann commented 10 months ago

Proposal

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

Web - Chat- Hmm it's not here page displayed on reloading page after selecting Request money
What is the root cause of that problem?
When we open the app by the deeplink https://dev.new.expensify.com:8082/create/request/start/1/317873788367143/manual, we add the report to the CENTRAL_PANE_NAVIGATOR by the below function: https://github.com/Expensify/App/blob/6fad557b600c658a06368697a791ff1acc7488d5/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L42-L43
In this case, the getTopMostReportIDFromRHP will return the 317873788367143, it is the random reportID is generated when we create iou request and have null value, that leads to the "Hmm it`s not here page" screen displayed.

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

We need to make sure when we open the deeplink https://dev.new.expensify.com:8082/create/request/start/1/317873788367143/manual, app does not add the report screen with reportID = 317873788367143 to CENTRAL_PANE_NAVIGATOR

We can do it by updating getTopMostReportIDFromRHP to:


const getTopMostReportIDFromRHP = (state: State): string => {
if (!state) {
    return '';
}
const topmostRightPane = state.routes.filter((route) => route.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR).at(-1);

if (topmostRightPane?.state) {
    return getTopMostReportIDFromRHP(topmostRightPane.state);
}
const topmostRoute = state.routes.at(-1);

if (topmostRoute?.params && topmostRoute.params.iouType === "request") {
return '';
}

if (topmostRoute?.state) { return getTopMostReportIDFromRHP(topmostRoute.state); } if (topmostRoute?.params && 'reportID' in topmostRoute.params && typeof topmostRoute.params.reportID === 'string' && topmostRoute.params.reportID) { return topmostRoute.params.reportID; }

return ''; };

What alternative solutions did you explore? (Optional)

NA

melvin-bot[bot] commented 10 months ago

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

adelekennedy commented 10 months ago

@cubuspl42 it looks like there are a few proposals to review!

cubuspl42 commented 10 months ago

@unicorndev-727

We need to consider above 3 routes so that getLastAccessedReportID works properly.

Consider how? I don't understand your solution, sorry.

@tienifr

fallback to the getLastAccessedReportID.

What's the UX of this solution? Does the "random" report ID stay in the URL bar?

We can optionally optimistically create the report for those reportID as well

This brings garbage collection challenges... Do you think this is a realistic option? Anything goes "wrong" (the user closes a tab, whatever) and we're left with garbage in this solution, right?

@DylanDylann

I think we have a different understanding what a deeplink is. In the context of this Web-specific issue, you probably just mean "URL".

We need to make sure when we open ... app does not add the report screen with reportID = 317873788367143 to CENTRAL_PANE_NAVIGATOR

You stick to the example, but what's the logic of the suggested solution?

I guess that it's something like "when te report ID in the URL points to a non-existing report, we...", but you didn't state this explicitly. Examples are good, but they can't be the only thing describing a solution.

We can update getTopMostReportIDFromRHP to:

You said "can". Is it an optional part of the solution?

Let's focus on the code diff now.

First, a nit. Isn't topmostRoute?.params && topmostRoute.params.iouType equivalent to topmostRoute?.params?.iouType (when converted to a boolean)?

Leaving nits aside... From the proposal itself, I don't think it's clear how the first change (not adding a route with invalid report ID to CENTRAL_PANE_NAVIGATOR) relates to the second (adjusting getTopMostReportIDFromRHP).

unidev727 commented 10 months ago

@unicorndev-727

We need to consider above 3 routes so that getLastAccessedReportID works properly.

Consider how? I don't understand your solution, sorry.

https://github.com/Expensify/App/blob/dc3e19bdcc95023d74cd7fdd433f6208707eaaf8/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts#L47

We need to add a new condition here if report with this reportId exists.

if (route?.params?.reportID && ReportUtils.getReport(route.params.reportID)) {

So if report doesn't exist, we can set lase accessed report id from getLastAccessedReportID.

unidev727 commented 10 months ago

@cubuspl42 I updated the proposal and added the result video.

cubuspl42 commented 10 months ago

@unicorndev-727

So, what actually happens after the reload in the proposed solution, as I understand it:

In the URL, we present the old random ID of a temporary report (which doesn't exist anymore)
The RHP presents the "Request money" flow associated with a new report with a new random ID

Is this correct?

unidev727 commented 10 months ago

Now it is.

cubuspl42 commented 10 months ago

This is a bit work-aroundish, if not hacky, to present some "garbage" from the URL, just because it makes the implementation easier.

cubuspl42 commented 10 months ago

Everyone,

I do not understand one important high-level thing here. Wy do we include the report ID in the URL, if this is not a persisted entity?

Why... /create/request/start/1/317873788367143/manual Why not... /create/request/start/1/manual

Also, what is 1?

unidev727 commented 10 months ago

That is transactionID.

MONEY_REQUEST_CREATE: {
        route: 'create/:iouType/start/:transactionID/:reportID',
        getRoute: (iouType: ValueOf<typeof CONST.IOU.TYPE>, transactionID: string, reportID: string) => `create/${iouType}/start/${transactionID}/${reportID}` as const,
    },

DylanDylann commented 10 months ago

@cubuspl42 I updated proposal based on your comment

cubuspl42 commented 10 months ago

@DylanDylann Oh, I misunderstood your proposal structure. The second bullet is the implementation of the idea from the first one. This changes a lot.

Again, nit: Isn't this equivalent to topmostRoute?.params?.iouType === "request"?

Also, what's the UX of this solution? How does the URL behave from user's perspective? Does the "old" random ID stay in the URL bar?

DylanDylann commented 10 months ago

@cubuspl42

Isn't this equivalent to topmostRoute?.params?.iouType === "request"?

What is "this" here?

Does the "old" random ID stay in the URL bar?

Yes. Because when user opens /create/request/start/1/317873788367143/manual, the RequestMoney screen is focused, so the reportID 317873788367143 still stays in the URL bar

cubuspl42 commented 10 months ago

Sorry for being unclear.

I meant: isn't the topmostRoute?.params && topmostRoute.params.iouType === "request" expression equivalent to the topmostRoute?.params?.iouType === "request" expression?

Yes. Because when user opens [...]

If I understand you correctly, this comment applies:

https://github.com/Expensify/App/issues/33990#issuecomment-1882812880

To double-ensure I get you right:

I understand it this way, that we kind of ignore the report ID in the URL (if it points to a non-existin report), but keep presenting it. For example, when I request... /create/request/start/1/133713371337133/manual, we'll generate a new ad-hoc report ID in-memory, but will keep presenting 133713371337133 in the URL bar.

tienifr commented 10 months ago

What's the UX of this solution? Does the "random" report ID stay in the URL bar?

@cubuspl42 yes, it stays as is, just that the report behind will be the correct one rather than Not found page

This brings garbage collection challenges... Do you think this is a realistic option? Anything goes "wrong" (the user closes a tab, whatever) and we're left with garbage in this solution, right?

@cubuspl42 yeah I think garbage collection is a problem, that's why its optional, only the fallback part should work fine

DylanDylann commented 10 months ago

@cubuspl42

I meant: isn't the topmostRoute?.params && topmostRoute.params.iouType === "request" expression equivalent to the topmostRoute?.params?.iouType === "request" expression?

Yes they are the same

that we kind of ignore the report ID in the URL (if it points to a non-existin report), but keep presenting it.

We always ignore the reportID in the URL that have the form /create/request/start/1/133713371337133/manual, regardless it is a non-existed report or not (when executing function getTopMostReportIDFromRHP)

unidev727 commented 10 months ago

@cubuspl42 I found how to send correct reportId. Plz check my updated proposal.

unidev727 commented 10 months ago

We always ignore the reportID in the URL that have the form /create/request/start/1/133713371337133/manual, regardless it is a non-existed report or not (when executing function getTopMostReportIDFromRHP)

If we create a money request and refresh browser, we always get reportId from /create/request/start/1/133713371337133/manual because we check RHP first here.

https://github.com/Expensify/App/blob/dc3e19bdcc95023d74cd7fdd433f6208707eaaf8/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L18-L23

And none-existed reportId is returned in this case. So it is important to check if report exist or not, I think.

yh-0218 commented 10 months ago

hi, @cubuspl42 what is opinion for my proposal

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

@cubuspl42, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

adelekennedy commented 10 months ago

lots of good back and forth here - it looks like there are two updated proposal to review but we are still open for more solutions

adelekennedy commented 10 months ago

@cubuspl42 I think there are two updated proposals to review after the last round of feedback

cubuspl42 commented 10 months ago

I'll have to come back to this tomorrow. I had some complications in other issues I handle, but I burned nearly all of my queue; we'll wrap this up soon

cubuspl42 commented 10 months ago

@DylanDylann Could you explain what app behavior is triggered when the special "undefined" ('') report ID is returned from the function you suggest modifying? Pointing to a code fragment that handles that case would also be helpful.

cubuspl42 commented 10 months ago

hi, @cubuspl42 what is opinion for my proposal

@yh-0218

Could you explain the consequences of adding the check you suggest? Doesn't it strongly restrict the app's functionality?

DylanDylann commented 10 months ago

@cubuspl42

what app behavior is triggered when the special "undefined" ('') report ID is returned from the function you suggest modifying

When we open the app by any URL, we add the report with reportId='' to the CENTRAL_PANE_NAVIGATOR. The screen stacks is [{name: "home"}, {name: CentralPaneNavigator, state: {routes: {name: Report, params: {reportID: ''}}}]
Then, in the ReportScreenIDSetter, it checks https://github.com/Expensify/App/blob/3e9d21c4beec51d82d081d930483ed9eafa39f5f/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts#L47 and in this case, route?.params?.reportID: false, so, it will call https://github.com/Expensify/App/blob/3e9d21c4beec51d82d081d930483ed9eafa39f5f/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts#L63 Now, the screen stacks is [{name: "home"}, {name: CentralPaneNavigator, state: {routes: {name: Report, params: {reportID: 'not-empty-report-ID'}}}]

cubuspl42 commented 10 months ago

@DylanDylann

I tried applying your diff, and it gives me type errors.

Does the solution work when refreshing the money request flow started in a context of a report, for example workspace? I'm talking about "Small plus" > "Request money".

DylanDylann commented 10 months ago

@cubuspl42 What type errors that you encountered? Can you share a screenshot?

cubuspl42 commented 10 months ago

@cubuspl42 What type errors that you encountered? Can you share a screenshot?

It's a TS error in my IDE. I hope it's not a misconfiguration on my side.

TS2339: Property 'iouType' does not exist on type 'object'.

DylanDylann commented 10 months ago

@cubuspl42

TS2339: Property 'iouType' does not exist on type 'object'.

Yes. I also encountered this error from my side but I think we can fix it in the detailed PR

cubuspl42 commented 10 months ago

I know it would be convenient for at least one party involved, but I'm having a reservation that could make the proposal not acceptable for selection. Again:

Does the solution work when refreshing the money request flow started in a context of a report, for example workspace? I'm talking about "Small plus" > "Request money".

DylanDylann commented 10 months ago

Does the solution work when refreshing the money request flow started in a context of a report, for example workspace? I'm talking about "Small plus" > "Request money".

Yes. It works well. You can try it. It works because based on here, in ReportScreenIDSetter, we have getLastAccessedReportID that will return the correct reportID

cubuspl42 commented 10 months ago

Okey, nevermind. I tested this myself.

Refreshing works. Navigation doesn't.

When we paste a link like...

https://dev.new.expensify.com:8082/create/request/start/1/5810782674686301/manual

...to the new tab (on the Web / big screen) the app opens the report 5810782674686301 and opens the New Money Request flow in RHP. After applying the change, it doesn't work anymore.

DylanDylann commented 10 months ago

@cubuspl42

Can you share a video?

After applying the change

What is the "change" that you mentioned?

cubuspl42 commented 10 months ago

Everyone,

Idea:

is the '0' report ID valid? I've seen the code falling back to it as if it was sort-of NULL.

Could we extract it as a NULL_REPORT_ID constant? When we start the Money Request flow from FAB, we'd go to...

https://dev.new.expensify.com:8082/create/request/start/1/0/manual

0 in the .../0/... is the NULL_REPORT_ID I'm talking about. If we see it during processing the URL, we fall back to last-accessed-report. Otherwise, we assume the report ID is a real report ID and treat it as we always did. If it's non-NULL but ends up being a deleted/corrupted/otherwise non-pointing to an actual report, we end up on the 404 view and consider this expected.

cubuspl42 commented 10 months ago

What is the "change" that you mentioned?

Your proposed change.

DylanDylann commented 10 months ago

@cubuspl42

I shared the video from my side. It works well or I misunderstood your reproduction steps?

Screencast from 17-01-2024 16:29:09.webm

cubuspl42 commented 10 months ago

Refreshing works. There's another issue. Exact reproduction steps:

Open a desktop Web browser
Open the first tab
- Open report A
- Press "Actions" > "Request money"
- Copy the URL
- Open report B (any other report)
Open a new tab. In this second tab, paste the previously copied URL
- [ ] Verify that the Request Money flow opens in a RHP
- [ ] Verify that the report A is the target of the IOU in the flow
- [ ] Verify that the central pane shows report A in the background

DylanDylann commented 10 months ago

@cubuspl42

Verify that the central pane shows report A in the background

I think it should be the report B. Because report B is the report that we are opening

cubuspl42 commented 10 months ago

@DylanDylann

Have you checked the current (production / main) app behavior? Have you noticed the "In the new tab, paste the previously copied URL" step?

I edited the steps trying to add more clarity.

DylanDylann commented 10 months ago

Have you checked the current (production / main) app behavior?

Yes, I checked. The current behavior in the staging is that, it will display the report A in the central. In other words, it will display the report that has reportID in /create/request/start/1/:reportID/manual, regardless that the report exists or not.
In this case (in staging), the report has existed, so it will display the report A in central.
But in case of the current bug (in staging), because the report has not existed, so the "Hmm its not here page" displayed

Expensify / App