Add title/message support and basic fallback for five icons push template#1003
Add title/message support and basic fallback for five icons push template#1003deeksha-rgb wants to merge 5 commits into
Conversation
WalkthroughFiveIconsTemplateData is restructured to consolidate deep links, background color, title, subtitle, and notification behavior into a baseContent field. TemplateDataFactory introduces toBasicTemplateData() conversion. TemplateRenderer adds validation with fallback to BasicTemplate for invalid or degraded templates. Content views, styles, and validators are updated to reference baseContent. Layout XMLs are restructured to include content view templates. ChangesFiveIcons Template Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
@CodeRabbit review. |
|
🧠 Learnings used✅ Action performedReview finished.
|
|
/vision-review-deep |
![clevertap-vision[bot]](https://avatars.githubusercontent.com/in/648995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Summary
This PR refactors FiveIconsTemplateData to embed a shared BaseContent (title, message, subtitle, colors, deep links, notification behavior) instead of duplicating those fields, wires title/message/subtitle into the five-icons layouts, and replaces the previous "suppress the notification" behavior with a graceful fallback to a basic title/message notification when the template is invalid or more than two icon bitmaps fail to load. The change is well-scoped, internally consistent with how BasicTemplateData/BigImageContentView already consume BaseContent, and the unit tests were updated to cover both the validation-failure and unloaded-icons fallback paths.
📊 Visual Overview
flowchart TD
A[renderNotification: FiveIconsTemplateData] --> B{Validator.validate}
B -- false --> F[buildBasicFallback]
B -- true --> C[Build FiveIconStyle\n+ download icons]
C --> D{unloaded icons > 2?}
D -- yes --> F
D -- no --> E[Return five-icon builder]
F --> G{Basic validation\ntitle && message ok?}
G -- yes --> H[Return basic builder]
G -- no --> I[Return null\nno notification]
Verdict
APPROVE
The refactor is clean and correct. createBaseContent carries over everything the old explicit fields provided (including notificationBehavior), the five-icons layouts now include content_view_small_multi_line_msg whose IDs (title, msg) plus the metadata include (app_name, timestamp, subtitle, sep_subtitle) match every setCustomContentView* call, RemoteViews silently skips actions for IDs missing from the layout-v31 variant, and the setSound/getCollapseKey/or() cleanups are behavior-preserving (and safer against ClassCastException). Only a few low-priority observations below — none blocking.
To retrigger this review, comment /vision-review-deep on the PR.
Reviewed by Vision AI
| } else { | ||
| fiveIconNotificationBuilder | ||
| PTLog.debug("Five Icons template validation failed, reverting to basic template.") | ||
| buildBasicFallback(templateData.toBasicTemplateData(), context, extras, notificationId, nb) |
There was a problem hiding this comment.
💡 Suggestion: buildBasicFallback runs the BASIC validator, which requires both a non-empty title and message (addBasicTextValidation → title + message checks). A five-icons payload that has a title but no pt_msg will therefore fail the fallback validation and buildBasicFallback returns null — i.e. no notification at all, the same outcome this PR set out to avoid. This is a real edge for existing five-icons campaigns (which historically only required deep links + images, not a message). Consider logging when the fallback itself can't build so the silent-drop is diagnosable, e.g.:
buildBasicFallback(...) ?: run {
PTLog.debug("Basic fallback also invalid (missing title/message); notification not rendered.")
null
}| data.baseContent.textData.subtitle, | ||
| data.baseContent.colorData.metaColor | ||
| ) | ||
| setCustomContentViewTitle(data.baseContent.textData.title) |
There was a problem hiding this comment.
🧹 Code Quality: The previous implementation fell back to Utils.getApplicationName(context) for the in-body title when the title was null/empty; that fallback is now gone. In practice the title resolves via getStringWithFallback(extras, PT_TITLE, NOTIF_TITLE) and the header still shows the app name via setCustomContentViewBasicKeys → R.id.app_name, so this is likely intentional alignment with BaseContent. Just confirming the dropped app-name body fallback is deliberate and not an oversight.
| val o = extras[Constants.WZRK_SOUND] | ||
| if (o is Boolean && o) { | ||
| val soundString = extras.getString(Constants.WZRK_SOUND) | ||
| val isDefaultSoundEnabled = extras.getBoolean(Constants.WZRK_SOUND, false) |
There was a problem hiding this comment.
🧹 Code Quality (minor): Calling both extras.getString(WZRK_SOUND) and extras.getBoolean(WZRK_SOUND, false) unconditionally means one of the two will always hit a Bundle type mismatch (the value is either a String or a Boolean, never both), emitting a benign Bundle warning to logcat. Functionally this is correct and safer than the old is Boolean/is String casts, but if log noise matters you could branch on extras.get(WZRK_SOUND)'s type first.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clevertap-pushtemplates/src/test/java/com/clevertap/android/pushtemplates/TemplateDataFactoryTest.kt (1)
616-618: ⚡ Quick winStrengthen fallback conversion assertions in this test.
Line 613 validates
toBasicTemplateData(), but the test should also lock down fallback invariants (actions == nulland empty media payload), since renderer fallback depends on this exact contract.Suggested test assertions
`@Test` fun `FiveIconsTemplateData_toBasicTemplateData should create correct BasicTemplateData`() { // Given val fiveIconsData = createSampleFiveIconsTemplateData() // When val basicData = with(TemplateDataFactory) { fiveIconsData.toBasicTemplateData() } // Then assertEquals(TemplateType.BASIC, basicData.templateType) assertEquals(fiveIconsData.baseContent, basicData.baseContent) + assertNull(basicData.actions) + assertNull(basicData.mediaData.bigImage.url) + assertEquals("", basicData.mediaData.bigImage.altText) + assertNull(basicData.mediaData.gif.url) + assertEquals(10, basicData.mediaData.gif.numberOfFrames) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clevertap-pushtemplates/src/test/java/com/clevertap/android/pushtemplates/TemplateDataFactoryTest.kt` around lines 616 - 618, The test verifying toBasicTemplateData() should also assert the fallback invariants required by the renderer: after calling toBasicTemplateData() on the subject (in TemplateDataFactoryTest.kt) add assertions that basicData.actions is null and that basicData.mediaPayload is empty (e.g., size == 0 or .isEmpty()) in addition to the existing checks for templateType and baseContent so the fallback contract is locked down.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/TemplateRenderer.kt`:
- Around line 138-164: The FiveIcons branch currently treats a null validator as
"invalid" and falls back to buildBasicFallback, but tests and other templates
expect a null validator to cause an immediate null return; change the logic in
TemplateRenderer.kt inside the is FiveIconsTemplateData branch to explicitly
handle ValidatorFactory.getValidator(templateData) == null by returning null
(i.e., mirror buildIfValid behavior) instead of executing the invalid-path
fallback; keep the existing validation check and image-unloaded fallback
(FiveIconStyle, fiveIconSmallContentView.getUnloadedFiveIconsCount(),
fiveIconBigContentView.getUnloadedFiveIconsCount(), fiveIconNotificationBuilder)
only when the validator is non-null.
---
Nitpick comments:
In
`@clevertap-pushtemplates/src/test/java/com/clevertap/android/pushtemplates/TemplateDataFactoryTest.kt`:
- Around line 616-618: The test verifying toBasicTemplateData() should also
assert the fallback invariants required by the renderer: after calling
toBasicTemplateData() on the subject (in TemplateDataFactoryTest.kt) add
assertions that basicData.actions is null and that basicData.mediaPayload is
empty (e.g., size == 0 or .isEmpty()) in addition to the existing checks for
templateType and baseContent so the fallback contract is locked down.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 860e81b9-c953-4dc0-b836-ea184211a864
📒 Files selected for processing (11)
clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/TemplateData.ktclevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/TemplateDataFactory.ktclevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/TemplateRenderer.ktclevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/content/FiveIconBigContentView.ktclevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/content/FiveIconSmallContentView.ktclevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/styles/FiveIconStyle.ktclevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/validators/ValidatorFactory.ktclevertap-pushtemplates/src/main/res/layout/five_cta_collapsed.xmlclevertap-pushtemplates/src/main/res/layout/five_cta_expanded.xmlclevertap-pushtemplates/src/test/java/com/clevertap/android/pushtemplates/TemplateDataFactoryTest.ktclevertap-pushtemplates/src/test/java/com/clevertap/android/pushtemplates/TemplateRendererTest.kt
| is FiveIconsTemplateData -> { | ||
| val validator = ValidatorFactory.getValidator(templateData) | ||
| if (validator?.validate() == true) { | ||
| val fiveIconStyle = FiveIconStyle(templateData, this, extras) | ||
| val fiveIconNotificationBuilder = fiveIconStyle.builderFromStyle( | ||
| context, | ||
| extras, | ||
| notificationId, | ||
| nb | ||
| ) | ||
|
|
||
| /** | ||
| * If most icon bitmaps fail to load, gracefully fall back to a basic | ||
| * title/message notification instead of suppressing the notification. | ||
| */ | ||
| if ((fiveIconStyle.fiveIconSmallContentView as FiveIconSmallContentView).getUnloadedFiveIconsCount() > 2 || | ||
| (fiveIconStyle.fiveIconBigContentView as FiveIconBigContentView).getUnloadedFiveIconsCount() > 2) { | ||
| PTLog.debug("More than 2 images were not retrieved in 5CTA Notification, reverting to basic template.") | ||
| buildBasicFallback(templateData.toBasicTemplateData(), context, extras, notificationId, nb) | ||
| } else { | ||
| fiveIconNotificationBuilder | ||
| } | ||
| } else { | ||
| fiveIconNotificationBuilder | ||
| PTLog.debug("Five Icons template validation failed, reverting to basic template.") | ||
| buildBasicFallback(templateData.toBasicTemplateData(), context, extras, notificationId, nb) | ||
| } | ||
| } |
There was a problem hiding this comment.
Verify behavior when FiveIcons validator is null.
When ValidatorFactory.getValidator(templateData) returns null, the condition at line 140 evaluates to false (since null == true is false), causing the code to fall through to the else branch at line 160-162 which calls buildBasicFallback. This differs from other templates where a null validator results in null being returned (via buildIfValid).
The test test_renderNotification_five_icons_template_null_validator expects null but doesn't mock the fallback path (toBasicTemplateData() and subsequent validator). Either the production code should explicitly handle the null validator case to return null, or the test needs to be updated to reflect the fallback behavior.
🐛 Proposed fix to handle null validator explicitly
is FiveIconsTemplateData -> {
val validator = ValidatorFactory.getValidator(templateData)
+ if (validator == null) {
+ PTLog.debug("Five Icons template validator is null, cannot render notification.")
+ null
+ } else if (validator.validate()) {
- if (validator?.validate() == true) {
val fiveIconStyle = FiveIconStyle(templateData, this, extras)
// ... rest of the code
} else {
PTLog.debug("Five Icons template validation failed, reverting to basic template.")
buildBasicFallback(templateData.toBasicTemplateData(), context, extras, notificationId, nb)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/TemplateRenderer.kt`
around lines 138 - 164, The FiveIcons branch currently treats a null validator
as "invalid" and falls back to buildBasicFallback, but tests and other templates
expect a null validator to cause an immediate null return; change the logic in
TemplateRenderer.kt inside the is FiveIconsTemplateData branch to explicitly
handle ValidatorFactory.getValidator(templateData) == null by returning null
(i.e., mirror buildIfValid behavior) instead of executing the invalid-path
fallback; keep the existing validation check and image-unloaded fallback
(FiveIconStyle, fiveIconSmallContentView.getUnloadedFiveIconsCount(),
fiveIconBigContentView.getUnloadedFiveIconsCount(), fiveIconNotificationBuilder)
only when the validator is non-null.
3 participants
Summary
baseContentTesting
TemplateDataFactoryTestTemplateRendererTestSummary by CodeRabbit
Bug Fixes
Chores