budaidev commented on code in PR #5740: URL: https://github.com/apache/fineract/pull/5740#discussion_r3095659985
########## fineract-working-capital-loan/src/main/resources/db/changelog/tenant/module/workingcapitalloan/parts/0028_wc_near_breach_configuration.xml: ########## @@ -0,0 +1,117 @@ +<?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="wclp-0026-1" author="fineract"> + <preConditions onFail="MARK_RAN"> + <not> + <tableExists tableName="m_wc_near_breach"/> + </not> + </preConditions> + <createTable tableName="m_wc_near_breach"> + <column name="id" type="BIGINT" autoIncrement="true"> + <constraints nullable="false" primaryKey="true" primaryKeyName="PK_m_wc_near_breach"/> + </column> + <column name="near_breach_name" type="VARCHAR(100)"> + <constraints nullable="false"/> + </column> + <column name="near_breach_frequency" type="INT"> + <constraints nullable="false"/> + </column> + <column name="near_breach_frequency_type" type="VARCHAR(50)"> + <constraints nullable="false"/> + </column> + <column name="near_breach_threshold" type="DECIMAL(19,6)"> + <constraints nullable="false"/> + </column> + </createTable> + <createIndex tableName="m_wc_near_breach" indexName="uq_m_wc_near_breach_name" unique="true"> + <column name="near_breach_name"/> + </createIndex> + </changeSet> + + <changeSet id="wclp-0026-2" author="fineract"> + <preConditions onFail="MARK_RAN"> + <tableExists tableName="m_wc_loan_product"/> + <not> + <columnExists tableName="m_wc_loan_product" columnName="near_breach_id"/> + </not> + </preConditions> + <addColumn tableName="m_wc_loan_product"> + <column name="near_breach_id" type="BIGINT"/> + </addColumn> + <addForeignKeyConstraint baseTableName="m_wc_loan_product" baseColumnNames="near_breach_id" + constraintName="FK_m_wc_loan_product_near_breach" referencedTableName="m_wc_near_breach" + referencedColumnNames="id"/> + </changeSet> + + <changeSet id="wclp-0026-3" author="fineract"> Review Comment: ```suggestion <changeSet id="wclp-0028-3" author="fineract"> ``` ########## fineract-working-capital-loan/src/main/resources/db/changelog/tenant/module/workingcapitalloan/parts/0028_wc_near_breach_configuration.xml: ########## @@ -0,0 +1,117 @@ +<?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="wclp-0026-1" author="fineract"> + <preConditions onFail="MARK_RAN"> + <not> + <tableExists tableName="m_wc_near_breach"/> + </not> + </preConditions> + <createTable tableName="m_wc_near_breach"> + <column name="id" type="BIGINT" autoIncrement="true"> + <constraints nullable="false" primaryKey="true" primaryKeyName="PK_m_wc_near_breach"/> + </column> + <column name="near_breach_name" type="VARCHAR(100)"> + <constraints nullable="false"/> + </column> + <column name="near_breach_frequency" type="INT"> + <constraints nullable="false"/> + </column> + <column name="near_breach_frequency_type" type="VARCHAR(50)"> + <constraints nullable="false"/> + </column> + <column name="near_breach_threshold" type="DECIMAL(19,6)"> + <constraints nullable="false"/> + </column> + </createTable> + <createIndex tableName="m_wc_near_breach" indexName="uq_m_wc_near_breach_name" unique="true"> + <column name="near_breach_name"/> + </createIndex> + </changeSet> + + <changeSet id="wclp-0026-2" author="fineract"> Review Comment: ```suggestion <changeSet id="wclp-0028-2" author="fineract"> ``` ########## fineract-working-capital-loan/src/main/resources/db/changelog/tenant/module/workingcapitalloan/parts/0028_wc_near_breach_configuration.xml: ########## @@ -0,0 +1,117 @@ +<?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="wclp-0026-1" author="fineract"> + <preConditions onFail="MARK_RAN"> + <not> + <tableExists tableName="m_wc_near_breach"/> + </not> + </preConditions> + <createTable tableName="m_wc_near_breach"> + <column name="id" type="BIGINT" autoIncrement="true"> + <constraints nullable="false" primaryKey="true" primaryKeyName="PK_m_wc_near_breach"/> + </column> + <column name="near_breach_name" type="VARCHAR(100)"> + <constraints nullable="false"/> + </column> + <column name="near_breach_frequency" type="INT"> + <constraints nullable="false"/> + </column> + <column name="near_breach_frequency_type" type="VARCHAR(50)"> + <constraints nullable="false"/> + </column> + <column name="near_breach_threshold" type="DECIMAL(19,6)"> + <constraints nullable="false"/> + </column> + </createTable> + <createIndex tableName="m_wc_near_breach" indexName="uq_m_wc_near_breach_name" unique="true"> + <column name="near_breach_name"/> + </createIndex> + </changeSet> + + <changeSet id="wclp-0026-2" author="fineract"> + <preConditions onFail="MARK_RAN"> + <tableExists tableName="m_wc_loan_product"/> + <not> + <columnExists tableName="m_wc_loan_product" columnName="near_breach_id"/> + </not> + </preConditions> + <addColumn tableName="m_wc_loan_product"> + <column name="near_breach_id" type="BIGINT"/> + </addColumn> + <addForeignKeyConstraint baseTableName="m_wc_loan_product" baseColumnNames="near_breach_id" + constraintName="FK_m_wc_loan_product_near_breach" referencedTableName="m_wc_near_breach" + referencedColumnNames="id"/> + </changeSet> + + <changeSet id="wclp-0026-3" author="fineract"> + <preConditions onFail="MARK_RAN"> + <tableExists tableName="m_wc_loan"/> + <not> + <columnExists tableName="m_wc_loan" columnName="near_breach_id"/> + </not> + </preConditions> + <addColumn tableName="m_wc_loan"> + <column name="near_breach_id" type="BIGINT"/> + </addColumn> + <addForeignKeyConstraint baseTableName="m_wc_loan" baseColumnNames="near_breach_id" + constraintName="FK_m_wc_loan_near_breach" referencedTableName="m_wc_near_breach" + referencedColumnNames="id"/> + </changeSet> + + <changeSet id="wclp-0026-4" author="fineract"> Review Comment: ```suggestion <changeSet id="wclp-0028-4" author="fineract"> ``` ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloanbreach/service/WorkingCapitalBreachWritePlatformServiceImpl.java: ########## @@ -128,49 +129,32 @@ private WorkingCapitalBreach updateAndPersistBreach(final WorkingCapitalBreach i : null; final BigDecimal breachAmount = request.breachAmount(); - if (isChanged(name, item.getName())) { + if (Validator.isChanged(name, item.getName())) { validateDuplicateName(name, item.getId()); item.setName(name); changes.put(NAME_PARAM, name); } - if (isChanged(breachFrequency, item.getBreachFrequency())) { + if (Validator.isChanged(breachFrequency, item.getBreachFrequency())) { item.setBreachFrequency(breachFrequency); changes.put(BREACH_FREQUENCY_PARAM, breachFrequency); } - if (isChanged(breachFrequencyType, item.getBreachFrequencyType())) { + if (Validator.isChanged(breachFrequencyType, item.getBreachFrequencyType())) { item.setBreachFrequencyType(breachFrequencyType); changes.put(BREACH_FREQUENCY_TYPE_PARAM, breachFrequencyType != null ? breachFrequencyType.name() : null); } - if (isChanged(breachAmountCalculationType, item.getBreachAmountCalculationType())) { + if (Validator.isChanged(breachAmountCalculationType, item.getBreachAmountCalculationType())) { item.setBreachAmountCalculationType(breachAmountCalculationType); changes.put(BREACH_AMOUNT_CALCULATION_TYPE_PARAM, breachAmountCalculationType != null ? breachAmountCalculationType.name() : null); } - if (isBigDecimalChanged(breachAmount, item.getBreachAmount())) { + if (Validator.isChanged(breachAmount, item.getBreachAmount())) { Review Comment: why did we changed from isBigDecimalChanged here? Shouldn't we use the original method (which was also moved to the Validator class)? ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/serialization/WorkingCapitalLoanApplicationDataValidator.java: ########## @@ -592,17 +594,34 @@ private void validateOverridables(final JsonElement element, final DataValidator .failWithCode("override.not.allowed.by.product"); } } + Long breachId = null; if (this.fromApiJsonHelper.parameterExists(WorkingCapitalLoanProductConstants.breachIdParamName, element)) { if (config.isBreach()) { - final Long breachId = this.fromApiJsonHelper.extractLongNamed(WorkingCapitalLoanProductConstants.breachIdParamName, - element); + breachId = this.fromApiJsonHelper.extractLongNamed(WorkingCapitalLoanProductConstants.breachIdParamName, element); baseDataValidator.reset().parameter(WorkingCapitalLoanProductConstants.breachIdParamName).value(breachId).ignoreIfNull() .longGreaterThanZero(); } else { baseDataValidator.reset().parameter(WorkingCapitalLoanProductConstants.breachIdParamName) .failWithCode("override.not.allowed.by.product"); } } + if (this.fromApiJsonHelper.parameterExists(WorkingCapitalLoanProductConstants.nearBreachIdParamName, element)) { Review Comment: I am just wondering here, is it possible that the nearBreachId parameter is not in the request? Maybe having a partial request. If yes, we are completely skipping this validation step, which might lead to inconsistent data. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloanproduct/serialization/WorkingCapitalLoanProductDataValidator.java: ########## @@ -442,12 +446,29 @@ private BigDecimal validateTermFields(final JsonElement element, final DataValid return principal; } - private void validateBreachField(final JsonElement element, final DataValidatorBuilder baseDataValidator) { + private Long validateBreachField(final JsonElement element, final DataValidatorBuilder baseDataValidator) { + Long breachId = null; if (this.fromApiJsonHelper.parameterExists(WorkingCapitalLoanProductConstants.breachIdParamName, element)) { - final Long breachId = this.fromApiJsonHelper.extractLongNamed(WorkingCapitalLoanProductConstants.breachIdParamName, element); + breachId = this.fromApiJsonHelper.extractLongNamed(WorkingCapitalLoanProductConstants.breachIdParamName, element); baseDataValidator.reset().parameter(WorkingCapitalLoanProductConstants.breachIdParamName).value(breachId).ignoreIfNull() .longGreaterThanZero(); } + return breachId; + } + + private void validateNearBreachField(final Long breachId, final JsonElement element, final DataValidatorBuilder baseDataValidator) { + if (this.fromApiJsonHelper.parameterExists(WorkingCapitalLoanProductConstants.nearBreachIdParamName, element)) { + final Long nearBreachId = this.fromApiJsonHelper.extractLongNamed(WorkingCapitalLoanProductConstants.nearBreachIdParamName, + element); + + if (breachId == null && nearBreachId != null) { + baseDataValidator.reset().parameter(WorkingCapitalLoanProductConstants.nearBreachIdParamName) + .failWithCode("cannot.enable.nearch.breach.without.breach"); Review Comment: ```suggestion .failWithCode("cannot.enable.near.breach.without.breach"); ``` ########## fineract-working-capital-loan/src/main/resources/db/changelog/tenant/module/workingcapitalloan/parts/0028_wc_near_breach_configuration.xml: ########## @@ -0,0 +1,117 @@ +<?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="wclp-0026-1" author="fineract"> Review Comment: ```suggestion <changeSet id="wclp-0028-1" author="fineract"> ``` ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/serialization/WorkingCapitalLoanApplicationDataValidator.java: ########## @@ -592,17 +594,34 @@ private void validateOverridables(final JsonElement element, final DataValidator .failWithCode("override.not.allowed.by.product"); } } + Long breachId = null; if (this.fromApiJsonHelper.parameterExists(WorkingCapitalLoanProductConstants.breachIdParamName, element)) { if (config.isBreach()) { - final Long breachId = this.fromApiJsonHelper.extractLongNamed(WorkingCapitalLoanProductConstants.breachIdParamName, - element); + breachId = this.fromApiJsonHelper.extractLongNamed(WorkingCapitalLoanProductConstants.breachIdParamName, element); baseDataValidator.reset().parameter(WorkingCapitalLoanProductConstants.breachIdParamName).value(breachId).ignoreIfNull() .longGreaterThanZero(); } else { baseDataValidator.reset().parameter(WorkingCapitalLoanProductConstants.breachIdParamName) .failWithCode("override.not.allowed.by.product"); } } + if (this.fromApiJsonHelper.parameterExists(WorkingCapitalLoanProductConstants.nearBreachIdParamName, element)) { + if (config.isBreach()) { + final Long nearBreachId = this.fromApiJsonHelper.extractLongNamed(WorkingCapitalLoanProductConstants.nearBreachIdParamName, + element); + baseDataValidator.reset().parameter(WorkingCapitalLoanProductConstants.nearBreachIdParamName).value(nearBreachId) + .ignoreIfNull().longGreaterThanZero(); + if (breachId == null && nearBreachId != null) { + baseDataValidator.reset().parameter(WorkingCapitalLoanProductConstants.nearBreachIdParamName) + .failWithCode("cannot.enable.nearch.breach.without.breach"); Review Comment: ```suggestion .failWithCode("cannot.enable.near.breach.without.breach"); ``` ########## integration-tests/src/test/java/org/apache/fineract/integrationtests/WorkingCapitalLoanApplicationCRUDTest.java: ########## @@ -170,10 +178,62 @@ public void testSubmitWithoutBreachParamsUsesProductBreachDefaults() { assertRepaymentFrequencyTypeEquals(breachFrequencyType, breach.get("breachFrequencyType")); assertRepaymentFrequencyTypeEquals(breachAmountCalculationType, breach.get("breachAmountCalculationType")); assertEqualBigDecimal(breachAmount, breach.get("breachAmount")); + final JsonObject nearBreach = data.getAsJsonObject("nearBreach"); + assertEquals(nearBreachName, nearBreach.get("name").getAsString()); applicationHelper.deleteById(loanId); productHelper.deleteWorkingCapitalLoanProductById(productId); breachHelper.delete(breachId); + nearBreachHelper.delete(nearBreachId); + } + + @Test + public void testNegativeSubmitWithBreachAndNearBreachParams() { + final String breachName = Utils.randomStringGenerator("Breach", 20); + final Integer breachFrequency = 30; + final String breachFrequencyType = "DAYS"; + final String breachAmountCalculationType = "PERCENTAGE"; + final BigDecimal breachAmount = BigDecimal.valueOf(10); + final Long breachId = createBreach(breachName, breachFrequency, breachFrequencyType, breachAmountCalculationType, breachAmount); + final Long productId = createProductWithBreachAndNearBreach(breachId, null, Boolean.TRUE); + final Long clientId = createClient(); + final String nearBreachName = Utils.randomStringGenerator("NearBreach", 20); + final Long nearBreachId = nearBreachHelper.create( + nearBreachHelper.nearBreachJson(nearBreachName, (breachFrequency + 10), breachFrequencyType, BigDecimal.valueOf(30.0))); + + final String json1 = new WorkingCapitalLoanApplicationTestBuilder() // + .withClientId(clientId) // + .withProductId(productId) // + .withPrincipal(BigDecimal.valueOf(5000)) // + .withPeriodPaymentRate(BigDecimal.ONE) // + .withTotalPayment(BigDecimal.valueOf(5500)) // + .withBreachId(null) // + .withNearBreachId(nearBreachId) // + .buildSubmitJson(); + CallFailedRuntimeException exception = assertThrows(CallFailedRuntimeException.class, () -> applicationHelper.submit(json1)); + + // Then + assertThat(exception.getStatus()).isEqualTo(400); + assertThat(exception.getDeveloperMessage()).contains("cannot.enable.nearch.breach.without.breach"); Review Comment: ```suggestion assertThat(exception.getDeveloperMessage()).contains("cannot.enable.near.breach.without.breach"); ``` ########## integration-tests/src/test/java/org/apache/fineract/integrationtests/WorkingCapitalLoanProductCRUDTest.java: ########## @@ -505,4 +520,83 @@ public void testHappyPath_CreateAndRetrieve_VerifyAllFields() { wclProductHelper.deleteWorkingCapitalLoanProductById(productId); } + + @Test + public void testNegativeCreateWorkingCapitalLoanProductWithNearBreach() { + final JsonObject createBody = breachHelper.breachJson(Utils.randomStringGenerator("Breach", 20), 15, "DAYS", "PERCENTAGE", + BigDecimal.valueOf(7.5)); + Long breachId = breachHelper.create(createBody); + WorkingCapitalBreachData expectedBreach = breachHelper.retrieveWorkingCapitalBreach(breachId); + + Long nearBreachId = nearBreachHelper.create(nearBreachHelper.nearBreachJson(Utils.randomStringGenerator("NearBreach", 20), + expectedBreach.getBreachFrequency(), expectedBreach.getBreachFrequencyType().getCode(), BigDecimal.valueOf(30.0))); + + // Given + final String uniqueName = "Test wcl Product " + UUID.randomUUID().toString().substring(0, 8); + final String uniqueShortName = "TW" + UUID.randomUUID().toString().substring(0, 2); + PostWorkingCapitalLoanProductsRequest requestFail1 = new WorkingCapitalLoanProductTestBuilder().withName(uniqueName) + .withShortName(uniqueShortName).withNearBreachId(nearBreachId).build(); + + // When + CallFailedRuntimeException exception = assertThrows(CallFailedRuntimeException.class, + () -> wclProductHelper.createWorkingCapitalLoanProduct(requestFail1)); + + // Then + assertThat(exception.getStatus()).isEqualTo(400); + assertThat(exception.getDeveloperMessage()).contains("cannot.enable.nearch.breach.without.breach"); Review Comment: ```suggestion assertThat(exception.getDeveloperMessage()).contains("cannot.enable.near.breach.without.breach"); ``` ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/domain/WorkingCapitalLoanPeriodFrequencyType.java: ########## @@ -65,6 +65,15 @@ public static WorkingCapitalLoanPeriodFrequencyType fromString(final String peri return null; } + public long toDays(int amount) { Review Comment: Do we use this anywhere? I saw the exact same logic in FrequencyTypeUtil -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
