Skip to content

FINERACT-2712: Fix duplicate defined name 500 in loan and savings opening templates - #6184

Open
oluexpert99 wants to merge 1 commit into
apache:developfrom
TECHSERVICES-LIMITED:bugfix/FINERACT-2712
Open

FINERACT-2712: Fix duplicate defined name 500 in loan and savings opening templates#6184
oluexpert99 wants to merge 1 commit into
apache:developfrom
TECHSERVICES-LIMITED:bugfix/FINERACT-2712

Conversation

@oluexpert99

Copy link
Copy Markdown
Contributor
  - GET /v1/loans/downloadtemplate and GET /v1/savingsaccounts/downloadtemplate returned
    HTTP 500 (IllegalArgumentException: The workbook already contains this name) on any
    tenant holding two products, or two loan charges, that resolve to the same Excel
    defined name. Both opening templates are unusable for the whole tenant and there is no
    API-side workaround.
  - LoanWorkbookPopulator.setNames and SavingsWorkbookPopulator.setNames create a block of
    per-product (and, for loans, per-charge) defined names keyed by the product or charge
    name, with no de-duplication. The second entry resolving to the same name calls
    Name.setNameName with a name the workbook already holds and POI rejects it. Excel
    defined names are case-insensitive, so names differing only in case or in surrounding
    whitespace collide too -- e.g. products "Target Savings" and "TARGET SAVINGS " both
    produce Interest_Compouding_TARGET_SAVINGS.
  - Guard each loop with a Set of names already emitted and skip a repeat, so every name
    produces its defined-name block exactly once. The key is the sanitised, upper-cased
    name, which is the exact string the workbook will hold, so two source values that
    differ only in stripped punctuation or case are treated as the collision they are.
  - Extract the sanitising step of AbstractWorkbookPopulator.setSanitized into a protected
    sanitizeName helper so the de-duplication key is computed by the same code that builds
    the name, rather than duplicating the regex. setSanitized keeps its behaviour exactly.
  - In both loops the name is now resolved before any createName call, so a skipped
    duplicate leaves no orphan Name object in the workbook.
  - Semantics of the skip: the FIRST entry with a given name keeps the defined name, so the
    dropdown for that name resolves to the first product's or charge's lookup row. The
    second entry's per-product defaults are not reachable through the dropdown -- which is
    inherent to keying defined names by a non-unique field, and is still strictly better
    than failing the download for every user of the tenant. The lookup sheet itself is
    unchanged and still lists every product, so the row indices behind the ranges stay
    correct.
  - Add unit tests for the loan charge loop, the loan product loop and the savings product
    loop. Each fails on the unfixed code with the exact production exception
    (CHARGE_NAME_LATE_FEE, FUND_MICRO_LOAN, Interest_Compouding_TARGET_SAVINGS). Note the
    loan product loop substitutes spaces before sanitising, so a trailing space yields a
    genuinely different name there and only a case-only clash collides; the charge and
    savings loops trim first, so both variants collide.

Description

Describe the changes made and why they were made. (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 our guidelines
  • 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 our 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
  • This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
  • If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

…ning templates

- GET /v1/loans/downloadtemplate and GET /v1/savingsaccounts/downloadtemplate returned
  HTTP 500 (IllegalArgumentException: The workbook already contains this name) on any
  tenant holding two products, or two loan charges, that resolve to the same Excel
  defined name. Both opening templates are unusable for the whole tenant and there is no
  API-side workaround.
- LoanWorkbookPopulator.setNames and SavingsWorkbookPopulator.setNames create a block of
  per-product (and, for loans, per-charge) defined names keyed by the product or charge
  name, with no de-duplication. The second entry resolving to the same name calls
  Name.setNameName with a name the workbook already holds and POI rejects it. Excel
  defined names are case-insensitive, so names differing only in case or in surrounding
  whitespace collide too -- e.g. products "Target Savings" and "TARGET SAVINGS " both
  produce Interest_Compouding_TARGET_SAVINGS.
- Guard each loop with a Set of names already emitted and skip a repeat, so every name
  produces its defined-name block exactly once. The key is the sanitised, upper-cased
  name, which is the exact string the workbook will hold, so two source values that
  differ only in stripped punctuation or case are treated as the collision they are.
- Extract the sanitising step of AbstractWorkbookPopulator.setSanitized into a protected
  sanitizeName helper so the de-duplication key is computed by the same code that builds
  the name, rather than duplicating the regex. setSanitized keeps its behaviour exactly.
- In both loops the name is now resolved before any createName call, so a skipped
  duplicate leaves no orphan Name object in the workbook.
- Semantics of the skip: the FIRST entry with a given name keeps the defined name, so the
  dropdown for that name resolves to the first product's or charge's lookup row. The
  second entry's per-product defaults are not reachable through the dropdown -- which is
  inherent to keying defined names by a non-unique field, and is still strictly better
  than failing the download for every user of the tenant. The lookup sheet itself is
  unchanged and still lists every product, so the row indices behind the ranges stay
  correct.
- Add unit tests for the loan charge loop, the loan product loop and the savings product
  loop. Each fails on the unfixed code with the exact production exception
  (CHARGE_NAME_LATE_FEE, FUND_MICRO_LOAN, Interest_Compouding_TARGET_SAVINGS). Note the
  loan product loop substitutes spaces before sanitising, so a trailing space yields a
  genuinely different name there and only a case-only clash collides; the charge and
  savings loops trim first, so both variants collide.

Signed-off-by: oluexpert99 <farooq@techservicehub.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant