You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "rymghosn (via GitHub)" <gi...@apache.org> on 2026/09/29 11:27:56 UTC
[PR] FINERACT-2878: Fix loan bulk-import template download on charge and office defined names, require dateFormat [fineract]
rymghosn opened a new pull request, #6543:
URL: https://github.com/apache/fineract/pull/6543
## Description
JIRA: [FINERACT-2878](https://issues.apache.org/jira/browse/FINERACT-2878)
`GET /loans/downloadtemplate` (the Loan Accounts bulk-import template) returns HTTP 500 in three situations, which the web app shows as a Download button that does nothing:
1. **A loan charge name contains a character Excel does not allow in a defined name** (`%`, `-`, `&`, `'`, …): `IllegalArgumentException: Invalid name: 'CHARGE_NAME_Service_Fee_5%-10%__VAT_': name must be letter, digit, period, or underscore`.
2. **Two office names map to the same defined name.** Office names are unique, but Excel defined names are case-insensitive and the sanitiser maps disallowed characters to `_`, so `Head Office` and `head office` (or `Head-Office`) both become `Group_Head_Office`: `IllegalArgumentException: The workbook already contains this name: Group_head_office`.
3. **`dateFormat` is omitted**: `NullPointerException: ... because "pFormat" is null` from `HSSFDataFormat.getFormat`, reached via `LoanProductSheetPopulator.populate`. The other `*/downloadtemplate` endpoints behave the same.
**Root cause and fix:**
- `LoanWorkbookPopulator.setNames` builds the `CHARGE_NAME_`/`CHARGE_AMOUNT_`/`CHARGE_AMOUNT_TYPE_<charge>` names with `replaceAll("[ )(]", "_")` instead of the shared `sanitizeName()` allowlist (FINERACT-2669) that every other named-range loop uses. FINERACT-2712 added the duplicate guard to this loop but kept the old regex. Now uses `sanitizeName()`. No formula references these names, so the template content is unchanged.
- The per-office `Staff_`/`Client_`/`Group_<office>` loop in the same method creates names for every office, because the sheet populators record a begin/end index even for an office with no rows. Unlike the product and charge loops (FINERACT-2712), it has no de-duplication guard. Added the same seen-names guard, keyed on the sanitised, upper-cased name.
- `BulkImportWorkbookPopulatorServiceImpl.getTemplate` never checked `dateFormat`. A blank value is now rejected up front with a `PlatformApiDataValidationException` (`error.msg.dateformat.mandatory`, parameter `dateFormat`), so every `downloadtemplate` endpoint answers 400.
Two commits: the office guard first, then the charge sanitiser and the `dateFormat` check.
**Tests:**
- `LoanWorkbookPopulatorTest`: a charge named `Service Fee 5%-10% (VAT)`, and two offices named `Head Office`/`head office`, both populate without throwing and emit one defined-name set.
- New `BulkImportWorkbookPopulatorServiceImplTest`: a null and a blank `dateFormat` are rejected with a validation error on `dateFormat`.
All four new tests fail on current `develop` and pass with the fix, and `spotlessJavaCheck` is clean.
**Live check** (local `bootRun` on a stock PostgreSQL schema):
- before:
- `GET /v1/loans/downloadtemplate?dateFormat=dd MMMM yyyy` with that charge configured → 500.
- After also creating office `head office` → 500.
- Without `dateFormat` → 500 (same on `/v1/savingsaccounts/downloadtemplate`).
- after:
- 200 with a valid `.xls`.
- 200 with the office present.
- 400 `The parameter dateFormat is mandatory` on both endpoints.
- The savings and loan-repayment templates with `dateFormat` still return 200.
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [x] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [x] 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.
- [x] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [x] Follow our [coding conventions](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 (no API change: a request without `dateFormat` could never succeed; it now gets a 400 instead of a 500)
- [x] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
- [x] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
- [x] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2878: Fix loan bulk-import template download on charge and office defined names, require dateFormat [fineract]
Posted by "rymghosn (via GitHub)" <gi...@apache.org>.
rymghosn commented on PR #6543:
URL: https://github.com/apache/fineract/pull/6543#issuecomment-5989084403
Rebased onto the latest `develop` (4684c66380). The change itself is the same, no code changes beyond the rebase; the touched unit tests and spotless pass locally. @IOhacker could you please take another look when you have a moment? Thanks!
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2878: Fix loan bulk-import template download on charge and office defined names, require dateFormat [fineract]
Posted by "rymghosn (via GitHub)" <gi...@apache.org>.
rymghosn commented on PR #6543:
URL: https://github.com/apache/fineract/pull/6543#issuecomment-6141195108
@adamsaghy Thanks for the review. Both points were right, and both are addressed in f7c0633998. I also rebased onto the latest `develop`.
**1. `dateFormat` required for every template.** You're right. I went through every populator, including the sheet populators each one delegates to. Offices, staff, users and chart of accounts never format a date, and neither do journal entries or share accounts. Each one either ignores `dateFormat` or only passes it to `createDateConstraint`, which accepts null. `getTemplate` now requires `dateFormat` only when the entity type is outside that set. The check still runs before any permission check or data fetch. Every other template reaches `getFormat(dateFormat)`, `DateTimeFormatter` or `writeDate` and fails without it, so those keep the 400. `BulkImportWorkbookPopulatorServiceImplTest` covers both outcomes: the six templates above return 200 without `dateFormat`, and loans, clients, centers, savings and loan repayments are rejected.
**2. Orphan `Name` objects in the office loop.** Confirmed. The three `createName()` calls ran before the guard, so a skipped office left unnamed `Name`s in the workbook. Each `Name` is now created inside its guard, as in the charge loop. `officesCollidingOnNameEmitOneDefinedNameSetPerScope` now also asserts that no unnamed `Name` remains. It fails on the previous version of the loop and passes now.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2878: Fix loan bulk-import template download on charge and office defined names, require dateFormat [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #6543:
URL: https://github.com/apache/fineract/pull/6543
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org