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 2021/11/25 16:36:38 UTC
[GitHub] [fineract] rrpawar96 opened a new pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
rrpawar96 opened a new pull request #1984:
URL: https://github.com/apache/fineract/pull/1984
## 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] rrpawar96 closed pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 closed pull request #1984:
URL: https://github.com/apache/fineract/pull/1984
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1008722767
> 2. Why do we need to create a new table for country codes? Can't we just treat it just like any other code value and use the existing code value infrastructure...
Hi @ptuomola, we have not created codes for Country, we are just using class/objects of it to fetch the values from the new table.
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1037225072
> 2. Why do we need to create a new table for country codes? Can't we just treat it just like any other code value and use the existing code value infrastructure...
@ptuomola, Please review, Comment has been addressed, the existed code-value infrastructure is been used.
--
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] fynmanoj commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
fynmanoj commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-998416158
@rrpawar96 , thanks for addressing the change request.
--
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] fynmanoj commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
fynmanoj commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-986018312
@rrpawar96 , this looks fine. Thanks for that change.
Could you please move the validation from domain object to validator class?
Also I am just curious,is there a specific reason for not using code-code value table for new drop down?
--
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] vidakovic commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1047968075
@rrpawar96 @francisguchie ... most of it looks ok for me... but the database migration scripts need to be rewritten for Liquibase... Flyway not used anymore.
--
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] fynmanoj commented on a change in pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
fynmanoj commented on a change in pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#discussion_r758322188
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/client/domain/ClientIdentifier.java
##########
@@ -58,20 +62,29 @@
private Integer active;
public static ClientIdentifier fromJson(final Client client, final CodeValue documentType, final JsonCommand command) {
+
final String documentKey = command.stringValueOfParameterNamed("documentKey");
final String description = command.stringValueOfParameterNamed("description");
final String status = command.stringValueOfParameterNamed("status");
- return new ClientIdentifier(client, documentType, documentKey, status, description);
+ String documentTypeValue = null;
+ if (documentType.label().equals("Passport")) {
+ documentTypeValue = command.stringValueOfParameterNamed("documentTypeValue");
+ if ("".equals(documentTypeValue)) {
+ throw new PlatformDataIntegrityException("Selection of Country is Mandatory", "error.msg.selection.of.country.mandatory");
Review comment:
Please move this validation to Validator class.
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-986007523
@fynmanoj , please re-review this PR, now we are fetching the countries list from the database so it can be modified directly from the database.
The documentTypeValue param is now used for passport(countries) and passed in String so that in future enhancement, the same param (documentTypeValue) can be used to pass values for dropdowns other than passports
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1009357314
> 1. If the column documentTypeValue is meant to capture the issuing country for the document (which actually can make sense for all kinds of documents, not just passports), would it not make sense to call it that? So how about calling the column e.g. document_issue_country_id rather than document_type_value? Value seems to suggest it's the counterpart for the document_type_id, which is not the case...
Comment has been addressed change the variableName from documentTypeValue to documentIssueCountry and not Id because we are saving countries name in String directly.
--
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 pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
galovics commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1076230610
@francisguchie would you elaborate more on this please? I'm not really sure I'm able to follow the "no internet" aspect of this change or Fineract for that matter.
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-997697730
> @rrpawar96 , this looks fine. Thanks for that change. Could you please move the validation from domain object to validator class?> Also I am just curious, is there a specific reason for not using code-code value table for new drop down?
@rrpawar96 have you addressed @fynmanoj request. On my side the functional test is working fine
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-979448970
@rrpawar96 @ptuomola @awasum this i have tested and works for me
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1057422938
@francisguchie @galovics, after running the latest build on an empty database, I have noticed one thing is that there are no permissions created in the m_permission table, probably once we get that permission data should fix this test case as well.
I am not quite sure about this but will be better if @galovics confirms the m_permission table data issue.
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1075562846
> Hi @rrpawar96 , yeah, I can see why. You're going to read the properties file for each SQL query, that's definitely not the way. What we should do instead:
>
> * Upon application startup, load the properties file and keep the mapping in memory
> * Upon the need to translate country codes to country names, use the mapping in the memory to resolve the country name
>
> Also, please make sure we're following the ResourceBundle naming convention, i.e. let's call the file `countries_en.properties` instead of `countries.properties`.
>
> Note: for the country translation, you could introduce a brand new service class to deal with the preloading and doing the actual translation.
@galovics
As a lay man, who might be using apache-fineract in a place with little to no internet, will this kind of complex update be able to work without internet?
--
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 #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
galovics commented on a change in pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#discussion_r818602019
##########
File path: fineract-provider/src/main/resources/db/changelog/tenant/parts/0002_initial_data.xml
##########
@@ -2204,7 +2204,7 @@
<column name="code" value="INR"/>
<column name="decimal_places" valueNumeric="2"/>
<column name="currency_multiplesof"/>
- <column name="display_symbol" value="₹"/>
+ <column name="display_symbol"/>
Review comment:
Same as above.
##########
File path: 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 getdocumentIssueCountry() {
+ return this.documentIssueCountry;
+ }
+
public String getDescription() {
return this.description;
}
- public void validateForCreate() {
+ public void validateForCreate(final CodeValue documentType, final JsonCommand command) {
final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors).resource("clientIdentifier");
baseDataValidator.reset().parameter("documentTypeId").value(this.documentTypeId).notNull().integerGreaterThanZero();
baseDataValidator.reset().parameter("documentKey").value(this.documentKey).notBlank();
+ if (documentType.label().equals(ClientApiConstants.clientIdentifierPassportParamName.toString())) {
Review comment:
Is there a need to actually call the toString() on that constant? It would also even better if we could switch the order of the equality check to be null-safe.
Thoughts?
##########
File path: fineract-provider/src/main/resources/db/changelog/tenant/parts/0002_initial_data.xml
##########
@@ -2672,7 +2672,7 @@
<column name="code" value="PYG"/>
<column name="decimal_places" valueNumeric="0"/>
<column name="currency_multiplesof"/>
- <column name="display_symbol" value="₲"/>
+ <column name="display_symbol"/>
Review comment:
Same as above.
##########
File path: fineract-provider/src/main/resources/db/changelog/tenant/parts/0002_initial_data.xml
##########
@@ -1916,7 +1916,7 @@
<column name="code" value="CRC"/>
<column name="decimal_places" valueNumeric="2"/>
<column name="currency_multiplesof"/>
- <column name="display_symbol" value="₡"/>
+ <column name="display_symbol"/>
Review comment:
Let's not change anything in the existing changelog files. If you would like to remove the display_symbol for these currencies, let's add a new changelog file with the changes needed.
Btw, any reason you decided to remove these?
--
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 pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
galovics commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1058241266
@rrpawar96 are you using MySQL locally? If so, make sure you set up collation for the server in its entirety. The Liquibase changeset you introduce will break existing develop deployments since you've changed the migration script in the middle. Plus the change you introduced is only MySQL compatible.
Please try to set up the collation for your MySQL installation first.
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1008603126
> 1. If the column documentTypeValue is meant to capture the issuing country for the document (which actually can make sense for all kinds of documents, not just passports), would it not make sense to call it that? So how about calling the column e.g. document_issue_country_id rather than document_type_value? Value seems to suggest it's the counterpart for the document_type_id, which is not the case...
@ptuomola & @rrpawar96 ,
The reason why only passport is needed for this function is that, as is in (Apache fineract) the validation is such that the document id is supposed to be unique in the database irrespective of the document selected. For example, if user selects drivers license, and enters 123456 for client X then later selects passport and selects 123456 then a validation errors shows up.
because 123456 has been used already.
Come to reality- In Africa where i have worked most of my life, as countries are fighting to reduce the financial exclusive percentages, **new / stringent KYC rules are being put in place**. For example in East and West Africa the central banks are pushing for one document only and that is the National ID. All other IDs are not allowed as KYC documents.
Then for Foreigners, only the Passport is recognized by the country hosting you.
This flexibility of various documents is being reduced to just National Id and Passports
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1048480594
@vidakovic you so much, @rrpawar96 please address this. Reach out to me if you need my help
--
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 #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
galovics commented on a change in pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#discussion_r816872516
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/client/command/ClientIdentifierCommand.java
##########
@@ -50,18 +54,30 @@ public String getDocumentKey() {
return this.documentKey;
}
+ public String getdocumentIssueCountry() {
+ return this.documentIssueCountry;
+ }
+
public String getDescription() {
return this.description;
}
- public void validateForCreate() {
+ public void validateForCreate(final JsonCommand command) {
final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors).resource("clientIdentifier");
baseDataValidator.reset().parameter("documentTypeId").value(this.documentTypeId).notNull().integerGreaterThanZero();
baseDataValidator.reset().parameter("documentKey").value(this.documentKey).notBlank();
+ final Long documentTypeId = command.longValueOfParameterNamed("documentTypeId");
+ if (documentTypeId == 1) {
+ final String documentIssueCountry = command.stringValueOfParameterNamed("documentIssueCountry");
+ if ("".equals(documentIssueCountry)) {
Review comment:
StringUtils.isBlank could make more sense here. Thoughts?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/client/domain/ClientIdentifier.java
##########
@@ -61,17 +64,24 @@ public static ClientIdentifier fromJson(final Client client, final CodeValue doc
final String documentKey = command.stringValueOfParameterNamed("documentKey");
final String description = command.stringValueOfParameterNamed("description");
final String status = command.stringValueOfParameterNamed("status");
- return new ClientIdentifier(client, documentType, documentKey, status, description);
+
+ String documentIssueCountry = null;
+ if (documentType.label().equals("Passport")) {
Review comment:
Any chance we can extract this magic String to somewhere?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/client/command/ClientIdentifierCommand.java
##########
@@ -50,18 +54,30 @@ public String getDocumentKey() {
return this.documentKey;
}
+ public String getdocumentIssueCountry() {
+ return this.documentIssueCountry;
+ }
+
public String getDescription() {
return this.description;
}
- public void validateForCreate() {
+ public void validateForCreate(final JsonCommand command) {
final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors).resource("clientIdentifier");
baseDataValidator.reset().parameter("documentTypeId").value(this.documentTypeId).notNull().integerGreaterThanZero();
baseDataValidator.reset().parameter("documentKey").value(this.documentKey).notBlank();
+ final Long documentTypeId = command.longValueOfParameterNamed("documentTypeId");
+ if (documentTypeId == 1) {
Review comment:
Can we somehow make this documentTypeId a non-magic number? Like what does 1 represent here?
##########
File path: fineract-provider/src/main/resources/db/changelog/tenant/parts/0006_client_identifier_passport_countrieslist.xml
##########
@@ -0,0 +1,1901 @@
+<?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)"/>
+ </addColumn>
+ <dropUniqueConstraint constraintName="unique_identifier_key" tableName="m_client_identifier"/>
+ <addUniqueConstraint columnNames="document_type_id, document_key, document_issue_country" constraintName="unique_identity" tableName="m_client_identifier"/>
+ </changeSet>
+ <changeSet author="fineract" id="2">
+ <insert tableName="m_code">
+ <column name="code_name" value="Countries"/>
+ <column name="is_system_defined" valueBoolean="true"/>
+ </insert>
+ </changeSet>
+ <changeSet author="fineract" id="3">
+ <insert tableName="m_code_value">
+ <column name="code_id" valueNumeric="35"/>
Review comment:
I assume this refers to the inserted m_code row right before this changeset. If so, then let's define the id of that to make sure it really gets id 35.
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1057368973
@rrpawar96 @galovics I see the fineract gradle build PostgreSQL is failing. What is the remedy here?
--
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 pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
galovics commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1057989103
@rrpawar96 Judging by the test error in the console, it's failing due to the fact that you removed some displaySymbols:
```
java.lang.NullPointerException: Cannot invoke "String.equals(Object)" because "this.displaySymbol" is null
at org.apache.fineract.integrationtests.CurrenciesTest.testUpdateCurrencySelection(CurrenciesTest.java:92)
```
--
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] fynmanoj commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
fynmanoj commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-981599347
@rrpawar96 , @francisguchie as per the Screenshots attached to JIRA ticket the new field appears to be a dropdown. where does the dropdown values are fetched from? Also if the dropdown values are predefined, why are we not using ID instead of String Value ?
--
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] rrpawar96 commented on a change in pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on a change in pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#discussion_r818969696
##########
File path: 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 getdocumentIssueCountry() {
+ return this.documentIssueCountry;
+ }
+
public String getDescription() {
return this.description;
}
- public void validateForCreate() {
+ public void validateForCreate(final CodeValue documentType, final JsonCommand command) {
final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors).resource("clientIdentifier");
baseDataValidator.reset().parameter("documentTypeId").value(this.documentTypeId).notNull().integerGreaterThanZero();
baseDataValidator.reset().parameter("documentKey").value(this.documentKey).notBlank();
+ if (documentType.label().equals(ClientApiConstants.clientIdentifierPassportParamName.toString())) {
Review comment:
Addressed,
--
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 pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
galovics commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1062908825
Hi @rrpawar96 , yeah, I can see why. You're going to read the properties file for each SQL query, that's definitely not the way.
What we should do instead:
# Upon application startup, load the properties file and keep the mapping in memory
# Upon the need to translate country codes to country names, use the mapping in the memory to resolve the country name
Also, please make sure we're following the ResourceBundle naming convention, i.e. let's call the file `countries_en.properties` instead of `countries.properties`.
Note: for the country translation, you could introduce a brand new service class to deal with the preloading and doing the actual translation.
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1062240656
Hi @galovics, I have created the countries.properties file for mapping but using it in this way, is taking 3 secs of delay for loading the countries' names on UI, wouldn't be fine if we consider taking direct English country names?
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1079780741
closing this PR, opened up a new cleaned PR: https://github.com/apache/fineract/pull/2190
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1034533445
> > 1. If the column documentTypeValue is meant to capture the issuing country for the document (which actually can make sense for all kinds of documents, not just passports), would it not make sense to call it that? So how about calling the column e.g. document_issue_country_id rather than document_type_value? Value seems to suggest it's the counterpart for the document_type_id, which is not the case...
>
> Comment has been addressed change the variableName from documentTypeValue to documentIssueCountry and not Id because we are saving countries name in String directly.
@ptuomola Kindly review
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-998231476
Hi @fynmanoj, @francisguchie comment has been addressed now, validation has moved to validator class.
We didn't use a code-value table since it won't be seen as good to keep more than 250 lists of countries in the code value table which we use generally for all codes, that's why we kept the table separate.
--
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] fynmanoj commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
fynmanoj commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-982244722
@francisguchie isn't it a good idea to fetch the predefined data set from the platform itself?, especially when it is a mandatory field? That would help others to upgrade effortlessly.
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1058377764
> @rrpawar96 are you using MySQL locally? If so, make sure you set up collation for the server in its entirety. The Liquibase changeset you introduce will break existing develop deployments since you've changed the migration script in the middle. Plus the change you introduced is only MySQL compatible.
>
> Please try to set up the collation for your MySQL installation first.
Thanks.
--
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 pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
galovics commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1058447788
@rrpawar96 I just looked at the last changes and I still have the question standing. Don't you think it would be better to use country ISO codes instead of the actual English country names in the DB?
My idea would be, store the ISO country codes in the DB and create a properties file with mapping the ISO country code to an English country name - if needed at all.
What do you think?
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1058138785
Hi @galovics, there is a character set issue while deploying the latest branch in local. So I was made the changes in the display_symbol table. Now created new ticket for it https://issues.apache.org/jira/browse/FINERACT-1516
Once the build passes I will create a new PR for this ticket.
--
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 #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
galovics commented on a change in pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#discussion_r822636941
##########
File path: fineract-provider/src/main/resources/countries.properties
##########
@@ -0,0 +1,245 @@
+#
+# 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.
+#
+
+AF=AFGHANISTAN
Review comment:
Doesn't it make sense to not have the country names in full uppercase but as a readable format, like `Albania` instead of `ALBANIA`?
--
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 edited a comment on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
galovics edited a comment on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1062908825
Hi @rrpawar96 , yeah, I can see why. You're going to read the properties file for each SQL query, that's definitely not the way.
What we should do instead:
* Upon application startup, load the properties file and keep the mapping in memory
* Upon the need to translate country codes to country names, use the mapping in the memory to resolve the country name
Also, please make sure we're following the ResourceBundle naming convention, i.e. let's call the file `countries_en.properties` instead of `countries.properties`.
Note: for the country translation, you could introduce a brand new service class to deal with the preloading and doing the actual translation.
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1055515630
> @rrpawar96 @francisguchie ... most of it looks ok for me... but the database migration scripts need to be rewritten for Liquibase... Flyway not used anymore.
@vidakovic have you had the time to look through and review changes made by @rrpawar96
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-981928025
@fynmanoj
This list is going to be hard coded i presume the number of counties in the world does not change that much and can be easily found here https://www.worldometers.info/geography/alphabetical-list-of-countries/
--
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] ptuomola commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1003320098
Two comments from my side:
1. If the column documentTypeValue is meant to capture the issuing country for the document (which actually can make sense for all kinds of documents, not just passports), would it not make sense to call it that? So how about calling the column e.g. document_issue_country_id rather than document_type_value? Value seems to suggest it's the counterpart for the document_type_id, which is not the case...
2. Why do we need to create a new table for country codes? Can't we just treat it just like any other code value and use the existing code value infrastructure...
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1037225072
> 2. Why do we need to create a new table for country codes? Can't we just treat it just like any other code value and use the existing code value infrastructure...
@ptuomola, Please review, Comment has been addressed, the existed code-value infrastructure is been used.
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1008593462
> 2. Why do we need to create a new table for country codes? Can't we just treat it just like any other code value and use the existing code value infrastructure...
@ptuomola , kindly allow me to comment on having an extra table.
Countries information is almost "static" changes are not made all the time. And if there is a new country, we can create a PR for it like we do for currencies. I think (i stand to be corrected) we get more validation / control options when we add an extra table.
--
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] github-actions[bot] commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1034338011
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.
--
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] francisguchie commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1047184410
@fynmanoj , @ptuomola and @vidakovic kindly help on reviewing this PR
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1048877398
@galovics @vidakovic Please review the migration script written for Liquibase.
--
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] francisguchie edited a comment on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
francisguchie edited a comment on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1008603126
> 1. If the column documentTypeValue is meant to capture the issuing country for the document (which actually can make sense for all kinds of documents, not just passports), would it not make sense to call it that? So how about calling the column e.g. document_issue_country_id rather than document_type_value? Value seems to suggest it's the counterpart for the document_type_id, which is not the case...
@ptuomola & @rrpawar96 ,
The reason why only passport is needed for this function is that, as is in (Apache fineract) the validation is such that the document id is supposed to be unique in the database irrespective of the document selected. For example, if user selects drivers license, and enters 123456 for client X then later selects passport and enters 123456 then a validation errors shows up.
because 123456 has been used already.
Come to reality- In Africa where i have worked most of my life, as countries are fighting to reduce the financial exclusion percentages **(some countries are at 80% financial exclusion)**, **new / stringent KYC rules are being put in place**. For example in East and West Africa the central banks are pushing for one document only and that is the National ID. All other IDs are not allowed as KYC documents. Meanwhile, for Foreigners, only the Passport is recognized when opening a "bank" account in any bank of the hosting country.
This flexibility of various documents is being reduced to just National Id and Passports
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-999115132
Travis fixed..
--
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] rrpawar96 commented on pull request #1984: FINERACT-1431:PassportDropDown-in-ClientIdentifier
Posted by GitBox <gi...@apache.org>.
rrpawar96 commented on pull request #1984:
URL: https://github.com/apache/fineract/pull/1984#issuecomment-1059132715
> @rrpawar96 I just looked at the last changes and I still have the question standing. Don't you think it would be better to use country ISO codes instead of the actual English country names in the DB? My idea would be, store the ISO country codes in the DB and create a properties file with mapping the ISO country code to an English country name - if needed at all. What do you think?
Hi @galovics, comment addressed, stored country in codes, and used Locale Library for conversion.
--
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