Skip to content
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

Merged
merged 47 commits into from
Dec 16, 2024
Merged

Anonymize TIM-Manager Solution #4

merged 47 commits into from
Dec 16, 2024

Conversation

mwodahl
Copy link
Collaborator

@mwodahl mwodahl commented Dec 13, 2024

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 .

Copy link
Member

@dmccoystephenson dmccoystephenson left a 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.

Copy link

@mcook42 mcook42 left a 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) {
Copy link

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.

Copy link
Member

@dmccoystephenson dmccoystephenson left a 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!

Copy link
Contributor

@payneBrandon payneBrandon left a 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

@payneBrandon payneBrandon merged commit 5b92e7b into main Dec 16, 2024
2 checks passed
@payneBrandon payneBrandon deleted the Anonymize-Solution branch December 16, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants