fix: Filter out blank recipients for outbound messages [DHIS2-21836] - #25213
Open
enricocolasante wants to merge 2 commits into
Open
enricocolasante wants to merge 2 commits into
enricocolasante wants to merge 2 commits into
Conversation
enricocolasante
marked this pull request as ready for review
September 15, 2026 07:39
teleivo
reviewed
Sep 15, 2026
teleivo
left a comment
teleivo
left a comment
Contributor
There was a problem hiding this comment.
- Why do we even add empty recipients in the first place? If we would not have any we would not need to filter/watch out for them
- If we cannot keep empty recipients out of an OutboundMessage, then we should at least filter them out at that level later one. Right now you are checking if a OutboundMessageBatch has any OutboundMessage with at least one non-empty recipient. You can still have a scenario where the batch has messages with no non-empty recipient
|
Contributor
Author
You are right, we don't need the empty recipients at all. |
teleivo
reviewed
Sep 16, 2026
| // Iterate over a copy: a channel the resolved recipient cannot receive on (e.g. an org unit | ||
| // contact with an email but no phone number) is dropped from the message so that the remaining | ||
| // deliverable channels are still sent, instead of aborting the whole send. | ||
| // Iterate over a copy: a channel whose recipient cannot be resolved at all (e.g. a tracked |
Contributor
There was a problem hiding this comment.
this is getting pretty long, do we need all of this?
| @Override | ||
| public String getOrganisationUnitRecipient(OrganisationUnit orgUnit) { | ||
| if (orgUnit.getEmail() == null) { | ||
| throw new IllegalQueryException("Organisation unit does not have an email address"); |
Contributor
There was a problem hiding this comment.
Throwing did remove the undeliverable channel from the persisted deliveryChannels. This still happens when the recipient is a tracked entity missing the attribute but not anymore when its the orgUnit missing an email. Not sure what the contract is for the persisted deliveryChannels.
Contributor
|
Can this have the same problem you are trying to fix for TE/OU? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes [DHIS2-21836]: when a program notification is configured for multiple delivery channels (SMS and Email) but the organisation unit contact only has details for one of them, the send used to be all-or-nothing or could be misreported as failed. Delivery is now resolved best-effort per channel — a channel with no deliverable, non-blank recipient is quietly skipped instead of blocking or failing the channels that do have one.
Changes
EmailDeliveryChannelStrategy,SmsDeliveryChannelStrategy):getOrganisationUnitRecipientis removed;setAttributesnow adds the org unit's email/phone directly, guarded byStringUtils.isNotBlank(...)so a blank string is treated the same as a missing one, not justnull.DeliveryChannelStrategy's corresponding abstract method is removed to match.EmailMessageBatchCreatorService,SmsMessageBatchCreatorService): a message with no resolved recipient for that channel is filtered out before anOutboundMessageis built, so no batch is ever created for it — it's discarded, not sent, nor reported asFAILED. Filtering now logs awarn(with the program message UID) so a discarded message still leaves a trace to diagnose why a notification wasn't delivered on a given channel.DefaultOutboundMessageBatchService: reverted to its original form — the earlier attempt at filtering "no recipient" batches at send time was removed since that responsibility now lives upstream in the batch creators (see above).DefaultProgramMessageService: updated the comment onsetAttributesBasedOnStrategyto reflect current behavior — the per-channel drop-on-exception path is now only for cases where a recipient can't be resolved at all (e.g. a tracked entity missing the required attribute type), not for an org unit simply missing one channel's contact detail.