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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
