adamsaghy commented on PR #6106:
URL: https://github.com/apache/fineract/pull/6106#issuecomment-5398108951
## Summary
The core idea is sound. `ProductToGLAccountMapping.charge` becomes a plain
`chargeId` column, the JPQL is
rewritten accordingly, and charge lookups move behind an
`AccountingChargeReadService` port implemented in
`fineract-charge`. The batched `findChargesByIds` also removes the old
per-mapping eager `@ManyToOne` fetch,
which is a genuine improvement.
However, the write-side validation that the removed
`ChargeRepositoryWrapper` was implicitly providing has not
been fully re-homed, and several read paths change their response shape.
Details below.
---
## Findings
### 1. Charge-existence validation is lost for working-capital loan products
`fineract-accounting/.../service/ProductToGLAccountMappingHelper.java:611`
Removing `chargeRepositoryWrapper.findOneWithNotFoundDetection(chargeId)`
from `saveChargeToFundSourceMapping`
drops validation for **every** caller that does not go through
`fineract-provider`'s `ProductToGLAccountMappingWritePlatformServiceImpl` —
where the new `validateChargesExist()`
guard lives.
The working-capital loan product path does not go through that service:
```
WorkingCapitalProductAccountingMappingServiceImpl.createAccountMapping
-> WorkingCapitalLoanProductToGLAccountMappingHelper.saveAdvancedMappings
(fineract-working-capital-loan)
-> ProductToGLAccountMappingHelper.saveChargesToGLAccountMappings
-> saveChargeToFundSourceMapping
```
**Scenario:** `POST /workingcapitalloanproducts` with
`accountingRule = ACCRUAL_WITH_DEFERRED_REVENUE_AMORTIZATION` and
`feeToIncomeAccountMappings: [{ "chargeId": 999999, "incomeAccountId":
<valid> }]`.
- **Before:** 404 `ChargeNotFoundException`.
- **After:** `saveAndFlush` hits the DB FK `FK_acc_product_mapping_m_charge`
(`0001_initial_schema.xml`, changeSet 693) → raw constraint violation /
HTTP 500.
The same gap applies to
`WorkingCapitalLoanProductToGLAccountMappingHelper.updateAdvancedMappings`.
**Suggested fix:** keep the validation inside
`saveChargeToFundSourceMapping` (via the new
`AccountingChargeReadService`, so no charge-domain dependency returns),
rather than duplicating it in one
caller. That preserves the guarantee for all current and future callers.
---
### 2. Nested `charge` object grows from 3 fields to the full `ChargeData`
`fineract-accounting/.../service/ProductToGLAccountMappingReadPlatformServiceImpl.java:273`
(and the equivalent code in
`WorkingCapitalLoanProductAdvancedAccountingReadHelper`)
Previously each entry was built inline as a stub:
```java
ChargeData.builder().id(...).name(...).penalty(...).build()
```
Now `AccountingChargeReadServiceImpl` returns `Charge.toData()`, which
populates `amount`, `currency`,
`chargeTimeType`, `chargeAppliesTo`, `chargeCalculationType`,
`chargePaymentMode`, `active`, `minCap`/`maxCap`,
`incomeOrLiabilityAccount`, `taxGroup`, and more.
**Scenario:** `GET /loanproducts/{id}` on a product with fee mappings. Any
client or integration test asserting
the shape of `feeToIncomeAccountMappings[].charge` breaks, and the payload
grows noticeably for products with
many charge mappings.
**Suggested fix:** project only `{id, name, penalty}` in the port (e.g. a
dedicated lightweight DTO or a
JPQL constructor expression)
---
### 3. Fee/penalty classification switches basis, with no data migration
`fineract-accounting/.../domain/ProductToGLAccountMappingRepository.java:53`
`findAllFeeMappings` / `findAllPenaltyMappings` (filtered on
`mapping.charge.penalty`) are replaced by
`findAllFeeToIncomeAccountMappings` /
`findAllPenaltyToIncomeAccountMappings` (filtered on
`financialAccountType` 4/5).
**Scenario:** a charge with `penalty = true` was configured through
`feeToIncomeAccountMappings`.
`saveChargeToFundSourceMapping` never validated fee-vs-penalty — the deleted
`// TODO Vishwas: Need to validate
if given charge is fee or Penalty` said exactly that — so the row is stored
with `financial_account_type = 4`
while `m_charge.is_penalty = true`.
- **Before:** `GET /loanproducts/{id}` returned it under
`penaltyToIncomeAccountMappings`.
- **After:** it is returned under `feeToIncomeAccountMappings`.
A client doing GET → PUT round-trips the mappings and silently moves them
between buckets.
The new behaviour is arguably the *more* consistent one — it matches what
`updateChargeToIncomeAccountMappings`
already used on the write side. But it is an unannounced behaviour change on
existing data and should at
minimum be called out in the PR description.
---
### 4. Empty charge mappings now serialize as `[]` instead of being omitted
`fineract-accounting/.../service/WorkingCapitalLoanProductAdvancedAccountingReadHelper.java:110`
(also `ProductToGLAccountMappingReadPlatformServiceImpl.java:289`)
`return result.isEmpty() ? null : result;` became `return result;`. Gson
omits `null` fields, so the key
previously did not appear in the JSON at all; it now appears as `[]`.
Worse, the sibling methods in the *same class* —
`fetchPaymentTypeToFundSourceMappings` (line 63) and
`fetchReasonMappings` (line 122) — still return `null` on empty. A single
response now mixes both conventions.
**Suggested fix:** revert to `null` for consistency
---
### 5. `validateChargesExist` runs even when the mappings are discarded
`fineract-provider/.../service/ProductToGLAccountMappingWritePlatformServiceImpl.java:66`
The call sits at the top of `createLoanProductToGLAccountMapping`, before
`accountingRuleType` is decoded.
**Scenario:** `POST /loanproducts` with `accountingRule = 1` (NONE) and a
leftover
`feeToIncomeAccountMappings` block containing a since-deleted `chargeId` —
e.g. a UI that keeps the
advanced-accounting section populated when the user switches the rule to
*None*. The `switch` hits
`case NONE: break;` and never persists or reads those arrays, so the request
previously succeeded. It now
throws `ChargeNotFoundException` (404).
**Suggested fix:** move the call into the branches that actually persist the
mappings, or guard it on
`accountingRuleType != NONE`.
---
## Design notes
**Dependency direction.** The PR removes `accounting → charge` but adds
`charge → accounting` in
`fineract-charge/dependencies.gradle`, purely so
`AccountingChargeReadServiceImpl` can implement an interface
owned by the accounting module. `ChargeData` already lives in
`fineract-core` — putting the read contract there
too would decouple both modules without introducing a new edge in the
opposite direction. As it stands, the
coupling is inverted rather than removed, and the new boundary test guards
only one of the two directions.
**`AccountingCrossFeatureBoundaryTest`.** A few things worth tightening:
- It only sees `fineract-accounting`'s own test classpath, so it can only
catch a re-added Gradle dependency.
Worth stating that limitation in a class comment so the guarantee isn't
over-read.
- `private static ApplicationModules modules` is lazily initialized without
synchronization — not safe under
parallel JUnit execution.
- `owningArtifact` falls back to `"(unknown-source)"` when
`JavaClass.getSource()` is empty, and that value is
not in `FOUNDATION_ARTIFACTS` — so it counts as a violation. False-failure
risk.
- `printAccountingCrossFeatureDependencyReport` is permanently disabled
behind an undocumented system property
(`-Daccounting.boundary.report=true`). Either document it or drop it.
- `com.tngtech.archunit` is imported directly but only arrives transitively
via `spring-modulith-core` —
declare it explicitly as a test dependency.
**Build.** The `test { maxHeapSize = '2g'; jvmArgs +=
['-XX:MaxMetaspaceSize=1g'] }` block in
`fineract-accounting/build.gradle` is a whole-module cost incurred by one
test. Please add a comment explaining
it is for the Spring Modulith full-classpath scan.
**Nit.** The blank line after the license header in
`fineract-accounting/dependencies.gradle` was removed for no
apparent reason — restore it to match every other module.
--
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]
