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

Always close the statement even after visiting multiple statement pages #9712

Closed
thienlnam opened this issue Jul 5, 2022 · 21 comments
Closed
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@thienlnam
Copy link
Contributor

thienlnam commented Jul 5, 2022

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. Copy this URL into your browser: https://new.expensify.com/statements/202112/
  2. Then this one: https://new.expensify.com/statements/202201/
  3. Then this one: https://new.expensify.com/statements/202202/
  4. Click the X on the statement page in the rightDocked side modal - observe you're returned to the URL in Fix spaces #2.
  5. Click the X again - observe that you're returned to the URL in Some initial fixes and code style updates #1.
  6. Click the X again - observe that the rightDocked side modal closes

Expected Result:

Describe what you think should've happened

  1. Clicking the close button (X) on the modal should close the statement modal and not redirect to the last statement you visited

Actual Result:

Describe what actually happened

  1. You get redirected to the last statement

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Yes, you can either just adjust the URL or just keep pressing X until you close the modal out

Platform:

Where is this issue occurring?

  • Web x
  • iOS
  • Android
  • Desktop App
  • Mobile Web x

Version Number: v1.1.78-8
Reproducible in staging?: Yes
Reproducible in production?: Yes
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

9aAfBSiKNJ

Upwork URL: https://www.upwork.com/jobs/~01ce48a3c10dcdbe87
Issue reported by:
Slack conversation:

View all open jobs on GitHub

@thienlnam thienlnam added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Jul 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 5, 2022

Triggered auto assignment to @sonialiap (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 5, 2022
@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Jul 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 5, 2022

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 5, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 5, 2022

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

@melvin-bot melvin-bot bot changed the title Always close the statement even after visiting multiple statement pages [$250] Always close the statement even after visiting multiple statement pages Jul 5, 2022
@trjExpensify
Copy link
Contributor

Job on Upwork here: https://www.upwork.com/jobs/~01ce48a3c10dcdbe87

@thienlnam, just to check, does the contributor need access to a beta for this?

@parasharrajat
Copy link
Member

Can someone improve the Action Performed and add steps for how to navigate to statements?

@marcaaron
Copy link
Contributor

+1 - it would be helpful to have a video of the existing behavior. I'm not even sure what is being asked here.

@trjExpensify
Copy link
Contributor

Fair, it's just copying those three URLs above into the browser and clicking X on the modal. I've improve the steps a bit with the below to make that clearer, gif included!

  1. Copy this URL into your browser: https://new.expensify.com/statements/202112/
  2. Then this one: https://new.expensify.com/statements/202201/
  3. Then this one: https://new.expensify.com/statements/202202/
  4. Click the X on the statement page in the rightDocked side modal - observe you're returned to the URL in Fix spaces #2.
  5. Click the X again - observe that you're returned to the URL in Some initial fixes and code style updates #1.
  6. Click the X again - observe that the rightDocked side modal closes

@thienlnam can confirm if there are beta requirements to know about for this project for external.

@parasharrajat
Copy link
Member

I may have found another issue. When I open the URL https://new.expensify.com/statements/202112/ on the browser. I see a blank page.
image.

Request return 404 for the page (the reason could be that report is not generated for my account yet). But I think it should be 200 with some sort of UI Or we can handle this ourselves in the App and show a UI when 404 is returned.

Logic would be:

  1. Make the fetch call directly to the statement resource URL and do not pass the URL to src of the iframe we are using.
  2. Now based on the response shows the Error UI etc.
  3. If the response is 200, use the frame.srcdoc prop and assign it to the response body.

@marcaaron
Copy link
Contributor

Ok, super weird now that I understand it...

@parasharrajat nice catch, but not really what we are trying to fix here so maybe we can create a new issue for that.

@marcaaron
Copy link
Contributor

WalletStatementPage is doing a Navigation.dismissModal(true)

onCloseButtonPress={() => Navigation.dismissModal(true)}

So why are we trying to navigate to the previous route in the history? Maybe this will be fixed in #9718.

@marcaaron
Copy link
Contributor

@thienlnam Anyway to reproduce this on dev? When I check out a statement it just looks like this...

2022-07-05_14-27-25

@thienlnam
Copy link
Contributor Author

WalletStatementPage is doing a Navigation.dismissModal(true)

Yeah I'm not sure, I messed around with it a bit and it seemed to be some react-navigation bug. Another thing I was thinking was that the way we've been accessing the statement is modifying the URL so maybe it has to do with that? But the history seems intact so I'm not sure

@thienlnam Anyway to reproduce this on dev? When I check out a statement it just looks like this...

Should be able to see the statement on dev, any console errors when you see this in the modal? Though I think if you're able to bring up the modal, you should be able to test it still by navigating to multiple statement links and clicking the X and just ignoring what is inside the modal

@thienlnam can confirm if there are beta requirements to know about for this project for external.

I think you can still test this without seeing what is actually in the modal since this is mostly about the navigation part of the modal

@marcaaron
Copy link
Contributor

seemed to be some react-navigation bug

I would guess it has more to do with our ever evolving "custom actions" than any issue with react-navigation, but both are possible. Will do some debugging.

@marcaaron
Copy link
Contributor

Pretty confused by this one. The navigation state looks normal, but for some reason going back here causes the entire app to reload. JS logs:

{
    "stale": false,
    "type": "stack",
    "key": "stack-9ocqjPgUWXqhOD9grZuur",
    "index": 1,
    "routeNames": [
        "Home",
        "ValidateLogin",
        "TransitionFromOldDot",
        "Settings",
        "NewChat",
        "NewGroup",
        "Search",
        "Details",
        "Report_Details",
        "Report_Settings",
        "Participants",
        "IOU_Request",
        "IOU_Bill",
        "EnablePayments",
        "IOU_Details",
        "AddPersonalBankAccount",
        "RequestCall",
        "IOU_Send",
        "Wallet_Statement"
    ],
    "routes": [
        {
            "name": "Home",
            "key": "Home-Zm_jAYw-L9GeUnMfw13KZ",
            "state": {
                "stale": false,
                "type": "drawer",
                "key": "drawer-wKHxJpsNfLvUExwJVK_o6",
                "index": 0,
                "routeNames": [
                    "Report"
                ],
                "history": [
                    {
                        "type": "route",
                        "key": "Report-yIDPQ02mIDKRz-TWT6h5A"
                    }
                ],
                "routes": [
                    {
                        "name": "Report",
                        "key": "Report-yIDPQ02mIDKRz-TWT6h5A",
                        "params": {
                            "reportID": "3686"
                        }
                    }
                ],
                "default": "closed"
            }
        },
        {
            "name": "Wallet_Statement",
            "state": {
                "stale": false,
                "type": "stack",
                "key": "stack-FmbiJDZuYsh5nCLc1_kD5",
                "index": 0,
                "routeNames": [
                    "WalletStatement_Root"
                ],
                "routes": [
                    {
                        "name": "WalletStatement_Root",
                        "params": {
                            "yearMonth": "202112"
                        },
                        "path": "/statements/202112/",
                        "key": "WalletStatement_Root-KQpPCD-_eoafkIAiP1JWR"
                    }
                ]
            },
            "key": "Wallet_Statement-rIXcjqtdOn4R40R2BV3j0"
        }
    ]
}
CustomActions.js?bcb8:26 {type: 'POP_TO_TOP'}
CustomActions.js?bcb8:27 stack-9ocqjPgUWXqhOD9grZuur
Log.js?f092:55 [info] [Onyx] set() called for key: modal properties: isVisible,willAlertModalBecomeVisible - ""
Log.js?f092:55 [info] [Onyx] set() called for key: session properties: loading,shouldShowComposeInput,authToken,accountID,email,encryptedAuthToken - ""
Log.js?f092:55 [info] Navigating to route - {"path":"/r/3686"}
Log.js?f092:55 [info] [Onyx] set() called for key: currentURL - ""
Navigated to http://localhost:8080/r/3686
log.js?1afd:24 [HMR] Waiting for update signal from WDS...
index.js?c8a3:89 Running application "NewExpensify" with appParams:
 {rootTag: '#root'} 
Development-level warnings: ON.
Performance optimizations: OFF.

@marcaaron
Copy link
Contributor

What's even weirder is that this doesn't happen with something like the "settings" page which is also a "modal stack". The state looks the same as this more or less.

@thienlnam
Copy link
Contributor Author

The navigation state looks normal, but for some reason going back here causes the entire app to reload

Yeah I think it's because we're accessing it by modifying the URL instead of having a dedicated button that opens Navigation.navigate(statement_route). Kind of like how if you click on another chat in the LHN the app won't reload, but if you manually edit the reportID in the URL it will cause it to reload. Not sure how it all plays into how the last page in the stack is the previous statement link though

@marcaaron
Copy link
Contributor

Ah ok, so, this is not unique to the statements. The same issue will happen if you navigate to something like

https://new.expensify.com/settings/payments
https://new.expensify.com/settings/preferences

@marcaaron
Copy link
Contributor

P.S. this issue is fixed by #9718 so I'm going to make it internal and assign myself.

@marcaaron marcaaron added Internal Requires API changes or must be handled by Expensify staff Improvement Item broken or needs improvement. Engineering and removed Exported External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Jul 6, 2022
@marcaaron marcaaron changed the title [$250] Always close the statement even after visiting multiple statement pages Always close the statement even after visiting multiple statement pages Jul 6, 2022
@trjExpensify
Copy link
Contributor

trjExpensify commented Jul 6, 2022

Kewl, taken it down from Upwork. Removing myself as a CM is not required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants
@trjExpensify @sonialiap @parasharrajat @thienlnam @marcaaron and others