Skip to content

Conversation

@dexter-onboard
Copy link

No description provided.

jonmmyers
jonmmyers previously approved these changes Jan 30, 2023
Copy link
Contributor

@jonmmyers jonmmyers left a comment

Choose a reason for hiding this comment

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

Couple things missing here, also I'd like @asmsh to review since I'm not that familiar with Java

jonmmyers
jonmmyers previously approved these changes Feb 9, 2023
Comment on lines 410 to 436
/// <summary>
/// createACHPull talks to the CreateACHPull endpoint of Dapi, with this DapiApp's appSecret.
/// </summary>
///
/// <param name="transfer">
/// the details of the transfer that should be initiate.
/// </param>
/// <param name="accessToken">
/// retrieved from the ExchangeToken process.
/// </param>
/// <param name="userSecret">
/// retrieved from the user login.
/// </param>
public CreateACHPullResponse createACHPull(ACH.ACHPull transfer, string accessToken, string userSecret) {
return this.c.createACHPull(transfer, accessToken, userSecret, "", null);
}

/// <summary>
/// createACHPull talks to the CreateACHPull endpoint of Dapi, with this DapiApp's appSecret.
/// </summary>
/// <param name="userSecret">
/// retrieved from the user login.
/// </param>
public GetACHPullResponse getACHPull(string accessToken, string userSecret) {
return this.c.getACHPull(accessToken, userSecret, "", null);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Move them to be after the getAccountsMetadata method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both methods should have 2 versions, one that has the operationID and userInputs parameters, and one that doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, in the case of getACHPull, the operationID has to be always passed, as it's a required field for the endpoint.

Copy link
Author

Choose a reason for hiding this comment

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

return respBody ?? new GetACHPullResponse("UNEXPECTED_RESPONSE", "Unexpected response body");
}

public class ACHPull {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to PullTransfer.

Copy link
Author

Choose a reason for hiding this comment

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

Comment on lines 82 to 84
this.senderID = transfer.senderID;
this.amount = transfer.amount;
this.description = transfer.description;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not work. These fields should be inside the transfer object field, not directly in the root.

Copy link
Author

Choose a reason for hiding this comment

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

/// This is a private constructor to this lib.
/// </summary>
[JsonConstructor]
internal CreateACHPullResponse(APIStatus status, bool success, string operationID) :
Copy link
Contributor

Choose a reason for hiding this comment

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

This constructor is missing the userInputs, type and msg fields.

Copy link
Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,13 @@
namespace Dapi.Types {
public class ACHGetTransfer {
Copy link
Contributor

@asmsh asmsh Feb 10, 2023

Choose a reason for hiding this comment

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

Rename it to ACHPullTransferInfo.

Copy link
Author

Choose a reason for hiding this comment

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

/// This is a private constructor to this lib.
/// </summary>
[JsonConstructor]
internal GetACHPullResponse(ACHGetTransfer transfer, string reference, APIStatus status, bool success, string operationID) :
Copy link
Contributor

Choose a reason for hiding this comment

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

This constructor is missing the userInputs, type and msg fields.

Copy link
Author

Choose a reason for hiding this comment

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

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