-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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-2152: API Create and retrieve interest pause #4222
FINERACT-2152: API Create and retrieve interest pause #4222
Conversation
@RequestBody(required = true, content = @Content(schema = @Schema(implementation = LoanInterestPauseApiResourceSwagger.LoanInterestPauseRequest.class))) | ||
@ApiResponses({ | ||
@ApiResponse(responseCode = "200", description = "OK", content = @Content(schema = @Schema(implementation = LoanInterestPauseApiResourceSwagger.LoanInterestPauseResponse.class))) }) | ||
public String createInterestPause(@PathParam("loanId") @Parameter(description = "loanId") final Long loanId, |
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.
You can return the DTO object. WE dont need explicit serialization. Also, if you return the DTO, we dont need to create Swagger schema either, it will do automatically.
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.
Fixed
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.
It is still returning String...
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.
CommandProcessingResult
/** | ||
* Swagger for LoanInterestPause API. | ||
*/ | ||
final class LoanInterestPauseApiResourceSwagger { |
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.
If you start using DTO as response, some of these classes are not needed
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.
Fixed
@Override | ||
public CommandProcessingResult processCommand(final JsonCommand command) { | ||
final ExternalId loanExternalId = command.getLoanExternalId(); | ||
final String startDate = command.stringValueOfParameterNamed("startDate"); |
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.
Fetch it as LocalDate
instead
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.
Fixed
public CommandProcessingResult processCommand(final JsonCommand command) { | ||
final ExternalId loanExternalId = command.getLoanExternalId(); | ||
final String startDate = command.stringValueOfParameterNamed("startDate"); | ||
final String endDate = command.stringValueOfParameterNamed("endDate"); |
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.
Fetch it as LocalDate
instead
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.
Fixed
...n/java/org/apache/fineract/portfolio/interestpauses/handler/InterestPauseCommandHandler.java
Show resolved
Hide resolved
public class InterestPauseData { | ||
|
||
private final Long id; | ||
private final String startDate; |
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.
LocalDate would be better!
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.
Fixed
private List<InterestPauseData> mapToInterestPauseData(List<LoanTermVariationsData> variations) { | ||
return variations.stream() | ||
.map(variation -> new InterestPauseData(variation.getId(), variation.getTermVariationApplicableFrom().toString(), | ||
variation.getDateValue().toString(), DateUtils.DEFAULT_DATE_FORMAT, "en")) |
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.
Hardcoded locale is incorrect. We might not needed at all. Return the system locale. If we change the response field to return LocalDate
we can use the date formatter from the converter: LocalDateJsonConverter
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.
Fixed
DateTimeFormatter formatter = DateTimeFormatter.ofPattern(dateFormat).withLocale(new Locale(locale)); | ||
return LocalDate.parse(date, formatter); | ||
} catch (DateTimeParseException e) { | ||
throw new IllegalArgumentException( |
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 the DateUtils.parseLocalDate()
method and it will handle the exceptions properly!
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.
Fixed
} | ||
|
||
if (!endDate.isAfter(startDate)) { | ||
throw new IllegalArgumentException("Interest pause end date must be later than the interest pause start date."); |
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.
Rather use GeneralPlatformDomainRuleException
!
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.
Fixed
} | ||
|
||
if (endDate.isAfter(loan.getMaturityDate())) { | ||
throw new IllegalArgumentException("Interest pause end date cannot be later than loan maturity date."); |
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.
Rather use GeneralPlatformDomainRuleException
!
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.
Fixed
LocalDate endDate = parseAndValidateDate(endDateStr, dateFormat, locale); | ||
|
||
if (startDate.isBefore(loan.getSubmittedOnDate())) { | ||
throw new IllegalArgumentException("Interest pause start date cannot be earlier than loan start date."); |
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.
Rather use GeneralPlatformDomainRuleException
!
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.
Fixed
|
||
private void validateInterestPauseDates(Loan loan, String startDateStr, String endDateStr, String dateFormat, String locale) { | ||
if (startDateStr == null || endDateStr == null) { | ||
throw new IllegalArgumentException("Both `startDate` and `endDate` parameters are mandatory."); |
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.
validateOrThrow("interest-pause", baseDataValidator -> {baseDataValidator.reset().parameter(<parameter_name>).value(<parameter_value>).notNull();
might be better here!
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.
Fixed
|
||
@Override | ||
public CommandProcessingResult processCommand(final JsonCommand command) { | ||
final Long loanId = command.getLoanId(); |
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.
These field values would be better in the service.
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.
Fixed
|
||
Long loanId = loanRepository.findIdByExternalId(loanExternalId); | ||
if (loanId == null) { | ||
throw new IllegalArgumentException("Loan not found with External ID: " + loanExternalId); |
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.
LoanNotFoundException would be better.. but if you inject LoanRepositoryWrapper, it has method to handle not found case
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.
Fixed
public Long createInterestPause(ExternalId loanExternalId, String startDate, String endDate, String dateFormat, String locale) { | ||
this.context.authenticatedUser(); | ||
|
||
Long loanId = loanRepository.findIdByExternalId(loanExternalId); |
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.
Not needed. Fetching id here and 3 lines later fetching the whole entity is weird. Fetch the entity and handle the not found case (see below how)
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.
Fixed
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 see no changes...
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.
Changed to loanRepositoryWrapper.findOneWithNotFoundDetection(loanExternalId)
throw new IllegalArgumentException("Loan not found with External ID: " + loanExternalId); | ||
} | ||
|
||
Loan loan = loanRepository.findById(loanId).orElseThrow(() -> new IllegalArgumentException("Loan not found with ID: " + loanId)); |
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.
See above how to handle not found case
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.
Fixed
|
||
validateInterestPauseDates(loan, startDate, endDate, dateFormat, locale); | ||
|
||
LocalDate startLocalDate = parseAndValidateDate(startDate, dateFormat, locale); |
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.
you can parse from command the LocalDate or by using the DateUtils.parseLocalDate()`
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.
Fixed
endLocalDate, false, loan); | ||
|
||
LoanTermVariations savedVariation = loanTermVariationsRepository.save(variation); | ||
loanTermVariationsRepository.flush(); |
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.
You can call saveAndFlush in one step...
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.
Fixed
LoanTermVariations savedVariation = loanTermVariationsRepository.save(variation); | ||
loanTermVariationsRepository.flush(); | ||
|
||
return savedVariation.getId(); |
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.
Build the CommandResult object here... not in the handler
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.
Fixed
public Long createInterestPauseByLoanId(Long loanId, String startDate, String endDate, String dateFormat, String locale) { | ||
this.context.authenticatedUser(); | ||
|
||
final Loan loan = loanRepository.findById(loanId) |
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.
See above comments
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.
Fixed
@@ -30,7 +30,8 @@ public enum LoanTermVariationType { | |||
GRACE_ON_INTEREST(7, "loanTermType.graceOnInterest"), // | |||
GRACE_ON_PRINCIPAL(8, "loanTermType.graceOnPrincipal"), // | |||
EXTEND_REPAYMENT_PERIOD(9, "loanTermType.extendRepaymentPeriod"), // | |||
INTEREST_RATE_FROM_INSTALLMENT(10, "loanTermType.interestRateFromInstallment"); // | |||
INTEREST_RATE_FROM_INSTALLMENT(10, "loanTermType.interestRateFromInstallment"), // | |||
INTEREST_PAUSE(11, "loanTermType.interestPause"); // |
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 the new type to the fromInt
method!
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.
Fixed
8ff5a07
to
3f9c84e
Compare
3f9c84e
to
d983244
Compare
|
||
final CommandProcessingResult result = this.commandsSourceWritePlatformService.logCommandSource(commandRequest); | ||
|
||
return this.toApiJsonSerializer.serialize(result); |
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.
No need for this. Just return the DTO AS-IS. Explicit serialization is not needed
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.
Fixed
|
||
this.context.authenticatedUser().validateHasReadPermission(RESOURCE_NAME_FOR_PERMISSIONS); | ||
|
||
return this.interestPauseReadPlatformService.retrieveInterestPausesByExternalId(loanExternalId).stream() |
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 dont need we need to map InterestPauseResponseDto
to InterestPauseResponseDto
:)
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.
Fixed
|
||
@Test | ||
public void testCreateInterestPauseByLoanId_validRequest_shouldSucceed() { | ||
Long loanId = 1L; |
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.
Depending on a hard coded loan is not correct. Please create your own loan application and use that as part of the testing!
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.
Fixed
...ation-tests/src/test/java/org/apache/fineract/integrationtests/LoanInterestPauseApiTest.java
Show resolved
Hide resolved
...ation-tests/src/test/java/org/apache/fineract/integrationtests/LoanInterestPauseApiTest.java
Show resolved
Hide resolved
private final RequestSpecification requestSpec; | ||
private final ResponseSpecification defaultResponseSpec; | ||
|
||
public InterestPauseHelper(final RequestSpecification requestSpec, final ResponseSpecification defaultResponseSpec) { |
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.
Please use the fineract-client
API instead!
Example:
org.apache.fineract.integrationtests.common.loans.LoanTransactionHelper#makeLoanRepayment(java.lang.Long, org.apache.fineract.client.models.PostLoansLoanIdTransactionsRequest)
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.
Fixed
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.
Kindly see my review!
d983244
to
65137ab
Compare
final ExternalId loanExternalId = command.getLoanExternalId(); | ||
result = interestPauseService.createInterestPause(loanExternalId, startDate, endDate, dateFormat, locale); | ||
} else { | ||
throw new IllegalArgumentException("Either loanId or loanExternalId must be provided."); |
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.
Please throw PlatformApiDataValidationException
. IllegalArgumentExceptions are not covered by any Exception handler, so it will fallback to HTTP 500 - Internal server error which is not good.
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.
Fixed
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
@Service |
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.
You should consider using Spring bean configuration, so it is easier to customize (override) this service.
Example:
org.apache.fineract.portfolio.loanaccount.service.LoanReadPlatformServiceImpl
and org.apache.fineract.portfolio.loanaccount.starter.LoanAccountConfiguration#loanReadPlatformService
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.
Fixed
@Override | ||
@Transactional(readOnly = true) | ||
public List<InterestPauseResponseDto> retrieveInterestPauses(Long loanId) { | ||
this.context.authenticatedUser(); |
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.
Authenticated user and permission is already checked on API layer. We dont need to do here again!
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
@Override | ||
@Transactional(readOnly = true) | ||
public List<InterestPauseResponseDto> retrieveInterestPausesByExternalId(String loanExternalId) { | ||
this.context.authenticatedUser(); |
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.
Authenticated user and permission is already checked on API layer. We dont need to do here again!
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
|
||
@Override | ||
@Transactional | ||
public CommandProcessingResult createInterestPause(ExternalId loanExternalId, LocalDate startDate, LocalDate endDate, String dateFormat, |
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.
Write operation should not be in a Read service. Would you mind to extract them?
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.
Moved to InterestPauseWritePlatformService
private CommandProcessingResult processInterestPause(Supplier<Loan> loanSupplier, LocalDate startDate, LocalDate endDate, | ||
String dateFormat, String locale) { | ||
|
||
this.context.authenticatedUser(); |
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.
Permissions were already checked in the Command processor, we dont need to do it again here!
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
@Transactional | ||
public CommandProcessingResult createInterestPauseByLoanId(Long loanId, LocalDate startDate, LocalDate endDate, String dateFormat, | ||
String locale) { | ||
return processInterestPause(() -> loanRepositoryWrapper.findOneWithNotFoundDetection(loanId), startDate, endDate, dateFormat, |
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.
Write operation should not be in a Read service. Would you mind to extract them?
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.
Moved to InterestPauseWritePlatformService
public CommandProcessingResult createInterestPause(ExternalId loanExternalId, LocalDate startDate, LocalDate endDate, String dateFormat, | ||
String locale) { | ||
return processInterestPause(() -> { | ||
Long loanId = loanRepositoryWrapper.findIdByExternalId(loanExternalId); |
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.
This is not needed:
loanRepositoryWrapper.findOneWithNotFoundDetection(loanId);
is covering the use case if the loan cannot be found!
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.
Changed to loanRepositoryWrapper.findOneWithNotFoundDetection(loanExternalId)
|
||
@Override | ||
@Transactional | ||
public CommandProcessingResult createInterestPauseByLoanId(Long loanId, LocalDate startDate, LocalDate endDate, String dateFormat, |
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 dont think we need the "ByLoanId" postfix, createInterestPause
would be enough...
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
|
||
@Override | ||
@Transactional(readOnly = true) | ||
public List<InterestPauseResponseDto> retrieveInterestPausesByExternalId(String loanExternalId) { |
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 dont think we need the "ByExternalId" postfix. retrieveInterestPauses
would be enough.
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
private final LoanRepositoryWrapper loanRepositoryWrapper; | ||
|
||
@Override | ||
@Transactional(readOnly = true) |
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.
We can move the @Transactional(readOnly = true)
on class level. All methods should be read-only in a Read service!
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
public CommandProcessingResult processCommand(final JsonCommand command) { | ||
CommandProcessingResult result; | ||
|
||
final LocalDate startDate = LocalDate.parse(command.stringValueOfParameterNamed("startDate")); |
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.
This is incorrect. You might wanna move the conversion of the fields into the service layer (i like to keep the validation and conversions at the same place, so anything that could fail is in one place, we dont need to figure out where exceptions might occurred...) and also first we should validate the incoming parameters, and just after we should try to convert them (or maybe doing in one shot the validation and conversion...that is fine). Also the conversion should happen based on the dateformat and locale... so to parse the incoming "startDate" as string into LocalDate, we need to use the "startDate + locale + dateformat" combo...
What do you think?
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.
Moved validation and parse to InterestPauseWritePlatformService
endDate, loan.getMaturityDate()); | ||
} | ||
|
||
if (!endDate.isAfter(startDate)) { |
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.
Start date and End date can be the same, which means for 1 day the interest is paused!
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.
Fixed
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.
Kindly check my updated review!
19c4770
to
08dd456
Compare
08dd456
to
d7f0956
Compare
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.
LGTM
Description
API Create and retrieve interest pause
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.