You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "kulminsky (via GitHub)" <gi...@apache.org> on 2024/11/05 11:29:41 UTC
[PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
kulminsky opened a new pull request, #4144:
URL: https://github.com/apache/fineract/pull/4144
Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag
1. New Advance Accounting rule to be added to map charge-off reason code with accounting mapping
2. User should be able to map reasons to any expense GL (General Ledger) account
- dropdown
- Can be configure multiple rules, but no duplication (cannot configured twice the same reason code)
3. When advance accounting is mapped- the default mapping for fraud flag will be overridden
## Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [x] Write the commit message as per https://github.com/apache/fineract/#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 for verifying the changes made.
- [x] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [x] 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
- [x] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "kulminsky (via GitHub)" <gi...@apache.org>.
kulminsky commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851992983
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java:
##########
@@ -1964,6 +1968,49 @@ private void validateChargeToIncomeAccountMappings(final DataValidatorBuilder ba
}
}
+ private void validateChargeOffToExpenseMappings(final DataValidatorBuilder baseDataValidator, final JsonElement element) {
Review Comment:
Done
##########
fineract-provider/src/main/resources/db/changelog/tenant/parts/0153_add_charge_off_reason_id_to_acc_product_mapping.xml:
##########
@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.3.xsd">
+
+ <changeSet id="1" author="fineract">
+ <addColumn tableName="acc_product_mapping">
+ <column name="charge_off_reason_id" type="BIGINT" defaultValueNumeric="NULL">
+ <constraints nullable="true"/>
+ </column>
+ </addColumn>
+ </changeSet>
+
+</databaseChangeLog>
Review Comment:
Done
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #4144:
URL: https://github.com/apache/fineract/pull/4144
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "kulminsky (via GitHub)" <gi...@apache.org>.
kulminsky commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851995394
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/LoanProductChargeOffReasonMappingsTest.java:
##########
@@ -0,0 +1,256 @@
+package org.apache.fineract.integrationtests;
+
+import static org.apache.fineract.integrationtests.common.funds.FundsResourceHandler.createFund;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.fineract.client.models.AllowAttributeOverrides;
+import org.apache.fineract.client.models.ChargeData;
+import org.apache.fineract.client.models.ChargeToGLAccountMapper;
+import org.apache.fineract.client.models.GetChargeOffReasonsToExpenseMappings;
+import org.apache.fineract.client.models.GetLoanFeeToIncomeAccountMappings;
+import org.apache.fineract.client.models.GetLoanPaymentChannelToFundSourceMappings;
+import org.apache.fineract.client.models.PostLoanProductsRequest;
+import org.apache.fineract.client.models.PutLoanProductsProductIdRequest;
+import org.apache.fineract.client.util.CallFailedRuntimeException;
+import org.apache.fineract.integrationtests.common.BusinessStepHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.apache.fineract.integrationtests.common.loans.LoanTestLifecycleExtension;
+import org.apache.fineract.integrationtests.common.loans.LoanTransactionHelper;
+import org.apache.fineract.integrationtests.common.products.DelinquencyBucketsHelper;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(LoanTestLifecycleExtension.class)
+public class LoanProductChargeOffReasonMappingsTest extends BaseLoanIntegrationTest {
+
+ private static ResponseSpecification responseSpec;
+ private static RequestSpecification requestSpec;
+ private static LoanTransactionHelper loanTransactionHelper;
+
+ @BeforeAll
+ public static void setup() {
+ Utils.initializeRESTAssured();
+ requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ requestSpec.header("Fineract-Platform-TenantId", "default");
+ responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ loanTransactionHelper = new LoanTransactionHelper(requestSpec, responseSpec);
+ BusinessStepHelper businessStepHelper = new BusinessStepHelper();
+ // setup COB Business Steps to prevent test failing due other integration test configurations
+ businessStepHelper.updateSteps("LOAN_CLOSE_OF_BUSINESS", "APPLY_CHARGE_TO_OVERDUE_LOANS", "LOAN_DELINQUENCY_CLASSIFICATION",
+ "CHECK_LOAN_REPAYMENT_DUE", "CHECK_LOAN_REPAYMENT_OVERDUE", "UPDATE_LOAN_ARREARS_AGING", "ADD_PERIODIC_ACCRUAL_ENTRIES",
+ "EXTERNAL_ASSET_OWNER_TRANSFER", "ACCRUAL_ACTIVITY_POSTING");
+ }
+
+ // TODO: Should be update
Review Comment:
Updated
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "kulminsky (via GitHub)" <gi...@apache.org>.
kulminsky commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851994572
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -455,4 +560,39 @@ public void deleteProductToGLAccountMapping(final Long loanProductId, final Port
this.accountMappingRepository.deleteAllInBatch(productToGLAccountMappings);
}
}
+
+ public void validateChargeOffMappingsInDatabase(final List<JsonObject> mappings) {
+ final List<ApiParameterError> validationErrors = new ArrayList<>();
+
+ for (JsonObject jsonObject : mappings) {
+ final Long expenseGlAccountId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue(), jsonObject);
+ final Long chargeOffReasonCodeValueId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue(), jsonObject);
+
+ // Validation: chargeOffReasonCodeValueId must exist in the database
+ CodeValue codeValue = this.codeValueRepositoryWrapper.findOneByCodeNameAndIdWithNotFoundDetection("ChargeOffReasons",
+ chargeOffReasonCodeValueId);
+ if (codeValue == null) {
+ validationErrors.add(ApiParameterError.parameterError("validation.msg.chargeoffreason.invalid",
+ "Charge-off reason with ID " + chargeOffReasonCodeValueId + " does not exist",
+ LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue()));
+ }
+
+ // Validation: expenseGLAccountId must exist as a valid Expense GL account
+ final GLAccount glAccount = getAccountByIdAndType(LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue(),
Review Comment:
Changed to reposiroty
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -455,4 +560,39 @@ public void deleteProductToGLAccountMapping(final Long loanProductId, final Port
this.accountMappingRepository.deleteAllInBatch(productToGLAccountMappings);
}
}
+
+ public void validateChargeOffMappingsInDatabase(final List<JsonObject> mappings) {
+ final List<ApiParameterError> validationErrors = new ArrayList<>();
+
+ for (JsonObject jsonObject : mappings) {
+ final Long expenseGlAccountId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue(), jsonObject);
+ final Long chargeOffReasonCodeValueId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue(), jsonObject);
+
+ // Validation: chargeOffReasonCodeValueId must exist in the database
+ CodeValue codeValue = this.codeValueRepositoryWrapper.findOneByCodeNameAndIdWithNotFoundDetection("ChargeOffReasons",
+ chargeOffReasonCodeValueId);
+ if (codeValue == null) {
+ validationErrors.add(ApiParameterError.parameterError("validation.msg.chargeoffreason.invalid",
+ "Charge-off reason with ID " + chargeOffReasonCodeValueId + " does not exist",
+ LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue()));
+ }
+
+ // Validation: expenseGLAccountId must exist as a valid Expense GL account
+ final GLAccount glAccount = getAccountByIdAndType(LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue(),
Review Comment:
Changed to repository
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851717064
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -356,6 +382,66 @@ public void updatePaymentChannelToFundSourceMappings(final JsonCommand command,
}
}
+ public void updateChargeOffReasonToGLAccountMappings(final JsonCommand command, final JsonElement element, final Long productId,
+ final Map<String, Object> changes, final PortfolioProductType portfolioProductType) {
+
+ final List<ProductToGLAccountMapping> existingChargeOffReasonToGLAccountMappings = this.accountMappingRepository
+ .findAllChargesOffReasonsMappings(productId, portfolioProductType.getValue());
+ final JsonArray chargeOffReasonToGLAccountMappingArray = this.fromApiJsonHelper
+ .extractJsonArrayNamed(LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue(), element);
+
+ final Map<Long, Long> inputChargeOffReasonToGLAccountMap = new HashMap<>();
+
+ final Set<Long> existingChargeOffReasons = new HashSet<>();
+ if (chargeOffReasonToGLAccountMappingArray != null) {
+ if (changes != null) {
+ changes.put(LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue(),
+ command.jsonFragment(LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue()));
+ }
+
+ for (int i = 0; i < chargeOffReasonToGLAccountMappingArray.size(); i++) {
+ final JsonObject jsonObject = chargeOffReasonToGLAccountMappingArray.get(i).getAsJsonObject();
+ final Long expenseGlAccountId = jsonObject.get(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue()).getAsLong();
+ final Long chargeOffReasonCodeValueId = jsonObject
+ .get(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue()).getAsLong();
+ inputChargeOffReasonToGLAccountMap.put(chargeOffReasonCodeValueId, expenseGlAccountId);
+ }
+
+ // If input map is empty, delete all existing mappings
+ if (inputChargeOffReasonToGLAccountMap.isEmpty()) {
+ this.accountMappingRepository.deleteAllInBatch(existingChargeOffReasonToGLAccountMappings);
+ } else {
+ for (final ProductToGLAccountMapping existingChargeOffReasonToGLAccountMapping : existingChargeOffReasonToGLAccountMappings) {
+ final Long currentChargeOffReasonId = existingChargeOffReasonToGLAccountMapping.getChargeOffReasonId();
+ if (currentChargeOffReasonId != null) {
+ existingChargeOffReasons.add(currentChargeOffReasonId);
+ // update existing mappings (if required)
+ if (inputChargeOffReasonToGLAccountMap.containsKey(currentChargeOffReasonId)) {
+ final Long newGLAccountId = inputChargeOffReasonToGLAccountMap.get(currentChargeOffReasonId);
+ if (!newGLAccountId.equals(existingChargeOffReasonToGLAccountMapping.getGlAccount().getId())) {
+ final GLAccount glAccount = getAccountById(LoanProductAccountingParams.FUND_SOURCE.getValue(),
Review Comment:
This look incorrect. Why FUND_SOURCE?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1832862949
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/productaccountmapping/service/ProductToGLAccountMappingWritePlatformServiceImpl.java:
##########
@@ -208,6 +208,7 @@ public void createLoanProductToGLAccountMapping(final Long loanProductId, final
// advanced accounting mappings
this.loanProductToGLAccountMappingHelper.savePaymentChannelToFundSourceMappings(command, element, loanProductId, null);
this.loanProductToGLAccountMappingHelper.saveChargesToIncomeAccountMappings(command, element, loanProductId, null);
+ this.loanProductToGLAccountMappingHelper.saveChargeOffReasonToExpenseAccountMappings(command, element, loanProductId, null);
Review Comment:
Dont forget to add to CASH_BASED accounting as well
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1831088825
##########
fineract-loan/src/main/java/org/apache/fineract/accounting/productaccountmapping/service/LoanProductToGLAccountMappingHelper.java:
##########
@@ -388,4 +388,15 @@ private GLAccountType getGLAccountType(final JsonElement element, final String p
return gLAccountType;
}
+ public void mapChargeOffReasonToExpenseAccount(JsonElement element, Long loanProductId, String expenseAccountParam) {
+ int productType = PortfolioProductType.LOAN.getValue();
+ int financialAccountType = GLAccountType.EXPENSE.getValue();
+
+ boolean isMapped = accountMappingRepository.existsByProductIdAndProductTypeAndFinancialAccountType(loanProductId, productType,
Review Comment:
What is this logic doing? Only
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851730951
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/ClientLoanIntegrationTest.java:
##########
@@ -5490,7 +5490,7 @@ public void chargeAdjustmentAccountingValidation() {
.withDaysInMonth("30").withDaysInYear("365").withMoratorium("0", "0")
.withFeeToIncomeAccountMapping(feeCharge.getResourceId(), uniqueIncomeAccountForFee.getResourceId())
.withPenaltyToIncomeAccountMapping(penaltyCharge.getResourceId(), uniqueIncomeAccountForPenalty.getResourceId())
- .withFeeAndPenaltyAssetAccount(assetFeeAndPenaltyAccount).build(null);
+ .withFeeAndPenaltyAssetAccount(assetFeeAndPenaltyAccount).withChargeOffReasonsToExpenseMappings(1L, 12L).build(null);
Review Comment:
This seems a little random. Would you mind to remove?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851728976
##########
fineract-provider/src/main/resources/db/changelog/tenant/parts/0153_add_charge_off_reason_id_to_acc_product_mapping.xml:
##########
@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.3.xsd">
+
+ <changeSet id="1" author="fineract">
+ <addColumn tableName="acc_product_mapping">
+ <column name="charge_off_reason_id" type="BIGINT" defaultValueNumeric="NULL">
Review Comment:
You can make a FK to the code value table since it is either null or a valid id of the code value table
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "kulminsky (via GitHub)" <gi...@apache.org>.
kulminsky commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1852010019
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -455,4 +552,37 @@ public void deleteProductToGLAccountMapping(final Long loanProductId, final Port
this.accountMappingRepository.deleteAllInBatch(productToGLAccountMappings);
}
}
+
+ public void validateChargeOffMappingsInDatabase(final List<JsonObject> mappings) {
+ final List<ApiParameterError> validationErrors = new ArrayList<>();
+
+ for (JsonObject jsonObject : mappings) {
+ final Long expenseGlAccountId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue(), jsonObject);
+ final Long chargeOffReasonCodeValueId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue(), jsonObject);
+
+ // Validation: chargeOffReasonCodeValueId must exist in the database
+ CodeValue codeValue = this.codeValueRepository.findByCodeNameAndId("ChargeOffReasons", chargeOffReasonCodeValueId);
+ if (codeValue == null) {
+ validationErrors.add(ApiParameterError.parameterError("validation.msg.chargeoffreason.invalid",
+ "Charge-off reason with ID " + chargeOffReasonCodeValueId + " does not exist",
+ LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue()));
+ }
+
+ // Validation: expenseGLAccountId must exist as a valid Expense GL account
+ final Optional<GLAccount> glAccount = accountRepository.findById(expenseGlAccountId);
Review Comment:
Will update
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1841812938
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/domain/ProductToGLAccountMappingRepository.java:
##########
@@ -61,4 +61,8 @@ List<ProductToGLAccountMapping> findAllPenaltyToIncomeAccountMappings(@Param("pr
@Param("productType") int productType);
List<ProductToGLAccountMapping> findByProductIdAndProductType(Long productId, int productType);
+
+ @Query("select mapping from ProductToGLAccountMapping mapping where mapping.productId =:productId and mapping.productType =:productType and mapping.paymentType is not NULL and mapping.chargeOffReasonId is not NULL")
+ List<ProductToGLAccountMapping> findAllChargesOffReasonsMappings(@Param("productId") Long productId,
Review Comment:
I dont think we need `mapping.paymentType is not NULL`
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1846637622
##########
fineract-provider/src/main/resources/db/changelog/tenant/parts/0153_add_charge_off_reason_id_to_acc_product_mapping.xml:
##########
@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.3.xsd">
+
+ <changeSet id="1" author="fineract">
+ <addColumn tableName="acc_product_mapping">
+ <column name="charge_off_reason_id" type="BIGINT" defaultValueNumeric="NULL">
+ <constraints nullable="true"/>
+ </column>
+ </addColumn>
+ </changeSet>
+
+</databaseChangeLog>
Review Comment:
Can you please attach some test cases as well? Integration test or E2E test.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1846636196
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java:
##########
@@ -1964,6 +1968,49 @@ private void validateChargeToIncomeAccountMappings(final DataValidatorBuilder ba
}
}
+ private void validateChargeOffToExpenseMappings(final DataValidatorBuilder baseDataValidator, final JsonElement element) {
Review Comment:
Can we please validate whether existing charge off code value id was provided (existing charge off code value in DB) and existing expense type gl account id was provided?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851731912
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/LoanProductChargeOffReasonMappingsTest.java:
##########
@@ -0,0 +1,256 @@
+package org.apache.fineract.integrationtests;
+
+import static org.apache.fineract.integrationtests.common.funds.FundsResourceHandler.createFund;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.fineract.client.models.AllowAttributeOverrides;
+import org.apache.fineract.client.models.ChargeData;
+import org.apache.fineract.client.models.ChargeToGLAccountMapper;
+import org.apache.fineract.client.models.GetChargeOffReasonsToExpenseMappings;
+import org.apache.fineract.client.models.GetLoanFeeToIncomeAccountMappings;
+import org.apache.fineract.client.models.GetLoanPaymentChannelToFundSourceMappings;
+import org.apache.fineract.client.models.PostLoanProductsRequest;
+import org.apache.fineract.client.models.PutLoanProductsProductIdRequest;
+import org.apache.fineract.client.util.CallFailedRuntimeException;
+import org.apache.fineract.integrationtests.common.BusinessStepHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.apache.fineract.integrationtests.common.loans.LoanTestLifecycleExtension;
+import org.apache.fineract.integrationtests.common.loans.LoanTransactionHelper;
+import org.apache.fineract.integrationtests.common.products.DelinquencyBucketsHelper;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(LoanTestLifecycleExtension.class)
+public class LoanProductChargeOffReasonMappingsTest extends BaseLoanIntegrationTest {
+
+ private static ResponseSpecification responseSpec;
+ private static RequestSpecification requestSpec;
+ private static LoanTransactionHelper loanTransactionHelper;
+
+ @BeforeAll
+ public static void setup() {
+ Utils.initializeRESTAssured();
+ requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ requestSpec.header("Fineract-Platform-TenantId", "default");
+ responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ loanTransactionHelper = new LoanTransactionHelper(requestSpec, responseSpec);
+ BusinessStepHelper businessStepHelper = new BusinessStepHelper();
+ // setup COB Business Steps to prevent test failing due other integration test configurations
+ businessStepHelper.updateSteps("LOAN_CLOSE_OF_BUSINESS", "APPLY_CHARGE_TO_OVERDUE_LOANS", "LOAN_DELINQUENCY_CLASSIFICATION",
+ "CHECK_LOAN_REPAYMENT_DUE", "CHECK_LOAN_REPAYMENT_OVERDUE", "UPDATE_LOAN_ARREARS_AGING", "ADD_PERIODIC_ACCRUAL_ENTRIES",
+ "EXTERNAL_ASSET_OWNER_TRANSFER", "ACCRUAL_ACTIVITY_POSTING");
+ }
+
+ // TODO: Should be update
Review Comment:
Please either uncomment or remove
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851726710
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -455,4 +560,39 @@ public void deleteProductToGLAccountMapping(final Long loanProductId, final Port
this.accountMappingRepository.deleteAllInBatch(productToGLAccountMappings);
}
}
+
+ public void validateChargeOffMappingsInDatabase(final List<JsonObject> mappings) {
+ final List<ApiParameterError> validationErrors = new ArrayList<>();
+
+ for (JsonObject jsonObject : mappings) {
+ final Long expenseGlAccountId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue(), jsonObject);
+ final Long chargeOffReasonCodeValueId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue(), jsonObject);
+
+ // Validation: chargeOffReasonCodeValueId must exist in the database
+ CodeValue codeValue = this.codeValueRepositoryWrapper.findOneByCodeNameAndIdWithNotFoundDetection("ChargeOffReasons",
+ chargeOffReasonCodeValueId);
+ if (codeValue == null) {
+ validationErrors.add(ApiParameterError.parameterError("validation.msg.chargeoffreason.invalid",
+ "Charge-off reason with ID " + chargeOffReasonCodeValueId + " does not exist",
+ LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue()));
+ }
+
+ // Validation: expenseGLAccountId must exist as a valid Expense GL account
+ final GLAccount glAccount = getAccountByIdAndType(LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue(),
Review Comment:
This throws `GLAccountNotFoundException` exception if it does not found the gl account, so `glAccount` will never be null.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "kulminsky (via GitHub)" <gi...@apache.org>.
kulminsky commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851994996
##########
fineract-provider/src/main/resources/db/changelog/tenant/parts/0153_add_charge_off_reason_id_to_acc_product_mapping.xml:
##########
@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.3.xsd">
+
+ <changeSet id="1" author="fineract">
+ <addColumn tableName="acc_product_mapping">
+ <column name="charge_off_reason_id" type="BIGINT" defaultValueNumeric="NULL">
Review Comment:
Added
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/ClientLoanIntegrationTest.java:
##########
@@ -5490,7 +5490,7 @@ public void chargeAdjustmentAccountingValidation() {
.withDaysInMonth("30").withDaysInYear("365").withMoratorium("0", "0")
.withFeeToIncomeAccountMapping(feeCharge.getResourceId(), uniqueIncomeAccountForFee.getResourceId())
.withPenaltyToIncomeAccountMapping(penaltyCharge.getResourceId(), uniqueIncomeAccountForPenalty.getResourceId())
- .withFeeAndPenaltyAssetAccount(assetFeeAndPenaltyAccount).build(null);
+ .withFeeAndPenaltyAssetAccount(assetFeeAndPenaltyAccount).withChargeOffReasonsToExpenseMappings(1L, 12L).build(null);
Review Comment:
Removed
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "kulminsky (via GitHub)" <gi...@apache.org>.
kulminsky commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1852058913
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -455,4 +552,37 @@ public void deleteProductToGLAccountMapping(final Long loanProductId, final Port
this.accountMappingRepository.deleteAllInBatch(productToGLAccountMappings);
}
}
+
+ public void validateChargeOffMappingsInDatabase(final List<JsonObject> mappings) {
+ final List<ApiParameterError> validationErrors = new ArrayList<>();
+
+ for (JsonObject jsonObject : mappings) {
+ final Long expenseGlAccountId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue(), jsonObject);
+ final Long chargeOffReasonCodeValueId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue(), jsonObject);
+
+ // Validation: chargeOffReasonCodeValueId must exist in the database
+ CodeValue codeValue = this.codeValueRepository.findByCodeNameAndId("ChargeOffReasons", chargeOffReasonCodeValueId);
+ if (codeValue == null) {
+ validationErrors.add(ApiParameterError.parameterError("validation.msg.chargeoffreason.invalid",
+ "Charge-off reason with ID " + chargeOffReasonCodeValueId + " does not exist",
+ LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue()));
+ }
+
+ // Validation: expenseGLAccountId must exist as a valid Expense GL account
+ final Optional<GLAccount> glAccount = accountRepository.findById(expenseGlAccountId);
Review Comment:
Updated
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1831092504
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/productaccountmapping/service/ProductToGLAccountMappingWritePlatformServiceImpl.java:
##########
@@ -209,6 +209,16 @@ public void createLoanProductToGLAccountMapping(final Long loanProductId, final
this.loanProductToGLAccountMappingHelper.savePaymentChannelToFundSourceMappings(command, element, loanProductId, null);
this.loanProductToGLAccountMappingHelper.saveChargesToIncomeAccountMappings(command, element, loanProductId, null);
break;
+ case CHARGE_OFF_FRAUD:
Review Comment:
We dont need this.... we should save all the configured mapping, but we dont need to differentiate them yet
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "kulminsky (via GitHub)" <gi...@apache.org>.
kulminsky commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851994101
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -455,4 +560,39 @@ public void deleteProductToGLAccountMapping(final Long loanProductId, final Port
this.accountMappingRepository.deleteAllInBatch(productToGLAccountMappings);
}
}
+
+ public void validateChargeOffMappingsInDatabase(final List<JsonObject> mappings) {
+ final List<ApiParameterError> validationErrors = new ArrayList<>();
+
+ for (JsonObject jsonObject : mappings) {
+ final Long expenseGlAccountId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue(), jsonObject);
+ final Long chargeOffReasonCodeValueId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue(), jsonObject);
+
+ // Validation: chargeOffReasonCodeValueId must exist in the database
+ CodeValue codeValue = this.codeValueRepositoryWrapper.findOneByCodeNameAndIdWithNotFoundDetection("ChargeOffReasons",
Review Comment:
Changed to repository
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1841815028
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingReadPlatformService.java:
##########
@@ -47,4 +47,5 @@ public interface ProductToGLAccountMappingReadPlatformService {
List<ChargeToGLAccountMapper> fetchFeeToIncomeAccountMappingsForShareProduct(Long productId);
+ List<ChargeToGLAccountMapper> fetchChargeOffReasonsToExpenseMappingsForSavingsProduct(Long savingsProductId);
Review Comment:
We dont want this mapping for Savings. Only for Loans.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1832862102
##########
fineract-loan/src/main/java/org/apache/fineract/accounting/productaccountmapping/service/LoanProductToGLAccountMappingHelper.java:
##########
@@ -388,4 +388,15 @@ private GLAccountType getGLAccountType(final JsonElement element, final String p
return gLAccountType;
}
+ public void mapChargeOffReasonToExpenseAccount(JsonElement element, Long loanProductId, String expenseAccountParam) {
Review Comment:
I guess we dont need this anymore
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1831088825
##########
fineract-loan/src/main/java/org/apache/fineract/accounting/productaccountmapping/service/LoanProductToGLAccountMappingHelper.java:
##########
@@ -388,4 +388,15 @@ private GLAccountType getGLAccountType(final JsonElement element, final String p
return gLAccountType;
}
+ public void mapChargeOffReasonToExpenseAccount(JsonElement element, Long loanProductId, String expenseAccountParam) {
+ int productType = PortfolioProductType.LOAN.getValue();
+ int financialAccountType = GLAccountType.EXPENSE.getValue();
+
+ boolean isMapped = accountMappingRepository.existsByProductIdAndProductTypeAndFinancialAccountType(loanProductId, productType,
Review Comment:
What is this logic doing? Only
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851724833
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -455,4 +560,39 @@ public void deleteProductToGLAccountMapping(final Long loanProductId, final Port
this.accountMappingRepository.deleteAllInBatch(productToGLAccountMappings);
}
}
+
+ public void validateChargeOffMappingsInDatabase(final List<JsonObject> mappings) {
+ final List<ApiParameterError> validationErrors = new ArrayList<>();
+
+ for (JsonObject jsonObject : mappings) {
+ final Long expenseGlAccountId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue(), jsonObject);
+ final Long chargeOffReasonCodeValueId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue(), jsonObject);
+
+ // Validation: chargeOffReasonCodeValueId must exist in the database
+ CodeValue codeValue = this.codeValueRepositoryWrapper.findOneByCodeNameAndIdWithNotFoundDetection("ChargeOffReasons",
Review Comment:
This throws `CodeValueNotFoundException` exception if it does not found the code value, so `codeValue` will never be null.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1852000460
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -455,4 +552,37 @@ public void deleteProductToGLAccountMapping(final Long loanProductId, final Port
this.accountMappingRepository.deleteAllInBatch(productToGLAccountMappings);
}
}
+
+ public void validateChargeOffMappingsInDatabase(final List<JsonObject> mappings) {
+ final List<ApiParameterError> validationErrors = new ArrayList<>();
+
+ for (JsonObject jsonObject : mappings) {
+ final Long expenseGlAccountId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue(), jsonObject);
+ final Long chargeOffReasonCodeValueId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue(), jsonObject);
+
+ // Validation: chargeOffReasonCodeValueId must exist in the database
+ CodeValue codeValue = this.codeValueRepository.findByCodeNameAndId("ChargeOffReasons", chargeOffReasonCodeValueId);
+ if (codeValue == null) {
+ validationErrors.add(ApiParameterError.parameterError("validation.msg.chargeoffreason.invalid",
+ "Charge-off reason with ID " + chargeOffReasonCodeValueId + " does not exist",
+ LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue()));
+ }
+
+ // Validation: expenseGLAccountId must exist as a valid Expense GL account
+ final Optional<GLAccount> glAccount = accountRepository.findById(expenseGlAccountId);
Review Comment:
This validation is checking whether the gl account does exist, but it does not check whether this gl account is expense type, right?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1831087318
##########
fineract-core/src/main/java/org/apache/fineract/accounting/common/AccountingRuleType.java:
##########
@@ -30,7 +30,9 @@ public enum AccountingRuleType {
NONE(1, "accountingRuleType.none", "No accounting"), //
CASH_BASED(2, "accountingRuleType.cash", "Cash based accounting"), //
ACCRUAL_PERIODIC(3, "accountingRuleType.accrual.periodic", "Periodic accrual accounting"), //
- ACCRUAL_UPFRONT(4, "accountingRuleType.accrual.upfront", "Upfront accrual accounting"); //
+ ACCRUAL_UPFRONT(4, "accountingRuleType.accrual.upfront", "Upfront accrual accounting"), //
+ CHARGE_OFF_FRAUD(5, "accountingRuleType.chargeOffFraud", "Charge-off accounting for fraud"), CHARGE_OFF_DELINQUENT(6,
Review Comment:
Charge-off is not a separate accounting rule type. Please undo this.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1831094939
##########
fineract-provider/src/test/java/org/apache/fineract/accounting/productaccountmapping/service/ProductToGLAccountMappingWritePlatformServiceTest.java:
##########
@@ -0,0 +1,124 @@
+/**
+ * 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.accounting.productaccountmapping.service;
+
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.anyInt;
+import static org.mockito.Mockito.anyLong;
+import static org.mockito.Mockito.anyString;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import java.util.Locale;
+import org.apache.fineract.accounting.common.AccountingConstants.LoanProductAccountingParams;
+import org.apache.fineract.accounting.common.AccountingRuleType;
+import org.apache.fineract.accounting.glaccount.domain.GLAccountType;
+import org.apache.fineract.accounting.producttoaccountmapping.domain.ProductToGLAccountMappingRepository;
+import org.apache.fineract.accounting.producttoaccountmapping.serialization.ProductToGLAccountMappingFromApiJsonDeserializer;
+import org.apache.fineract.accounting.producttoaccountmapping.service.SavingsProductToGLAccountMappingHelper;
+import org.apache.fineract.accounting.producttoaccountmapping.service.ShareProductToGLAccountMappingHelper;
+import org.apache.fineract.infrastructure.core.api.JsonCommand;
+import org.apache.fineract.infrastructure.core.serialization.FromJsonHelper;
+import org.apache.fineract.portfolio.PortfolioProductType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+class ProductToGLAccountMappingWritePlatformServiceImplTest {
+
+ @Mock
+ private ProductToGLAccountMappingRepository glAccountMappingRepository;
+
+ @Mock
+ private LoanProductToGLAccountMappingHelper loanProductToGLAccountMappingHelper;
+
+ @Mock
+ private ProductToGLAccountMappingFromApiJsonDeserializer deserializer;
+
+ @Mock
+ private SavingsProductToGLAccountMappingHelper savingsProductToGLAccountMappingHelper;
+
+ @Mock
+ private ShareProductToGLAccountMappingHelper shareProductToGLAccountMappingHelper;
+
+ @Mock
+ private FromJsonHelper fromApiJsonHelper;
+
+ @Mock
+ private JsonCommand command;
+
+ @Mock
+ private JsonElement element;
+
+ private ProductToGLAccountMappingWritePlatformServiceImpl service;
+
+ private static final Long LOAN_PRODUCT_ID = 1L;
+ private static final int PRODUCT_TYPE = PortfolioProductType.LOAN.getValue();
+ private static final int FINANCIAL_ACCOUNT_TYPE_EXPENSE = GLAccountType.EXPENSE.getValue();
+ private static final String CHARGE_OFF_FRAUD_EXPENSE = LoanProductAccountingParams.CHARGE_OFF_FRAUD_EXPENSE.getValue();
+
+ @BeforeEach
Review Comment:
I dont see any added value of having classes that are fully mocked out... I would rather see integration test that is testing whether incoming request got saved properly, and we can fetch created mappings. It can be updated, etc.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "kulminsky (via GitHub)" <gi...@apache.org>.
kulminsky commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1852008408
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -455,4 +552,37 @@ public void deleteProductToGLAccountMapping(final Long loanProductId, final Port
this.accountMappingRepository.deleteAllInBatch(productToGLAccountMappings);
}
}
+
+ public void validateChargeOffMappingsInDatabase(final List<JsonObject> mappings) {
+ final List<ApiParameterError> validationErrors = new ArrayList<>();
+
+ for (JsonObject jsonObject : mappings) {
+ final Long expenseGlAccountId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue(), jsonObject);
+ final Long chargeOffReasonCodeValueId = this.fromApiJsonHelper
+ .extractLongNamed(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue(), jsonObject);
+
+ // Validation: chargeOffReasonCodeValueId must exist in the database
+ CodeValue codeValue = this.codeValueRepository.findByCodeNameAndId("ChargeOffReasons", chargeOffReasonCodeValueId);
+ if (codeValue == null) {
+ validationErrors.add(ApiParameterError.parameterError("validation.msg.chargeoffreason.invalid",
+ "Charge-off reason with ID " + chargeOffReasonCodeValueId + " does not exist",
+ LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue()));
+ }
+
+ // Validation: expenseGLAccountId must exist as a valid Expense GL account
+ final Optional<GLAccount> glAccount = accountRepository.findById(expenseGlAccountId);
Review Comment:
Yes
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1806: Charge-off Expense Accounting entries based on charge-off reason instead of fraud flag [fineract]
Posted by "kulminsky (via GitHub)" <gi...@apache.org>.
kulminsky commented on code in PR #4144:
URL: https://github.com/apache/fineract/pull/4144#discussion_r1851993688
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -356,6 +382,66 @@ public void updatePaymentChannelToFundSourceMappings(final JsonCommand command,
}
}
+ public void updateChargeOffReasonToGLAccountMappings(final JsonCommand command, final JsonElement element, final Long productId,
+ final Map<String, Object> changes, final PortfolioProductType portfolioProductType) {
+
+ final List<ProductToGLAccountMapping> existingChargeOffReasonToGLAccountMappings = this.accountMappingRepository
+ .findAllChargesOffReasonsMappings(productId, portfolioProductType.getValue());
+ final JsonArray chargeOffReasonToGLAccountMappingArray = this.fromApiJsonHelper
+ .extractJsonArrayNamed(LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue(), element);
+
+ final Map<Long, Long> inputChargeOffReasonToGLAccountMap = new HashMap<>();
+
+ final Set<Long> existingChargeOffReasons = new HashSet<>();
+ if (chargeOffReasonToGLAccountMappingArray != null) {
+ if (changes != null) {
+ changes.put(LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue(),
+ command.jsonFragment(LoanProductAccountingParams.CHARGE_OFF_REASONS_TO_EXPENSE.getValue()));
+ }
+
+ for (int i = 0; i < chargeOffReasonToGLAccountMappingArray.size(); i++) {
+ final JsonObject jsonObject = chargeOffReasonToGLAccountMappingArray.get(i).getAsJsonObject();
+ final Long expenseGlAccountId = jsonObject.get(LoanProductAccountingParams.EXPENSE_GL_ACCOUNT_ID.getValue()).getAsLong();
+ final Long chargeOffReasonCodeValueId = jsonObject
+ .get(LoanProductAccountingParams.CHARGE_OFF_REASON_CODE_VALUE_ID.getValue()).getAsLong();
+ inputChargeOffReasonToGLAccountMap.put(chargeOffReasonCodeValueId, expenseGlAccountId);
+ }
+
+ // If input map is empty, delete all existing mappings
+ if (inputChargeOffReasonToGLAccountMap.isEmpty()) {
+ this.accountMappingRepository.deleteAllInBatch(existingChargeOffReasonToGLAccountMappings);
+ } else {
+ for (final ProductToGLAccountMapping existingChargeOffReasonToGLAccountMapping : existingChargeOffReasonToGLAccountMappings) {
+ final Long currentChargeOffReasonId = existingChargeOffReasonToGLAccountMapping.getChargeOffReasonId();
+ if (currentChargeOffReasonId != null) {
+ existingChargeOffReasons.add(currentChargeOffReasonId);
+ // update existing mappings (if required)
+ if (inputChargeOffReasonToGLAccountMap.containsKey(currentChargeOffReasonId)) {
+ final Long newGLAccountId = inputChargeOffReasonToGLAccountMap.get(currentChargeOffReasonId);
+ if (!newGLAccountId.equals(existingChargeOffReasonToGLAccountMapping.getGlAccount().getId())) {
+ final GLAccount glAccount = getAccountById(LoanProductAccountingParams.FUND_SOURCE.getValue(),
Review Comment:
Removed, use byId
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org