-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Anonymize TIM-Manager Solution #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! I just had a few comments.
cv-data-service-library/src/main/java/com/trihydro/library/helpers/DbInteractions.java
Show resolved
Hide resolved
cv-data-service-library/src/main/java/com/trihydro/library/helpers/JsonToJavaConverter.java
Outdated
Show resolved
Hide resolved
cv-data-service-library/src/test/resources/application.properties
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
|
||
protected DbInteractionsProps dbConfig; | ||
protected Utility utility; | ||
protected EmailHelper emailHelper; | ||
private EmailHelper emailHelper; | ||
|
||
@Autowired | ||
public void InjectDependencies(DbInteractionsProps props, Utility _utility, EmailHelper _emailHelper) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note(non-blocking): the standard way to let Spring manage the component bean and its dependencies is to define a constructor. For example, instead of
@Autowired
public void InjectDependencies(DbInteractionsProps props, Utility _utility, EmailHelper _emailHelper) {
we would do something like
public DbInteractions(DbInteractionsProps props, Utility utility, EmailHelper emailHelper) {
This removes the need for the @Autowired
annotation and makes it clear which parameters this class needs when instantiated.
This is a note only. I would not suggest changing the constructor in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me & the unit tests pass!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! code compiles, all tests pass. Previously reviewed in private repo
This PR includes the anonymization changes that were approved in the WyoCV repo in addition to the changes present in #1. This PR also includes the BOWR TIM type addition from #3 .