Hi Kapil, Thank you for raising the concerns below. I’ll need some additional details to fully understand your points:
Collection Sheet API – You mentioned it appears non-functional and contains several logical errors. If it’s indeed not working, that’s a separate, high-priority discussion. Could you clarify which logical errors you were referring to, and what specifically makes you think it’s non-functional? Service annotations – You noted that service methods are not annotated with @Service and that beans are defined manually. Are you referring to the CollectionSheetWritePlatformServiceJpaRepositoryImpl bean being defined via configuration? Repository wrappers annotated with @Service – You mentioned that this mandates full unit test coverage but that they should ideally be annotated with @Component. Could you point out the exact classes you had in mind? As for the other points, I agree we can refactor and remove redundant logic—please feel free to suggest specific improvements or start work on them immediately! However, be careful by moving anything into the fineract-core… We are aiming to keep it as small as possible as everything is built on top of this module! If collection sheet are used for loans and savings - for example - than the recommended move is NOT to move this logic into core! Either: - we split the logic into fineract-loan and fineract-savings - Move the logic into a new module - Leave it in fineract-provider for now Shall you have any questions, please let us know! Regards, Adam > On 2025. Aug 11., at 12:09, Kapil Panchal > <kapil.panchal.developm...@gmail.com> wrote: > > Hi Adam, > > I’m currently working on FINERACT-2290 and have a few questions before I > submit a pull request. > > The Collection Sheet API in its current state appears non-functional and > contains several logical errors. It seems there was an earlier attempt to > convert from a JSON string request parameter to a class-based request object, > but: > > Certain fields are missing. > > The serializer is not correctly populating the objects, which causes the > conditional checks to be bypassed and results in incorrect (false) responses. > > This change set is high risk because it touches most of the loan and savings > product logic. I’ve had to refactor almost all major methods. Extensive > integration and end-to-end testing will be required to ensure there are no > regressions, especially in edge cases. At present, there are no unit or > integration tests for this functionality, and test creation is outside the > current ticket scope. I’ve been iterating on this for a while, and only today > have I reached a stable state after several experimental and build-breaking > attempts. > > Key Observations: > > Service methods are not annotated with @Service; instead, beans are defined > manually. > > Repository wrappers are annotated with @Service. This mandates full unit test > coverage for these methods, but they should ideally be annotated with > @Component. > > I agree with prior discussions on separating bean validation — having a > dedicated @Component validation class allows the request object to handle > checks independent of database queries. > > Validation components can also perform database-related validations; these > can be injected into service classes for cleaner architecture. > > Such validation components should be placed in Fineract-Core so they are > reusable across modules, reducing future refactoring needs. > > The current design of having commands in Fineract-Core and > handlers/services/repositories in respective modules is good — it cleanly > decouples command definition from execution. > > There is extensive use of this. in singleton contexts (API, Service, > Repository). While not harmful, it’s unnecessary boilerplate. > > Multiple redundant intermediate DTOs exist where the request DTO itself could > be reused for data transfer. > > I found redundant logic — e.g., a for loop with a break statement that > effectively executes only once; this can be simplified. > > Some JDBC template queries use reserved SQL keywords, causing exceptions. > Refactoring these queries resolves the issue and returns proper response > objects. > > Suggestions: > > Where appropriate, large tickets should be broken into subtasks to manage > complexity and reviewability. > > It may help to have a dedicated developer-only Slack channel for technical > discussions. This could complement other community spaces if there’s a need > to keep certain conversations more focused. > > What are your thoughts on the above? > > Thanks, > Kapil
