From 6cc15e0fe99b20154a56d32e4bc90277fb0a8e9b Mon Sep 17 00:00:00 2001 From: oluexpert99 Date: Sun, 26 Jul 2026 21:51:36 +0100 Subject: [PATCH] FINERACT-2708: Fix duplicate defined name 500 in bulk-import transaction templates - Downloading the savings, recurring-deposit or fixed-deposit transaction template failed with HTTP 500 (IllegalArgumentException: The workbook already contains this name: Account___) on tenants where one client holds more than one account. The template is unusable for the whole tenant; there is no API workaround. - setNames builds one Account___ defined name per client, collecting clients by walking the account lookup table and starting a new run whenever the client name changes. That run detection uses a case-sensitive String.equals, but the table it walks was just sorted with SavingsAccountData.ClientNameComparator, which compares the names case-INSENSITIVELY (both upper-cased). Two clients whose names differ only in case therefore compare equal to the sort and may be interleaved, which splits one client's accounts into two runs; the second run re-adds the same client and the per-client loop then asks POI to create a defined name that already exists. - Guard the collection so a client name that has already been recorded is not added again, mirroring the guard LoanRepaymentWorkbookPopulator.setNames already has. Each client contributes exactly one defined name, whatever its account count; nothing changes for tenants that never hit the collision. - Apply the same guard to all three affected populators (SavingsTransactionsWorkbookPopulator, RecurringDepositTransactionWorkbookPopulator, FixedDepositTransactionWorkbookPopulator), which carry identical copies of the loop. - Add a unit test per populator: three accounts (client 18, client 19 whose name differs from 18's only in case, then client 18 again) reproduce the split-run ordering. Each test fails on the unfixed code with the exact production exception and asserts one defined name per client, still distinct when upper-cased since Excel defined names are case-insensitive. Signed-off-by: oluexpert99 --- ...edDepositTransactionWorkbookPopulator.java | 8 +- ...ngDepositTransactionWorkbookPopulator.java | 8 +- .../SavingsTransactionsWorkbookPopulator.java | 8 +- ...positTransactionWorkbookPopulatorTest.java | 83 +++++++++++++++++++ ...positTransactionWorkbookPopulatorTest.java | 83 +++++++++++++++++++ ...ingsTransactionsWorkbookPopulatorTest.java | 82 ++++++++++++++++++ 6 files changed, 266 insertions(+), 6 deletions(-) create mode 100644 fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/fixeddeposits/FixedDepositTransactionWorkbookPopulatorTest.java create mode 100644 fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/recurringdeposit/RecurringDepositTransactionWorkbookPopulatorTest.java create mode 100644 fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/savings/SavingsTransactionsWorkbookPopulatorTest.java diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/fixeddeposits/FixedDepositTransactionWorkbookPopulator.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/fixeddeposits/FixedDepositTransactionWorkbookPopulator.java index 6b9c5903c3f..7a55276899f 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/fixeddeposits/FixedDepositTransactionWorkbookPopulator.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/fixeddeposits/FixedDepositTransactionWorkbookPopulator.java @@ -193,8 +193,12 @@ private void setNames(Sheet worksheet) { startIndex = i + 2; clientName = savingsAccounts.get(i).getClientName(); clientId = savingsAccounts.get(i).getClientId(); - clientsWithActiveSavings.add(clientName); - clientIdsWithActiveSavings.add(clientId); + // Guard against a duplicate Account___ defined name when a client name recurs (e.g. a + // client with more than one account) — mirrors LoanRepaymentWorkbookPopulator.setNames. + if (!clientsWithActiveSavings.contains(clientName)) { + clientsWithActiveSavings.add(clientName); + clientIdsWithActiveSavings.add(clientId); + } } if (i == savingsAccounts.size() - 1) { endIndex = i + 2; diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/recurringdeposit/RecurringDepositTransactionWorkbookPopulator.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/recurringdeposit/RecurringDepositTransactionWorkbookPopulator.java index b932ae053d8..76545b6b622 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/recurringdeposit/RecurringDepositTransactionWorkbookPopulator.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/recurringdeposit/RecurringDepositTransactionWorkbookPopulator.java @@ -194,8 +194,12 @@ private void setNames(Sheet worksheet) { startIndex = i + 2; clientName = savingsAccounts.get(i).getClientName(); clientId = savingsAccounts.get(i).getClientId(); - clientsWithActiveSavings.add(clientName); - clientIdsWithActiveSavings.add(clientId); + // Guard against a duplicate Account___ defined name when a client name recurs (e.g. a + // client with more than one account) — mirrors LoanRepaymentWorkbookPopulator.setNames. + if (!clientsWithActiveSavings.contains(clientName)) { + clientsWithActiveSavings.add(clientName); + clientIdsWithActiveSavings.add(clientId); + } } if (i == savingsAccounts.size() - 1) { endIndex = i + 2; diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/savings/SavingsTransactionsWorkbookPopulator.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/savings/SavingsTransactionsWorkbookPopulator.java index e6efd40d4ff..9f824b2d004 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/savings/SavingsTransactionsWorkbookPopulator.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/populator/savings/SavingsTransactionsWorkbookPopulator.java @@ -193,8 +193,12 @@ private void setNames(Sheet worksheet) { startIndex = i + 2; clientName = savingsAccounts.get(i).getClientName(); clientId = savingsAccounts.get(i).getClientId(); - clientsWithActiveSavings.add(clientName); - clientIdsWithActiveSavings.add(clientId); + // Guard against a duplicate Account___ defined name when a client name recurs (e.g. a + // client with more than one account) — mirrors LoanRepaymentWorkbookPopulator.setNames. + if (!clientsWithActiveSavings.contains(clientName)) { + clientsWithActiveSavings.add(clientName); + clientIdsWithActiveSavings.add(clientId); + } } if (i == savingsAccounts.size() - 1) { endIndex = i + 2; diff --git a/fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/fixeddeposits/FixedDepositTransactionWorkbookPopulatorTest.java b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/fixeddeposits/FixedDepositTransactionWorkbookPopulatorTest.java new file mode 100644 index 00000000000..8b401bd2059 --- /dev/null +++ b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/fixeddeposits/FixedDepositTransactionWorkbookPopulatorTest.java @@ -0,0 +1,83 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.bulkimport.populator.fixeddeposits; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.time.LocalDate; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import org.apache.fineract.infrastructure.bulkimport.populator.ClientSheetPopulator; +import org.apache.fineract.infrastructure.bulkimport.populator.ExtrasSheetPopulator; +import org.apache.fineract.infrastructure.bulkimport.populator.OfficeSheetPopulator; +import org.apache.fineract.portfolio.savings.data.SavingsAccountApplicationTimelineData; +import org.apache.fineract.portfolio.savings.data.SavingsAccountData; +import org.apache.poi.hssf.usermodel.HSSFWorkbook; +import org.apache.poi.ss.usermodel.Name; +import org.apache.poi.ss.usermodel.Workbook; +import org.junit.jupiter.api.Test; + +class FixedDepositTransactionWorkbookPopulatorTest { + + private static final String DATE_FORMAT = "dd MMMM yyyy"; + private static final LocalDate ACTIVATED_ON = LocalDate.of(2024, 1, 1); + + private SavingsAccountData account(Long clientId, String clientName, String accountNo) { + SavingsAccountApplicationTimelineData timeline = mock(SavingsAccountApplicationTimelineData.class); + when(timeline.getActivatedOnDate()).thenReturn(ACTIVATED_ON); + SavingsAccountData account = mock(SavingsAccountData.class); + when(account.getClientId()).thenReturn(clientId); + when(account.getClientName()).thenReturn(clientName); + when(account.getAccountNo()).thenReturn(accountNo); + when(account.getSavingsProductName()).thenReturn("Regular Fixed Deposit"); + when(account.getTimeline()).thenReturn(timeline); + return account; + } + + // A client must contribute exactly one Account___ defined name however many accounts it has. The + // lookup table is sorted with SavingsAccountData.ClientNameComparator, which compares client names + // CASE-INSENSITIVELY, while setNames detects a new client with a CASE-SENSITIVE equals. A second client whose + // name differs only in case therefore sorts in between one client's two accounts and splits them into two runs; + // the second run re-emitted the same defined name and POI rejected it with + // "IllegalArgumentException: The workbook already contains this name: Account___", failing the whole + // template download with HTTP 500. + @Test + void clientWithSeveralAccountsEmitsOneDefinedName() throws Exception { + List accounts = new ArrayList<>(List.of(account(18L, "maryam yusuf", "000000018"), + account(19L, "MARYAM YUSUF", "000000019"), account(18L, "maryam yusuf", "000000020"))); + + try (Workbook workbook = new HSSFWorkbook()) { + FixedDepositTransactionWorkbookPopulator populator = new FixedDepositTransactionWorkbookPopulator( + new OfficeSheetPopulator(List.of()), new ClientSheetPopulator(List.of(), List.of()), + new ExtrasSheetPopulator(List.of(), List.of(), List.of()), accounts); + + assertDoesNotThrow(() -> populator.populate(workbook, DATE_FORMAT)); + + List names = workbook.getAllNames().stream().map(Name::getNameName).filter(name -> name.startsWith("Account_")) + .toList(); + assertEquals(2, names.size(), "expected one defined name per client, got: " + names); + assertEquals(2, names.stream().map(name -> name.toUpperCase(Locale.ROOT)).distinct().count(), + "defined names collide case-insensitively: " + names); + } + } +} diff --git a/fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/recurringdeposit/RecurringDepositTransactionWorkbookPopulatorTest.java b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/recurringdeposit/RecurringDepositTransactionWorkbookPopulatorTest.java new file mode 100644 index 00000000000..f2a0019c159 --- /dev/null +++ b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/recurringdeposit/RecurringDepositTransactionWorkbookPopulatorTest.java @@ -0,0 +1,83 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.bulkimport.populator.recurringdeposit; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.time.LocalDate; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import org.apache.fineract.infrastructure.bulkimport.populator.ClientSheetPopulator; +import org.apache.fineract.infrastructure.bulkimport.populator.ExtrasSheetPopulator; +import org.apache.fineract.infrastructure.bulkimport.populator.OfficeSheetPopulator; +import org.apache.fineract.portfolio.savings.data.SavingsAccountApplicationTimelineData; +import org.apache.fineract.portfolio.savings.data.SavingsAccountData; +import org.apache.poi.hssf.usermodel.HSSFWorkbook; +import org.apache.poi.ss.usermodel.Name; +import org.apache.poi.ss.usermodel.Workbook; +import org.junit.jupiter.api.Test; + +class RecurringDepositTransactionWorkbookPopulatorTest { + + private static final String DATE_FORMAT = "dd MMMM yyyy"; + private static final LocalDate ACTIVATED_ON = LocalDate.of(2024, 1, 1); + + private SavingsAccountData account(Long clientId, String clientName, String accountNo) { + SavingsAccountApplicationTimelineData timeline = mock(SavingsAccountApplicationTimelineData.class); + when(timeline.getActivatedOnDate()).thenReturn(ACTIVATED_ON); + SavingsAccountData account = mock(SavingsAccountData.class); + when(account.getClientId()).thenReturn(clientId); + when(account.getClientName()).thenReturn(clientName); + when(account.getAccountNo()).thenReturn(accountNo); + when(account.getSavingsProductName()).thenReturn("Regular Recurring Deposit"); + when(account.getTimeline()).thenReturn(timeline); + return account; + } + + // A client must contribute exactly one Account___ defined name however many accounts it has. The + // lookup table is sorted with SavingsAccountData.ClientNameComparator, which compares client names + // CASE-INSENSITIVELY, while setNames detects a new client with a CASE-SENSITIVE equals. A second client whose + // name differs only in case therefore sorts in between one client's two accounts and splits them into two runs; + // the second run re-emitted the same defined name and POI rejected it with + // "IllegalArgumentException: The workbook already contains this name: Account___", failing the whole + // template download with HTTP 500. + @Test + void clientWithSeveralAccountsEmitsOneDefinedName() throws Exception { + List accounts = new ArrayList<>(List.of(account(18L, "maryam yusuf", "000000018"), + account(19L, "MARYAM YUSUF", "000000019"), account(18L, "maryam yusuf", "000000020"))); + + try (Workbook workbook = new HSSFWorkbook()) { + RecurringDepositTransactionWorkbookPopulator populator = new RecurringDepositTransactionWorkbookPopulator( + new OfficeSheetPopulator(List.of()), new ClientSheetPopulator(List.of(), List.of()), + new ExtrasSheetPopulator(List.of(), List.of(), List.of()), accounts); + + assertDoesNotThrow(() -> populator.populate(workbook, DATE_FORMAT)); + + List names = workbook.getAllNames().stream().map(Name::getNameName).filter(name -> name.startsWith("Account_")) + .toList(); + assertEquals(2, names.size(), "expected one defined name per client, got: " + names); + assertEquals(2, names.stream().map(name -> name.toUpperCase(Locale.ROOT)).distinct().count(), + "defined names collide case-insensitively: " + names); + } + } +} diff --git a/fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/savings/SavingsTransactionsWorkbookPopulatorTest.java b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/savings/SavingsTransactionsWorkbookPopulatorTest.java new file mode 100644 index 00000000000..20bcfb4f4b6 --- /dev/null +++ b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/bulkimport/populator/savings/SavingsTransactionsWorkbookPopulatorTest.java @@ -0,0 +1,82 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.bulkimport.populator.savings; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.time.LocalDate; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import org.apache.fineract.infrastructure.bulkimport.populator.ClientSheetPopulator; +import org.apache.fineract.infrastructure.bulkimport.populator.ExtrasSheetPopulator; +import org.apache.fineract.infrastructure.bulkimport.populator.OfficeSheetPopulator; +import org.apache.fineract.portfolio.savings.data.SavingsAccountApplicationTimelineData; +import org.apache.fineract.portfolio.savings.data.SavingsAccountData; +import org.apache.poi.hssf.usermodel.HSSFWorkbook; +import org.apache.poi.ss.usermodel.Name; +import org.apache.poi.ss.usermodel.Workbook; +import org.junit.jupiter.api.Test; + +class SavingsTransactionsWorkbookPopulatorTest { + + private static final String DATE_FORMAT = "dd MMMM yyyy"; + private static final LocalDate ACTIVATED_ON = LocalDate.of(2024, 1, 1); + + private SavingsAccountData account(Long clientId, String clientName, String accountNo) { + SavingsAccountApplicationTimelineData timeline = mock(SavingsAccountApplicationTimelineData.class); + when(timeline.getActivatedOnDate()).thenReturn(ACTIVATED_ON); + SavingsAccountData account = mock(SavingsAccountData.class); + when(account.getClientId()).thenReturn(clientId); + when(account.getClientName()).thenReturn(clientName); + when(account.getAccountNo()).thenReturn(accountNo); + when(account.getSavingsProductName()).thenReturn("Regular Savings"); + when(account.getTimeline()).thenReturn(timeline); + return account; + } + + // A client must contribute exactly one Account___ defined name however many accounts it has. The + // lookup table is sorted with SavingsAccountData.ClientNameComparator, which compares client names + // CASE-INSENSITIVELY, while setNames detects a new client with a CASE-SENSITIVE equals. A second client whose + // name differs only in case therefore sorts in between one client's two accounts and splits them into two runs; + // the second run re-emitted the same defined name and POI rejected it with + // "IllegalArgumentException: The workbook already contains this name: Account___", failing the whole + // template download with HTTP 500. + @Test + void clientWithSeveralAccountsEmitsOneDefinedName() throws Exception { + List accounts = new ArrayList<>(List.of(account(18L, "maryam yusuf", "000000018"), + account(19L, "MARYAM YUSUF", "000000019"), account(18L, "maryam yusuf", "000000020"))); + + try (Workbook workbook = new HSSFWorkbook()) { + SavingsTransactionsWorkbookPopulator populator = new SavingsTransactionsWorkbookPopulator(new OfficeSheetPopulator(List.of()), + new ClientSheetPopulator(List.of(), List.of()), new ExtrasSheetPopulator(List.of(), List.of(), List.of()), accounts); + + assertDoesNotThrow(() -> populator.populate(workbook, DATE_FORMAT)); + + List names = workbook.getAllNames().stream().map(Name::getNameName).filter(name -> name.startsWith("Account_")) + .toList(); + assertEquals(2, names.size(), "expected one defined name per client, got: " + names); + assertEquals(2, names.stream().map(name -> name.toUpperCase(Locale.ROOT)).distinct().count(), + "defined names collide case-insensitively: " + names); + } + } +}