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

Create mock ach bank account #101

Merged
merged 3 commits into from
Jan 7, 2021
Merged

Create mock ach bank account #101

merged 3 commits into from
Jan 7, 2021

Conversation

timmy-circle
Copy link
Contributor

@timmy-circle timmy-circle commented Dec 23, 2020

New feature

Add support to create mock ACH bank account with specified balance and account numbuer/routing/description.
This endpoint will return a mock plaid processor token that can be use in circle sandbox to create an ach fiat account.

Payments APIs screen shot

Screen Shot 2021-01-05 at 12 59 25 PM

Payouts APIs screen shot

@geehsien Per our discussion, I also bring in creating ach accounts to payouts API.
Screen Shot 2021-01-05 at 12 59 57 PM

<v-form>
<v-text-field
v-model="formData.account.accountNumber"
label="ACH Account Number"
Copy link
Contributor

Choose a reason for hiding this comment

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

should make this and following fields required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember chatting with @kristinfritsch that we shouldn't be adding restriction/validation on UI?

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, not sure if we can enforce some rules in the class, also could at least do a hint (prompt xxx is required)

Copy link
Contributor

@kristinfritsch kristinfritsch Jan 6, 2021

Choose a reason for hiding this comment

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

The reason we don't want to put restrictions on the UI is to make testing of edge cases easier (e.g. error cases of the api). That doesn't really apply to mock apis. I think it's fine to make these fields required if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okie. Will update.

<p>
<v-chip small color="primary warning">POST</v-chip>
<a href="/debug/ach/mocks/create">
Create or update a mock ACH bank account
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the mock endpoint support update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good catch. No update

Copy link
Contributor

@kristinfritsch kristinfritsch left a comment

Choose a reason for hiding this comment

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

Small comments, otherwise looks good

<v-form>
<v-text-field
v-model="formData.account.accountNumber"
label="ACH Account Number"
Copy link
Contributor

@kristinfritsch kristinfritsch Jan 6, 2021

Choose a reason for hiding this comment

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

The reason we don't want to put restrictions on the UI is to make testing of edge cases easier (e.g. error cases of the api). That doesn't really apply to mock apis. I think it's fine to make these fields required if needed.

/>

<v-btn
depressed
Copy link
Contributor

@kristinfritsch kristinfritsch Jan 6, 2021

Choose a reason for hiding this comment

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

This button really should only be enabled for sandbox. E.g. we hide that button for on the mock wire page /debug/payments/mocks/wire. I don't think that's the best solution, but that's the pattern right now, so let's keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed.

Copy link
Contributor

@huaweigu huaweigu left a comment

Choose a reason for hiding this comment

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

lgtm

@MasterXen MasterXen merged commit 8efa787 into circlefin:master Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants