Skip to content
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

FINERACT 2120: Add new command parameters withdrawToLinkedAccount and disburseToLinkedAccount and integrate with PHEE #4046

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

kaibalya-fynarfin
Copy link
Contributor

Description

Describe the changes made and why they were made.

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per https://github.com/apache/fineract/#pull-requests

  • Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.

  • Create/update unit or integration tests for verifying the changes made.

  • Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.

  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes

  • Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

@@ -28,6 +28,13 @@ apply plugin: 'com.google.cloud.tools.jib'
apply plugin: 'org.springframework.boot'
apply plugin: 'se.thinkcode.cucumber-runner'


repositories {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this repository!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This repository is required for this dependency implementation 'org.pheesdk:PaymentHubSDK:1.0.0'

Copy link
Contributor

@adamsaghy adamsaghy Sep 4, 2024

Choose a reason for hiding this comment

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

You cannot put your own repository into a community project. If the payment hub sdk is not available on a public repository that is not controlled by your organization, it has absolutely no place in opersource, community Fineract.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be possible to load the Payment Hub SDK dependency at runtime if it's hosted in a private JFrog Artifactory?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is absolutely a no-go. If Payment Hub SDK is needed, let's fix it over there to be published to a publicly accessed repository, for example Maven central.

@@ -164,15 +171,15 @@ configurations.driver.each {File file ->
task createDB {
description= "Creates the MariaDB Database. Needs database name to be passed (like: -PdbName=someDBname)"
doLast {
def sql = Sql.newInstance( 'jdbc:mariadb://localhost:3306/', mysqlUser, mysqlPassword, 'org.mariadb.jdbc.Driver' )
def sql = Sql.newInstance( 'jdbc:mariadb://localhost:3307/', mysqlUser, mysqlPassword, 'org.mariadb.jdbc.Driver' )
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reverse these changes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do that

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment shouldn't be resolved since the changes weren't yet made.

sql.execute( 'CREATE DATABASE '+"`$dbName` CHARACTER SET utf8mb4" )
}
}

task dropDB {
description= "Drops the specified MariaDB database. The database name has to be passed (like: -PdbName=someDBname)"
doLast {
def sql = Sql.newInstance( 'jdbc:mariadb://localhost:3306/', mysqlUser, mysqlPassword, 'org.mariadb.jdbc.Driver' )
def sql = Sql.newInstance( 'jdbc:mariadb://localhost:3307/', mysqlUser, mysqlPassword, 'org.mariadb.jdbc.Driver' )
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reverse these changes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do that

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment shouldn't be resolved since the changes weren't yet made.

@@ -52,6 +53,7 @@
*/

@Getter
@Setter
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need Setter annotation here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this resolved?

@@ -35,6 +36,7 @@

@Entity
@Getter
@Setter
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did introduced setters here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this resolved?

@@ -225,6 +232,7 @@ bootRun {
dependencies {
implementation 'org.mariadb.jdbc:mariadb-java-client'
implementation 'org.postgresql:postgresql'
implementation 'org.pheesdk:PaymentHubSDK:1.0.0'
Copy link
Contributor

@adamsaghy adamsaghy Sep 4, 2024

Choose a reason for hiding this comment

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

I dont think it is right to be here. Would you move to the dependencies.gradle? Also it might be better to have it in a separate module. Most of the ppl would not need payment hub support...
@vidakovic What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

The version shouldn't be here that's for sure. In terms of packaging the PaymentHubSDK with Fineract, there are 2 options:

  • bundle with Fineract as it is
  • extract into a separate module where the payment hub dependency is there - in this case there needs to be build adjustments so that Fineract is published in 2 versions, with and without payment hub

I'm not sure how much extra weight the Payment Hub SDK is, but if it's just DTOs and API classes, I'd say it's acceptable to be bundled with Fineract.

@Service
@RequiredArgsConstructor
@CommandType(entity = "LOAN", action = "DISBURSETOLINKEDACCOUNT")
public class DisburseLoanToLinkedAccountCommandHandler implements NewCommandSourceHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider moving payment hub ee related stuff to a separate module?

@@ -2123,6 +2126,7 @@ public static void validateOrThrow(String resource, Consumer<DataValidatorBuilde
baseDataValidator.accept(dataValidatorBuilder);

if (!dataValidationErrors.isEmpty()) {
logger.info(dataValidationErrors.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this! Exception handler can decide whether they are logging something or not. Also dont use "INFO" level logging for logging out exceptions!

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this comment resolved?

@@ -295,6 +303,139 @@ public CommandProcessingResult disburseLoan(Long loanId, JsonCommand command, Bo
return disburseLoan(loanId, command, isAccountTransfer, false);
}

@Transactional
@Override
public CommandProcessingResult disburseLoanToLinkedAccount(Long loanId, JsonCommand command, Boolean isAccountTransfer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the part of this method is duplication. Would you consider extracting into methods the common logics?

InteropIdentifier identifier1 = interopService.getIdentifierByAccountId(portfolioAccountData.getId());
String payerIdentifierType = InteropIdentifierType.ACCOUNT_ID.toString();
String payerIdentifierValue = loan.getAccountNumber();
String payeeIdentifierType = identifier1.getType().toString();
Copy link
Contributor

@adamsaghy adamsaghy Sep 4, 2024

Choose a reason for hiding this comment

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

Please dont use the toString of an enum. There is no explicit toString for this enum so it is basically give you the value of name(). Please use that if that's the information you are looking for. Anyone who override the toString method of this enum in the future will effectively break you logic.

}

InteropIdentifier identifier1 = interopService.getIdentifierByAccountId(portfolioAccountData.getId());
String payerIdentifierType = InteropIdentifierType.ACCOUNT_ID.toString();
Copy link
Contributor

@adamsaghy adamsaghy Sep 4, 2024

Choose a reason for hiding this comment

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

Please dont use the toString of an enum. There is no explicit toString for this enum so it is basically give you the value of name(). Please use that if that's the information you are looking for. Anyone who override the toString method of this enum in the future will effectively break you logic.

try {
String id = sdkDisbursalService.processDisbursal(payerIdentifierType, payerIdentifierValue, payeeIdentifierType,
payeeIdentifierValue, amount, currency);
logger.info("Payment hub transaction started with transaction id: " + id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please dont log info that something was started or finished. It has no additional value. If you need this operation log, mark it as debug hence info is the default level.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an extra, please use placeholders instead of concatenating the parameters.

payeeIdentifierValue, amount, currency);
logger.info("Payment hub transaction started with transaction id: " + id);
} catch (Exception e) {
logger.error(e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please dont just silently swallow the outcome. How can the disbursement action be successful when the sdkDisbursalService failed for any reason...

Please review this logic!

String id = null;
try {
id = transferService.processPayment(payerType, payerId, payeeType, payeeId, amount, currencyCode);
logger.info(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this info log from here. If you want it for debug purposes, please use debug level and also use proper descriptive messages. Logging the value of the id field without any messages is useless.

id = transferService.processPayment(payerType, payerId, payeeType, payeeId, amount, currencyCode);
logger.info(id);
} catch (SdkApiException e) {
logger.error("Error status code: " + e.getStatusCode());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please have proper exception handling. Logging out the error is not effective and deciding whether something was successful or not merely whether the returned id is null or not is not recommended!

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, use placeholders.

} catch (SdkApiException e) {
logger.error("Error status code: " + e.getStatusCode());
logger.error("Error status code: " + e.getResponseBody());
System.out.println(e.getResponseBody());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the System.out.println.

logger.error("Error status code: " + e.getResponseBody());
System.out.println(e.getResponseBody());
} catch (Exception e) {
logger.error(e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Some as above.


@Override
public String processDisbursal(String payerType, String payerId, String payeeType, String payeeId, String amount, String currencyCode)
throws SdkValidationException, SdkApiException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are catching Exception, i dont really understand how would this method throw any of these exceptions....

@@ -67,7 +70,7 @@ public SavingsAccountDomainServiceJpa(final SavingsAccountRepositoryWrapper savi
final JournalEntryWritePlatformService journalEntryWritePlatformService,
final ConfigurationDomainService configurationDomainService, final PlatformSecurityContext context,
final DepositAccountOnHoldTransactionRepository depositAccountOnHoldTransactionRepository,
final BusinessEventNotifierService businessEventNotifierService) {
final BusinessEventNotifierService businessEventNotifierService, @Lazy final InteropService interopService) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why lazy?

@Transactional
@Override
public CommandProcessingResult withdrawToLinkedAccount(final Long savingsId, final JsonCommand command) {

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems majorly a copy-paste of existing functionality, i kindly asking you to extract the common logic into methods and reuse them.

changes.put(PaymentDetailConstants.routingCodeParamName, identifierType);
changes.put(PaymentDetailConstants.accountNumberParamName, identifierValue);

paymentDetail.setRoutingCode(identifierType.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please dont use toString. Use explicitly the method or field value that you need!


this.savingsAccountTransactionDataValidator.validateTransactionWithPivotDate(transactionDate, account);

SdkWithdrawalService sdkWithdrawalService = new SdkWithdrawalServiceImpl();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rework this to be a Spring bean, so if someone wanna override it eventually, they can...

this.savingsAccountTransactionDataValidator.validateTransactionWithPivotDate(transactionDate, account);

SdkWithdrawalService sdkWithdrawalService = new SdkWithdrawalServiceImpl();
String id = sdkWithdrawalService.processWithdrawal(identifierType.toString(), identifierValue, paymentDetail.getRoutingCode(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please dont use toString method values...

@@ -405,6 +414,58 @@ public CommandProcessingResult withdrawal(final Long savingsId, final JsonComman
.build();
}

@Transactional
@Override
public CommandProcessingResult withdrawToLinkedAccount(final Long savingsId, final JsonCommand command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider moving this into a separate, standalone module?

@@ -26,7 +26,7 @@ fineract.security.oauth.enabled=${FINERACT_SECURITY_OAUTH_ENABLED:false}
fineract.security.2fa.enabled=${FINERACT_SECURITY_2FA_ENABLED:false}

fineract.tenant.host=${FINERACT_DEFAULT_TENANTDB_HOSTNAME:localhost}
fineract.tenant.port=${FINERACT_DEFAULT_TENANTDB_PORT:3306}
fineract.tenant.port=${FINERACT_DEFAULT_TENANTDB_PORT:3307}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reverse this change!

@@ -355,7 +355,7 @@ server.tomcat.threads.min-spare=${FINERACT_SERVER_TOMCAT_THREADS_MIN_SPARE:10}
spring.security.oauth2.resourceserver.jwt.issuer-uri=${FINERACT_SERVER_OAUTH_RESOURCE_URL:http://localhost:9000/auth/realms/fineract}

spring.datasource.hikari.driverClassName=${FINERACT_HIKARI_DRIVER_SOURCE_CLASS_NAME:org.mariadb.jdbc.Driver}
spring.datasource.hikari.jdbcUrl=${FINERACT_HIKARI_JDBC_URL:jdbc:mariadb://localhost:3306/fineract_tenants}
spring.datasource.hikari.jdbcUrl=${FINERACT_HIKARI_JDBC_URL:jdbc:mariadb://localhost:3307/fineract_tenants}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reverse this change!

@@ -20,6 +20,12 @@ description = 'Fineract Integration Tests'

apply plugin: 'com.bmuschko.cargo'

repositories {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reverse this change!

Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

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

Kindly see my review!

Also please dont forget the execute code formatting and static code analysis:
./gradlew spotlessApply spotbugsMain spotbugsTest checkstyleMain checkstyleTest

Also, i dont see any integration tests or unit tests to cover any of the functionalities which is an important aspect of any contribution!

public String processWithdrawal(String payerType, String payerId, String payeeType, String payeeId, String amount, String currencyCode)
throws SdkValidationException, SdkApiException {
String id = null;
transferService.setPlatformTenantId("gorilla");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this hardcoded values!

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's hold off on these PRs... until we get some additional discussion. An SDK does not belong here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kaibalya-fynarfin would you please announce these intended changes and the pr on the Fineract DEV mail list? we have a feeling it should be discussed over there with the community!

@Override
public String processDisbursal(String payerType, String payerId, String payeeType, String payeeId, String amount, String currencyCode)
throws SdkValidationException, SdkApiException {
transferService.setBaseUrl("http://localhost:1111");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this hardcoded values!

public String processDisbursal(String payerType, String payerId, String payeeType, String payeeId, String amount, String currencyCode)
throws SdkValidationException, SdkApiException {
transferService.setBaseUrl("http://localhost:1111");
transferService.setPlatformTenantId("gorilla");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this hardcoded values!

@adamsaghy
Copy link
Contributor

@kaibalya-fynarfin Would you mind to announce this PR and new functionality of "Add features like withdrawal to linked account and disburse to linked account to increase interoperability between FSPs using Payment Hub APIs and Interop table" on the Fineract DEV mail list?

@@ -52,6 +53,7 @@
*/

@Getter
@Setter
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this resolved?


@Provider
@Component
@Scope("singleton")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed.

@@ -35,6 +36,7 @@

@Entity
@Getter
@Setter
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this resolved?

@@ -28,6 +28,13 @@ apply plugin: 'com.google.cloud.tools.jib'
apply plugin: 'org.springframework.boot'
apply plugin: 'se.thinkcode.cucumber-runner'


repositories {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is absolutely a no-go. If Payment Hub SDK is needed, let's fix it over there to be published to a publicly accessed repository, for example Maven central.

@@ -164,15 +171,15 @@ configurations.driver.each {File file ->
task createDB {
description= "Creates the MariaDB Database. Needs database name to be passed (like: -PdbName=someDBname)"
doLast {
def sql = Sql.newInstance( 'jdbc:mariadb://localhost:3306/', mysqlUser, mysqlPassword, 'org.mariadb.jdbc.Driver' )
def sql = Sql.newInstance( 'jdbc:mariadb://localhost:3307/', mysqlUser, mysqlPassword, 'org.mariadb.jdbc.Driver' )
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment shouldn't be resolved since the changes weren't yet made.


public interface SdkDisbursalService {

String processDisbursal(String payerType, String payerId, String payeeType, String payeeId, String amount, String currencyCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like this method signature. Everything is a string. Please use a DTO instead so you can't mess up the order of parameters.

Also, the amount is a String? Why?


public class SdkDisbursalServiceImpl implements SdkDisbursalService {

private static final Logger logger = LoggerFactory.getLogger(SdkWithdrawalServiceImpl.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Lombok instead.

Comment on lines +32 to +38
private TransferService transferService;

public SdkDisbursalServiceImpl() {
TransferService transferService = new TransferService();
this.transferService = transferService;

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be used like this, but rather as a Spring Bean.

id = transferService.processPayment(payerType, payerId, payeeType, payeeId, amount, currencyCode);
logger.info(id);
} catch (SdkApiException e) {
logger.error("Error status code: " + e.getStatusCode());
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, use placeholders.


public interface SdkWithdrawalService {

String processWithdrawal(String payerType, String payerId, String payeeType, String payeeId, String amount, String currencyCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for the other.

@galovics
Copy link
Contributor

galovics commented Oct 8, 2024

@kaibalya-fynarfin are you planning to fix this PR?

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