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
