[GitHub] ebenezergraham edited a comment on issue #5: Unit and component Tests, Rest and command updates and Bug Fixes

[GitBox <gi...@apache.org>]

ebenezergraham edited a comment on issue #5: Unit and component Tests,Rest and command updates and Bug Fixes
URL: https://github.com/apache/fineract-cn-notifications/pull/5#issuecomment-415146388
 
 
   Checklist for myself
   [x]1.) You need to run the rat tests. At least three files are missing their headers, and there may be other problems. To do this run:
   ./gradlew rat
   2.) You need to run your tests. SMSConfigurationTest doesn't compile. TestEmailService doesn't pass. TestSMSService doesn't pass. All tests should compile and run.
   3.) What is the purpose of the Event domain object?
   4.) You probably only need one permittable group for this service. Currently you have three.
   5.) Remove IAMATeapotException. You never return this exception from any of your HTTP methods, so you can delete that throws clause without replacement from the feign client.
   6.) Good job for adding validation tests cases. Please add one for each of your domain objects (the classes in the package org.apache.fineract.cn.notification.api.v1.domain) . Please test all of the constraints in your domain objects, not just the identifier constraints.
   7.) The constraints in your domain objects do not match the constraints in your sql. They need to match. For example: some of your fields in SQL are strings with length 255, but you allow strings with length 512 in through the domain object validation.
   8.) Your feign client needs proper function names for the notification end point. Currently the name is the generic "entity" from the template project.
   9.) Your domain objects should all contain equals, hashcode and toString. This makes testing and debugging easier.
   10.) Your NotificationEventConstants still contains SAMPLE events from the template project. Remove these please.
   11.) TestEmailService.prepareMocks is dead code. You can remove it.
   12.) TestEmailService.retrieveEmailConfiguration is redundant. You're checking the same functionality that shouldCreateEmailConfigurationEntity already checks. In the same way, TestsSMSService.retrieveSMSConfiguration is also redundant. And for the same reason migration V2, and V3 do not make sense. This data should not be part of the production code.
   13.) commands need names which reflect the action to be executed. For example EmailConfigurationCommand should be CreateEmailConfigurationCommand. You will eventually want to add a ChangeEmailConfigurationCommand.
   14.) MigrateServiceCommand is dead code. You already have InitializeServiceCommand, so you can delete Migrate.
   15.) You forgot to remove the TemplateEntity and the TemplateRepository.
   16.) ConfigurationProperties SMSSender should move the configuration reading code into a package named config. The names of the properties need to be defined in a consistent camelback style.
   17.) Why are you setting the Engine and the defaultl character set in the initialsetup sql?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services