galovics commented on code in PR #5771: URL: https://github.com/apache/fineract/pull/5771#discussion_r3221536544
########## fineract-working-capital-loan/src/main/resources/db/changelog/tenant/module/workingcapitalloan/parts/0036_wc_loan_period_payment_rate_change.xml: ########## @@ -0,0 +1,135 @@ +<?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 author="fineract" id="wcl-0036-1-create-rate-change-table"> + <preConditions onFail="MARK_RAN"> + <not> + <tableExists tableName="m_wc_loan_period_payment_rate_change"/> + </not> + </preConditions> + <createTable tableName="m_wc_loan_period_payment_rate_change"> + <column autoIncrement="true" name="id" type="BIGINT"> + <constraints nullable="false" primaryKey="true"/> + </column> + <column name="wc_loan_id" type="BIGINT"> + <constraints nullable="false"/> + </column> + <column name="effective_date" type="DATE"> + <constraints nullable="false"/> + </column> + <column name="previous_rate" type="DECIMAL(19,6)"> + <constraints nullable="false"/> + </column> + <column name="new_rate" type="DECIMAL(19,6)"> + <constraints nullable="false"/> + </column> + <column name="is_reversed" type="BOOLEAN" defaultValueBoolean="false"> + <constraints nullable="false"/> + </column> + <column name="reversed_on_date" type="DATE"/> + <column name="created_by" type="BIGINT"> + <constraints nullable="false"/> + </column> + <column name="last_modified_by" type="BIGINT"> + <constraints nullable="false"/> + </column> + <column name="version" type="INT" defaultValueNumeric="0"> + <constraints nullable="false"/> + </column> + </createTable> + </changeSet> + + <changeSet author="fineract" id="wcl-0036-2-audit-columns-my" context="mysql"> + <addColumn tableName="m_wc_loan_period_payment_rate_change"> + <column name="created_on_utc" type="DATETIME(6)"> + <constraints nullable="false"/> + </column> + <column name="last_modified_on_utc" type="DATETIME(6)"> + <constraints nullable="false"/> + </column> + </addColumn> + </changeSet> + + <changeSet author="fineract" id="wcl-0036-2-audit-columns-pg" context="postgresql"> + <addColumn tableName="m_wc_loan_period_payment_rate_change"> + <column name="created_on_utc" type="TIMESTAMP WITH TIME ZONE"> + <constraints nullable="false"/> + </column> + <column name="last_modified_on_utc" type="TIMESTAMP WITH TIME ZONE"> + <constraints nullable="false"/> + </column> + </addColumn> + </changeSet> + + <changeSet author="fineract" id="wcl-0036-3-rate-change-fks"> + <addForeignKeyConstraint baseColumnNames="wc_loan_id" baseTableName="m_wc_loan_period_payment_rate_change" + constraintName="fk_wc_rate_change_loan" referencedColumnNames="id" + referencedTableName="m_wc_loan"/> + <addForeignKeyConstraint baseColumnNames="created_by" baseTableName="m_wc_loan_period_payment_rate_change" + constraintName="fk_wc_rate_change_created_by" referencedColumnNames="id" + referencedTableName="m_appuser"/> + <addForeignKeyConstraint baseColumnNames="last_modified_by" baseTableName="m_wc_loan_period_payment_rate_change" + constraintName="fk_wc_rate_change_last_modified_by" referencedColumnNames="id" + referencedTableName="m_appuser"/> + </changeSet> + + <changeSet author="fineract" id="wcl-0036-4-rate-change-index"> + <createIndex tableName="m_wc_loan_period_payment_rate_change" indexName="idx_wc_rate_change_loan_date"> + <column name="wc_loan_id"/> + <column name="effective_date"/> + </createIndex> + </changeSet> + + <changeSet author="fineract" id="wcl-0036-6-update-rate-permission"> + <preConditions onFail="MARK_RAN"> + <sqlCheck expectedResult="0"> + SELECT COUNT(*) FROM m_permission WHERE code = 'UPDATERATE_WORKINGCAPITALLOAN'; + </sqlCheck> + </preConditions> + <insert tableName="m_permission"> + <column name="grouping" value="portfolio"/> + <column name="code" value="UPDATERATE_WORKINGCAPITALLOAN"/> + <column name="entity_name" value="WORKINGCAPITALLOAN"/> + <column name="action_name" value="UPDATERATE"/> + <column name="can_maker_checker" valueBoolean="false"/> + </insert> + </changeSet> + + <changeSet author="fineract" id="wcl-0036-7-undo-rate-change-permission"> Review Comment: Where's the endpoint / handler / service method for this? I can't find any `UNDORATECHANGE` implementation, just the permission row. Either drop this changeset until you actually implement undo, or include the implementation in this PR. Dead permissions are confusing. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/api/WorkingCapitalLoanApiResource.java: ########## @@ -310,4 +313,102 @@ private CommandProcessingResult handleStateTransition(final Long loanId, final S return this.commandsSourceWritePlatformService.logCommandSource(commandRequest); } + @PUT + @Path("{loanId}/discount") Review Comment: Is this discount endpoint related to FINERACT-2455? Looks like a separate feature. Let's split it into its own PR. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/serialization/WorkingCapitalLoanDataValidator.java: ########## @@ -95,6 +96,9 @@ public class WorkingCapitalLoanDataValidator { WorkingCapitalLoanConstants.noteParamName, WorkingCapitalLoanConstants.transactionDateParamName)); private static final Set<String> CREDIT_BALANCE_REFUND_SUPPORTED_PARAMETERS = new HashSet<>(REPAYMENT_SUPPORTED_PARAMETERS); + private static final Set<String> UPDATE_RATE_SUPPORTED_PARAMETERS = new HashSet<>( + Arrays.asList("locale", WorkingCapitalLoanConstants.periodPaymentRateParamName, WorkingCapitalLoanConstants.noteParamName)); Review Comment: There's a `WorkingCapitalLoanConstants.localeParameterName` already. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/serialization/WorkingCapitalLoanDataValidator.java: ########## @@ -616,6 +620,58 @@ public void validateCreditBalanceRefund(final String json, final WorkingCapitalL throwExceptionIfValidationWarningsExist(dataValidationErrors); } + public void validateUpdatePeriodPaymentRate(final String json, final WorkingCapitalLoan loan) { Review Comment: Tests please. No unit test covers: blank json, non-active loan, missing `periodPaymentRate`, negative rate, same-as-current rate, below product min, above product max, note too long. Same for `WorkingCapitalLoanWritePlatformServiceImpl.updatePeriodPaymentRate`. The feign tests only happy-path it. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanWritePlatformServiceImpl.java: ########## @@ -668,6 +671,57 @@ public CommandProcessingResult creditBalanceRefund(final Long loanId, final Json .withClientId(loan.getClientId()).withLoanId(loanId).with(changes).build(); } + @Override + @Transactional + public CommandProcessingResult updatePeriodPaymentRate(final Long loanId, final JsonCommand command) { + final WorkingCapitalLoan loan = this.loanRepository.findById(loanId) + .orElseThrow(() -> new WorkingCapitalLoanNotFoundException(loanId)); + this.validator.validateUpdatePeriodPaymentRate(command.json(), loan); + + final BigDecimal newRate = this.fromApiJsonHelper.extractBigDecimalNamed(WorkingCapitalLoanConstants.periodPaymentRateParamName, + command.parsedJson(), new HashSet<>()); + final BigDecimal previousRate = loan.getLoanProductRelatedDetails().getPeriodPaymentRate(); + + final LocalDate businessDate = DateUtils.getBusinessLocalDate(); + + final List<WorkingCapitalLoanPeriodPaymentRateChange> activeChanges = this.rateChangeRepository + .findByWorkingCapitalLoanIdAndReversedFalse(loanId); + for (final WorkingCapitalLoanPeriodPaymentRateChange active : activeChanges) { + active.reverse(businessDate); + } + if (!activeChanges.isEmpty()) { + this.rateChangeRepository.saveAll(activeChanges); + } + + loan.getLoanProductRelatedDetails().setPeriodPaymentRate(newRate); + + final WorkingCapitalLoanPeriodPaymentRateChange rateChange = WorkingCapitalLoanPeriodPaymentRateChange.create(loan, businessDate, + previousRate, newRate); + this.rateChangeRepository.save(rateChange); + + try { + this.amortizationScheduleWriteService.regenerateAmortizationScheduleOnRateChange(loan, newRate); + } catch (IllegalStateException | IllegalArgumentException e) { Review Comment: This is using exceptions as control flow. `IllegalStateException` / `IllegalArgumentException` are thrown by `Validate.notNull` and any number of unrelated places - you'll catch and mask real bugs here. Either validate preconditions upfront in the validator (preferred), or have the calculator throw a domain-specific checked exception you can react to. ########## fineract-working-capital-loan/src/main/resources/db/changelog/tenant/module/workingcapitalloan/parts/0036_wc_loan_period_payment_rate_change.xml: ########## @@ -0,0 +1,135 @@ +<?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 author="fineract" id="wcl-0036-1-create-rate-change-table"> Review Comment: The changeset IDs jump: 1, 2-my, 2-pg, 3, 4, 6, 7. What happened to 5? Renumber please. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/serialization/WorkingCapitalLoanDataValidator.java: ########## @@ -616,6 +620,58 @@ public void validateCreditBalanceRefund(final String json, final WorkingCapitalL throwExceptionIfValidationWarningsExist(dataValidationErrors); } + public void validateUpdatePeriodPaymentRate(final String json, final WorkingCapitalLoan loan) { + if (StringUtils.isBlank(json)) { + throw new InvalidJsonException(); + } + + if (loan.getLoanStatus() != LoanStatus.ACTIVE) { + throw new PlatformApiDataValidationException("validation.msg.wc.loan.rate.change.not.allowed", Review Comment: Why mix early `throw` with the `DataValidatorBuilder` pattern? Collect all errors via the builder and throw once at the end like the other validate methods in this class. Otherwise the client gets one error at a time which is annoying. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanAmortizationScheduleReadServiceImpl.java: ########## @@ -36,21 +37,17 @@ @Transactional(readOnly = true) public class WorkingCapitalLoanAmortizationScheduleReadServiceImpl implements WorkingCapitalLoanAmortizationScheduleReadService { - // TODO: currency should come from loan product once WCL lifecycle is implemented - private static final MonetaryCurrency DEFAULT_CURRENCY = new MonetaryCurrency("USD", 2, null); - private final WorkingCapitalLoanRepository loanRepository; private final ProjectedAmortizationScheduleRepositoryWrapper scheduleRepositoryWrapper; private final ProjectedAmortizationScheduleMapper mapper; @Override public ProjectedAmortizationScheduleData retrieveAmortizationSchedule(final Long loanId) { - if (!loanRepository.existsById(loanId)) { - throw new WorkingCapitalLoanNotFoundException(loanId); - } + final WorkingCapitalLoan loan = loanRepository.findById(loanId).orElseThrow(() -> new WorkingCapitalLoanNotFoundException(loanId)); final MathContext mc = MoneyHelper.getMathContext(); - final ProjectedAmortizationScheduleModel model = scheduleRepositoryWrapper.readModel(loanId, mc, DEFAULT_CURRENCY) + final CurrencyData currency = WorkingCapitalLoanAmortizationScheduleWriteServiceImpl.resolveCurrency(loan); Review Comment: Static call into the write service impl from the read service. Extract `resolveCurrency` into its own helper / domain method on `WorkingCapitalLoan`. The read service shouldn't depend on the write service's impl class. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanPeriodPaymentRateChangeReadServiceImpl.java: ########## @@ -0,0 +1,47 @@ +/** + * 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.portfolio.workingcapitalloan.service; + +import java.util.List; +import lombok.RequiredArgsConstructor; +import org.apache.fineract.portfolio.workingcapitalloan.data.WorkingCapitalLoanPeriodPaymentRateChangeData; +import org.apache.fineract.portfolio.workingcapitalloan.domain.WorkingCapitalLoanPeriodPaymentRateChange; +import org.apache.fineract.portfolio.workingcapitalloan.repository.WorkingCapitalLoanPeriodPaymentRateChangeRepository; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; + +@Service +@RequiredArgsConstructor +@Transactional(readOnly = true) +public class WorkingCapitalLoanPeriodPaymentRateChangeReadServiceImpl implements WorkingCapitalLoanPeriodPaymentRateChangeReadService { + + private final WorkingCapitalLoanPeriodPaymentRateChangeRepository repository; + + @Override + public List<WorkingCapitalLoanPeriodPaymentRateChangeData> retrieveRateChangeHistory(final Long loanId) { + return repository.findByWorkingCapitalLoanIdOrderByIdDesc(loanId).stream().map(e -> toData(e, loanId)).toList(); Review Comment: No check that the loan actually exists. Bogus `loanId` returns an empty list silently. Let's 404 on missing loan. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanWritePlatformServiceImpl.java: ########## @@ -668,6 +671,57 @@ public CommandProcessingResult creditBalanceRefund(final Long loanId, final Json .withClientId(loan.getClientId()).withLoanId(loanId).with(changes).build(); } + @Override + @Transactional + public CommandProcessingResult updatePeriodPaymentRate(final Long loanId, final JsonCommand command) { + final WorkingCapitalLoan loan = this.loanRepository.findById(loanId) + .orElseThrow(() -> new WorkingCapitalLoanNotFoundException(loanId)); + this.validator.validateUpdatePeriodPaymentRate(command.json(), loan); + + final BigDecimal newRate = this.fromApiJsonHelper.extractBigDecimalNamed(WorkingCapitalLoanConstants.periodPaymentRateParamName, + command.parsedJson(), new HashSet<>()); + final BigDecimal previousRate = loan.getLoanProductRelatedDetails().getPeriodPaymentRate(); + + final LocalDate businessDate = DateUtils.getBusinessLocalDate(); + + final List<WorkingCapitalLoanPeriodPaymentRateChange> activeChanges = this.rateChangeRepository + .findByWorkingCapitalLoanIdAndReversedFalse(loanId); + for (final WorkingCapitalLoanPeriodPaymentRateChange active : activeChanges) { + active.reverse(businessDate); + } + if (!activeChanges.isEmpty()) { + this.rateChangeRepository.saveAll(activeChanges); Review Comment: Why call `saveAll`? The entities you fetched are managed within the `@Transactional` method. Same for `loanRepository.saveAndFlush(loan)` further below. ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/domain/WorkingCapitalLoanPeriodPaymentRateChange.java: ########## @@ -0,0 +1,79 @@ +/** + * 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.portfolio.workingcapitalloan.domain; + +import jakarta.persistence.Column; +import jakarta.persistence.Entity; +import jakarta.persistence.FetchType; +import jakarta.persistence.JoinColumn; +import jakarta.persistence.ManyToOne; +import jakarta.persistence.Table; +import jakarta.persistence.Version; +import java.math.BigDecimal; +import java.time.LocalDate; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.Setter; +import org.apache.fineract.infrastructure.core.domain.AbstractAuditableWithUTCDateTimeCustom; + +@Getter +@Setter +@NoArgsConstructor +@Entity +@Table(name = "m_wc_loan_period_payment_rate_change") +public class WorkingCapitalLoanPeriodPaymentRateChange extends AbstractAuditableWithUTCDateTimeCustom<Long> { + + @ManyToOne(fetch = FetchType.LAZY) + @JoinColumn(name = "wc_loan_id", nullable = false) + private WorkingCapitalLoan workingCapitalLoan; + + @Column(name = "effective_date", nullable = false) + private LocalDate effectiveDate; + + @Column(name = "previous_rate", scale = 6, precision = 19, nullable = false) + private BigDecimal previousRate; + + @Column(name = "new_rate", scale = 6, precision = 19, nullable = false) + private BigDecimal newRate; + + @Column(name = "is_reversed", nullable = false) + private boolean reversed; Review Comment: Nothing currently writes `reversedByUser` or other reversal audit fields for reversal - is that intentional, or are we missing reversal audit columns vs the `m_wc_loan_transaction` reversal pattern? ########## fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanAmortizationScheduleWriteServiceImpl.java: ########## @@ -180,21 +183,56 @@ public RepaymentAmortizationData applyRepayment(final WorkingCapitalLoan loan, f private BigDecimal sumRunningNpv(final ProjectedAmortizationScheduleModel model) { final MathContext mc = MoneyHelper.getMathContext(); BigDecimal result = BigDecimal.ZERO; - for (ProjectedPayment payment : model.payments()) { + for (ProjectedPayment payment : model.projectedPayments()) { if (payment.paymentNo() > 0 && payment.npvValue() != null && payment.npvValue().getAmount() != null) { result = result.add(payment.npvValue().getAmount(), mc); } } return result; } - private MonetaryCurrency resolveCurrency(final WorkingCapitalLoan loan) { + @Override + public void regenerateAmortizationScheduleOnRateChange(final WorkingCapitalLoan loan, final BigDecimal newRate) { + Validate.notNull(loan, "loan must not be null"); + Validate.notNull(newRate, "newRate must not be null"); + + final MathContext mc = MoneyHelper.getMathContext(); + final CurrencyData currency = resolveCurrency(loan); + final ProjectedAmortizationScheduleModel model = scheduleRepositoryWrapper.readModel(loan.getId(), mc, currency) + .orElseThrow(() -> new IllegalStateException("Projected amortization schedule is not found for loan " + loan.getId())); + + final LocalDate businessDate = DateUtils.getBusinessLocalDate(); + final LocalDate loanDisbursementDate = resolveLoanDisbursementDate(loan); + final int splitDayIndex = (int) ChronoUnit.DAYS.between(loanDisbursementDate, businessDate); + final LocalDate modelRateChangeDate = model.expectedDisbursementDate().plusDays(splitDayIndex); + + model.clearLastRateSegment(); + + calculator.applyRateChange(model, newRate, modelRateChangeDate); + + scheduleRepositoryWrapper.writeModel(loan, model); + } + + private LocalDate resolveLoanDisbursementDate(final WorkingCapitalLoan loan) { + if (loan.getDisbursementDetails() != null && !loan.getDisbursementDetails().isEmpty()) { + final WorkingCapitalLoanDisbursementDetails detail = loan.getDisbursementDetails().getFirst(); + if (detail.getActualDisbursementDate() != null) { + return detail.getActualDisbursementDate(); + } + if (detail.getExpectedDisbursementDate() != null) { + return detail.getExpectedDisbursementDate(); Review Comment: The validator only allows rate changes for `LoanStatus.ACTIVE`. An active loan must have an actual disbursement date - so taking the first disbursement detail and falling back to expected is suspicious. Shouldn't we be using the actual disbursement date strictly here, and throwing a proper domain exception (not `IllegalStateException`) if it's missing? -- 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]
