Aman-Mittal commented on PR #204: URL: https://github.com/apache/fineract-consumer-facing/pull/204#issuecomment-6212495450
> Hi maintainers, a note on the failing CI before this goes further. > > LayeringArchitectureTest.featuresDoNotCallFineractFeignClientsDirectly fails on this PR. TransfersCommandServiceImpl is one of the services frozen in the ArchUnit baseline for that rule, and the baseline records its exact constructor signature. Adding the LoansQueryService dependency (needed to check the loan’s totalOutstanding and status) changes that signature, so the store treats it as a new violation. No new Feign client dependencies were added. > > I don’t want to work around this (for example by hand-editing the baseline or using setter injection) without your input. Two options: > > Preparatory refactor: move the three Feign calls (AccountTransfersApi, ClientApi, SavingsAccountApi) out of TransfersCommandServiceImpl into an infrastructure gateway, as a separate behavior-preserving PR. The baseline then shrinks, and this PR is rebased on top. Explicit baseline update: update the constructor lines for this class in the baseline in this PR, with your approval, since the violation count stays the same. > > Which would you prefer? Is anyone already migrating the frozen services? I’ll keep this PR in draft until we agree on the approach. Thanks for flagging this rather than working around it. Please don't do either of the workarounds: the store is intentionally shrink-only, and a hand-edited signature (even at equal count) is indistinguishable from a new violation to ShrinkOnlyViolationStore, so we don't accept those edits. Setter injection would just hide the same dependency. Option 1 is the way to go. Please open a small, behavior-preserving PR that moves the ClientApi / SavingsAccountApi / AccountTransfersApi calls out of TransfersCommandServiceImpl into a narrow interface + impl under consumer.infrastructure (see OwnedAccountsCache / ClientStandingChecker for infrastructure classes that call the generated clients). Keep FineractCaller translation so the transfers error codes don't change. The store lines for this class should then drop out on the next ./gradlew test --tests '*Architecture*'; commit the shrunken file. While you're there, add a lightweight loan lookup that returns only status and payoff amount, with no repaymentSchedule,transactions associations. Then rebase this PR on it and drop the LoansQueryService dependency. See my inline comments: getLoan is heavier than needed, and for interest-recalculating products the cap should probably be the prepayLoan template amount rather than totalOutstanding. Nobody is currently migrating the frozen services as far as I know, so go ahead. I've added my other review comments inline. -- 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]
