-
Notifications
You must be signed in to change notification settings - Fork 6
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
[PANC-156] created credit to loan account worker #9
base: develop
Are you sure you want to change the base?
Conversation
2397fca
to
1b784bd
Compare
public String disburseLoan(String fineractTenantId, LoanDisbursementRequestDto loanDisbursementRequestDto, String channelRequest, | ||
String basicAuthHeader) { | ||
HttpHeaders headers = new HttpHeaders(); | ||
headers.add("Content-Type", "application/json"); |
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.
Use string constant for headers
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.
done
channelRequestDTO = objectMapper.readValue(channelRequest, TransactionChannelRequestDTO.class); | ||
} catch (Exception ex) { | ||
logger.info(ex.getMessage()); | ||
return null; |
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.
Don't return null it might cause a NPE
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 am required to return null because that would help me to know wheather an exception has been caused, i will set the zeebe variables accordingly.
requestBody = objectMapper.writeValueAsString(loanDisbursementRequestDto); | ||
channelRequestDTO = objectMapper.readValue(channelRequest, TransactionChannelRequestDTO.class); | ||
} catch (Exception ex) { | ||
logger.info(ex.getMessage()); |
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.
Properly log the error with an message and use logger.error
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.
done
logger.info("Response: {} status: {}", exchange.getBody().toString(), exchange.getStatusCode()); | ||
return exchange.getBody(); | ||
} catch (Exception ex) { | ||
logger.error(ex.getMessage()); |
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.
log the error with proper message
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.
done
RestTemplateUtil restTemplateUtil = new RestTemplateUtil(); | ||
ResponseEntity<String> exchange = restTemplateUtil.exchange(url, HttpMethod.POST, headers, requestBody); | ||
logger.info(exchange.toString()); | ||
logger.info("Response: {} status: {}", exchange.getBody().toString(), exchange.getStatusCode()); |
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.
Don't log the response body as it might contain sensitive info
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.
removed
if (isAmsLocalEnabled) { | ||
Map<String, Object> variables = job.getVariablesAsMap(); | ||
logger.info(variables.toString()); | ||
String fineractTenantId = variables.get("tenantId").toString(); |
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.
Use Zeebe variables here instead of hardcoding "tenantId" do this for all the variables
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.
done for all the variables.
|
||
public class LoanDisbursementRequestDtoHelper { | ||
|
||
public LoanDisbursementRequestDto createLoanDisbursementRequestDto(String originDate) { |
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.
Add proper exception handling to deal with potential parsing errors
|
||
private static final Logger logger = LoggerFactory.getLogger(RestTemplateUtil.class); | ||
|
||
private String keystorePath = "keystore.jks"; |
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.
Don't hardcode the path and password as these are sensitive info
return new RestTemplate(factory); | ||
} catch (Exception e) { | ||
|
||
throw new RuntimeException("Error creating RestTemplate with custom SSL context", e); |
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.
Include more descriptive logging before throwing the exception. Additionally, consider creating a custom exception to encapsulate the specific error scenario
private RestTemplate createCustomRestTemplate() { | ||
try { | ||
KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); | ||
keyStore.load(new FileInputStream(new File(keystorePath)), keystorePassword.toCharArray()); |
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.
Use a try-with-resources block to ensure that the FileInputStream is closed properly, if not closed properly it might cause resource leak
created credit to loan account worker for the loan disbursement bpmn.