[GitHub] [fineract] galovics commented on a diff in pull request #2758: FINERACT-1760: Enhanced external id support for loan transactions

[GitBox <gi...@apache.org>]

galovics commented on code in PR #2758:
URL: https://github.com/apache/fineract/pull/2758#discussion_r1031431071


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/domain/ExternalId.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * 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.infrastructure.core.domain;
+
+import java.io.Serial;
+import java.io.Serializable;
+import java.util.UUID;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import org.apache.fineract.infrastructure.core.exception.PlatformInternalServerException;
+
+/**
+ * ExternalId Value object
+ */
+@Getter
+@EqualsAndHashCode
+@Builder
+@AllArgsConstructor

Review Comment:
   Instead of the generated constructor, we could define the invariant for this class as following:
   - it can have a null value associated, that means an empty ExternalId
   - if the value is not null, it should not be blank.
   
   Also, I'd create 2 constructors. One for internal use to create the empty ExternalId reference, and one for checking whether the incoming value is not blank
   
   ```
   public ExternalId(String value) {
       if (StringUtils.isBlank(value)) {
           throw new IllegalArgumentException(...)
       }
       this.value = value;
   }
   
   private ExternalId() {
       this.value = null;
   }
   ```
   
   Something like this. Thoughts?



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/domain/ExternalId.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * 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.infrastructure.core.domain;
+
+import java.io.Serial;
+import java.io.Serializable;
+import java.util.UUID;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import org.apache.fineract.infrastructure.core.exception.PlatformInternalServerException;
+
+/**
+ * ExternalId Value object
+ */
+@Getter
+@EqualsAndHashCode
+@Builder
+@AllArgsConstructor
+public class ExternalId implements Serializable {
+
+    @Serial
+    private static final long serialVersionUID = 1;
+    private static final ExternalId empty = new ExternalId(null);
+    private final String value;
+
+    /**
+     * @return Create a new ExternalId object where value is a newly generated UUID
+     */
+    public static ExternalId generate() {
+        return new ExternalId(UUID.randomUUID().toString());
+    }
+
+    /**
+     * @return Create and return an empty ExternalId object
+     */
+    public static ExternalId empty() {
+        return empty;
+    }
+
+    /**
+     * @return whether value was set for the ExternalId object (return false if value is null)
+     */
+    public boolean isSet() {
+        return value != null;
+    }
+
+    /**
+     * Throws exception if value was not set (value is null) for this object
+     *
+     * @throws PlatformInternalServerException
+     *             if value was not set (value is null) for this object
+     */
+    public void throwErrorIfItsNotSet() {
+        if (!isSet()) {
+            throw new PlatformInternalServerException("error.external.id.is.not.set", "Internal state violation: External id is not set");
+        }
+    }
+
+    @Override
+    public String toString() {

Review Comment:
   @ToString lombok annotation?



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/domain/ExternalId.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * 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.infrastructure.core.domain;
+
+import java.io.Serial;
+import java.io.Serializable;
+import java.util.UUID;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import org.apache.fineract.infrastructure.core.exception.PlatformInternalServerException;
+
+/**
+ * ExternalId Value object
+ */
+@Getter
+@EqualsAndHashCode
+@Builder

Review Comment:
   I'd say let's not allow a Builder for a value object. Right now it's not an issue but in the future if a value object includes more than 1 field, it could end up in an inconsistent state where all the invariants are not fulfilled. (For example think about a client's name. It'll consist of a firstname and a lastname. And that'll be an invariant for the ClientName value object to have a firstname and a lastname. With the builder, we can't enforce to have those 2 values set, only with a constructor)



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/domain/ExternalId.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * 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.infrastructure.core.domain;
+
+import java.io.Serial;
+import java.io.Serializable;
+import java.util.UUID;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import org.apache.fineract.infrastructure.core.exception.PlatformInternalServerException;
+
+/**
+ * ExternalId Value object
+ */
+@Getter
+@EqualsAndHashCode
+@Builder
+@AllArgsConstructor
+public class ExternalId implements Serializable {
+
+    @Serial
+    private static final long serialVersionUID = 1;
+    private static final ExternalId empty = new ExternalId(null);
+    private final String value;
+
+    /**
+     * @return Create a new ExternalId object where value is a newly generated UUID
+     */
+    public static ExternalId generate() {
+        return new ExternalId(UUID.randomUUID().toString());
+    }
+
+    /**
+     * @return Create and return an empty ExternalId object
+     */
+    public static ExternalId empty() {
+        return empty;
+    }
+
+    /**
+     * @return whether value was set for the ExternalId object (return false if value is null)
+     */
+    public boolean isSet() {
+        return value != null;
+    }
+
+    /**
+     * Throws exception if value was not set (value is null) for this object
+     *
+     * @throws PlatformInternalServerException
+     *             if value was not set (value is null) for this object
+     */
+    public void throwErrorIfItsNotSet() {

Review Comment:
   Not sure I'd put this here but I can live with it. Naming-wise probably we can just rename it to `throwExceptionIfEmpty` and the `isSet` could be also renamed to `isEmpty` to be consistent with the naming for an empty ExternalId (`empty` method)



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/service/serialization/mapper/loan/LoanTransactionDataMapper.java:
##########
@@ -22,9 +22,11 @@
 import org.apache.fineract.infrastructure.event.external.service.serialization.mapper.support.AvroMapperConfig;
 import org.apache.fineract.portfolio.loanaccount.data.LoanTransactionData;
 import org.mapstruct.Mapper;
+import org.mapstruct.Mapping;
 
 @Mapper(config = AvroMapperConfig.class)
 public interface LoanTransactionDataMapper {
 
+    @Mapping(target = "externalId", source = "externalId.value")

Review Comment:
   Same as above.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:
##########
@@ -421,15 +436,21 @@ public Long transferFunds(final AccountTransferDTO accountTransferDTO) {
                 this.savingsAccountAssembler.setHelpers(toSavingsAccount);
             }
             LoanTransaction loanTransaction = null;
+
+            ExternalId externalId = accountTransferDTO.getTxnExternalId();
+            if ((externalId == null || !externalId.isSet()) && configurationDomainService.isExternalIdAutoGenerationEnabled()) {

Review Comment:
   Same as above.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:
##########
@@ -474,13 +495,22 @@ public AccountTransferDetails repayLoanWithTopup(AccountTransferDTO accountTrans
             this.loanAccountAssembler.setHelpers(toLoanAccount);
         }
 
+        ExternalId externalIdForDisbursement = accountTransferDTO.getTxnExternalId();
+        ExternalId externalIdForRepayment = ExternalId.empty();
+        if ((externalIdForDisbursement == null || !externalIdForDisbursement.isSet())

Review Comment:
   Same as above.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanReadPlatformService.java:
##########
@@ -155,4 +156,7 @@ LoanScheduleData retrieveRepaymentSchedule(Long loanId, RepaymentScheduleRelated
 
     List<LoanTransactionRelationData> retrieveLoanTransactionRelationsByLoanTransactionId(Long loanTransactionId);
 
+    Long retrieveLoanTransactionIdByExternalId(ExternalId externalId);
+
+    Long retrieveLoanIdByExternalId(String externalId);

Review Comment:
   Any reason this externalId is a string?



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:
##########
@@ -329,12 +339,17 @@ public Long transferFunds(final AccountTransferDTO accountTransferDTO) {
                     accountTransferDTO.getFmt(), accountTransferDTO.getTransactionDate(), accountTransferDTO.getTransactionAmount(),
                     accountTransferDTO.getPaymentDetail(), transactionBooleanValues, backdatedTxnsAllowedTill);
 
-            LoanTransaction loanTransaction = null;
+            LoanTransaction loanTransaction;
+
+            ExternalId externalId = accountTransferDTO.getTxnExternalId();
+            if ((externalId == null || !externalId.isSet()) && configurationDomainService.isExternalIdAutoGenerationEnabled()) {

Review Comment:
   Since the ExternalId is a value object and it can represent an empty ExternalId. Can we skip the nullcheck completely and make sure the getTxnExternalId actually returns an instance?



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/service/serialization/mapper/loan/LoanAccountDataMapper.java:
##########
@@ -33,4 +35,7 @@ public interface LoanAccountDataMapper {
     @Mapping(source = "topup", target = "isTopup")
     @Mapping(source = "interestRecalculationEnabled", target = "isInterestRecalculationEnabled")
     LoanAccountDataV1 map(LoanAccountData source);
+
+    @Mapping(target = "externalId", source = "externalId.value")

Review Comment:
   You can do it generally as well. Create a Mapper just for the ExternalId -> String mapping and add it to the `AvroMapperConfig`, just like I did with the `AvroDateTimeMapper`.



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