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

DMP-4381: Automatically update createdBy and lastModifiedBy when persisting entities to the database #2308

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

Ben-Edwards-cgi
Copy link
Contributor

Links

Jira

Change description

Git Diff Summary

This Git Diff introduces enhancements to the authentication and entity management components of the application. Key changes include the addition of auditing capabilities for tracking user account interactions in the AuthenticationConfiguration, CreatedBaseEntity, and CreatedModifiedBaseEntity classes.

Highlights

  • AuthenticationConfiguration Updates:

    • Added an AuditorAware<UserAccountEntity> bean to provide information about the currently authenticated user.
    • Introduced error handling for retrieving user accounts.
  • Entity Changes:

    • In CreatedBaseEntity, added @CreatedBy annotation to the createdBy field to track the user who created the entity.
    • In CreatedModifiedBaseEntity, added @LastModifiedBy annotation to the lastModifiedBy field to track the user who last modified the entity.

These modifications enhance the tracking of user interactions with entities, improving the auditing capability of the application.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[X] No

@Ben-Edwards-cgi Ben-Edwards-cgi requested review from a team as code owners November 22, 2024 14:03
@Ben-Edwards-cgi Ben-Edwards-cgi requested review from davet1985, jackmaloney, hemantasharma1129, narinder17, Johns293 and nman-hmcts and removed request for a team November 22, 2024 14:03
Copy link
Contributor

@chrisbellingham-hmcts chrisbellingham-hmcts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this PR should also include removal of any code that explicitly sets the createdBy and lastModifiedBy values in our various services/components, such that this new mechanism gets used?

Comment on lines 24 to 29
@Lazy
@Autowired
private UserIdentity userIdentity;

@Autowired
private Clock clock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency in our approach to dependency injection, can we make these final and use constructor based injection (either with a manually written constructor or @RequiredArgsConstructor)? Should also allow you to eliminate the NoArgs and AllArgs constructor annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wont work here as UserIdentity needs to be lazy loaded to avoid circular references,
However, I have manually created the constructed with the relevant annotations to resolve this

Comment on lines 24 to 29
@Lazy
@Autowired
private UserIdentity userIdentity;

@Autowired
private Clock clock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general pattern elsewhere is to inject CurrentTimeHelper rather than Clock, consider aligning with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline: Clock is industry standard / Java best practices, will depreciate CurrentTimeHelper and update best practice guides

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants