FINERACT-2152: API update and delete interest pause #4235
Conversation
kulminsky
force-pushed
the
FINERACT-2152/delete_and_update_interest_pause_period
branch
from
fbf927f to
44c9a81
Compare
|
The implementation does not include the addition of audit fields (created_by, created_on_date, updated_by, updated_on_date) to the database schema, as specified in the acceptance criteria. Could you confirm if this will be handled in a separate PR or if it was overlooked? |
| LocalDate startDate = parseDate(startDateString, dateFormat, locale); | ||
| LocalDate endDate = parseDate(endDateString, dateFormat, locale); | ||
|
|
||
| validateInterestPauseDates(loan, startDate, endDate, dateFormat, locale); |
There was a problem hiding this comment.
There is lack of validation to ensure that operations on interest pauses are restricted to active loans.
There was a problem hiding this comment.
Also there are many missing validations:
- only supported for progressive loans and with recalculate interest = true
- Multiple Interest pause can happen in a loan life cycle, but no overlapping should be allowed
- Loan must be Active
|
Also, I have not been able to find the implementation of transaction reprocessing to reflect changes after updating or deleting an interest pause. This was mentioned in the task, so I'm asking, but please correct me if I'm wrong |
| @@ -136,6 +136,9 @@ public class CommandSource extends AbstractPersistableCustom<Long> { | |||
| @Column(name = "loan_external_id", length = 100) | |||
| private ExternalId loanExternalId; | |||
|
|
|||
| @Column(name = "term_variation_id", length = 100) | |||
| private Long termVariationId; | |||
There was a problem hiding this comment.
No need for new field. Using resourceId is more than enough for this use case.
| @@ -45,6 +45,7 @@ public class CommandWrapper { | |||
| private final Long organisationCreditBureauId; | |||
| private final String jobName; | |||
| private final ExternalId loanExternalId; | |||
| private final Long termVariationId; | |||
| @@ -49,18 +49,21 @@ public class CommandWrapperBuilder { | |||
| private String jobName; | |||
| private String idempotencyKey; | |||
| private ExternalId loanExternalId; | |||
| private Long termVariationId; | |||
There was a problem hiding this comment.
No need for new field. You might wanna use rather the entityId
| @@ -72,33 +72,35 @@ public final class JsonCommand { | |||
| private final Long organisationCreditBureauId; | |||
| private final String jobName; | |||
| private final ExternalId loanExternalId; | |||
| private final Long termVariationId; | |||
There was a problem hiding this comment.
No need for new field. You might wanna use rather the entityId
| public Response deleteInterestPause(@PathParam("loanId") @Parameter(description = "loanId") final Long loanId, | ||
| @PathParam("variationId") @Parameter(description = "variationId") final Long variationId) { | ||
|
|
||
| this.context.authenticatedUser().validateHasReadPermission(RESOURCE_NAME_FOR_PERMISSIONS); |
There was a problem hiding this comment.
I know i have missed this in the previous PR, but i noticed here... Just because the user has read permission on the loan, it shall not be able to add, modify or delete interest pauses! If we are validating permission that should be "UPDATE LOAN" in this case.
| @PathParam("variationId") @Parameter(description = "variationId") final Long variationId, | ||
| @RequestBody(required = true) final InterestPauseRequestDto request) { | ||
|
|
||
| this.context.authenticatedUser().validateHasReadPermission(RESOURCE_NAME_FOR_PERMISSIONS); |
There was a problem hiding this comment.
I know i have missed this in the previous PR, but i noticed here... Just because the user has read permission on the loan, it shall not be able to add, modify or delete interest pauses! If we are validating permission that should be "UPDATE LOAN" in this case.
| variation.setTermApplicableFrom(startDate); | ||
| variation.setDateValue(endDate); | ||
|
|
||
| LoanTermVariations updatedVariation = loanTermVariationsRepository.saveAndFlush(variation); |
There was a problem hiding this comment.
Immediate save and flush is not needed...
|
|
||
| validateInterestPauseDates(loan, startDate, endDate, dateFormat, locale); | ||
|
|
||
| LoanTermVariations variation = loanTermVariationsRepository.findByIdAndLoanId(variationId, loanId).orElseThrow( |
There was a problem hiding this comment.
Should be filtered to "INTEREST_PAUSE" type loan term variations!
kulminsky
force-pushed
the
FINERACT-2152/delete_and_update_interest_pause_period
branch
6 times, most recently
from
7bd7081 to
46a2c7a
Compare
kulminsky
force-pushed
the
FINERACT-2152/delete_and_update_interest_pause_period
branch
from
46a2c7a to
ccab6d3
Compare
Description
API update and delete 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.