Logging empty push notifications #2986
Conversation
OrangeAndGreen
requested review from
Jignesh-dimagi,
avazirna,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThe changes update the Sequence Diagram(s)sequenceDiagram
participant S as CommCareFirebaseMessagingService
participant L as Logger
S->>S: Call showNotification(notificationTitle, notificationText, ...)
alt Notification title and text are empty/whitespace
S->>L: Logger.exception("Empty push notification encountered", throwable)
else Valid notification content
S->>S: Process and display notification
end
Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
221-224: Good implementation of empty notification logging.This change effectively addresses the PR objective by detecting and logging empty push notifications. The logging includes the action associated with the notification, which will help in understanding the circumstances under which these empty notifications occur.
Consider enhancing the logging by including more context from the payload (like channel ID, message ID, etc. if available) to further aid in debugging.
if (Strings.isEmptyOrWhitespace(notificationTitle) && Strings.isEmptyOrWhitespace(notificationText)) { - Logger.exception("Empty push notification", - new Throwable(String.format("Empty notification for action '%s'", action))); + Logger.exception("Empty push notification", + new Throwable(String.format("Empty notification for action '%s', payload: %s", + action, payloadData.toString()))); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java(3 hunks)
🔇 Additional comments (2)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (2)
18-18: Appropriate import for the new empty notification check.The addition of
Stringsimport from Google's common utilities is necessary for using theisEmptyOrWhitespacemethod in the new code.
236-236: Minor formatting improvement.The added space after the
ifkeyword improves code readability and maintains consistent style throughout the codebase.
|
Think we should also add similar validations on server side which fails the process to send notification if these fields are empty |
|
Agreed, I'll pass that on to the server team so they can prioritize it in. |
https://dimagi.atlassian.net/browse/CCCT-786
cross-request: dimagi/commcare-core#1455
Technical Summary
As shown in the ticket, users are sometimes shown "empty" push notifications, i.e. with no title or text. It seems this is possible if the server sends certain payloads, so this PR adds some logging to help us better understand more about when it happens.
Feature Flag
ConnectID Messaging
Safety Assurance
Safety story
Simple under-the-hood logging, very little risk
Automated test coverage
No automated tests for ConnectID yet.
QA Plan
No testing required or useful.
Labels and Review