You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "devi-pathak2263 (via GitHub)" <gi...@apache.org> on 2026/03/26 11:47:50 UTC
[PR] Prevent no-op updates for approved amount modification (FINERACT-2541) [fineract]
devi-pathak2263 opened a new pull request, #5696:
URL: https://github.com/apache/fineract/pull/5696
…ation test (FINERACT-2541)
## 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 [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#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](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [ ] Follow our [coding conventions](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
- [ ] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2556: Prevent no-op updates for approved amount modification [fineract]
Posted by "devi-pathak2263 (via GitHub)" <gi...@apache.org>.
devi-pathak2263 commented on code in PR #5696:
URL: https://github.com/apache/fineract/pull/5696#discussion_r3007683459
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanApprovedAmountWritePlatformServiceImpl.java:
##########
@@ -58,6 +59,12 @@ public CommandProcessingResult modifyLoanApprovedAmount(final Long loanId, final
BigDecimal newApprovedAmount = command.bigDecimalValueOfParameterNamed(LoanApiConstants.amountParameterName);
+ if (newApprovedAmount.compareTo(loan.getApprovedPrincipal()) == 0) {
Review Comment:
Thanks for the suggestion! I’ve refactored the duplicated validation into a helper method and extracted the error message into a constant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2541: Prevent no-op updates for approved amount modification [fineract]
Posted by "devi-pathak2263 (via GitHub)" <gi...@apache.org>.
devi-pathak2263 commented on PR #5696:
URL: https://github.com/apache/fineract/pull/5696#issuecomment-4136157732
> @devi-pathak2263 This PR seems not much to do with [FINERACT-2541](https://issues.apache.org/jira/browse/FINERACT-2541)....
Thanks for the feedback!
This is a small follow-up improvement related to FINERACT-2541. While reviewing the existing implementation, I noticed that updating the approved amount with the same value still triggers history creation and events.
This PR adds validation to prevent such no-op updates and includes test coverage for it.
Please let me know if you’d prefer this to be linked to a separate issue instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent no-op updates for approved amount modification (FINERACT-2541) [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5696:
URL: https://github.com/apache/fineract/pull/5696#issuecomment-4135971058
@devi-pathak2263 This PR seems not much to do with FINERACT-2541....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2556: Prevent no-op updates for approved amount modification [fineract]
Posted by "devi-pathak2263 (via GitHub)" <gi...@apache.org>.
devi-pathak2263 commented on PR #5696:
URL: https://github.com/apache/fineract/pull/5696#issuecomment-4155672229
Looks like some CI jobs failed due to dependency resolution issues and timeouts. Could someone please re-run the checks when possible? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2541: Prevent no-op updates for approved amount modification [fineract]
Posted by "devi-pathak2263 (via GitHub)" <gi...@apache.org>.
devi-pathak2263 commented on PR #5696:
URL: https://github.com/apache/fineract/pull/5696#issuecomment-4136285786
> > > @devi-pathak2263 This PR seems not much to do with [FINERACT-2541](https://issues.apache.org/jira/browse/FINERACT-2541)....
> >
> >
> > Thanks for the feedback!
> > This is a small follow-up improvement related to [FINERACT-2541](https://issues.apache.org/jira/browse/FINERACT-2541). While reviewing the existing implementation, I noticed that updating the approved amount with the same value still triggers history creation and events.
> > This PR adds validation to prevent such no-op updates and includes test coverage for it.
> > Please let me know if you’d prefer this to be linked to a separate issue instead.
>
> [FINERACT-2541](https://issues.apache.org/jira/browse/FINERACT-2541) is for external investor related changes...
>
> This PR is for loan approval... it's defo not relevant to [FINERACT-2541](https://issues.apache.org/jira/browse/FINERACT-2541) :)
Thanks for the clarification, and apologies for the confusion!
I understand now that this is not related to FINERACT-2541. I came across this while reviewing the implementation and thought it might fit as a follow-up, but I see the distinction now.
I’ll create a separate JIRA issue for this improvement and update the PR accordingly.
Thanks again for the guidance!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2556: Prevent no-op updates for approved amount modification [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #5696:
URL: https://github.com/apache/fineract/pull/5696
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2556: Prevent no-op updates for approved amount modification [fineract]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on code in PR #5696:
URL: https://github.com/apache/fineract/pull/5696#discussion_r3005107526
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanApprovedAmountWritePlatformServiceImpl.java:
##########
@@ -58,6 +59,12 @@ public CommandProcessingResult modifyLoanApprovedAmount(final Long loanId, final
BigDecimal newApprovedAmount = command.bigDecimalValueOfParameterNamed(LoanApiConstants.amountParameterName);
+ if (newApprovedAmount.compareTo(loan.getApprovedPrincipal()) == 0) {
Review Comment:
Seems like duplicated code: Make and helper function, and maintain constants for long strings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2556: Prevent no-op updates for approved amount modification [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #5696:
URL: https://github.com/apache/fineract/pull/5696#discussion_r3028515498
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanApprovedAmountWritePlatformServiceImpl.java:
##########
@@ -100,15 +104,16 @@ public CommandProcessingResult modifyLoanAvailableDisbursementAmount(Long loanId
.add(newAvailableDisbursementAmount);
changes.put("newApprovedAmount", newApprovedAmount);
+ validateNewApprovedAmountDiffersFromCurrent(loan, newApprovedAmount);
Review Comment:
I dont think its valid to check whether approved amount was changed... it is valid operation to change the expected disbursement amount only... Am I missing something here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2541: Prevent no-op updates for approved amount modification [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5696:
URL: https://github.com/apache/fineract/pull/5696#issuecomment-4136174823
> > @devi-pathak2263 This PR seems not much to do with [FINERACT-2541](https://issues.apache.org/jira/browse/FINERACT-2541)....
>
> Thanks for the feedback!
>
> This is a small follow-up improvement related to [FINERACT-2541](https://issues.apache.org/jira/browse/FINERACT-2541). While reviewing the existing implementation, I noticed that updating the approved amount with the same value still triggers history creation and events.
>
> This PR adds validation to prevent such no-op updates and includes test coverage for it.
>
> Please let me know if you’d prefer this to be linked to a separate issue instead.
FINERACT-2541 is for external investor related changes...
This PR is for loan approval... it's defo not relevant to FINERACT-2541 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent no-op updates for approved amount modification (FINERACT-2541) [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5696:
URL: https://github.com/apache/fineract/pull/5696#issuecomment-4135977386
@devi-pathak2263 Please follow PR and commit title naming conventions:
`FINERACT-XXXX: Short description of the PR`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org