From 8d2ccc4fc84b4a02065030169eddc564ae2a4fd8 Mon Sep 17 00:00:00 2001 From: Azizbek Khushvakov Date: Fri, 6 Dec 2024 16:37:09 +0500 Subject: [PATCH] Optimize transaction creation, Fix unit tests --- .../financedata/FinanceDataService.java | 69 +++++++++++-------- .../folio/rest/impl/FinanceDataApiTest.java | 3 +- .../financedata/FinanceDataServiceTest.java | 24 +++---- .../java/org/folio/util/CopilotGenerated.java | 25 +++++++ 4 files changed, 78 insertions(+), 43 deletions(-) create mode 100644 src/test/java/org/folio/util/CopilotGenerated.java diff --git a/src/main/java/org/folio/services/financedata/FinanceDataService.java b/src/main/java/org/folio/services/financedata/FinanceDataService.java index 37081f5c..3e72bb5e 100644 --- a/src/main/java/org/folio/services/financedata/FinanceDataService.java +++ b/src/main/java/org/folio/services/financedata/FinanceDataService.java @@ -17,7 +17,6 @@ import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.folio.okapi.common.GenericCompositeFuture; import org.folio.rest.core.RestClient; import org.folio.rest.core.models.RequestContext; import org.folio.rest.core.models.RequestEntry; @@ -78,24 +77,25 @@ public Future putFinanceData(FyFinanceDataCollection financeDataCollection return succeededFuture(); } - validateFinanceDataCollection(financeDataCollection); + validateFinanceDataCollection(financeDataCollection, getFiscalYearId(financeDataCollection)); + return processAllocationTransaction(financeDataCollection, requestContext) .compose(v -> updateFinanceData(financeDataCollection, requestContext)) .onSuccess(asyncResult -> processLogs(financeDataCollection, requestContext, COMPLETED)) .onFailure(asyncResult -> processLogs(financeDataCollection, requestContext, ERROR)); } - private void validateFinanceDataCollection(FyFinanceDataCollection financeDataCollection) { + private void validateFinanceDataCollection(FyFinanceDataCollection financeDataCollection, String fiscalYearId) { for (int i = 0; i < financeDataCollection.getFyFinanceData().size(); i++) { var financeData = financeDataCollection.getFyFinanceData().get(i); - validateFinanceDataFields(financeData, i); + validateFinanceDataFields(financeData, i, fiscalYearId); + var allocationChange = BigDecimal.valueOf(financeData.getBudgetAllocationChange()); var initialAllocation = BigDecimal.valueOf(financeData.getBudgetInitialAllocation()); if (allocationChange.doubleValue() < 0 && allocationChange.abs().doubleValue() > initialAllocation.doubleValue()) { - var params = List.of(new Parameter().withKey(String.format("financeData[%s].budgetAllocationChange", i)) - .withValue(String.valueOf(financeData.getBudgetAllocationChange()))); - var error = new Error().withMessage("Allocation change cannot be greater than initial allocation").withParameters(params); + var error = createError("Allocation change cannot be greater than initial allocation", + String.format("financeData[%s].budgetAllocationChange", i), String.valueOf(financeData.getBudgetAllocationChange())); throw new HttpException(422, new Errors().withErrors(List.of(error))); } } @@ -103,31 +103,34 @@ private void validateFinanceDataCollection(FyFinanceDataCollection financeDataCo private Future processAllocationTransaction(FyFinanceDataCollection fyFinanceDataCollection, RequestContext requestContext) { - var transactionsFuture = fyFinanceDataCollection.getFyFinanceData().stream() - .map(financeData -> createAllocationTransaction(financeData, requestContext)) - .toList(); + return fiscalYearService.getFiscalYearById(getFiscalYearId(fyFinanceDataCollection), requestContext) + .map(fiscalYear -> createAllocationTransactions(fyFinanceDataCollection, fiscalYear.getCurrency())) + .compose(transactions -> createBatchTransaction(transactions, requestContext)); + } - return GenericCompositeFuture.join(transactionsFuture) - .compose(compositeFuture -> { - List transactions = compositeFuture.list(); - return createBatchTransaction(transactions, requestContext); - }); + private String getFiscalYearId(FyFinanceDataCollection fyFinanceDataCollection) { + return fyFinanceDataCollection.getFyFinanceData().get(0).getFiscalYearId(); } - private Future createAllocationTransaction(FyFinanceData financeData, RequestContext requestContext) { + private List createAllocationTransactions(FyFinanceDataCollection financeDataCollection, String currency) { + return financeDataCollection.getFyFinanceData().stream() + .map(financeData -> createAllocationTransaction(financeData, currency)) + .toList(); + } + + private Transaction createAllocationTransaction(FyFinanceData financeData, String currency) { var allocation = calculateAllocation(financeData); - var transaction = new Transaction() + log.info("Creating allocation transaction for fund '{}' and budget '{}' with allocation '{}'", + financeData.getFundId(), financeData.getBudgetId(), allocation); + + return new Transaction() .withTransactionType(Transaction.TransactionType.ALLOCATION) .withId(UUID.randomUUID().toString()) .withAmount(allocation) .withFiscalYearId(financeData.getFiscalYearId()) .withToFundId(financeData.getFundId()) - .withSource(Transaction.Source.USER); - log.info("Creating allocation transaction for fund '{}' and budget '{}' with allocation '{}'", - financeData.getFundId(), financeData.getBudgetId(), allocation); - - return fiscalYearService.getFiscalYearById(financeData.getFiscalYearId(), requestContext) - .map(fiscalYear -> transaction.withCurrency(fiscalYear.getCurrency())); + .withSource(Transaction.Source.USER) + .withCurrency(currency); } private Double calculateAllocation(FyFinanceData financeData) { @@ -151,7 +154,7 @@ private void processLogs(FyFinanceDataCollection financeDataCollection, RequestContext requestContext, FundUpdateLog.Status status) { var jobDetails = new JobDetails().withAdditionalProperty("fyFinanceData", financeDataCollection.getFyFinanceData()); var fundUpdateLog = new FundUpdateLog().withId(UUID.randomUUID().toString()) - .withJobName("Update finance data") + .withJobName("Update finance data") // TODO: Update job name generation .withStatus(status) .withRecordsCount(financeDataCollection.getTotalRecords()) .withJobDetails(jobDetails) @@ -159,9 +162,16 @@ private void processLogs(FyFinanceDataCollection financeDataCollection, fundUpdateLogService.createFundUpdateLog(fundUpdateLog, requestContext); } - private void validateFinanceDataFields(FyFinanceData financeData, int i) { + private void validateFinanceDataFields(FyFinanceData financeData, int i, String fiscalYearId) { var errors = new Errors().withErrors(new ArrayList<>()); + if (!financeData.getFiscalYearId().equals(fiscalYearId)) { + errors.getErrors().add(createError( + String.format("Fiscal year ID must be the same as other fiscal year ID(s) '[%s]' in the request", fiscalYearId), + String.format("financeData[%s].fiscalYearId", i), financeData.getFiscalYearId()) + ); + } + validateField(errors, String.format("financeData[%s].fundCode", i), financeData.getFundCode(), "Fund code is required"); validateField(errors, String.format("financeData[%s].fundName", i), financeData.getFundName(), "Fund name is required"); validateField(errors, String.format("financeData[%s].fundDescription", i), financeData.getFundDescription(), "Fund description is required"); @@ -183,9 +193,14 @@ private void validateFinanceDataFields(FyFinanceData financeData, int i) { private void validateField(Errors errors, String fieldName, Object fieldValue, String errorMessage) { if (fieldValue == null) { - var params = List.of(new Parameter().withKey(fieldName).withValue("null")); - errors.getErrors().add(new Error().withMessage(errorMessage).withParameters(params)); + errors.getErrors().add(createError(errorMessage, fieldName, "null")); } } + + private Error createError(String message, String key, String value) { + log.warn("Validation error: {}", message); + var param = new Parameter().withKey(key).withValue(value); + return new Error().withMessage(message).withParameters(List.of(param)); + } } diff --git a/src/test/java/org/folio/rest/impl/FinanceDataApiTest.java b/src/test/java/org/folio/rest/impl/FinanceDataApiTest.java index 28b4821e..533f90a6 100644 --- a/src/test/java/org/folio/rest/impl/FinanceDataApiTest.java +++ b/src/test/java/org/folio/rest/impl/FinanceDataApiTest.java @@ -48,6 +48,7 @@ import org.folio.rest.jaxrs.model.FyFinanceDataCollection; import org.folio.services.financedata.FinanceDataService; import org.folio.services.protection.AcqUnitsService; +import org.folio.util.CopilotGenerated; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; @@ -56,7 +57,7 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; - +@CopilotGenerated(partiallyGenerated = true) public class FinanceDataApiTest { @Autowired diff --git a/src/test/java/org/folio/services/financedata/FinanceDataServiceTest.java b/src/test/java/org/folio/services/financedata/FinanceDataServiceTest.java index 0418cb05..2e7cf039 100644 --- a/src/test/java/org/folio/services/financedata/FinanceDataServiceTest.java +++ b/src/test/java/org/folio/services/financedata/FinanceDataServiceTest.java @@ -22,7 +22,6 @@ import java.util.UUID; import io.vertx.core.Context; -import io.vertx.core.Future; import io.vertx.core.Vertx; import io.vertx.junit5.VertxExtension; import io.vertx.junit5.VertxTestContext; @@ -186,24 +185,19 @@ void negative_testPutFinanceData_FailureInProcessAllocationTransaction(VertxTest } @Test - void testCreateAllocationTransactionUsingReflection(VertxTestContext vertxTestContext) throws Exception { - FyFinanceData data = createValidFyFinanceData(); - FiscalYear fiscalYear = new FiscalYear().withCurrency("USD"); - - when(fiscalYearService.getFiscalYearById(any(), any())).thenReturn(succeededFuture(fiscalYear)); + void testCreateAllocationTransactionUsingReflection() throws Exception { + var data = createValidFyFinanceData(); + var fiscalYear = new FiscalYear().withCurrency("USD"); // Use reflection to access the private method - var method = FinanceDataService.class.getDeclaredMethod("createAllocationTransaction", FyFinanceData.class, RequestContext.class); + var method = FinanceDataService.class.getDeclaredMethod("createAllocationTransaction", FyFinanceData.class, String.class); method.setAccessible(true); - Future future = (Future) method.invoke(financeDataService, data, requestContextMock); - future.onComplete(vertxTestContext.succeeding(transaction -> { - assertEquals(Transaction.TransactionType.ALLOCATION, transaction.getTransactionType()); - assertEquals(data.getFundId(), transaction.getToFundId()); - assertEquals(fiscalYear.getCurrency(), transaction.getCurrency()); - assertEquals(150.0, transaction.getAmount()); // Assuming initial allocation is 100 and change is 50 - vertxTestContext.completeNow(); - })); + Transaction transaction = (Transaction) method.invoke(financeDataService, data, fiscalYear.getCurrency()); + assertEquals(Transaction.TransactionType.ALLOCATION, transaction.getTransactionType()); + assertEquals(data.getFundId(), transaction.getToFundId()); + assertEquals(fiscalYear.getCurrency(), transaction.getCurrency()); + assertEquals(150.0, transaction.getAmount()); // Assuming initial allocation is 100 and change is 50 } @Test diff --git a/src/test/java/org/folio/util/CopilotGenerated.java b/src/test/java/org/folio/util/CopilotGenerated.java new file mode 100644 index 00000000..6a4c6aca --- /dev/null +++ b/src/test/java/org/folio/util/CopilotGenerated.java @@ -0,0 +1,25 @@ +package org.folio.util; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import org.springframework.core.annotation.AliasFor; + +/** + * Annotation to indicate that test(s) in the class are generated by GitHub Copilot. + *

+ * Set value or partiallyGenerated attribute to true + * if the generated test(s) were significantly modified/altered by the developer. + */ +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.SOURCE) +public @interface CopilotGenerated { + + @AliasFor("partiallyGenerated") + boolean value() default false; + + @AliasFor("value") + boolean partiallyGenerated() default false; +}