-
Notifications
You must be signed in to change notification settings - Fork 572
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
Add BECS Source Type #1568
Add BECS Source Type #1568
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.
Great work @shengwei-stripe this is looking really good! I left some minor comments about styling to try and be consistent across the library but otherwise this looks ready!
@@ -0,0 +1,19 @@ | |||
namespace Stripe |
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 should be named SourceAuBecsDebit
since that's the name of the whole type.
public string Last3 { get; set; } | ||
|
||
[JsonProperty("fingerprint")] | ||
public string Fingerprint { get; set; } |
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 should all be ordered alphabetically.
@@ -0,0 +1,13 @@ | |||
namespace Stripe |
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.
Same for naming, you're missing Au
in the name (won't say it for others)
public string BsbNumber { get; set; } | ||
|
||
[JsonProperty("account_number")] | ||
public string AccountNumber { get; set; } |
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.
Order alphabetically (won't say for the other ones)
@@ -0,0 +1,12 @@ | |||
namespace Stripe |
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 and the other one should be SourceMandateAcceptanceOfflineOptions
instead.
@remi-stripe thanks for the review. As discussed, I have made the fix
Let me know if it looks ok. Thank you :) |
Thanks for the fixes @shengwei-stripe, this looks great now. Re-assigning to @ob-stripe to vet the PR and do the release tomorrow! |
Released as 25.4.0. |
Online
andOffline
typePlease comment on
E.g.
BecsDebit
vsAuBecsDebit
,SourceMandateOnlineOptions
andSourceMandateOfflineOptions