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

[P2P Statements] Add Wallet Statements Page + Modal #7663

Merged
merged 31 commits into from
Feb 18, 2022

Conversation

nickmurray47
Copy link
Contributor

@nickmurray47 nickmurray47 commented Feb 9, 2022

Details

Add a new community package react-native-webview to render the Wallet Statements page in-app on mobile. For web/desktop, have the new Wallet Statements page open link to oldDot.

Fixed Issues

$ GH_LINK

Tests

  1. Run in console npm i to install the node_module.
  2. Comment out this line https://github.com/Expensify/Expensidev/blob/main/config/www/nginx.www.conf#L24 and restart nginx from the vm sudo systemctl restart nginx
  3. Replace the IOU_BILL onSelected property in SidebarScreen.js so that we can test navigating to the statements page by clicking on Split Bill from the global menu.
    onSelected: () => Navigation.navigate(ROUTES.getWalletStatementWithDateRoute(202112)),
    
  4. Login to an account with Wallet transactions
  5. Click on Split Bill, verify you are taken to the Statements page rendering in a new modal.
  6. [On web/mWeb] Go to http://localhost:8080/statements/202201 and verify you also see the modal.
  7. Logout and log in to an account with no statements and verify you see this screen.

Screen Shot 2022-02-18 at 11 49 53 AM

QA Steps

  1. Can probably only QA on web for right now and would need to use an account that has wallet transactions, feel free to ping me and I can test on my end.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2022-02-17 at 11 54 31 AM

Desktop

Screen Shot 2022-02-17 at 3 41 05 PM

iOS

Screen Shot 2022-02-11 at 12 06 22 AM

Android

Screen Shot 2022-02-10 at 11 58 50 PM

@nickmurray47 nickmurray47 self-assigned this Feb 9, 2022
@nickmurray47 nickmurray47 requested a review from a team as a code owner February 9, 2022 21:51
@MelvinBot MelvinBot requested review from marcochavezf and removed request for a team February 9, 2022 21:52
@nickmurray47
Copy link
Contributor Author

nickmurray47 commented Feb 10, 2022

Just to put down where we are with progress right now, I discovered that the original plan of using react-native-webview would work on basically every platform except for Web.

After chatting briefly with @thienlnam, we said that an acceptable solution would be to use our current Linking.openURL method to open the oldDot statements page in a new tab on web and use the webview to display it on iOS/android/desktop.

@botify
Copy link

botify commented Feb 10, 2022

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

@nickmurray47 nickmurray47 changed the title [WIP] [P2P Statements] Add New Dot Modal [P2P Statements] Add New Dot Modal Feb 11, 2022
@nickmurray47
Copy link
Contributor Author

Need to add screenshots + QA steps then this PR is ready to go!

Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far, a few comments

src/ROUTES.js Outdated Show resolved Hide resolved
src/components/WalletStatementModal/index.js Outdated Show resolved Hide resolved
src/pages/wallet/WalletStatementPage.js Outdated Show resolved Hide resolved
src/pages/wallet/WalletStatementPage.js Show resolved Hide resolved
@nickmurray47 nickmurray47 changed the title [P2P Statements] Add New Dot Modal [P2P Statements] Add Wallet Statements Page + Modal Feb 11, 2022
@thienlnam
Copy link
Contributor

It would also be good to add testing steps for web and mobile web to just visit http://localhost:8080/statements/202201

@marcochavezf
Copy link
Contributor

Followed the test steps and working fine on web:

Screen Shot 2022-02-18 at 10 58 14

But on Android I'm getting this error:

Screen Shot 2022-02-18 at 11 07 12

And this message in the logs:

 WARN  Encountered an error loading page {"canGoBack": false, "canGoForward": false, "code": -2, "description": "net::ERR_NAME_NOT_RESOLVED", "loading": false, "target": 7557, "title": "", "url": "https://www.expensify.com.dev/statement.php?period=202112"}

Not sure if missing something for Android 🤔

@marcochavezf
Copy link
Contributor

It would also be good to add testing steps for web and mobile web to just visit http://localhost:8080/statements/202201

Oh I didn't see this message, maybe that's why I'm getting the error on Android

@nickmurray47
Copy link
Contributor Author

nickmurray47 commented Feb 18, 2022

But on Android I'm getting this error

@marcochavezf This looks related to the nginx config I mentioned in step two? Have you tried testing on iOS as well? I think one step I might've missed out on was running npm i before the react-native-link step. I just added that step in and re-tested Android and works fine for me.

Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked to Nick about this 1:1 but this also isn't being used yet so we can test the configuration stuff on staging. Although it would be good to make sure this works in the future on dev so other developers can test it

@nickmurray47 nickmurray47 added the InternalQA This pull request required internal QA label Feb 18, 2022
@nickmurray47
Copy link
Contributor Author

Although it would be good to make sure this works in the future on dev so other developers can test it

Will open up a PR for this!

@marcochavezf marcochavezf merged commit 7af6dfd into main Feb 18, 2022
@marcochavezf marcochavezf deleted the nmurray-p2p-statements-modal branch February 18, 2022 21:02
@MelvinBot
Copy link

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

@botify
Copy link

botify commented Feb 18, 2022

@marcochavezf looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@marcochavezf
Copy link
Contributor

Hmm this is weird, the tests already passed, I'm going to remove the Emergency label because this is not the case I think 🤔

@nickmurray47
Copy link
Contributor Author

Another one of these, yeah should not be a problem

@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@deetergp
Copy link
Contributor

deetergp commented Feb 18, 2022

That's weird. Why was I auto-assigned (not assigned as a reviewer) to a pull request? Especially one that was merged already?!

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcochavezf in version: 1.1.40-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@nickmurray47
Copy link
Contributor Author

@deetergp I believe you were auto-assigned as the QA-er since this PR is marked as internal QA. However, I think we'd actually need to test this on Prod and not Staging.

@OSBotify
Copy link
Contributor

OSBotify commented Mar 1, 2022

🚀 Deployed to production by @yuwenmemon in version: 1.1.40-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
InternalQA This pull request required internal QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants