Aman-Mittal commented on code in PR #231: URL: https://github.com/apache/fineract-consumer-facing/pull/231#discussion_r4431026561
########## consumer/src/test/resources/archunit_store/feature-classes-do-not-depend-on-the-generated-fineract-feign-clients-fd283b04.txt: ########## Review Comment: This adds nine `BeneficiariesCommandServiceImpl` entries back into the store. `main` already moved beneficiaries behind `BeneficiariesFineractClient` (fdb9a7f), and the store is a `ShrinkOnlyViolationStore`, which never allows new lines (see the header). It looks like the file was regenerated from a stale checkout. The only change here should be deleting the `SavingsQueryServiceImpl` lines. #228 edits this same file, so whichever of the two lands second will need a rebase. ########## consumer/src/main/java/org/apache/fineract/consumer/infrastructure/fineractclient/service/SavingsFineractClient.java: ########## Review Comment: Each method here is a 1:1 passthrough of the generated API, so the feature service still has to supply Fineract transport details. `searchSavingsAccountTransactions` takes 16 positional arguments, and `LOCALE`/`DATE_FORMAT` remain in `SavingsQueryServiceImpl`. `LoansQueryFineractClient` on main keeps the fixed parameters (associations, locale, date format) inside the adapter and exposes domain-shaped methods. Could this follow the same pattern? For example `getAccount(id)` instead of `retrieveSavingsAccount(id, null, null, null)`, and a search method that takes the query fields you actually use. Also note that #155 adds a second wrapper around `SavingsAccountApi` (`SavingsAccountApiClient.retrieveSavingsAccount`). It would be good to agree on a single read adapter for the two PRs. ########## consumer/src/main/java/org/apache/fineract/consumer/savings/query/service/SavingsQueryServiceImpl.java: ########## Review Comment: Nits: - The `.map(...)` chain in `toBrowseTemplate` lost its continuation indent. - The import block was reordered, which isn't related to this change. - The trailing newline was dropped here and in the two new files. Reverting these would keep the diff limited to the refactor. -- 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]
