galovics commented on a change in pull request #2190:
URL: https://github.com/apache/fineract/pull/2190#discussion_r839318163
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/domain/CodeRepository.java
##########
@@ -24,4 +24,6 @@
public interface CodeRepository extends JpaRepository<Code, Long>,
JpaSpecificationExecutor<Code> {
Code findOneByName(String name);
+
+ Code findOneById(Long id);
Review comment:
Why can't we use the other method, `findOneByName`?
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/CodeConstants.java
##########
@@ -29,7 +29,7 @@
public enum CodevalueJSONinputParams {
CODEVALUE_ID("id"), NAME("name"), POSITION("position"),
DESCRIPTION("description"), IS_ACTIVE("isActive"), IS_MANDATORY(
- "isMandatory");
+ "isMandatory"), COUNTRYCODE("28");
Review comment:
Why did we hardcode the 28 id? This definitely shoud be something like
`country` or `countryCode` or something like that.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/domain/ClientIdentifier.java
##########
@@ -48,6 +48,9 @@
@Column(name = "document_key", length = 1000)
private String documentKey;
+ @Column(name = "document_issue_country", length = 1000)
Review comment:
Also, let's not give the false impression that this field is going to
contain an actual country name but the code so let's name it appropriately like
`document_issuing_country_code`.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CountryNamesTranslationServiceImpl.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.io.IOException;
+import java.io.UncheckedIOException;
+import java.util.Properties;
+import org.springframework.stereotype.Service;
+
+@Service
+public class CountryNamesTranslationServiceImpl {
+
+ public String retrieveTranslationByCode(String value) {
+ Properties countryProperties = new Properties();
+
+ try {
+ countryProperties
+
.load(CountryNamesTranslationServiceImpl.class.getClassLoader().getResourceAsStream("countries_en.properties"));
Review comment:
You could use Spring's ClasspathResource to get the countries data.
##########
File path:
fineract-provider/src/main/resources/db/changelog/tenant/parts/0009_client_identifier_passport_countrieslist.xml
##########
@@ -0,0 +1,2062 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+
xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.1.xsd";>
+ <changeSet author="fineract" id="1">
+ <addColumn tableName="m_client_identifier">
+ <column name="document_issue_country" type="VARCHAR(50)"/>
Review comment:
Let's call this column `document_issuing_country_code` to represent
exactly what's gonna be stored.
Also, this is very much not in line with the JPA definition in terms of
length (1000 vs 50).
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CountryNamesTranslationServiceImpl.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.io.IOException;
+import java.io.UncheckedIOException;
+import java.util.Properties;
+import org.springframework.stereotype.Service;
+
+@Service
+public class CountryNamesTranslationServiceImpl {
+
+ public String retrieveTranslationByCode(String value) {
+ Properties countryProperties = new Properties();
Review comment:
With this implementation, the translation is read every time we try to
translate a country code. Let's load the translations on startup time and work
with that.
On the other hand, please rename the method since it doesn't indicate
anything specific to countries but as a general translation method.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/domain/ClientIdentifier.java
##########
@@ -48,6 +48,9 @@
@Column(name = "document_key", length = 1000)
private String documentKey;
+ @Column(name = "document_issue_country", length = 1000)
Review comment:
Let's be very strict with this. If the country code is 2-3 characters,
let's stick with the minimum length instead of 1000. I think 3 should be good
enough.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CodeValueReadPlatformServiceImpl.java
##########
@@ -54,12 +58,17 @@ public String schema() {
public CodeValueData mapRow(final ResultSet rs,
@SuppressWarnings("unused") final int rowNum) throws SQLException {
final Long id = rs.getLong("id");
- final String value = rs.getString("value");
+ final Long codeId = rs.getLong("codeId");
+ String value = rs.getString("value");
final Integer position = rs.getInt("position");
final String description = rs.getString("description");
final boolean isActive = rs.getBoolean("isActive");
final boolean mandatory = rs.getBoolean("mandatory");
+ if
(codeId.equals(CodevalueJSONinputParams.COUNTRYCODE.getValueLong())) {
Review comment:
I'm not convinced that architecturally the translation should be here.
Let's think about data layer and presentation layer. Where the business
logic is implemented, or the database is touched, let's call that the data
layer. On that level, we should definitely not take care of translation.
When we are presenting the data - i.e. returning the value through an API -
we definitely could translate the country code into a readable country name.
However, even in that case, I simply wouldn't just stick with the readable
country name on the API response but include the country code as well so if the
users of the API would like to differentiate between countries, they don't need
to hardcode the readable country names in different languages.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CountryNamesTranslationServiceImpl.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.io.IOException;
+import java.io.UncheckedIOException;
+import java.util.Properties;
+import org.springframework.stereotype.Service;
+
+@Service
+public class CountryNamesTranslationServiceImpl {
+
+ public String retrieveTranslationByCode(String value) {
+ Properties countryProperties = new Properties();
+
+ try {
+ countryProperties
+
.load(CountryNamesTranslationServiceImpl.class.getClassLoader().getResourceAsStream("countries_en.properties"));
Review comment:
Also, you could simply use a ResourceBundle implementation to resolve
the proper countries file according to the language settings.
##########
File path:
fineract-provider/src/main/resources/db/changelog/tenant/parts/0008_client_identifier_passport_countrieslist.xml
##########
@@ -0,0 +1,2062 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review comment:
I assume this file can be deleted since it's never referenced.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/codes/service/CountryNamesTranslationServiceImpl.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.io.IOException;
+import java.io.UncheckedIOException;
+import java.util.Properties;
+import org.springframework.stereotype.Service;
+
+@Service
+public class CountryNamesTranslationServiceImpl {
Review comment:
If no interface, why the Impl suffix?
--
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]
