fineract
fineract copied to clipboard
FINERACT-1431: Country Drop down for Passport - identities
Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated Apache Fineract JIRA ticket.
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.
@vidakovic & @galovics
Kindly is this PR can be merged or there is something to work on
@francisguchie I commented a couple of things that need to be addressed first. Hope that makes sense.
@galovics excuse my omissions please - let me work with @rrpawar96 to get this solved
@rrpawar96 have you managed to look at what we discussed ?
@francisguchie, @galovics Apologies for the late. @galovics, Comments have been addressed and now the properties of countries are getting loaded while booting. I have done a small twist I have removed the country code from database and providing the country code and country name to user from properties file itself. So it goes like this:
- when the user fetches the country list from code value API, it provides countries list (along with country code) from properties file (getCountriesList() function- loaded in startup)
- when the user selects country and saves in the identifier, country-code gets saved. and when the user fetches this identifier, the country-code gets translated to country name. (translateCountryList() function - loaded in startup) So only country which is saved in identifier is getting translated instead of translating whole country list (which would have been required in step (1) but now its not, and loading the properties file and the translation at startup time. Let me know, if it is ok with this way.
@galovics thank you for this detailed reviews
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.
@rrpawar96
we are going to redo this PR is this ok with you?
Hi @francisguchie, and @galovics, The comments have been addressed, Please re-review.
@rrpawar96 , as we wait for @galovics i will do the functional tests
@rrpawar96
The functional tests are ok for - I hope @galovics can recheck code soon
@rrpawar96 there are conflicts on this branch
@rrpawar96 .. I see the great work you are doing here. Please just fix the merge conflicts and am sure @galovics will be glad to merge this one... So close.
@rrpawar96 ... might seem like a minor thing, but let's be consistent with the PR titles... here the title should be:
FINERACT-1431: Country Drop down for Passport - identities
This ensures that things are properly matched in Jira (aka that this PR is linked to the Jira ticket).... and it's also nicer to read. The camel case thing here is hard to grok if you read the git logs... and using spaces (after the colon) is not forbidden; please keep in mind that someone needs to read this.
@rrpawar96 ... might seem like a minor thing, but let's be consistent with the PR titles... here the title should be:
FINERACT-1431: Country Drop down for Passport - identities
This ensures that things are properly matched in Jira (aka that this PR is linked to the Jira ticket).... and it's also nicer to read. The camel case thing here is hard to grok if you read the git logs... and using spaces (after the colon) is not forbidden; please keep in mind that someone needs to read this.
Comment has been addressed @vidakovic
Hope it is ready now to get merged @vidakovic @galovics
@rrpawar96 @galovics I like the progress thank you very much
@galovics @awasum kindly advise what is left to be done on this PR
@rrpawar96 this branch has Conflicts please address them
@rrpawar96 , @francisguchie , Please add unit or integration test and address the merge conflicts so this can be merged. I know it has taken long but we almost there.
@rrpawar96 i am available for any input i will share what i have so we can be fast on this
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.