Thank you Francis- your inputs would be greatly appreciated. Rahul
I went through the code and have already commented my feedback on the files. Here is the list for your reference according to the priority: Critical Changes: 1) Remove hyphens from child account numbers: I have explained in depth why we should do this in my feedback. 2) Revert persistence.xml change as suggested by Michael. 3) Add concurrency fix in GSIM. Non-critical changes: 4) Align GLIM account creation mechanism with GSIM( if possible). 5) Avoid appending sql params in sql string and instead pass them as object params. 6) Omit use of glimID and gsimID from commandProcessing result 7)Remove unwanted whitespaces and new lines in PR. 8) extract functions out of repetitive code in LoanApplicationWritePlatformServiceJpaRepositoryImpl and SavingsAccountWritePlatformServiceJpaRepositoryImpl I would request you to address above stuff based on the priority. This PR has a go ahead from me, if critical changes as addressed. If you need help, please feel to ping me and I would be happy to help you. Regards, Nikhil On Sat, Apr 18, 2020 at 1:42 PM Francis Guchie <[email protected]> wrote: > Yes Nikhil, > > I will review > > My Kind Regards > > Francis Guchie Kirago > *Skype:* francisguchie > *Whatsapp: *232 79 19 44 07 > *LINKEDIN:* https://www.linkedin.com/in/francis-guchie-kirago-a4379617/ > twitter: @FrancisGuchie > > > > > On Sat, Apr 18, 2020 at 5:12 PM Nikhil Pawar <[email protected]> wrote: > >> ++ Francis >> If you have time could you please help in doing functional review. >> >> I will also go through the code and provide my feedback. But I would >> strongly recommend that we merge this PR , provided it does not break any >> existing functionality. Rebasing this huge chunk of code takes lot of >> effort and now that we have a rebased code - this opportunity should not be >> missed. >> >> Besides that, this feature also has front end support. The next task to >> be picked up would be to rebase front end code. Once we have front end >> support- it would be much easier for functional reviewers to test this >> feature. >> >> Regards, >> Nikhil >> >> On Sat, Apr 18, 2020 at 12:35 PM Michael Vorburger <[email protected]> >> wrote: >> >>> Community, >>> >>> Could parties interested in >>> https://issues.apache.org/jira/browse/FINERACT-603 please help with >>> code reviewing https://github.com/apache/fineract/pull/738 ? >>> >>> If nobody cares enough to spend time to help doing reviews in the next >>> say 2 weeks, then I propose we'll just merge it.... >>> >>> Tx! >>> M. >>> _______________________ >>> Michael Vorburger >>> http://www.vorburger.ch >>> >>> >>> On Mon, Mar 23, 2020 at 2:24 AM Michael Vorburger <[email protected]> >>> wrote: >>> >>>> On Fri, Mar 20, 2020 at 5:28 AM RAHUL PAWAR <[email protected]> >>>> wrote: >>>> >>>>> Hi Ed, >>>>> >>>>> Yes, the build is passing locally in machine. >>>>> >>>> >>>> That is very curious, I don't see how that's possible; you should see >>>> the same failures that Travis CI reports... >>>> >>>> ...do make sure that you rebase to the latest master, and run a full >>>> "./gradlew build" and you will reproduce the Checkstyle errors that Travis >>>> shows if you click on Details. >>>> >>>> I've also just commented on the >>>> https://github.com/apache/fineract/pull/738 with some more suggestions >>>> re. Checkstyle, for anyone interested to read. >>>> >>>> >>>>> Thanks. >>>>> >>>>> On Fri, Mar 20, 2020 at 6:37 AM Ed Cable <[email protected]> wrote: >>>>> >>>>>> Wonderful news. We'll try to get it merged promptly. >>>>>> >>>>>> I'm adding @Dev <[email protected]> >>>>>> >>>>>> It's showing the Travis CI build is failing - the build is passing >>>>>> locally for you? >>>>>> >>>>>> On Thu, Mar 19, 2020 at 7:04 AM RAHUL PAWAR <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> Hi Ed, >>>>>>> >>>>>>> The Integration Test for glim_gsim has been completed and along with >>>>>>> it, also fixed some bugs regarding it. >>>>>>> >>>>>>> Pr: https://github.com/apache/fineract/pull/738 >>>>>>> >>>>>>> Thankyou. >>>>>>> >>>>>>> On Thu, Jan 2, 2020 at 8:22 AM Ed Cable <[email protected]> wrote: >>>>>>> >>>>>>>> Thank you for the update. >>>>>>>> >>>>>>>> On Wed, Jan 1, 2020, 07:02 RAHUL PAWAR <[email protected]> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hi Ed, >>>>>>>>> I am still working on testcases, will be completed soon. >>>>>>>>> >>>>>>>>> On Wed, Nov 13, 2019 at 7:23 AM Nikhil Pawar <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> I am going to take this up with Rahul this holidays, will keep >>>>>>>>>> you posted. >>>>>>>>>> >>>>>>>>>> On Tue, Nov 12, 2019 at 8:45 PM Ed Cable <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Hi Rahul and Nikhil, >>>>>>>>>>> >>>>>>>>>>> I would love to get this work shipped to the community. Did you >>>>>>>>>>> get a chance to write the necessary test cases? It's likely we'll >>>>>>>>>>> target >>>>>>>>>>> 1.5 release for this. >>>>>>>>>>> >>>>>>>>>>> See https://github.com/apache/fineract/pull/527 >>>>>>>>>>> >>>>>>>>>>> and https://issues.apache.org/jira/browse/FINERACT-603 >>>>>>>>>>> >>>>>>>>>>> Ed >>>>>>>>>>> >>>>>>>>>>> >>>>>> >>>>>> -- >>>>>> *Ed Cable* >>>>>> President/CEO, Mifos Initiative >>>>>> [email protected] | Skype: edcable | Mobile: +1.484.477.8649 >>>>>> >>>>>> *Collectively Creating a World of 3 Billion Maries | * >>>>>> http://mifos.org <http://facebook.com/mifos> >>>>>> <http://www.twitter.com/mifos> >>>>>> >>>>>>
