elnafateh commented on PR #6420: URL: https://github.com/apache/fineract/pull/6420#issuecomment-6535487027
> Hi @elnafateh, thanks for the PR and sorry it took this long to get to it. > > Unfortunately the timing is bad here. [FINERACT-2639](https://issues.apache.org/jira/browse/FINERACT-2639) was already implemented and merged yesterday via #6698: `LoanRepositoryWrapper.findOneInUserScope` and `SavingsAccountRepositoryWrapper.findOneInUserScope` now validate the office hierarchy, `create()` uses the `InUserScope` assemblers for every transfer type, `undo()` checks both offices, and there's `AccountTransferOfficeHierarchyAccessIntegrationTest` covering the sibling-office rejection. So the `create()` and `refundByTransfer()` parts of this PR just run the same check a second time. There's also #6260 open (draft) on the same ticket and the same files. Let's not have three PRs for one ticket. > > The bigger problem is that the rebase went wrong somewhere. The PR deletes 3 step definitions from `LoanRepaymentStepDef` that `LoanTransactionAdjustment.feature` and `LoanRepayment-Part4.feature` still use on develop, and re-adds 3 old steps nothing uses. It also rewrites `AccountTransfersWritePlatformServiceImplTest` and drops the existing undo tests that came with #6698. None of that is related to the office hierarchy, and as it stands the E2E suite would fail with undefined steps. > > What's actually new compared to develop is the check inside `transferFunds()` / `repayLoanWithTopup()` and the one hidden in `checkClientOrGroupActive`. I'm not convinced about the first one, see inline. Could you rebase on the latest develop, drop everything #6698 already covers, restore the unrelated files, and only keep what's actually missing? Thoughts? > > ### fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:147 > ```java > final SavingsAccount toSavingsAccount = this.savingsAccountAssembler.assembleFromInUserScope(toSavingsId, > backdatedTxnsAllowedTill); > validateTransferAccess(null, null, fromSavingsAccount, toSavingsAccount); > ``` > > > Both accounts were just loaded via `assembleFromInUserScope`, which since #6698 already calls `context.validateAccessRights(account.office().getHierarchy())`. This checks the same thing twice. Same for line 174, 205 and for `refundByTransfer` (line 653). > > ### fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:326 > ```java > toLoanAccount = accountTransferDetails.toLoanAccount(); > } > validateTransferAccess(toLoanAccount, null, fromSavingsAccount, null); > ``` > > > `transferFunds` is the internal path, not a user-initiated transfer. It's called from standing instructions, the deposit maturity / transfer-interest flows, loan charge payment from a linked savings account and `GuarantorDomainServiceImpl` ("Payment from guarantor savings"). The guarantor's savings account can easily be in a different branch than the borrower's loan, so with this check a branch user acting on their own loan would get a 403 because of the guarantor's office. Is that intended? I think this would break legit cross-office flows. If we really want it here, I'd like to see tests for those flows, not only the REST transfer. > > ### fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:600 > ```java > private void validateTransferAccess(final Loan fromLoanAccount, final Loan toLoanAccount, final SavingsAccount fromSavingsAccount, > final SavingsAccount toSavingsAccount) { > ``` > > > There's already a `validateAccessRights(AccountTransferDetails)` a few lines above that does the same thing. Four nullable params where every call site passes two `null`s isn't easy to read either. If anything survives the rebase, let's reuse what's there. > > ### fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java:2190 > ```java > private void checkClientOrGroupActive(final Loan loan) { > validateLoanAccess(loan); > ``` > > > Hiding an authorization check in a method called `checkClientOrGroupActive` is surprising. Nobody reading a call site will guess that it also does an office check. Most callers already load the loan via `assembleFromInUserScope`, so it's mostly a duplicate anyway. If there are call sites that load the loan outside the user scope and should be checked, let's switch those to the `InUserScope` loader the way #6698 did. Same for `SavingsAccountWritePlatformServiceJpaRepositoryImpl:1012`. > > ### fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/stepdef/loan/LoanRepaymentStepDef.java:721 > ```java > - @When("Customer adjust {int}th {string} transaction made on {string} with {string} payment type on {string} with {double} EUR transaction amount") > - public void adjustTransactionWithAutoIdempotencyKey(...) > - @Then("Customer is forbidden to adjust {string}th {string} transaction made on {string} with {double} EUR transaction amount due to a validation error:") > - @Then("Adjusted transaction is created with {double} amount and {string} payment type") > ``` > > > Why removing these? `LoanTransactionAdjustment.feature` and `LoanRepayment-Part4.feature` use them on develop. Same question for `makeRepaymentByExternalId`, `makeLoanRepaymentFails` and `makeLoanRepaymentUndoAfterChargeOff` at line 175+, which nothing calls. Looks like an old version of the file came back during conflict resolution. Pls restore it to develop and only add the 1 new step. > > ### fineract-provider/src/test/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImplTest.java:73 > ```java > - void givenLoanToSavingsRefund_whenUndo_thenSavingsUndoneLoanAdjustedAndTransferReversed() { > - void givenLoanToSavingsDisbursement_whenUndo_thenRejectedBeforeAnythingIsReversed() { > - void givenAlreadyReversedTransfer_whenUndo_thenThrowsGeneralPlatformDomainRuleException() { > +@MockitoSettings(strictness = Strictness.LENIENT) > ``` > > > Same here, the existing undo tests are gone. Pls add the new tests next to them instead of replacing the file. Also, why `LENIENT`? It's usually a sign that the stubbing is broader than what the test needs. > > Thanks. Thanks for the review. I’ve updated the PR accordingly, Pls review the update whenever you can. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
