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

feat: Remove old TEA change log [DHIS2-18477] #19388

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

Conversation

muilpp
Copy link
Contributor

@muilpp muilpp commented Dec 4, 2024

This PR removes the old TEA change log endpoint, service, and store.
While working on this, I realized a few things:

  • Initially, my idea was to keep the TEA change log delete method, similar to what we did for event change logs. This way, when a TE is deleted, we could also delete all related change logs from the new table and the old one. However, the problem with this approach is that if the implementation decides to drop the old table, the project will no longer compile because the Hibernate mapping won't find that table. While this compilation error could be resolved by using JDBC instead, it would lead to runtime errors, which doesn't improve the situation. For this reason, I decided to remove the method altogether, meaning no change logs will be deleted from the old TEA change log table anymore. The same applies to event change logs. These two tables are completely the responsibility of the implementation.
  • I'm using plainValue for TEA instead of value to ensure that if the value is encrypted, the encrypted value doesn’t end up in the change log table.
  • When manually merging two entities, the deduplication service was adding a DELETE change log entry on behalf of the duplicated TE. That TE was then deleted along with all its related change logs, including the one just created. I removed the step where that DELETE change log entry is created.
  • TrackedEntityAuditService and TrackedEntityAuditStore should not be deprecated. This was probably due to confusion caused by the inconsistent naming between "change log" and "audit."
  • DefaultTrackedEntityAttributeValueService.updateTrackedEntityAttributeValue was only used in tests, so I am removing it as well.

@muilpp muilpp marked this pull request as ready for review December 5, 2024 10:46
@muilpp muilpp requested a review from a team as a code owner December 5, 2024 10:46
Copy link
Contributor

@enricocolasante enricocolasante left a comment

Choose a reason for hiding this comment

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

I always get confused when we talk about audit.
Is TrackedEntityAudit really used by someone? Maybe we can ask one more time, hopefully last time, to Morten to be sure it is needed.
It seems to me that it is just auditing when something it is DELETED and randomly auditing when something is READ.

@@ -36,7 +36,6 @@
/**
* @author Abyot Asalefew Gizaw [email protected]
*/
@Deprecated(since = "2.42", forRemoval = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is not deprecate, should be moved outside deprecated package?

@@ -246,76 +243,6 @@ public RootNode getAggregateDataValueAudit(
return rootNode;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to add this removal somewhere in the release-notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do, it's my next and hopefully final step when it comes to change logs.

@muilpp
Copy link
Contributor Author

muilpp commented Dec 5, 2024

I always get confused when we talk about audit. Is TrackedEntityAudit really used by someone? Maybe we can ask one more time, hopefully last time, to Morten to be sure it is needed. It seems to me that it is just auditing when something it is DELETED and randomly auditing when something is READ.

I don't know if someone is using the audit feature, I'll contact Morten to clarify that. Until then, I think it should not be deprecated.

Copy link

sonarcloud bot commented Dec 5, 2024

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.

2 participants