fineract icon indicating copy to clipboard operation
fineract copied to clipboard

FINERACT-1431: Country Drop down for Passport - identities

Open rrpawar96 opened this issue 2 years ago • 22 comments

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.

rrpawar96 avatar Mar 26 '22 21:03 rrpawar96

@vidakovic & @galovics

Kindly is this PR can be merged or there is something to work on

francisguchie avatar Apr 04 '22 13:04 francisguchie

@francisguchie I commented a couple of things that need to be addressed first. Hope that makes sense.

galovics avatar Apr 04 '22 13:04 galovics

@galovics excuse my omissions please - let me work with @rrpawar96 to get this solved

francisguchie avatar Apr 04 '22 14:04 francisguchie

@rrpawar96 have you managed to look at what we discussed ?

francisguchie avatar May 04 '22 08:05 francisguchie

@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:

  1. 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)
  2. 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.

rrpawar96 avatar May 11 '22 22:05 rrpawar96

@galovics thank you for this detailed reviews

francisguchie avatar May 12 '22 12:05 francisguchie

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.

github-actions[bot] avatar Jun 12 '22 00:06 github-actions[bot]

@rrpawar96
we are going to redo this PR is this ok with you?

francisguchie avatar Jun 14 '22 06:06 francisguchie

Hi @francisguchie, and @galovics, The comments have been addressed, Please re-review.

rrpawar96 avatar Jun 22 '22 09:06 rrpawar96

@rrpawar96 , as we wait for @galovics i will do the functional tests

francisguchie avatar Jun 22 '22 14:06 francisguchie

@rrpawar96

The functional tests are ok for - I hope @galovics can recheck code soon

francisguchie avatar Jul 12 '22 08:07 francisguchie

@rrpawar96 there are conflicts on this branch

francisguchie avatar Jul 12 '22 09:07 francisguchie

@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.

awasum avatar Aug 10 '22 18:08 awasum

@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.

vidakovic avatar Aug 22 '22 10:08 vidakovic

@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

rrpawar96 avatar Aug 22 '22 13:08 rrpawar96

Hope it is ready now to get merged @vidakovic @galovics

rrpawar96 avatar Aug 22 '22 14:08 rrpawar96

@rrpawar96 @galovics I like the progress thank you very much

francisguchie avatar Aug 22 '22 17:08 francisguchie

@galovics @awasum kindly advise what is left to be done on this PR

francisguchie avatar Aug 23 '22 20:08 francisguchie

@rrpawar96 this branch has Conflicts please address them

francisguchie avatar Aug 26 '22 12:08 francisguchie

@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.

awasum avatar Sep 05 '22 08:09 awasum

@rrpawar96 i am available for any input i will share what i have so we can be fast on this

francisguchie avatar Sep 05 '22 08:09 francisguchie

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.

github-actions[bot] avatar Oct 06 '22 00:10 github-actions[bot]