-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Re-adding paywithwallet functionality for manual requests #19503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes looking good, testing might be quite pain though
@sobitneupane @MariaHCD One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
okay I think this is ready. I tried to test it as best I could but hit a couple bumps along the way but at best I can tell nothing is more broken with this 😅 |
If there are no major concerns from reviewers could someone add the |
|
Reviewer Checklist
Screenshots/VideosTesting the normal pay money request functionality as the wallet set up is tricky and easier to test on staging Mobile Web - SafariCant login due to some cahing issues Android
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can CP and test this properly in staging
Tested scenarios 1 & 3 on web which worked as expected (added to the reviewer checklist as well). Couldn't test 2. because of an issue with the Bancorp sandbox token. I think we're good here 🙏🏼 |
…wallet Re-adding paywithwallet functionality for manual requests (cherry picked from commit 83555a9)
…-19503 🍒 Cherry pick PR #19503 to staging 🍒
🚀 Cherry-picked to staging by https://github.com/MariaHCD in version: 1.3.18-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
@mountiny @yuwenmemon Can you pls QA this internally |
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.18-2 🚀
|
we had David test it and he said it worked though I'm still waiting for confirmation to verify the logs. |
thread is here for others to follow along https://expensify.slack.com/archives/C049HHMV9SM/p1685034722453609?thread_ts=1684571441.343629&cid=C049HHMV9SM |
🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀
|
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/285824
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
You'll need to get some things set up first, for internal engineers you'll want to use the cliscripts and probably 2 different accounts to test.
Testing with IOU with Withdrawal account only
accountA - create a verified withdrawal bank account
And make sure the wallet is upgraded to Gold
Then send a money request from another account (accountB) to accountA, in accountA click the
Pay with Expensify
button and ensure your logs show that we calledPayMoneyRequestWithWallet
notPayMoneyRequest
Confirmed no errors showed and that the IOU was paid
And verified in the db that rows were properly inserted into the DB
Testing IOU with deposit only bank account and card on line
Set up accountA so that it only has a deposit-only bank account and a gold wallet
Send an IOU from accountB to accountA
As accountA, pay via expensify
Ensure logs show we hit
PayManualRequestWithWallet
... :sad-panda: I couldn't actually get this to work b/c of an error trying to talk to the bancorp sandbox URL ...
Testing Expense Report Manual Request goes through PayManualRequest
created a request from a workspace member to the workspace
Make sure you have a bank account set up, you might likely have to create it manually in new dot and then CQ to set it to a validated state:
As the workspace admin go in and click Pay with Expensify
Verified we made the correct API request
We don't actually add the reimbursement though https://github.com/Expensify/Auth/blob/74b89a906c3734cd4991296bee2395efe3434d00/auth/lib/ACHReimbursement.cpp#L1778-L1781 so I can't confirm the data in my DB but can at least verify that log line was hit:
Offline tests
None specifically since this requires you be connected to the internet to actually settle an IOU.
QA Steps
I am not sure if we have testing set up for Policy Expense Chat to get reimbursements from a workspace or not.
If we do then we should test that flow AND the IOU/P2P flow.
We can also have David test this since he currently has an IOU that needs to be paid if we CP this staging.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
N/A really see tests I guess.
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android