feat: Column Connector for Payments #351
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
Tested cases
Fails with the following error {
"errorCode": "INTERNAL",
"errorMessage": "WorkflowId length exceeds limit."
}
The following statuses are triggered and processed:
|
internal/connectors/plugins/public/column/client/bank_account_creation.go
Outdated
Show resolved
Hide resolved
|
|
||
| var response ReversePayoutResponse | ||
| var errRes columnError | ||
| _, err = c.httpClient.Do(ctx, request, &response, errRes) |
There was a problem hiding this comment.
| _, err = c.httpClient.Do(ctx, request, &response, errRes) | |
| _, err = c.httpClient.Do(ctx, request, &response, &errRes) |
| addressLine1 := models.ExtractNamespacedMetadata(newExternalBankAccount.Metadata, client.ColumnAddressLine1MetadataKey) | ||
|
|
||
| if addressLine1 != "" { | ||
|
|
||
| if newExternalBankAccount.Country == nil { | ||
| return fmt.Errorf("country is required") | ||
| } | ||
|
|
||
| } | ||
|
|
||
| if addressLine1 == "" && newExternalBankAccount.Country != nil { | ||
| return fmt.Errorf("metadata field country is not required when addressLine1 is not provided") | ||
| } |
There was a problem hiding this comment.
As far as I can tell from the documentation the following fields should always be present in the Bank Account creation request:
- address.line_1
- address.city
- address.country_code
So I think we can simply check if all these fields are present, and if not return a validation error. The split of validation logic here + the validation logic in validateAddress is not optimal in my opinion as it creates a double validation and makes the conditions in the code harder to understand and therefore easier to create bugs.
Can we simplify it so that the validation function is the one that returns the errors? Perhaps it could also extract address data into an intermediate Address struct that can be be used in the requests to both the BankAccount creation and Payout initiation requests.
There was a problem hiding this comment.
I don't see much improvement on this front unfortunately. I understand that there is added complexity due to the validateAddress function being shared between transfer and bank account requests. How can we make this simpler (eg. fewer branches of code, less nested if statements)?
My suggestion would be to add a layer of separation between the extraction of metadata and the actual validation (what fields are required for which type of request). If you want to share code between two functions there is a need for a good abstraction to make that easy to read, debug and modify.
For example:
- Extract All the metadata into a
ColumnAddressstruct which contains all the fields related to address that will be used to make the request - Pass your populated
ColumnAddressstruct to the validation function. The validation function now only checks if fields are valid and returns typed validation errors. It does not need to know about metadata tags (clean separation of concerns). If the validation concerns are different between bank account and transfer you might even want two different validation functions. - Since you've already extracted all the fields into a struct you can get rid of all the metadata extraction happening in your code after the validation and use
ColumnAddressto populate the request.
Such an approach would reduce nested if statements, reduce repetition and make functions more single-purpose (which also makes them easier to unit test and maintain).
bd4a950 to
ed24670
Compare
internal/connectors/plugins/public/column/client/counterparties.go
Outdated
Show resolved
Hide resolved
| if addressLine1 != "" { | ||
| city := models.ExtractNamespacedMetadata(addressMetadata, client.ColumnAddressCityMetadataKey) | ||
|
|
||
| if city == "" { | ||
| return ErrMissingMetadataAddressCity | ||
| } | ||
|
|
||
| if country == "" { | ||
| if isCountryInMetadata { | ||
| return ErrMissingMetadataCountry | ||
| } else { | ||
| return ErrMissingCountry | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| if addressLine1 != "" { | ||
| 8000 | return nil | |
| } |
There was a problem hiding this comment.
| if addressLine1 != "" { | |
| city := models.ExtractNamespacedMetadata(addressMetadata, client.ColumnAddressCityMetadataKey) | |
| if city == "" { | |
| return ErrMissingMetadataAddressCity | |
| } | |
| if country == "" { | |
| if isCountryInMetadata { | |
| return ErrMissingMetadataCountry | |
| } else { | |
| return ErrMissingCountry | |
| } | |
| } | |
| } | |
| if addressLine1 != "" { | |
| return nil | |
| } | |
| if addressLine1 != "" { | |
| city := models.ExtractNamespacedMetadata(addressMetadata, client.ColumnAddressCityMetadataKey) | |
| if city == "" { | |
| return ErrMissingMetadataAddressCity | |
| } | |
| if country == "" { | |
| if isCountryInMetadata { | |
| return ErrMissingMetadataCountry | |
| } else { | |
| return ErrMissingCountry | |
| } | |
| } | |
| return nil | |
| } |
ed24670 to
cef1d8b
Compare
|
Column api uses |
ACH is a specific type of bank transfer where the initiator of the transaction is not necessarily the debtor. The two combinations are possible:
The most important thing is that we add money when we expect it to be added and subtract it when it should be subtracted. Could you double check that the Column SDK is handling both cases correctly? If you think not we will have to file a bug report with them.
Are you able to find the error in the logs? Would be easier for me to help you debug if you could tell me the error message you observed. |
Thanks for the clarification, yes the amount is being added and subtracted correctly.
Thanks! we found some. We will forward them to you. |
| } | ||
|
|
||
| func (p *Plugin) initWebhookConfig() map[client.EventCategory]webhookConfig { | ||
| p.webhookConfigs = map[client.EventCategory]webhookConfig{ |
There was a problem hiding this comment.
Same as the increase PR, we want to have all payments, which means we want to have the pending ones, the rejecting ones, the canceled ones etc... You need to handle the other webhooks from the list of events here: https://column.com/docs/workingwithapi/events-and-webhooks#all-event-types
|
We have prepared a new estimate of 185 credits for this pull request. |
This PR was created by GitStart to address the requirements from this ticket: LNK-39.
Integrate Column Connector for the Payments Service
Features