-
Notifications
You must be signed in to change notification settings - Fork 363
chore: Rename Event hibernate object to TrackerEvent [DHIS2-19865] #21431
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
Conversation
dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/Enrollment.java
Dismissed
Show dismissed
Hide dismissed
...service-tracker/src/main/java/org/hisp/dhis/tracker/imports/preheat/mappers/EventMapper.java
Dismissed
Show dismissed
Hide dismissed
...gration/src/test/java/org/hisp/dhis/program/notification/ProgramNotificationServiceTest.java
Dismissed
Show dismissed
Hide dismissed
...gration/src/test/java/org/hisp/dhis/program/notification/ProgramNotificationServiceTest.java
Dismissed
Show dismissed
Hide dismissed
...gration/src/test/java/org/hisp/dhis/program/notification/ProgramNotificationServiceTest.java
Dismissed
Show dismissed
Hide dismissed
...gration/src/test/java/org/hisp/dhis/program/notification/ProgramNotificationServiceTest.java
Dismissed
Show dismissed
Hide dismissed
...gration/src/test/java/org/hisp/dhis/program/notification/ProgramNotificationServiceTest.java
Dismissed
Show dismissed
Hide dismissed
...gration/src/test/java/org/hisp/dhis/program/notification/ProgramNotificationServiceTest.java
Dismissed
Show dismissed
Hide dismissed
cd73ea7
to
e51d901
Compare
e51d901
to
fab8a7b
Compare
dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/TrackerEvent.java
Dismissed
Show dismissed
Hide dismissed
dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/TrackerEvent.java
Dismissed
Show dismissed
Hide dismissed
dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/TrackerEvent.java
Dismissed
Show dismissed
Hide dismissed
@@ -90,19 +90,21 @@ public enum Objects { | |||
PROGRAM("program", Program.class), | |||
TRACKEDENTITY("trackedEntity", TrackedEntity.class), | |||
ENROLLMENT("enrollment", Enrollment.class), | |||
EVENT("event", Event.class), | |||
EVENT("event", TrackerEvent.class), // Event inclues tracker and single events |
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.
EVENT("event", TrackerEvent.class), // Event inclues tracker and single events | |
EVENT("event", TrackerEvent.class), // Event includes tracker and single events |
@@ -90,19 +90,21 @@ public enum Objects { | |||
PROGRAM("program", Program.class), | |||
TRACKEDENTITY("trackedEntity", TrackedEntity.class), | |||
ENROLLMENT("enrollment", Enrollment.class), | |||
EVENT("event", Event.class), | |||
EVENT("event", TrackerEvent.class), // Event inclues tracker and single events |
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.
why are both types of event included if only the TrackerEvent
class is used?
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.
Class field is only used in tests and do not represent the type that is going to be used. I deprecated the field clazz
for this class.
The string event
is what it is used and now it includes all entities in table event
, later it will include all entities from tables trackerevent
and singleevent
.
@@ -182,7 +182,7 @@ public void handleEvents( | |||
"delete from event where attributeoptioncomboid in (%s)".formatted(aocIds)); | |||
} else { | |||
log.info("Merging source events as dataMergeStrategy is LAST_UPDATED"); | |||
|
|||
// TODO(DHIS2-19702): Should we consider single events? |
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.
Yes. At the moment it is possible to have AOC in both Single Event and Tracker Event.
So I think we should handle it.
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.
I have a few comments like this in this PR.
Right now things are working because we have all the events in one table, but when we separate the table, we need to take care of those cases.
The question is quite rhetorical.
* @param request merge request | ||
*/ | ||
public void handleEventDataValues( | ||
List<DataElement> sources, DataElement target, MergeRequest request) { | ||
Set<UID> sourceDeUids = UID.of(sources.toArray(new DataElement[0])); | ||
DataMergeStrategy mergeStrategy = request.getDataMergeStrategy(); | ||
|
||
// TODO(DHIS2-19702): Should we consider single events |
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.
I think we should handle single events here as well. Since data elements could be either for Tracker or for Single Events.
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.
Will do in DHIS2-19702.
Right now things are working because there is only one event table.
migrate(request, "Enrollment", PARAM_ORG_UNIT); | ||
// TODO(DHIS2-19702): should we consider single events? |
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.
Yes.
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.
Will do in DHIS2-19702.
Right now things are working because there is only one event table.
@@ -237,12 +237,16 @@ public DataSummary getSystemStatisticsSummary() { | |||
dataValueCount.put(30, dataValueService.getDataValueCount(30)); | |||
statistics.setDataValueCount(dataValueCount); | |||
|
|||
// TODO(DHIS2-19702): Should we consider single events? |
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.
Yes. I would also suggest we can expand the data statistics response to include trackerEvent count and singleEvent count separately (in addition to setting the existing eventCount as the sum for backward compatibility).
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.
Will do in DHIS2-19702.
Right now things are working because there is only one event table.
We can have separate and aggregated statistics. Will do when we are separating tables.
@@ -401,7 +402,7 @@ public List<String> canCreate( | |||
|
|||
@Override | |||
public List<String> canUpdate( | |||
@Nonnull UserDetails user, Event event, boolean skipOwnershipCheck) { | |||
@Nonnull UserDetails user, TrackerEvent event, boolean skipOwnershipCheck) { |
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.
SingleEvent access check already handled separately? Then this method could be further simplified maybe
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.
Yes, there is already a canRead
for both types.
canUpdate
is only used when checking relationships, it will come in next PR where I am fixing relationshipItem
modeling.
|
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.
One thing that was not clear to me, but maybe already handled/planned to handle is ProgramMessage and ProgramNotification for Single Events. Maybe you already have it in your mind.
Yes, it is present in next step section in the description. |
Follow up of #21408 work.
Rename
Event
hibernate object toTrackerEvent
.At this moment we have
SingleEvent
andTrackerEvent
objects with respective mapping that are both pointing toevent
table.The PR is huge because
Event
object is used in a lot of places but there is not so much logic in the PR.Luckily, OpenAPI is still working the same because
view/Event
object was annotated with@OpenApi.Shared(name = "TrackerEvent")
and now with the renaming everything is just working the same way.Next steps
SingleEvent
andTrackerEvent
inRelationshipItem
and separate columns inrelationshipitem
table.SingleEvent
andTrackerEvent
inProgramNotificationInstance
and separate columns inprogramnotificationinstance
table.SingleEvent
andTrackerEvent
inProgramMessage
and separate columns inprogrammessage
table.eventchangelog
table tosingleeventchangelog
andtrackereventchangelog
tablesevent_notes
table tosingleevent_notes
andtrackerevent_notes
tablesevent
table totrackerevent
andsingleevent
and update all foreign keys and solve all addedTODO
s