-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: (payments) add stripe v3 connector #1724
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a Stripe payment connector to the payments module, enhancing the system's ability to manage payment-related operations. Key additions include methods for fetching accounts, balances, external accounts, and payments, as well as configuration and error handling functionalities. The implementation includes unit tests to ensure the reliability of the new features and a structured workflow for task management. Changes
Assessment against linked issues
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
e5cff0a
to
fded87d
Compare
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
components/payments/internal/connectors/plugins/public/stripe/client/client.go
Show resolved
Hide resolved
&expandSourceRefundPaymentIntent, | ||
} | ||
|
||
itr := c.balanceTransactionClient.List(&stripe.BalanceTransactionListParams{ |
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.
Since balance transactions are ordered starting from the most recent transactions, are we sure that the backward fetching is handled correctly and all adjustments are inserted correctly ?
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.
Actually, we're gonna have a problem, since the balance transactions are started from the most recent, you will fetch the whole history without problems, but when new payments are coming you will never fetch them since they are on the other side of the list
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.
Ok I see now that the original implementation was using created[lt]
to fetch balance transactions. Will see how I can replicate that with the SDK.
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've updated it to fetch transactions, accounts and external accounts in ASC order
) | ||
|
||
// root account reference is internal so we don't pass it to Stripe API clients | ||
func resolveAccount(ref *string) *string { |
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.
Do you need to pass a pointer here for the reference ?
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 don't need to. We can change it to a simple string if you prefer.
More just stylistic because the v2 implementation was passing around a potentially nillable account to methods like Translate that behave slightly different depending on whether an account was specified or not. So I am treating from.Reference
from the cache as something that we expect might be nil across the board (as opposed to having empty string checks).
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 reduced the number of pointers to strings
components/payments/internal/connectors/plugins/public/stripe/accounts.go
Show resolved
Hide resolved
needed := req.PageSize - len(accounts) | ||
|
||
newState := AccountsState{ | ||
InitFinished: oldState.InitFinished, |
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.
Why not setting the LastID with the oldstate LastID ?
If the call to return accounts returns none (for example last page, but empty), you will reset the state and refetch everything since the LastID on the next call will be empty
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.
Oops. Good catch.
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 is addressed
components/payments/internal/connectors/plugins/public/stripe/client/accounts.go
Outdated
Show resolved
Hide resolved
components/payments/internal/connectors/plugins/public/stripe/client/external_accounts.go
Outdated
Show resolved
Hide resolved
components/payments/internal/connectors/plugins/public/stripe/external_accounts.go
Outdated
Show resolved
Hide resolved
fded87d
to
c122200
Compare
ENG-1369