[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Fineract CN GSoC 2018 Interns: Send your PRs


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 <egraham15@xxxxxxxxxxxxxx>
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, <awasum@xxxxxxxxxx> 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 <egraham15@xxxxxxxxxxxxxx
>> >
>> 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 <awasum@xxxxxxxxxx> 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 <
>> > egraham15@xxxxxxxxxxxxxx>
>> > > 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 <awasum@xxxxxxxxxx>
>> wrote:
>> > > >
>> > > > > Graham,
>> > > > >
>> > > > > Have you been able to solve your problem with the authentication?
>> > > > >
>> > > > > On Thu, Aug 2, 2018 at 6:58 PM Awasum Yannick <awasum@xxxxxxxxxx>
>> > > 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 <
>> > > ruphinekengne@xxxxxxxxx
>> > > > >
>> > > > > > 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 <awasum@xxxxxxxxxx>
>> > 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 <pembemiriam007@xxxxxxxxx> , 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
>> > > > > >> >
>> > > > > >>
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>