FINERACT-2362: Collection Sheet - [Refactor] - Save Collection Sheet - Individual Collection Sheet

[kapil-panchal]

Description

Code review comments from #5056 added.

The following was changed:

1. Refactored request objects.
2. Refactored mysql statements.
3. Added Exception handling.

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per https://github.com/apache/fineract/#pull-requests
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

[Aman-Mittal]

Sharing AI code review for report based on ticket and code changes.
NOTE: These are for reference only so it can be dismissed

⚠️ Code Review Summary: DTO and Request Inconsistencies

1. Inconsistent Field Names Between Request and DTO Classes

Observation:

• Request payloads use different field names than their corresponding DTOs.
  • bulkDisbursementTransactions includes: externalId, receiptNumber, bankNumber, etc.
  • bulkSavingsDueTransactions includes: savingsId, externalId, etc.

Code Changes:

• DisbursementTransactionsRequest and SavingDueTransactionRequest DTOs were updated with fields like loanId, transactionAmount, and paymentTypeId.

Red Flag:

• bulkSavingsDueTransactions request contains savingsId, but the SavingDueTransactionRequest DTO does not define savingsId.
  → This will cause the savingsId field to be ignored during deserialization or trigger a binding error if strict mapping is enabled.

---

2. Missing Fields in DTOs Used by Requests

Observation:

• Several request fields are not represented in their corresponding DTOs.

Request Type	Missing Fields in DTO	DTO Name
bulkRepaymentTransactions	externalId, receiptNumber, bankNumber	RepaymentTransactionRequest
bulkSavingsDueTransactions	externalId, receiptNumber, bankNumber, savingsId	SavingDueTransactionRequest

Red Flag:

• Missing fields will be ignored silently or cause incomplete data mapping.
• This can result in data loss or failed validation downstream.

---

3. Lombok Annotation Changes

Change:

• Replaced @Data and @FieldNameConstants with @Getter and @Setter.

Impact:

• This is acceptable for standard use but introduces risk if any logic relies on the generated field name constants from @FieldNameConstants.

Red Flag:

• Possible breakage in reflection-based mapping, query building, or frameworks using field constants.

---

4. SQL Query Alias Updates

Change:

• In CollectionSheetReadPlatformServiceImpl, SQL alias renamed from of → ofc.

Impact:

• The change improves clarity, but any downstream code referencing the old alias (e.g., result mappers or extractors) could fail.

Minor Red Flag:

• Verify consistency across related SQL queries, mappers, and entity references.

---

5. Exception Handling Adjustments

Change:

• Updated to throw PlatformApiDataValidationException with the original error message.

Impact:

• This improves validation visibility and consistency.

Note:

• Ensure all relevant validation details (e.g., field name, rejected value, message) are correctly passed.
• No critical issues identified, but clarity and completeness of error responses should be verified.

---

6. Request Object vs. DTO Structure Alignment

Observation:

• All three bulk operations expect lists in requests — consistent with code:
  • ✅ bulkDisbursementTransactions
  • ✅ bulkRepaymentTransactions
  • ✅ bulkSavingsDueTransactions

However:

• Field name mismatches and missing DTO properties (as noted above) will prevent accurate mapping and may lead to:
  • Partial deserialization
  • Silent data loss
  • Validation or persistence errors

---

7. Potential Serialization / Deserialization Issues

Observation:

• Jackson (or equivalent) will:
  • Ignore unknown fields → risk of silent data loss.
  • Throw exceptions if strict validation is configured.

Risk:

• Requests may fail with "Unrecognized field" errors.
• Missing DTO fields could lead to incomplete transaction data being processed.

---

8. Response Object Limitations

Observation:

• Response objects currently omit detailed transaction information (e.g., externalId, receiptNumber).

Impact:

• This is acceptable for high-level success responses but may not satisfy clients expecting per-transaction confirmations.

Recommendation:

• Consider enhancing the response to include key identifiers (IDs, statuses) for better traceability.

---

🔴 Summary of Red Flags and Recommendations

Category	Issue	Severity	Recommendation
Missing DTO Fields	savingsId, externalId, receiptNumber, bankNumber missing	🔴 Critical	Add missing fields or adjust request models for consistency
Field Name Mismatches	Request vs DTO naming differences	🔴 Critical	Standardize field names across layers
Lombok Annotation Change	Removed @FieldNameConstants	�\dfe0 Medium	Check reflection/mapping dependencies
SQL Alias Change	of → ofc	�\dfe1 Low	Verify consistency in dependent queries/mappers
Exception Handling	Improved but needs completeness	�\dfe2 Low	Ensure detailed, user-friendly validation errors
Serialization Risk	Silent data loss on unmapped fields	🔴 Critical	Enable strict validation or add missing DTO fields
Response Completeness	Missing transaction identifiers	�\dfe1 Low	Include key IDs in responses for client confirmation

[kapilpanchal123]

The above AI changes are not necessary as it is just comparing the changes with the previous changes. The previous changes was what made this feature not work? The new feature changes that I have made works by default!

[Aman-Mittal]

Thanks for clarification, No more Questions for my side. LGTM.

[kapilpanchal123]

:) Thanks Aman!

[kapilpanchal123]

I am unaware of the merge process, how can this card be merged to develop upstream?

[Aman-Mittal]

@adamsaghy Please have a look at this PR,