-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Get parameters for Plaid link token based on platform #6411
Conversation
@nickmurray47 adding you here since I think these changes might intersect with the work you are doing for https://github.com/Expensify/Expensify/issues/160148 |
|
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.
Looks good to me! On this:
tested this using the steps in the linked PR above. However, also saw that the OAuth stuff isn't really testable with the Plaid sandbox
Does this mean the Platypus OAuth sandbox credentials are not sufficient? What do we need to test for in this PR (or are we just gonna test the whole flow together)?
Good question. I'm actually not sure yet, but the docs had this to say: I'm guessing the "sandbox-only" institution they're referring to is Platypus OAuth, but really unsure if our "dev" instance that everyone can access is set up for OAuth testing or not. I think what we need to test is:
|
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.
this all sounds good and lgtm
Chatting with @Dal-Papa now about testing for Mobile-Expensify, but I think we can use a similar testing strategy for this PR to verify that mobile is working. Which is:
I don't have a BofA account to test this with otherwise I'd do it myself. Going to make this internal QA in any case. |
|
Ok this is ready to be merged - but I think we are still stuck since we need actual accounts set up with participating banks to test and are only able to test BofA with the Plaid dev app anyway. Thinking more on this... really we should test them all to make sure they work before the change is deployed so we might as well use the Plaid production app Development secret. IMO we shouldn't skip this step (and should do it for all the participating banks) because we have no idea if those connections will actually work unless we test them first. Two options I can think of:
This option actually cannot work because we must use the Development secret anyway. Seems the Production one won't necessarily send the user through the OAuth flow. |
Discussed a 3rd option with @nickmurray47 1:1 which is to add a beta and allow non engineers to also test the flow. But if we do this idea we still need to solve for how to "switch" to the Development secret for test purposes under the beta. Ultimately, I'm not sure it's worth building out that kind of functionality yet - but one benefit is that it opens up more testing options (i.e. non engineers could test) and enable us to switch on the beta once testing is complete (even if that happens after 11/30). |
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 look good to me (I was catching up on the OAuth migration to understand this a little bit more) 👍🏽
5a5d8e1
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.
Looks even better to me now that I get it 😆
@marcaaron once this is implemented, would this resolve the Plaid issue that Chase customers were having in this GH? |
@VictoriaExpensify Looks unrelated to me as OAuth should not be forced for any users yet. |
Triggered auto assignment to @sketchydroide ( |
@marcochavezf looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
Get parameters for Plaid link token based on platform (cherry picked from commit 890a8ef)
🚀 Deployed to staging by @marcochavezf in version: 1.1.17-8 🚀
|
Gonna check this off the deploy list. There doesn't seem to be a clear way to test this beyond regressions for the VBA flow. It's basically Production QA. |
🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀
|
There is no emergency here, so I'm removing the label. Seems the checklist was not completed |
cc @Dal-Papa unsure if we have yet added the necessaryThis is done nowredirect_uri
andandroid_package_name
fields to the production instance of Plaid, but I think these native versions will not work properly until we do.Details
Passes the
redirect_uri
andandroid_name
fields toPlaid_GetLinkToken
commandFixed Issues
IDK if there is a specific issue. It's more of a follow up to: #6362
Tests
I tested this using the steps in the linked PR above. However, also saw that the OAuth stuff is not testable with the Plaid sandbox.
I think we won't know for sure unless we test against production, but can simulate this by modifying our local `Web-Secure to use the Plaid dev app Development secret and connecting with BofA.
QA Steps
Internal / See Comments below
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android