myrle-krantz 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-415014208 Unfortunately comments in github aren't threaded, so this is going to be a large dump. I'm going to number my comments so that you can reference the comments when responding: 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. <still adding points, I'll remove this tag when I'm done>
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services
