Hi Yannick, Thanks for the feedback:
*Review comments below:1.) Your TestNotificationGateways class contents some syntax errors on line57, 59, 82, 111. I see things are red when opened in IntelliJ.* I have eliminated this Class altogether and I think I would be better to group them into services ie SMS and email *2.) Why do you have code commented out everywhere? I think when you nolonger need a function or feature, just remove it and add it later. That's* *why there is a commit history.* Thanks, I will use this practice from now on. *3.) Write unit tests for your code.* *I have written a few and will push them on to GitHub by end of today.* *4.) Fineract CN uses 2 line spaces and not tabs when beginning new line ofcode.* Thanks for the pointing this out. I noticed the consistency too. I have adjust my IDE to use 2 line spaces. *5.) In HelperRestController, line 74 has a problem, you are passing the customer ID without thetenant as in the method signature.* I have eliminated this rest controller as well and maintained only the NotificationRestController for simplicity. Maybe I will introduce again when the services get complex. *6.) You have an application.properties and application.yml files in urcode. Is there is a reason for that? I think one of them will do.* I wanted to use .yml however, I couldn't inject values from the yml file and so I used the application as a workaround. Also, this is temporary. I will eliminate the .application file once I start configuring the SMS service by reading the authentication details from the repository *7.) Is there a reason for using spring-boot-starter-mail1.4.3 instead of 1.4.1? This springs in another version of boot which might**bring problems*. No there is no particular reason, The spring documentation read concerning sending emails in Spring used this version hence, I intuitively used that. However, but now that you mention the reason to use 1.4.1, I will make the change in my next commit. Thanks for the review *At your service,* *Ebenezer Graham* *BSc (Hons) Computing* [image: EmailSignature.png] African Leadership University, Power Mill Road, Pamplemousses, Mauritius. *skype*: ebenezer.graham GitHub <https://github.com/ebenezergraham> | LinkedIn <https://www.linkedin.com/in/ebenezer-graham/> | Twitter <https://twitter.com/pactmart> | Facebook <https://www.facebook.com/pactmart> www.pactmart.com | Freelancing made easy. *“Talk is cheap, show me the code.” *- *Linus Torvalds* On 18 August 2018 at 18:57, Ebenezer Graham <[email protected]> wrote: > Yes Yannick. > > I intend on writing component tests. I haven't done that because I am > currently writing exams. But the good news is that my last paper is on the > 20th so you will see progress after the 20th. > > I see why my authentication is not working. The role and permission > declarations are in the service. I use this approach. Thanks > > > On Sat, 18 Aug 2018, 14:29 Awasum Yannick, <[email protected]> wrote: > >> Hey Graham, >> >> Is see you have updated your PR. Th proper files have been ignored. >> Do you have component test for your service? Do you plan to write unit >> tests? >> >> For the other authentication issue, I think Cabrel had solved the problem >> in one of his commits here: >> >> https://github.com/cabrelkemfang/fineract-cn-datamigration/blob/ >> 0edbfe55b46936c94008afed32cc0e413f3ac882/service/src/main/ >> java/org/apache/fineract/cn/datamigration/service/Connector/ >> DatamigrarionConnector.java >> >> You just create a special user in demo server and give it the permission >> required to access customer and use @Value to pick up the values as shown. >> Just copy the above into your code and do the required adjustment in demo >> server and you should be good to go. >> >> Review comments below: >> 1.) Your TestNotificationGateways class contents some syntax errors on >> line >> 57, 59, 82, 111. I see things are red when opened in IntelliJ. >> 2.) Why do you have code commented out everywhere? I think when you no >> longer need a function or feature, just remove it and add it later. Thats >> why there is a commit history. >> 3.) Write unit tests for your code. >> 4.) Fineract CN uses 2 line spaces and not tabs when beginning new line of >> code. >> 5.) In HelperRestController >> , line 74 has a problem, you are passing the customer ID witthout the >> tenant as in the method signature. >> 6.) You have an application.properties and application.yml files in ur >> code. Is there is reason for that? I think one of them will do. >> 7.) Is there a reason for using spring-boot-starter-mail >> 1.4.3 instead of 1.4.1? This springs in another version of boot which >> might >> bring problems. >> >> Will you be able to address these problems so your code can be merged? >> >> >> >> On Sun, Aug 12, 2018 at 8:23 PM Ebenezer Graham <[email protected] >> > >> wrote: >> >> > Alright, >> > >> > PR for the service - https://github.com/apache/ >> fineract-cn-notifications/ >> > pull/1 <https://github.com/apache/fineract-cn-notifications/pull/1> >> > I have already submitted a PR for the Demo-server - >> > https://github.com/apache/fineract-cn-demo-server/pull/19 >> > >> > *At your service,* >> > >> > *Ebenezer Graham* >> > >> > *BSc (Hons) Computing* >> > >> > >> > [image: EmailSignature.png] >> > >> > African Leadership University, >> > >> > Power Mill Road, Pamplemousses, >> > >> > Mauritius. >> > >> > >> > >> > *skype*: >> > ebenezer.graham >> > GitHub <https://github.com/ebenezergraham> | LinkedIn >> > <https://www.linkedin.com/in/ebenezer-graham/> | Twitter >> > <https://twitter.com/pactmart> | Facebook >> > <https://www.facebook.com/pactmart> >> > www.pactmart.com | Freelancing made easy. >> > >> > >> > *“Talk is cheap, show me the code.” *- *Linus Torvalds* >> > >> > >> > >> > On 12 August 2018 at 22:14, Awasum Yannick <[email protected]> wrote: >> > >> > > Hello Graham, >> > > >> > > Please send a PR of what you have done. Maybe someone can help. >> > > >> > > On Sun, Aug 12, 2018 at 6:32 PM Ebenezer Graham < >> > [email protected]> >> > > wrote: >> > > >> > > > Hi Awasum, >> > > > >> > > > No, I am haven't been able to get customer detail. I have created >> the >> > > user, >> > > > role with READ permissions to the customer service and set the >> tenant >> > and >> > > > user context. But still getting the same error. >> > > > >> > > > Also, Courage volunteered to help me out. >> > > > >> > > > Apologies for the late reply, I wanted to get back to you with good >> > news >> > > > but things are not going as planned. >> > > > >> > > > *At your service,* >> > > > >> > > > *Ebenezer Graham* >> > > > >> > > > *BSc (Hons) Computing* >> > > > >> > > > >> > > > [image: EmailSignature.png] >> > > > >> > > > African Leadership University, >> > > > >> > > > Power Mill Road, Pamplemousses, >> > > > >> > > > Mauritius. >> > > > >> > > > >> > > > >> > > > *skype*: >> > > > ebenezer.graham >> > > > GitHub <https://github.com/ebenezergraham> | LinkedIn >> > > > <https://www.linkedin.com/in/ebenezer-graham/> | Twitter >> > > > <https://twitter.com/pactmart> | Facebook >> > > > <https://www.facebook.com/pactmart> >> > > > www.pactmart.com | Freelancing made easy. >> > > > >> > > > >> > > > *“Talk is cheap, show me the code.” *- *Linus Torvalds* >> > > > >> > > > >> > > > >> > > > On 2 August 2018 at 22:02, Awasum Yannick <[email protected]> >> wrote: >> > > > >> > > > > Graham, >> > > > > >> > > > > Have you been able to solve your problem with the authentication? >> > > > > >> > > > > On Thu, Aug 2, 2018 at 6:58 PM Awasum Yannick <[email protected]> >> > > wrote: >> > > > > >> > > > > > Hello Ruphine, Pembe, >> > > > > > >> > > > > > I merged your PRs for the front end. Thanks very much for your >> > > > > > contributions so far. Keep up. >> > > > > > >> > > > > > Ruphine am still to review the new PR for the Groups backend. >> > > > > > >> > > > > > Write unit tests for the work done so far for both backend and >> > front >> > > > end >> > > > > > if you touched both. >> > > > > > >> > > > > > Thanks. >> > > > > > Awasum >> > > > > > >> > > > > > On Thu, Aug 2, 2018 at 4:58 PM Ruphine Kengne < >> > > [email protected] >> > > > > >> > > > > > wrote: >> > > > > > >> > > > > >> Hello, >> > > > > >> >> > > > > >> I have send some PRs as requested. >> > > > > >> >> > > > > >> One to fineract-cn-fims-web-app available here , >> > > > > >> https://github.com/apache/fineract-cn-fims-web-app/pulls >> > > > > >> >> > > > > >> and the other to fineract-cn-group available here , >> > > > > >> https://github.com/apache/fineract-cn-group/pulls >> > > > > >> >> > > > > >> >> > > > > >> Regard >> > > > > >> Ruphine kengne >> > > > > >> >> > > > > >> On 28 July 2018 at 09:08, Awasum Yannick <[email protected]> >> > wrote: >> > > > > >> >> > > > > >> > Hello Interns, >> > > > > >> > >> > > > > >> > Given we are near the end of GSoC 2018, I am recommending for >> > > > everyone >> > > > > >> to >> > > > > >> > start sending in the PRs for the work we have done so far. >> > > > > >> > >> > > > > >> > I will be available to start reviewing today or tomorrow >> > (Sunday). >> > > > > >> > @Pembe Miriam <[email protected]> , I already started >> > with >> > > > > yours >> > > > > >> > and >> > > > > >> > left some comments here: >> > > > > >> > https://github.com/apache/fineract-cn-group-finance/pull/2 >> > > > > >> > >> > > > > >> > I also encourage you all to write unit tests for the work >> done >> > so >> > > > far >> > > > > >> as we >> > > > > >> > might not be able to merge your PRs without proper tests. >> > > > > >> > >> > > > > >> > Also write user and developer documentations for your work >> and >> > add >> > > > > more >> > > > > >> > clarity on user stories and requirements which have evolved >> over >> > > the >> > > > > >> course >> > > > > >> > of the GSoC period so others can get context. Use confluence. >> > > > > >> > >> > > > > >> > I am available to answer questions if you are having problems >> > > mostly >> > > > > in >> > > > > >> my >> > > > > >> > night period. I'm at GMT + 1 time zone. Lets try to get >> everyone >> > > > > >> unblocked >> > > > > >> > and complete the GSoC project within the next few weeks. >> > > > > >> > >> > > > > >> > >> > > > > >> > Thanks. >> > > > > >> > Awasum >> > > > > >> > >> > > > > >> >> > > > > > >> > > > > >> > > > >> > > >> > >> >
