You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2022/03/26 21:46:19 UTC
[GitHub] [fineract] rrpawar96 opened a new pull request #2190: FINERACT-1431:ClientIdentifierPassportCountries
rrpawar96 opened a new pull request #2190:
URL: https://github.com/apache/fineract/pull/2190
## 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/api-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
[GitHub] [fineract] galovics commented on a change in pull request #2190: FINERACT-1431:ClientIdentifierPassportCountries
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org