[GitHub] [fineract] galovics commented on a diff in pull request #2190: FINERACT-1431:ClientIdentifierPassportCountries

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

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


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/api/CodeValuesApiResource.java:
##########
@@ -104,7 +109,11 @@ public String retrieveAllCodeValues(@Context final UriInfo uriInfo,
 
         this.context.authenticatedUser().validateHasReadPermission(this.resourceNameForPermissions);
 
-        final Collection<CodeValueData> codeValues = this.readPlatformService.retrieveAllCodeValues(codeId);
+        Collection<CodeValueData> codeValues = this.readPlatformService.retrieveAllCodeValues(codeId);

Review Comment:
   So this is a question. The retrieveAllCodeValues method is not returning the resolved country list but the retrieveCodeValue doesn't which is strange. Both should behave the same way for code values.
   
   But rather, a step back. Do we want to add this extra behavior to retrieving the code values? Isn't it possible that API users/the UI will need the unresolved country codes?
   
   I'd rather think of this as a brand new set of APIs (getting the list of supported countries (in a resolved form along with their country codes, and a single one). That way we don't need to add this magic behavior to the code value APIs and everybody will know what to expect. 



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/api/CodeValuesApiResource.java:
##########
@@ -80,16 +82,19 @@ public class CodeValuesApiResource {
     private final DefaultToApiJsonSerializer<CodeValueData> toApiJsonSerializer;
     private final ApiRequestParameterHelper apiRequestParameterHelper;
     private final PortfolioCommandSourceWritePlatformService commandsSourceWritePlatformService;
+    private final CountryNamesTranslationService countryNamesTranslationService;
 
     @Autowired
     public CodeValuesApiResource(final PlatformSecurityContext context, final CodeValueReadPlatformService readPlatformService,

Review Comment:
   Minor thing, if you're already touching the constructor, you could switch it to a Lombok constructor (`@RequiredArgsConstructor`).



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CountryNamesTranslationService.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.infrastructure.codes.service;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.ResourceBundle;
+import org.apache.fineract.infrastructure.codes.data.CodeValueData;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.PropertySource;
+import org.springframework.stereotype.Service;
+
+@Service
+@Configuration
+@PropertySource("classpath:countries_en.properties")
+@ConfigurationProperties
+@EnableAutoConfiguration
+public class CountryNamesTranslationService {
+
+    private static final Logger LOG = LoggerFactory.getLogger(CountryNamesTranslationService.class);

Review Comment:
   For logging, please use the `@Slf4j` annotation.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/command/ClientIdentifierCommand.java:
##########
@@ -20,25 +20,32 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.fineract.infrastructure.codes.domain.CodeValue;
+import org.apache.fineract.infrastructure.core.api.JsonCommand;
 import org.apache.fineract.infrastructure.core.data.ApiParameterError;
 import org.apache.fineract.infrastructure.core.data.DataValidatorBuilder;
 import org.apache.fineract.infrastructure.core.exception.PlatformApiDataValidationException;
+import org.apache.fineract.infrastructure.core.exception.PlatformDataIntegrityException;
+import org.apache.fineract.portfolio.client.api.ClientApiConstants;
 
 /**
  * Immutable command for creating or updating details of a client identifier.
  */
 public class ClientIdentifierCommand {
 
     private final Long documentTypeId;
+    private final String documentIssueCountryCode;
     private final String documentKey;
     private final String description;
     private final String status;
 
-    public ClientIdentifierCommand(final Long documentTypeId, final String documentKey, final String statusString,
-            final String description) {
+    public ClientIdentifierCommand(final Long documentTypeId, final String documentIssueCountryCode, final String documentKey,

Review Comment:
   Could be replaced with a `@RequiredArgsConstructor`



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CountryNamesTranslationService.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.infrastructure.codes.service;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.ResourceBundle;
+import org.apache.fineract.infrastructure.codes.data.CodeValueData;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.PropertySource;
+import org.springframework.stereotype.Service;
+
+@Service
+@Configuration
+@PropertySource("classpath:countries_en.properties")
+@ConfigurationProperties
+@EnableAutoConfiguration
+public class CountryNamesTranslationService {
+
+    private static final Logger LOG = LoggerFactory.getLogger(CountryNamesTranslationService.class);
+
+    @Bean
+    public Collection<CodeValueData> getCountriesList() {

Review Comment:
   Using the CodeValueData DTO for this purpose I think is the wrong approach because we're trying to push in the concept of resolved data whereas the model doesn't support it.



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CountryNamesTranslationService.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.infrastructure.codes.service;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.ResourceBundle;
+import org.apache.fineract.infrastructure.codes.data.CodeValueData;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.PropertySource;
+import org.springframework.stereotype.Service;
+
+@Service
+@Configuration

Review Comment:
   Why the Configuration annotation?



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CountryNamesTranslationService.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.infrastructure.codes.service;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.ResourceBundle;
+import org.apache.fineract.infrastructure.codes.data.CodeValueData;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.PropertySource;
+import org.springframework.stereotype.Service;
+
+@Service
+@Configuration
+@PropertySource("classpath:countries_en.properties")
+@ConfigurationProperties
+@EnableAutoConfiguration

Review Comment:
   Why the EnableAutoConfiguration annotation?



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/command/ClientIdentifierCommand.java:
##########
@@ -50,18 +57,29 @@ public String getDocumentKey() {
         return this.documentKey;
     }
 
+    public String getdocumentIssueCountryCode() {

Review Comment:
   Simply create the getters using the Lombok `@Getter` annotation



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CountryNamesTranslationService.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.infrastructure.codes.service;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.ResourceBundle;
+import org.apache.fineract.infrastructure.codes.data.CodeValueData;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.PropertySource;
+import org.springframework.stereotype.Service;
+
+@Service
+@Configuration
+@PropertySource("classpath:countries_en.properties")
+@ConfigurationProperties
+@EnableAutoConfiguration
+public class CountryNamesTranslationService {

Review Comment:
   Here's the approach I'd take, sorry for the wall of text ahead.
   
   - One interface for example named CountryService
       - A method for example called getCountryList that returns all the countries supported in the system
       - A method for example called getCountryByCode that returns a single country
       - The "returned country" simply is a DTO, let's call it CountryDTO for simplicity. It has 2 fields, countryCode and countryName
       - Also, both the methods on the interface must accept a parameter called locale/language that represents in which language the user wants to have the country code resolved
   - Implementation-wise, we'll have a CountryServiceImpl
       - It'll retrieve the appropriate country code(s)
       - Using the country code(s), it resolves the proper country name according to the locale that was passed along the method from the properties file that was already pre-loaded upon startup
       - For pre-loading, you could simply use the InitializableBean interface that'll give you the opportunity to do things after the Spring context has been built and store the mappings in a simple Map<Locale, Set<String>> or similar to that



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/serialization/ClientIdentifierCommandFromApiJsonDeserializer.java:
##########
@@ -43,7 +43,8 @@ public final class ClientIdentifierCommandFromApiJsonDeserializer extends Abstra
     /**
      * The parameters supported for this command.
      */
-    private final Set<String> supportedParameters = new HashSet<>(Arrays.asList("documentTypeId", "documentKey", "status", "description"));
+    private final Set<String> supportedParameters = new HashSet<>(

Review Comment:
   Why not replace it with `Sets#of`?



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/service/ClientIdentifierReadPlatformServiceImpl.java:
##########
@@ -103,7 +106,14 @@ public ClientIdentifierData mapRow(final ResultSet rs, @SuppressWarnings("unused
             final String documentTypeName = rs.getString("documentType");
             final CodeValueData documentType = CodeValueData.instance(documentTypeId, documentTypeName);
             final String status = ClientIdentifierStatus.fromInt(rs.getInt("status")).getCode();
-            return ClientIdentifierData.singleItem(id, clientId, documentType, documentKey, status, description);
+
+            String documentIssueCountry = null;
+            if (documentTypeName.equals(ClientApiConstants.clientIdentifierPassportParamName)) {
+                Map<String, String> translateCountryList = CountryNamesTranslationService.translateCountryList();
+                documentIssueCountry = translateCountryList.get(rs.getString("documentIssueCountryCode"));

Review Comment:
   Yeah this is exactly what I meant by the interface suggestion above. The client preferably should only tell the service "I want a resolved country name for this country code" and it shouldn't play around the collection if it doesn't need all of them.



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