You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2021/08/30 14:11:33 UTC

[GitHub] [fineract] ptuomola commented on a change in pull request #1770: Feat: Collateral module

ptuomola commented on a change in pull request #1770:
URL: https://github.com/apache/fineract/pull/1770#discussion_r698477273



##########
File path: fineract-provider/src/main/java/org/apache/fineract/commands/service/CommandWrapperBuilder.java
##########
@@ -1814,6 +1822,32 @@ public CommandWrapperBuilder updateCollateral(final Long loanId, final Long coll
         return this;
     }
 
+    public CommandWrapperBuilder updateCollateralProduct(final Long collateralId) {
+        this.actionName = "UPDATE";
+        this.entityName = "COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.href = "/collateral-management/" + collateralId;
+        return this;
+    }
+
+    public CommandWrapperBuilder updateClientCollateralProduct(final Long clientId, final Long collateralId) {
+        this.actionName = "UPDATE";
+        this.entityName = "CLIENT_COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.clientId = clientId;
+        this.href = "/clients/" + clientId + "/collateral" + collateralId;
+        return this;

Review comment:
       Should there be a "/" before the collateralId as a separator? 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/commands/service/CommandWrapperBuilder.java
##########
@@ -1823,6 +1857,31 @@ public CommandWrapperBuilder deleteCollateral(final Long loanId, final Long coll
         return this;
     }
 
+    public CommandWrapperBuilder deleteCollateralProduct(final Long collateralId) {
+        this.actionName = "DELETE";
+        this.entityName = "COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.href = "/collateral-management/delete/" + collateralId;
+        return this;
+    }
+
+    public CommandWrapperBuilder deleteClientCollateralProduct(final Long collateralId, final Long clientId) {
+        this.actionName = "DELETE";
+        this.entityName = "CLIENT_COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.clientId = clientId;
+        this.href = "/collateral-management/" + collateralId + "/clients/" + clientId;
+        return this;
+    }
+
+    public CommandWrapperBuilder addClientCollateralProduct(final Long clientId) {
+        this.actionName = "CREATE";
+        this.entityName = "CLIENT_COLLATERAL_PRODUCT";
+        this.clientId = clientId;
+        this.href = "/collateral/clients/" + clientId + "/create";
+        return this;

Review comment:
       Same as above - should this not be consistent?

##########
File path: fineract-provider/src/main/java/org/apache/fineract/commands/service/CommandWrapperBuilder.java
##########
@@ -1823,6 +1857,31 @@ public CommandWrapperBuilder deleteCollateral(final Long loanId, final Long coll
         return this;
     }
 
+    public CommandWrapperBuilder deleteCollateralProduct(final Long collateralId) {
+        this.actionName = "DELETE";
+        this.entityName = "COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.href = "/collateral-management/delete/" + collateralId;
+        return this;
+    }
+
+    public CommandWrapperBuilder deleteClientCollateralProduct(final Long collateralId, final Long clientId) {
+        this.actionName = "DELETE";
+        this.entityName = "CLIENT_COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.clientId = clientId;
+        this.href = "/collateral-management/" + collateralId + "/clients/" + clientId;
+        return this;

Review comment:
       Shouldn't this be consistent with the above ones... ie /clients/+ clientid + /collateral/+ collateralid

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/collateralmanagement/api/CollateralAPIConstants.java
##########
@@ -0,0 +1,46 @@
+/**
+ * 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.collateralmanagement.api;
+
+public final class CollateralAPIConstants {
+

Review comment:
       Why wrap the enum in a class? Just have the enum - look at AccountingRuleJsonInputParams as an example

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/collateralmanagement/api/CollateralManagementAPIResource.java
##########
@@ -0,0 +1,189 @@
+/**
+ * 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.collateralmanagement.api;
+
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.ArraySchema;
+import io.swagger.v3.oas.annotations.media.Content;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import io.swagger.v3.oas.annotations.responses.ApiResponse;
+import io.swagger.v3.oas.annotations.responses.ApiResponses;
+import io.swagger.v3.oas.annotations.tags.Tag;
+import java.util.Collection;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.UriInfo;
+import org.apache.fineract.commands.domain.CommandWrapper;
+import org.apache.fineract.commands.service.CommandWrapperBuilder;
+import org.apache.fineract.commands.service.PortfolioCommandSourceWritePlatformService;
+import org.apache.fineract.infrastructure.codes.service.CodeValueReadPlatformService;
+import org.apache.fineract.infrastructure.core.api.ApiRequestParameterHelper;
+import org.apache.fineract.infrastructure.core.data.CommandProcessingResult;
+import org.apache.fineract.infrastructure.core.serialization.DefaultToApiJsonSerializer;
+import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext;
+import org.apache.fineract.organisation.monetary.data.CurrencyData;
+import org.apache.fineract.organisation.monetary.service.CurrencyReadPlatformService;
+import org.apache.fineract.portfolio.collateralmanagement.api.CollateralAPIConstants.CollateralManagementJSONInputParams;
+import org.apache.fineract.portfolio.collateralmanagement.data.CollateralManagementData;
+import org.apache.fineract.portfolio.collateralmanagement.service.ClientCollateralManagementReadPlatformService;
+import org.apache.fineract.portfolio.collateralmanagement.service.CollateralManagementReadPlatformService;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Scope;
+import org.springframework.stereotype.Component;
+
+@Path("/collateral-management")
+@Component
+@Scope("singleton")
+@Tag(name = "Collateral Management", description = "Collateral Management is for managing collateral operations")
+public class CollateralManagementAPIResource {
+

Review comment:
       Should be CollateralManagementApiResource

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/collateralmanagement/domain/CollateralManagementDomain.java
##########
@@ -0,0 +1,154 @@
+/**
+ * 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.collateralmanagement.domain;
+
+import java.math.BigDecimal;
+import java.util.HashSet;
+import java.util.Set;
+import javax.persistence.CascadeType;
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.FetchType;
+import javax.persistence.JoinColumn;
+import javax.persistence.ManyToOne;
+import javax.persistence.OneToMany;
+import javax.persistence.Table;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.fineract.infrastructure.core.api.JsonCommand;
+import org.apache.fineract.infrastructure.core.domain.AbstractPersistableCustom;
+import org.apache.fineract.organisation.monetary.domain.ApplicationCurrency;
+import org.apache.fineract.portfolio.collateralmanagement.api.CollateralAPIConstants;
+
+@Entity
+@Table(name = "m_collateral_management")
+public class CollateralManagementDomain extends AbstractPersistableCustom {
+
+    @Column(name = "name", length = 20, columnDefinition = " ")
+    private String name;
+
+    @Column(name = "quality", nullable = false, length = 40)
+    private String quality;
+
+    @Column(name = "base_price", nullable = false, scale = 5, precision = 20)
+    private BigDecimal basePrice;
+
+    @Column(name = "unit_type", nullable = false, length = 10)
+    private String unitType;
+
+    @Column(name = "pct_to_base", nullable = false, scale = 5, precision = 20)
+    private BigDecimal pctToBase;
+
+    @ManyToOne
+    @JoinColumn(name = "currency")
+    private ApplicationCurrency currency;
+
+    @OneToMany(mappedBy = "collateral", cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.EAGER)
+    private Set<ClientCollateralManagement> clientCollateralManagements = new HashSet<>();
+
+    public CollateralManagementDomain() {
+
+    }
+
+    private CollateralManagementDomain(final String quality, final BigDecimal basePrice, final String unitType, final BigDecimal pctToBase,
+            final ApplicationCurrency currency, final String name) {
+        this.basePrice = basePrice;
+        this.currency = currency;
+        this.pctToBase = pctToBase;
+        this.unitType = unitType;
+        this.quality = quality;
+        this.name = name;
+    }
+
+    public static CollateralManagementDomain createNew(JsonCommand jsonCommand, final ApplicationCurrency applicationCurrency) {
+        /**
+         * TODO: Add data validity errors.
+         */
+

Review comment:
       Is this to-do still valid - validations to be added? 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/collateralmanagement/service/CollateralManagementWritePlatformServiceImpl.java
##########
@@ -0,0 +1,179 @@
+/**
+ * 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.collateralmanagement.service;
+
+import com.google.gson.JsonElement;
+import java.math.BigDecimal;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.fineract.infrastructure.core.api.JsonCommand;
+import org.apache.fineract.infrastructure.core.data.ApiParameterError;
+import org.apache.fineract.infrastructure.core.data.CommandProcessingResult;
+import org.apache.fineract.infrastructure.core.data.CommandProcessingResultBuilder;
+import org.apache.fineract.infrastructure.core.data.DataValidatorBuilder;
+import org.apache.fineract.infrastructure.core.exception.PlatformApiDataValidationException;
+import org.apache.fineract.infrastructure.core.serialization.FromJsonHelper;
+import org.apache.fineract.organisation.monetary.domain.ApplicationCurrency;
+import org.apache.fineract.organisation.monetary.domain.ApplicationCurrencyRepository;
+import org.apache.fineract.portfolio.collateralmanagement.api.CollateralAPIConstants;
+import org.apache.fineract.portfolio.collateralmanagement.domain.ClientCollateralManagement;
+import org.apache.fineract.portfolio.collateralmanagement.domain.CollateralManagementDomain;
+import org.apache.fineract.portfolio.collateralmanagement.domain.CollateralManagementRepositoryWrapper;
+import org.apache.fineract.portfolio.collateralmanagement.exception.CollateralCannotBeDeletedException;
+import org.apache.fineract.portfolio.collateralmanagement.exception.CollateralNotFoundException;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+import org.springframework.transaction.annotation.Transactional;
+
+@Service
+public class CollateralManagementWritePlatformServiceImpl implements CollateralManagementWritePlatformService {
+
+    private final CollateralManagementRepositoryWrapper collateralManagementRepositoryWrapper;
+    private final ApplicationCurrencyRepository applicationCurrencyRepository;
+    private final FromJsonHelper fromApiJsonHelper;
+
+    @Autowired
+    public CollateralManagementWritePlatformServiceImpl(final CollateralManagementRepositoryWrapper collateralManagementRepositoryWrapper,
+            final ApplicationCurrencyRepository applicationCurrencyRepository, final FromJsonHelper fromApiJsonHelper) {
+        this.collateralManagementRepositoryWrapper = collateralManagementRepositoryWrapper;
+        this.applicationCurrencyRepository = applicationCurrencyRepository;
+        this.fromApiJsonHelper = fromApiJsonHelper;
+    }
+
+    @Transactional
+    @Override
+    public CommandProcessingResult createCollateral(final JsonCommand jsonCommand) {
+
+        validateForCreation(jsonCommand);
+        final String currencyParamName = jsonCommand
+                .stringValueOfParameterNamed(CollateralAPIConstants.CollateralManagementJSONInputParams.CURRENCY.getValue());
+        final ApplicationCurrency applicationCurrency = this.applicationCurrencyRepository.findOneByCode(currencyParamName);
+
+        final CollateralManagementDomain collateral = CollateralManagementDomain.createNew(jsonCommand, applicationCurrency);
+        this.collateralManagementRepositoryWrapper.create(collateral);
+        return new CommandProcessingResultBuilder().withCommandId(jsonCommand.commandId()).withEntityId(collateral.getId()).build();
+    }
+
+    private void validateForCreation(JsonCommand jsonCommand) {
+        final JsonElement jsonElement = this.fromApiJsonHelper.parse(jsonCommand.json());
+        final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+        final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors).resource("collateral-management");
+
+        if (!jsonCommand.parameterExists("locale")) {
+            baseDataValidator.reset().parameter("locale").notNull().failWithCode("locale.not.exists");
+        } else {
+            final String locale = this.fromApiJsonHelper.extractStringNamed("locale", jsonElement);
+            baseDataValidator.reset().parameter("locale").value(locale).notNull();
+        }
+
+        if (!jsonCommand.parameterExists(CollateralAPIConstants.CollateralManagementJSONInputParams.CURRENCY.getValue())) {
+            baseDataValidator.reset().parameter(CollateralAPIConstants.CollateralManagementJSONInputParams.CURRENCY.getValue()).notNull()
+                    .failWithCode("currency.not.exists");
+        } else {
+            final String currency = this.fromApiJsonHelper.extractStringNamed("currency", jsonElement);
+            baseDataValidator.reset().parameter("currency").value(currency).notNull();
+        }
+
+        if (!jsonCommand.parameterExists("quality")) {
+            baseDataValidator.reset().parameter(CollateralAPIConstants.CollateralManagementJSONInputParams.QUALITY.getValue()).notNull()
+                    .failWithCode("quality.not.exists");
+        } else {
+            final String quality = this.fromApiJsonHelper.extractStringNamed("quality", jsonElement);
+            baseDataValidator.reset().parameter("quality").value(quality).notNull();
+        }
+
+        if (!jsonCommand.parameterExists("basePrice")) {
+            baseDataValidator.reset().parameter("basePrice").notNull().notBlank().failWithCode("basePrice.not.exists");
+        } else {
+            final BigDecimal basePrice = this.fromApiJsonHelper.extractBigDecimalWithLocaleNamed("basePrice", jsonElement);
+            baseDataValidator.reset().parameter("basePrice").value(basePrice).notNull().positiveAmount();
+        }
+
+        if (!jsonCommand.parameterExists("unitType")) {
+            baseDataValidator.reset().parameter("unitType").notNull().notBlank().failWithCode("unitType.not.exists");
+        } else {
+            final String unitType = this.fromApiJsonHelper.extractStringNamed("unitType", jsonElement);
+            baseDataValidator.reset().parameter("unitType").value(unitType).notNull();
+        }
+
+        if (!jsonCommand.parameterExists("pctToBase")) {
+            baseDataValidator.reset().parameter("pctToBase").notNull().notBlank().failWithCode("pctToBase.not.exists");
+        } else {
+            final BigDecimal basePrice = this.fromApiJsonHelper.extractBigDecimalWithLocaleNamed("pctToBase", jsonElement);
+            baseDataValidator.reset().parameter("pctToBase").value(basePrice).notNull().positiveAmount();
+        }
+
+        if (!jsonCommand.parameterExists("name")) {
+            baseDataValidator.reset().parameter("name").notNull().notBlank().failWithCode("name.not.exists");
+        } else {
+            final String unitType = this.fromApiJsonHelper.extractStringNamed("name", jsonElement);
+            baseDataValidator.reset().parameter("name").value(unitType).notNull();
+        }
+
+        if (!dataValidationErrors.isEmpty()) {
+            throw new PlatformApiDataValidationException(dataValidationErrors);
+        }
+    }
+
+    @Transactional
+    @Override
+    public CommandProcessingResult updateCollateral(final Long collateralId, JsonCommand jsonCommand) {
+        final CollateralManagementDomain collateral = this.collateralManagementRepositoryWrapper.getCollateral(collateralId);
+        final String currencyParamName = CollateralAPIConstants.CollateralManagementJSONInputParams.CURRENCY.getValue();
+        final ApplicationCurrency applicationCurrency = this.applicationCurrencyRepository
+                .findOneByCode(jsonCommand.stringValueOfParameterNamed(currencyParamName));
+        if (jsonCommand.isChangeInStringParameterNamed(currencyParamName, applicationCurrency.getCode())) {
+            final String newValue = jsonCommand.stringValueOfParameterNamed(currencyParamName);
+            applicationCurrency.setCode(newValue);
+        }
+        collateral.update(jsonCommand, applicationCurrency);
+        this.collateralManagementRepositoryWrapper.update(collateral);
+        /**
+         * TODO: Return changes.
+         */
+        return new CommandProcessingResultBuilder().withCommandId(jsonCommand.commandId()).withEntityId(jsonCommand.entityId()).build();

Review comment:
       Todo still valid? 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/collateralmanagement/domain/ClientCollateralManagementRepositoryWrapper.java
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.collateralmanagement.domain;
+
+import java.math.BigDecimal;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.fineract.portfolio.client.domain.Client;
+import org.apache.fineract.portfolio.client.domain.ClientRepositoryWrapper;
+import org.apache.fineract.portfolio.collateralmanagement.data.ClientCollateralManagementData;
+import org.apache.fineract.portfolio.collateralmanagement.exception.ClientCollateralNotFoundException;
+import org.apache.fineract.portfolio.loanproduct.domain.LoanProduct;
+import org.apache.fineract.portfolio.loanproduct.domain.LoanProductRepository;
+import org.apache.fineract.portfolio.loanproduct.exception.LoanProductNotFoundException;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+
+@Service
+public class ClientCollateralManagementRepositoryWrapper {
+
+    private final ClientCollateralManagementRepository clientCollateralManagementRepository;
+    private final ClientRepositoryWrapper clientRepositoryWrapper;
+    private final LoanProductRepository loanProductRepository;
+
+    @Autowired
+    public ClientCollateralManagementRepositoryWrapper(final ClientCollateralManagementRepository clientCollateralManagementRepository,
+            final ClientRepositoryWrapper clientRepositoryWrapper, final LoanProductRepository loanProductRepository) {
+        this.clientCollateralManagementRepository = clientCollateralManagementRepository;
+        this.clientRepositoryWrapper = clientRepositoryWrapper;
+        this.loanProductRepository = loanProductRepository;
+    }
+
+    public List<ClientCollateralManagement> getCollateralsPerClient(final Long clientId) {
+        final Client client = this.clientRepositoryWrapper.findOneWithNotFoundDetection(clientId);
+        return this.clientCollateralManagementRepository.findByClientId(client);
+    }
+
+    public List<ClientCollateralManagementData> getClientCollateralData(final Long clientId, final Long prodId) {
+        final Client client = this.clientRepositoryWrapper.findOneWithNotFoundDetection(clientId);
+        String currency = null;
+        if (prodId != null) {
+            final LoanProduct loanProduct = this.loanProductRepository.findById(prodId)
+                    .orElseThrow(() -> new LoanProductNotFoundException(prodId));
+            currency = loanProduct.getCurrency().getCode();
+        }
+        List<ClientCollateralManagement> clientCollateralManagements = this.clientCollateralManagementRepository.findByClientId(client);
+        List<ClientCollateralManagementData> clientCollateralManagementDataSet = new ArrayList<>();
+        for (ClientCollateralManagement clientCollateralManagement : clientCollateralManagements) {
+            BigDecimal quantity = clientCollateralManagement.getQuantity();
+            BigDecimal basePrice = null;
+            BigDecimal pctToBase = null;
+            if (BigDecimal.ZERO.compareTo(quantity) == 0) {
+                basePrice = BigDecimal.ZERO;
+                pctToBase = BigDecimal.ZERO;
+            } else {
+                basePrice = clientCollateralManagement.getCollaterals().getBasePrice();
+                pctToBase = clientCollateralManagement.getCollaterals().getPctToBase().divide(BigDecimal.valueOf(100));
+            }
+            BigDecimal total = quantity.multiply(basePrice);
+            BigDecimal totalCollateralValue = total.multiply(pctToBase);

Review comment:
       Would it make sense to move all this logic (the previous 10 lines or so) to the ClientCollateralManagementData class?

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/client/service/ClientReadPlatformServiceImpl.java
##########
@@ -313,12 +322,28 @@ public ClientData retrieveOne(final Long clientId) {
             final ClientData clientData = this.jdbcTemplate.queryForObject(sql, this.clientMapper,
                     new Object[] { hierarchySearchString, hierarchySearchString, clientId });
 
+            // Get client collaterals
+            final Collection<ClientCollateralManagement> clientCollateralManagements = this.clientCollateralManagementRepositoryWrapper
+                    .getCollateralsPerClient(clientId);
+            final Set<ClientCollateralManagementData> clientCollateralManagementDataSet = new HashSet<>();
+
+            // Map to client collateral data class
+            for (ClientCollateralManagement clientCollateralManagement : clientCollateralManagements) {
+                BigDecimal pctToBase = clientCollateralManagement.getCollaterals().getPctToBase().divide(BigDecimal.valueOf(100));
+                BigDecimal basePrice = clientCollateralManagement.getCollaterals().getBasePrice();
+                BigDecimal quantity = clientCollateralManagement.getQuantity();
+                BigDecimal total = basePrice.multiply(quantity);
+                BigDecimal totalCollateral = total.multiply(pctToBase);
+                clientCollateralManagementDataSet

Review comment:
       If this logic of calculating total and totalCollateral is always the same, why don't we calculate this within the ClientCollateralManagementData class rather than here? 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/collateralmanagement/api/CollateralAPIConstants.java
##########
@@ -0,0 +1,46 @@
+/**
+ * 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.collateralmanagement.api;
+
+public final class CollateralAPIConstants {
+
+    public enum CollateralManagementJSONInputParams {
+

Review comment:
       Naming convention - should be Json not JSON

##########
File path: fineract-provider/src/main/java/org/apache/fineract/commands/service/CommandWrapperBuilder.java
##########
@@ -1814,6 +1822,32 @@ public CommandWrapperBuilder updateCollateral(final Long loanId, final Long coll
         return this;
     }
 
+    public CommandWrapperBuilder updateCollateralProduct(final Long collateralId) {
+        this.actionName = "UPDATE";
+        this.entityName = "COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.href = "/collateral-management/" + collateralId;
+        return this;
+    }
+
+    public CommandWrapperBuilder updateClientCollateralProduct(final Long clientId, final Long collateralId) {
+        this.actionName = "UPDATE";
+        this.entityName = "CLIENT_COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.clientId = clientId;
+        this.href = "/clients/" + clientId + "/collateral" + collateralId;
+        return this;
+    }
+
+    public CommandWrapperBuilder deleteLoanCollateral(final Long loanId, final Long collateralId) {
+        this.actionName = "DELETE";
+        this.entityName = "LOAN_COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.loanId = loanId;
+        this.href = "/loans/" + loanId + "/collateral" + collateralId;
+        return this;

Review comment:
       Same as above

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/collateralmanagement/api/ClientCollateralManagementAPIResource.java
##########
@@ -0,0 +1,203 @@
+/**
+ * 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.collateralmanagement.api;
+
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.ArraySchema;
+import io.swagger.v3.oas.annotations.media.Content;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import io.swagger.v3.oas.annotations.responses.ApiResponse;
+import io.swagger.v3.oas.annotations.responses.ApiResponses;
+import io.swagger.v3.oas.annotations.tags.Tag;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.UriInfo;
+import org.apache.fineract.commands.domain.CommandWrapper;
+import org.apache.fineract.commands.service.CommandWrapperBuilder;
+import org.apache.fineract.commands.service.PortfolioCommandSourceWritePlatformService;
+import org.apache.fineract.infrastructure.codes.service.CodeValueReadPlatformService;
+import org.apache.fineract.infrastructure.core.api.ApiRequestParameterHelper;
+import org.apache.fineract.infrastructure.core.data.CommandProcessingResult;
+import org.apache.fineract.infrastructure.core.serialization.DefaultToApiJsonSerializer;
+import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext;
+import org.apache.fineract.portfolio.collateralmanagement.data.ClientCollateralManagementData;
+import org.apache.fineract.portfolio.collateralmanagement.data.LoanCollateralTemplateData;
+import org.apache.fineract.portfolio.collateralmanagement.domain.ClientCollateralManagement;
+import org.apache.fineract.portfolio.collateralmanagement.service.ClientCollateralManagementReadPlatformService;
+import org.springframework.context.annotation.Scope;
+import org.springframework.stereotype.Component;
+
+@Path("/clients/{clientId}/collaterals")
+@Component
+@Scope("singleton")
+@Tag(name = "Client Collateral Management", description = "Client Collateral Management is for managing collateral operations")
+public class ClientCollateralManagementAPIResource {
+

Review comment:
       Naming convention - should be ManagementApiResource not ManagementAPIResource

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/importhandler/loan/LoanImportHandler.java
##########
@@ -295,6 +300,21 @@ private LoanAccountData readLoan(Row row, String locale, String dateFormat) {
                         ImportHandlerUtils.readAsDate(LoanConstants.CHARGE_DUE_DATE_2, row), null));
             }
         }
+
+        List<LoanCollateralManagementData> loanCollateralManagementData = new ArrayList<>();
+
+        if (collateralId != null) {
+            if (ImportHandlerUtils.readAsDouble(LoanConstants.LOAN_COLLATERAL_QUANTITY, row) != null) {
+                loanCollateralManagementData.add(new LoanCollateralManagementData(collateralId,
+                        BigDecimal.valueOf(ImportHandlerUtils.readAsDouble(LoanConstants.LOAN_COLLATERAL_QUANTITY, row)), null, null,
+                        null));
+            } else {
+                throw new InvalidAmountOfCollateralQuantity(null);
+            }
+        } else {
+            throw new PlatformDataIntegrityException("404", "err.parameter.collateral.not.found");
+        }

Review comment:
       This doesn't seem right. The code at the end of this function says that loanCollateralManagementData is not needed for all types of loans - yet here we throw an exception if collateralId == null. What about those loans that don't need collateral (e.g. loanType.equals("jlg") below)? 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/commands/service/CommandWrapperBuilder.java
##########
@@ -1823,6 +1857,31 @@ public CommandWrapperBuilder deleteCollateral(final Long loanId, final Long coll
         return this;
     }
 
+    public CommandWrapperBuilder deleteCollateralProduct(final Long collateralId) {
+        this.actionName = "DELETE";
+        this.entityName = "COLLATERAL_PRODUCT";
+        this.entityId = collateralId;
+        this.href = "/collateral-management/delete/" + collateralId;
+        return this;

Review comment:
       Do we need the /delete/ in the href... is it not clear from the action that this is a delete? 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/collateralmanagement/api/ClientCollateralManagementAPIResource.java
##########
@@ -0,0 +1,203 @@
+/**
+ * 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.collateralmanagement.api;
+
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.ArraySchema;
+import io.swagger.v3.oas.annotations.media.Content;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import io.swagger.v3.oas.annotations.responses.ApiResponse;
+import io.swagger.v3.oas.annotations.responses.ApiResponses;
+import io.swagger.v3.oas.annotations.tags.Tag;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.UriInfo;
+import org.apache.fineract.commands.domain.CommandWrapper;
+import org.apache.fineract.commands.service.CommandWrapperBuilder;
+import org.apache.fineract.commands.service.PortfolioCommandSourceWritePlatformService;
+import org.apache.fineract.infrastructure.codes.service.CodeValueReadPlatformService;
+import org.apache.fineract.infrastructure.core.api.ApiRequestParameterHelper;
+import org.apache.fineract.infrastructure.core.data.CommandProcessingResult;
+import org.apache.fineract.infrastructure.core.serialization.DefaultToApiJsonSerializer;
+import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext;
+import org.apache.fineract.portfolio.collateralmanagement.data.ClientCollateralManagementData;
+import org.apache.fineract.portfolio.collateralmanagement.data.LoanCollateralTemplateData;
+import org.apache.fineract.portfolio.collateralmanagement.domain.ClientCollateralManagement;
+import org.apache.fineract.portfolio.collateralmanagement.service.ClientCollateralManagementReadPlatformService;
+import org.springframework.context.annotation.Scope;
+import org.springframework.stereotype.Component;
+
+@Path("/clients/{clientId}/collaterals")
+@Component
+@Scope("singleton")
+@Tag(name = "Client Collateral Management", description = "Client Collateral Management is for managing collateral operations")
+public class ClientCollateralManagementAPIResource {
+
+    private final DefaultToApiJsonSerializer<ClientCollateralManagement> apiJsonSerializerService;
+    private final DefaultToApiJsonSerializer<ClientCollateralManagementData> apiJsonSerializerDataService;
+    private final DefaultToApiJsonSerializer<LoanCollateralTemplateData> apiJsonSerializerForLoanCollateralTemplateService;
+    private final ApiRequestParameterHelper apiRequestParameterHelper;
+    private final PortfolioCommandSourceWritePlatformService commandsSourceWritePlatformService;
+    private final PlatformSecurityContext context;
+    private final CodeValueReadPlatformService codeValueReadPlatformService;
+    private final ClientCollateralManagementReadPlatformService clientCollateralManagementReadPlatformService;
+    private static final Set<String> RESPONSE_DATA_PARAMETERS = new HashSet<>(
+            Arrays.asList("name", "quantity", "total", "totalCollateral", "clientId", "loanTransactionData"));
+
+    public ClientCollateralManagementAPIResource(final DefaultToApiJsonSerializer<ClientCollateralManagement> apiJsonSerializerService,
+            final ApiRequestParameterHelper apiRequestParameterHelper,
+            final PortfolioCommandSourceWritePlatformService commandsSourceWritePlatformService, final PlatformSecurityContext context,
+            final CodeValueReadPlatformService codeValueReadPlatformService,
+            final ClientCollateralManagementReadPlatformService clientCollateralManagementReadPlatformService,
+            final DefaultToApiJsonSerializer<ClientCollateralManagementData> apiJsonSerializerDataService,
+            final DefaultToApiJsonSerializer<LoanCollateralTemplateData> apiJsonSerializerForLoanCollateralTemplateService) {
+        this.apiJsonSerializerService = apiJsonSerializerService;
+        this.apiRequestParameterHelper = apiRequestParameterHelper;
+        this.commandsSourceWritePlatformService = commandsSourceWritePlatformService;
+        this.context = context;
+        this.codeValueReadPlatformService = codeValueReadPlatformService;
+        this.clientCollateralManagementReadPlatformService = clientCollateralManagementReadPlatformService;
+        this.apiJsonSerializerDataService = apiJsonSerializerDataService;
+        this.apiJsonSerializerForLoanCollateralTemplateService = apiJsonSerializerForLoanCollateralTemplateService;
+    }
+
+    @GET
+    @Produces({ MediaType.APPLICATION_JSON })
+    @Consumes({ MediaType.APPLICATION_JSON })
+    @Operation(summary = "Get Clients Collateral Products", description = "Get Collateral Product of a Client")
+    @ApiResponses({
+            @ApiResponse(responseCode = "200", description = "OK", content = @Content(array = @ArraySchema(schema = @Schema(implementation = ClientCollateralManagementAPIResourceSwagger.GetClientCollateralManagementsResponse.class)))) })
+    public String getClientCollateral(@PathParam("clientId") @Parameter(description = "clientId") final Long clientId,
+            @Context final UriInfo uriInfo, @QueryParam("prodId") @Parameter(description = "prodId") final Long prodId) {
+
+        this.context.authenticatedUser().validateHasReadPermission(
+                CollateralAPIConstants.CollateralManagementJSONInputParams.CLIENT_COLLATERAL_PRODUCT_READ_PERMISSION.getValue());
+
+        List<ClientCollateralManagementData> collateralProductList = null;
+
+        if (prodId != null) {
+            collateralProductList = this.clientCollateralManagementReadPlatformService.getClientCollaterals(clientId, prodId);
+        } else {
+            collateralProductList = this.clientCollateralManagementReadPlatformService.getClientCollaterals(clientId, null);
+        }

Review comment:
       Why do we need this if... why not just pass the prodId as second parameter? If it's null then you are passing null

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/collateralmanagement/api/CollateralManagementAPIResourceSwagger.java
##########
@@ -0,0 +1,159 @@
+/**
+ * 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.collateralmanagement.api;
+
+import io.swagger.v3.oas.annotations.media.Schema;
+import java.math.BigDecimal;
+
+final class CollateralManagementAPIResourceSwagger {
+

Review comment:
       Api not API

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanAccountDomainServiceJpa.java
##########
@@ -524,6 +545,20 @@ public void recalculateAccruals(Loan loan, boolean isInterestCalcualtionHappened
                 throw new GeneralPlatformDomainRuleException(globalisationMessageCode, e.getMessage(), e);
             }
         }
+
+        // Update loan transaction on repayment.

Review comment:
       Is this code in the right function? It's currently in recalculateAccruals, which is called for many reasons e.g. when making charge payments. Shouldn't the collateral be released only once the loan is paid off? 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanApplicationCommandFromApiJsonHelper.java
##########
@@ -847,38 +871,67 @@ public void validateForModify(final String json, final LoanProduct loanProduct,
             }
         }
 
-        // collateral
-        final String collateralParameterName = "collateral";
-        if (element.isJsonObject() && this.fromApiJsonHelper.parameterExists(collateralParameterName, element)) {
-            final JsonObject topLevelJsonElement = element.getAsJsonObject();
-            final Locale locale = this.fromApiJsonHelper.extractLocaleParameter(topLevelJsonElement);
-            if (topLevelJsonElement.get("collateral").isJsonArray()) {
-
-                final Type collateralParameterTypeOfMap = new TypeToken<Map<String, Object>>() {}.getType();
-                final Set<String> supportedParameters = new HashSet<>(Arrays.asList("id", "type", "value", "description"));
-                final JsonArray array = topLevelJsonElement.get("collateral").getAsJsonArray();
-                for (int i = 1; i <= array.size(); i++) {
-                    final JsonObject collateralItemElement = array.get(i - 1).getAsJsonObject();
-
-                    final String collateralJson = this.fromApiJsonHelper.toJson(collateralItemElement);
-                    this.fromApiJsonHelper.checkForUnsupportedParameters(collateralParameterTypeOfMap, collateralJson, supportedParameters);
-
-                    final Long collateralTypeId = this.fromApiJsonHelper.extractLongNamed("type", collateralItemElement);
-                    baseDataValidator.reset().parameter("collateral").parameterAtIndexArray("type", i).value(collateralTypeId).notNull()
-                            .integerGreaterThanZero();
-
-                    final BigDecimal collateralValue = this.fromApiJsonHelper.extractBigDecimalNamed("value", collateralItemElement,
-                            locale);
-                    baseDataValidator.reset().parameter("collateral").parameterAtIndexArray("value", i).value(collateralValue)
-                            .ignoreIfNull().positiveAmount();
+        final String loanTypeParameterName = "loanType";
+        final String loanTypeStr = this.fromApiJsonHelper.extractStringNamed(loanTypeParameterName, element);
+        baseDataValidator.reset().parameter(loanTypeParameterName).value(loanTypeStr).notNull();
 
-                    final String description = this.fromApiJsonHelper.extractStringNamed("description", collateralItemElement);
-                    baseDataValidator.reset().parameter("collateral").parameterAtIndexArray("description", i).value(description).notBlank()
-                            .notExceedingLengthOf(500);
+        if (!StringUtils.isBlank(loanTypeStr)) {
+            final AccountType loanType = AccountType.fromName(loanTypeStr);
+            baseDataValidator.reset().parameter(loanTypeParameterName).value(loanType.getValue()).inMinMaxRange(1, 4);

Review comment:
       I don't think we should be comparing against hardcoded range... you can use e.g. AccountNumerations.loanType() and check if the result is INVALID...  or baseDataValidator.isOneOfEnumValues()

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanApplicationWritePlatformServiceJpaRepositoryImpl.java
##########
@@ -627,6 +636,29 @@ private void checkForProductMixRestrictions(final List<Long> activeLoansLoanProd
         }
     }
 
+    // private void updateLoanCollateral(Set<LoanCollateralManagement> loanCollateralManagementSet) {
+    // this.loanCollateralManagementRepository.saveAll(loanCollateralManagementSet);
+    // }
+    //
+    // private void updateClientCollaterals(Set<ClientCollateralManagement> clientCollateralManagementSet) {
+    // this.clientCollateralManagementRepository.saveAll(clientCollateralManagementSet);
+    // }
+    //
+    // private boolean isLoanCollateralUpdatable(JsonCommand command) {
+    // boolean isExist = this.fromJsonHelper.parameterExists("collaterals", command.parsedJson());
+    // if (!isExist) {
+    // return false;
+    // }
+    //
+    // int length = this.fromJsonHelper.extractJsonArrayNamed("collaterals", command.parsedJson()).size();
+    //
+    // if (length == 0) {
+    // return false;
+    // }

Review comment:
       Should this code be commented out?

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java
##########
@@ -494,10 +498,14 @@ private Loan(final String accountNo, final Client client, final Group group, fin
             this.charges = null;
             this.summary = new LoanSummary();
         }
-        if (collateral != null && !collateral.isEmpty()) {
-            this.collateral = associateWithThisLoan(collateral);
+
+        /**
+         * TODO: Apply for other loan type if required.
+         */
+        if (loanType.equals(1) && collateral != null && !collateral.isEmpty()) {

Review comment:
       Todo still valid?




-- 
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