fix: Clean up notification effect [DHIS2-17243] #17936
Conversation
...his-service-program-rule/src/main/java/org/hisp/dhis/programrule/api/NotificationEffect.java
Outdated
Show resolved
Hide resolved
...his-service-program-rule/src/main/java/org/hisp/dhis/programrule/api/NotificationEffect.java
Outdated
Show resolved
Hide resolved
...his-service-program-rule/src/main/java/org/hisp/dhis/programrule/api/NotificationEffect.java
Outdated
Show resolved
Hide resolved
...dhis-service-program-rule/src/main/java/org/hisp/dhis/programrule/api/RuleEngineEffects.java
Outdated
Show resolved
Hide resolved
...dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/job/NotificationSender.java
Outdated
Show resolved
Hide resolved
|
|
||
| programNotificationInstanceService.save(notificationInstance); | ||
|
|
||
| log.info(String.format(LOG_MESSAGE, template.getUid())); |
There was a problem hiding this comment.
Logging should be done using parameterized messages https://www.slf4j.org/faq.html#logging_performance
Should that be an info log? To me this is debug. We log to much IMHO. This is why we have the logging group combing through logs 😅
Ask yourself who would want to see this and what should they do with it? Logs should be actionable by the operator of DHIS2.
There was a problem hiding this comment.
@enricocolasante I think we should still address this one
There was a problem hiding this comment.
I just removed it.
I think it didn't really make any sense to log a scheduled notification and not the sent one.
Scheduled notifications are saved somewhere in the system. If there is any need to debug it will be better to check the DB or the endpoint if we have one to understand what happened
...dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/job/NotificationSender.java
Outdated
Show resolved
Hide resolved
...dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/job/NotificationSender.java
Outdated
Show resolved
Hide resolved
...dhis-service-program-rule/src/main/java/org/hisp/dhis/programrule/api/RuleEngineEffects.java
Outdated
Show resolved
Hide resolved
...service-tracker/src/main/java/org/hisp/dhis/tracker/imports/DefaultTrackerImportService.java
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -92,7 +93,7 @@ | |||
| return entityReport.stream().flatMap(e -> e.getErrorReports().stream()).toList(); | |||
| } | |||
|
|
|||
| public List<TrackerSideEffectDataBundle> getSideEffectDataBundles() { | |||
| return sideEffectDataBundles; | |||
| public List<TrackerNotificationDataBundle> getNotificationDataBundles() { | |||
Check notice
Code scanning / CodeQL
Exposing internal representation Note
|
Notification effects can be simplified and shaped in a better way to be handled in tracker.
NotificationEffectto hold all and only the relevant information to correctly handle notifications. Removed the type and ruleUid.RuleActionScheduleMessageImplementerandRuleActionSendMessageImplementerand createNotificationSenderthat will send the notification if no date is provided or schedule the notification on the provided date.