Hi Adam,
Following up on the Collection Sheet API.
*API Overview*
The API currently exposes two query parameters:
1.
*generateCollectionSheet:* GET request.
-
Throws an exception due to SQL errors in the JDBC statement — the
query uses reserved SQL keywords.
2.
*saveCollectionSheet:* POST request.
-
Does not throw an exception, but the results returned are incorrect.
3. *Fineract-Core:* Yes, I agree it has to be lightweight, but the
commands, request and response objects are lightweight. All the logic and
validations are handled by the respective modules including command
handlers.
*Deserializer Issue*
In CollectionSheetBulkDisbursalCommandFromApiJsonDeserializer.java, the
current logic:
if (element.isJsonObject()) {
if (topLevelJsonElement.has("bulkDisbursementTransactions")
&&
topLevelJsonElement.get("bulkDisbursementTransactions").isJsonArray())
{
final JsonArray array =
topLevelJsonElement.get("bulkDisbursementTransactions").getAsJsonArray();
loanDisbursementTransactions = ...
…always returns false because it assumes "bulkDisbursementTransactions" is
a JSON array.
A working approach would be:
if (topLevelJsonElement != null && topLevelJsonElement.isJsonObject()) {
JsonObject root = topLevelJsonElement.getAsJsonObject();
if (root.has("bulkDisbursementTransactions")) {
JsonElement disbursementElement =
root.get("bulkDisbursementTransactions");
if (disbursementElement != null && disbursementElement.isJsonObject()) {
JsonObject disbursementObj = disbursementElement.getAsJsonObject();
if (disbursementObj.has("bulkRepaymentTransactions")) {
JsonElement savingsElement =
disbursementObj.get("bulkRepaymentTransactions");
if (savingsElement != null && savingsElement.isJsonArray()) {
JsonArray array = savingsElement.getAsJsonArray();
...
This correctly navigates the object structure and returns the expected
values.
*Annotations*
-
*Wrapper classes annotated with @Service*: I am changing all
the @Service to @Component
1.
PaymentTypeRepositoryWrapper
2.
LoanRepositoryWrapper
3.
HolidayRepositoryWrapper
4.
WorkingDaysRepositoryWrapper
-
*Service classes not annotated with @Service but defined via
configuration beans*: I am going ahead and annotating it with @Service
as later we can deprecate the configuration class
1.
PaymentDetailWritePlatformServiceJpaRepositoryImpl
2.
AccountAssociationsReadPlatformServiceImpl
3.
CollectionSheetReadPlatformServiceImpl
4.
CollectionSheetWritePlatformServiceJpaRepositoryImpl
5.
LoanDisbursementService
6.
LoanUtilService
7.
DepositAccountWritePlatformServiceJpaRepositoryImpl
*Suggestion – API Versioning for all *
https://issues.apache.org/jira/browse/FINERACT-2169 cards.
Given the scale of changes here, this may be a good opportunity to
consider *API
versioning*.
For example:
-
Current API: /v1/collectionsheet
-
New implementation: /v2/collectionsheet (using a separate API class)
This would allow us to refactor and restructure with more freedom while
keeping backward compatibility intact. This would also mean that each API
will now be rigorous with unit/integration/e2e tests.
What are your thoughts on proceeding with versioning for this change?
Thanks,
Kapil
On Mon, Aug 11, 2025 at 5:12 PM Ádám Sághy <[email protected]> wrote:
> Hi Kapil,
>
> Thank you for raising the concerns below. I’ll need some additional
> details to fully understand your points:
>
> 1.
>
> *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?
> 2.
>
> *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?
> 3.
>
> *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 <
> [email protected]> 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
>
>
>
