You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "adamsaghy (via GitHub)" <gi...@apache.org> on 2024/05/29 20:05:25 UTC
[PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
adamsaghy opened a new pull request, #3909:
URL: https://github.com/apache/fineract/pull/3909
## Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update unit or integration tests for verifying the changes made.
- [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3909:
URL: https://github.com/apache/fineract/pull/3909#discussion_r1629405267
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanDisbursementDetailsAssembler.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * 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.loanaccount.service;
+
+import com.google.gson.JsonArray;
+import com.google.gson.JsonObject;
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Locale;
+import lombok.RequiredArgsConstructor;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.fineract.infrastructure.core.serialization.FromJsonHelper;
+import org.apache.fineract.portfolio.loanaccount.api.LoanApiConstants;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanDisbursementDetails;
+
+@RequiredArgsConstructor
+public class LoanDisbursementDetailsAssembler {
+
+ private final FromJsonHelper fromApiJsonHelper;
+
+ public List<LoanDisbursementDetails> fetchDisbursementData(final JsonObject command) {
+ final Locale locale = this.fromApiJsonHelper.extractLocaleParameter(command);
+ final String dateFormat = this.fromApiJsonHelper.extractDateFormatParameter(command);
+ List<LoanDisbursementDetails> disbursementDatas = new ArrayList<>();
+ if (command.has(LoanApiConstants.disbursementDataParameterName)) {
+ final JsonArray disbursementDataArray = command.getAsJsonArray(LoanApiConstants.disbursementDataParameterName);
+ if (disbursementDataArray != null && !disbursementDataArray.isEmpty()) {
+ int i = 0;
Review Comment:
Yes, LGTM. I am happy to add your changes as part of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3909:
URL: https://github.com/apache/fineract/pull/3909#discussion_r1629405267
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanDisbursementDetailsAssembler.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * 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.loanaccount.service;
+
+import com.google.gson.JsonArray;
+import com.google.gson.JsonObject;
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Locale;
+import lombok.RequiredArgsConstructor;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.fineract.infrastructure.core.serialization.FromJsonHelper;
+import org.apache.fineract.portfolio.loanaccount.api.LoanApiConstants;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanDisbursementDetails;
+
+@RequiredArgsConstructor
+public class LoanDisbursementDetailsAssembler {
+
+ private final FromJsonHelper fromApiJsonHelper;
+
+ public List<LoanDisbursementDetails> fetchDisbursementData(final JsonObject command) {
+ final Locale locale = this.fromApiJsonHelper.extractLocaleParameter(command);
+ final String dateFormat = this.fromApiJsonHelper.extractDateFormatParameter(command);
+ List<LoanDisbursementDetails> disbursementDatas = new ArrayList<>();
+ if (command.has(LoanApiConstants.disbursementDataParameterName)) {
+ final JsonArray disbursementDataArray = command.getAsJsonArray(LoanApiConstants.disbursementDataParameterName);
+ if (disbursementDataArray != null && !disbursementDataArray.isEmpty()) {
+ int i = 0;
Review Comment:
Yes, LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on code in PR #3909:
URL: https://github.com/apache/fineract/pull/3909#discussion_r1629802832
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/mapper/LoanChargeMapper.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * 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.loanaccount.mapper;
+
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.fineract.infrastructure.core.config.MapstructMapperConfig;
+import org.apache.fineract.organisation.monetary.domain.MonetaryCurrency;
+import org.apache.fineract.portfolio.loanaccount.command.LoanChargeCommand;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanCharge;
+import org.mapstruct.Mapper;
+import org.mapstruct.Mapping;
+
+@Mapper(config = MapstructMapperConfig.class)
+public interface LoanChargeMapper {
+
+ @Mapping(target = "id", source = "loanCharge.id")
+ @Mapping(target = "chargeId", source = "loanCharge.charge.id")
+ @Mapping(target = "amount", expression = "java(loanCharge.getAmount(currency).getAmount())")
+ @Mapping(target = "chargeTimeType", source = "loanCharge.chargeTimeType.value")
+ @Mapping(target = "chargeCalculationType", source = "loanCharge.chargeCalculation.value")
+ @Mapping(target = "dueDate", source = "loanCharge.dueDate")
+ LoanChargeCommand map(LoanCharge loanCharge, MonetaryCurrency currency);
+
+ default Set<LoanChargeCommand> map(Set<LoanCharge> loanCharge, MonetaryCurrency currency) {
Review Comment:
Great! Mapstruct... I think you could get away without a default function implementation and just provide:
```
Set<LoanChargeCommand> map(Set<LoanCharge> loanCharge, MonetaryCurrency currency);
```
I think Mapstruct is smart enough to create automatically a for loop and use the single value implementation on each element; with just the parameter "loanCharge" I'd be 100% sure, with the additional currency parameter I'd say 90%.
Minor issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "kjozsa (via GitHub)" <gi...@apache.org>.
kjozsa commented on code in PR #3909:
URL: https://github.com/apache/fineract/pull/3909#discussion_r1624569729
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanDisbursementDetailsAssembler.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * 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.loanaccount.service;
+
+import com.google.gson.JsonArray;
+import com.google.gson.JsonObject;
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Locale;
+import lombok.RequiredArgsConstructor;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.fineract.infrastructure.core.serialization.FromJsonHelper;
+import org.apache.fineract.portfolio.loanaccount.api.LoanApiConstants;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanDisbursementDetails;
+
+@RequiredArgsConstructor
+public class LoanDisbursementDetailsAssembler {
+
+ private final FromJsonHelper fromApiJsonHelper;
+
+ public List<LoanDisbursementDetails> fetchDisbursementData(final JsonObject command) {
+ final Locale locale = this.fromApiJsonHelper.extractLocaleParameter(command);
+ final String dateFormat = this.fromApiJsonHelper.extractDateFormatParameter(command);
+ List<LoanDisbursementDetails> disbursementDatas = new ArrayList<>();
+ if (command.has(LoanApiConstants.disbursementDataParameterName)) {
+ final JsonArray disbursementDataArray = command.getAsJsonArray(LoanApiConstants.disbursementDataParameterName);
+ if (disbursementDataArray != null && !disbursementDataArray.isEmpty()) {
+ int i = 0;
Review Comment:
@adamsaghy I made an attempt to improve this code slightly further.
Please check my commit at https://github.com/apache/fineract/commit/d544a35b6ae0172808bc63c396402bd4dcdd07ae and tell me if you wish to have a PR of this version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3909:
URL: https://github.com/apache/fineract/pull/3909#discussion_r1629887892
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/mapper/LoanChargeMapper.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * 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.loanaccount.mapper;
+
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.fineract.infrastructure.core.config.MapstructMapperConfig;
+import org.apache.fineract.organisation.monetary.domain.MonetaryCurrency;
+import org.apache.fineract.portfolio.loanaccount.command.LoanChargeCommand;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanCharge;
+import org.mapstruct.Mapper;
+import org.mapstruct.Mapping;
+
+@Mapper(config = MapstructMapperConfig.class)
+public interface LoanChargeMapper {
+
+ @Mapping(target = "id", source = "loanCharge.id")
+ @Mapping(target = "chargeId", source = "loanCharge.charge.id")
+ @Mapping(target = "amount", expression = "java(loanCharge.getAmount(currency).getAmount())")
+ @Mapping(target = "chargeTimeType", source = "loanCharge.chargeTimeType.value")
+ @Mapping(target = "chargeCalculationType", source = "loanCharge.chargeCalculation.value")
+ @Mapping(target = "dueDate", source = "loanCharge.dueDate")
+ LoanChargeCommand map(LoanCharge loanCharge, MonetaryCurrency currency);
+
+ default Set<LoanChargeCommand> map(Set<LoanCharge> loanCharge, MonetaryCurrency currency) {
Review Comment:
I was getting the following exception:
```
error: The return type Set<LoanChargeCommand> is an abstract class or interface. Provide a non abstract / non interface result type or a factory method.
Set<LoanChargeCommand> map(Set<LoanCharge> loanCharge, MonetaryCurrency currency);
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on code in PR #3909:
URL: https://github.com/apache/fineract/pull/3909#discussion_r1629794928
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/command/LoanChargeCommand.java:
##########
@@ -18,44 +18,26 @@
*/
package org.apache.fineract.portfolio.loanaccount.command;
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import java.io.Serializable;
import java.math.BigDecimal;
import java.time.LocalDate;
+import lombok.Data;
import org.apache.fineract.portfolio.loanaccount.domain.LoanCharge;
/**
* Java object representation of {@link LoanCharge} API JSON.
*/
-public class LoanChargeCommand implements Comparable<LoanChargeCommand> {
+@Data
+public class LoanChargeCommand implements Serializable {
Review Comment:
... minor: technically needs this
```
@Serial
private static final long serialVersionUID = 1L;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #3909:
URL: https://github.com/apache/fineract/pull/3909
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "SavvyPabi (via GitHub)" <gi...@apache.org>.
SavvyPabi commented on PR #3909:
URL: https://github.com/apache/fineract/pull/3909#issuecomment-3873718396
Hi @adamsaghy @vidakovic
Great work on this restructure! Now that the foundational cleanup for loan creation/modification is merged, I wanted to share a Product Perspective on the logical next steps for the validation layer.
To build on this new structure, I suggest we look at standardizing our Business Validation & Rejection logic:
Standardized Reason Codes: Moving from generic strings to specific codes, so front-end apps can provide clearer user guidance.
Audit Persistence: Ensuring that "Rejected" or "Draft" attempts are logged in the audit trail rather than discarded—a major requirement for banking compliance.
Tiered Validation: Implementing a "fail-fast" approach (Technical/Schema checks first, then Business Policy) to optimize performance for high-volume environments.
I'm happy to help document a set of Standardized Business Reason Codes that align with international microfinance standards if that helps the post-refactor roadmap!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #3909:
URL: https://github.com/apache/fineract/pull/3909#issuecomment-2152809122
@taskain7 @vidakovic @kjozsa @ruchiD @alberto-art3ch Please as your time lets you give me a review. I know this is a monster PR, but it was unfortunately required to have a clearer restructure...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2090: Restructure Loan creation and modification [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3909:
URL: https://github.com/apache/fineract/pull/3909#discussion_r1629893918
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/mapper/LoanChargeMapper.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * 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.loanaccount.mapper;
+
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.fineract.infrastructure.core.config.MapstructMapperConfig;
+import org.apache.fineract.organisation.monetary.domain.MonetaryCurrency;
+import org.apache.fineract.portfolio.loanaccount.command.LoanChargeCommand;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanCharge;
+import org.mapstruct.Mapper;
+import org.mapstruct.Mapping;
+
+@Mapper(config = MapstructMapperConfig.class)
+public interface LoanChargeMapper {
+
+ @Mapping(target = "id", source = "loanCharge.id")
+ @Mapping(target = "chargeId", source = "loanCharge.charge.id")
+ @Mapping(target = "amount", expression = "java(loanCharge.getAmount(currency).getAmount())")
+ @Mapping(target = "chargeTimeType", source = "loanCharge.chargeTimeType.value")
+ @Mapping(target = "chargeCalculationType", source = "loanCharge.chargeCalculation.value")
+ @Mapping(target = "dueDate", source = "loanCharge.dueDate")
+ LoanChargeCommand map(LoanCharge loanCharge, MonetaryCurrency currency);
+
+ default Set<LoanChargeCommand> map(Set<LoanCharge> loanCharge, MonetaryCurrency currency) {
Review Comment:
If i changed to return HashSet, it generated a dummy method:
```
@Override
public HashSet<LoanChargeCommand> map(Set<LoanCharge> loanCharge, MonetaryCurrency currency) {
if ( loanCharge == null && currency == null ) {
return null;
}
HashSet<LoanChargeCommand> hashSet = new HashSet<LoanChargeCommand>();
return hashSet;
}
--
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