feat(acl- 111): add support to get transactions for merchant account #216
Conversation
….GetAsync - remove OneOf.SourceGenerator - improved OneOfJsonConverter - fixed uriExtensions.AppendQueryParameters
| var statusTypeDiscriminator = string.Join("_", statusValue, discriminatorValue); | ||
| if (!string.IsNullOrWhiteSpace(statusTypeDiscriminator) | ||
| && (_descriptor.TypeFactories.TryGetValue(statusTypeDiscriminator, out typeFactory))) |
There was a problem hiding this comment.
For the payout payments resulted from the GetTransactions for the MerchantAccounts, the discriminator is the concatenation of the status and type field: status_type.
There was a problem hiding this comment.
for context: On the Java lib we treat them as the same item, just with some optional fields, for simplicity
| string merchantAccountId, | ||
| DateTimeOffset from, | ||
| DateTimeOffset to, | ||
| bool getPaginatedResult = true, |
There was a problem hiding this comment.
Does it make sense to set this by default, without giving the option to opt-out initially (and without exposing anything here at the interface level)? I'd be surprised if there are new or old customers building the integration now without pagination. As such, I would personally treat that as an exception when/if needed (I don't have context on the clients that are currently interested). Mine is just a suggestion though, If we are on the same page, I'd suggest you to anyway double-check on this with Fede and MAP.
On the java lib we had added an overload because we had the unpaginated endpoint already integrated :(
There was a problem hiding this comment.
good point, I will set it to false by default
| @@ -83,8 +83,50 @@ public async Task<ApiResponse<GetPaymentSourcesResponse>> GetPaymentSources(stri | |||
| } | |||
|
|
|||
| return await _apiClient.GetAsync<GetPaymentSourcesResponse>( | |||
| _baseUri.Append($"merchant-accounts/{merchantAccountId}/payment-sources?user_id={userId}"), | |||
| _baseUri.Append($"{merchantAccountId}/payment-sources?user_id={userId}"), | |||
There was a problem hiding this comment.
Did the base URI change? Or, there was an oversight in the previous implementation?
There was a problem hiding this comment.
it didn't change. The previous implementation was wrong as the merchant-account segment of the url was already added here
| merchantAccountId.NotNullOrWhiteSpace(nameof(merchantAccountId)); | ||
| merchantAccountId.NotAUrl(nameof(merchantAccountId)); | ||
| to.GreaterThan(from, nameof(to), nameof(from)); | ||
| cursor.NotAUrl(nameof(cursor)); |
There was a problem hiding this comment.
Why are we validating this?
There was a problem hiding this comment.
For security reasons. A few months ago we had a security breach where consumers of the SDK could inject URLs into string parameters. This check prevents it.
There was a problem hiding this comment.
Ah, good to know! Is this validated also at the API level if you know the answer? I think we could replicate this on the Java lib as well, I just would like to know how urgent it is (This does not help with the case where a malicious user use a previous lib version...)
There was a problem hiding this comment.
TBH I am not aware of any issues at api level. Maybe it is worth implementing it in the Java lib as well,
| var statusTypeDiscriminator = string.Join("_", statusValue, discriminatorValue); | ||
| if (!string.IsNullOrWhiteSpace(statusTypeDiscriminator) | ||
| && (_descriptor.TypeFactories.TryGetValue(statusTypeDiscriminator, out typeFactory))) |
There was a problem hiding this comment.
for context: On the Java lib we treat them as the same item, just with some optional fields, for simplicity
tl-mauro-franchi
force-pushed
the
acl-111/add-support-to-get-transactions-for-merchant-account
branch
from
5be428c to
8500139
Compare
tl-mauro-franchi
deleted the
acl-111/add-support-to-get-transactions-for-merchant-account
branch
No description provided.