Skip to content
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.

Sign up for GitHub

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

fix: image not shown when image url in notification payload #172

Merged
merged 2 commits into from
Feb 17, 2023
Merged

fix: image not shown when image url in notification payload #172

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7022b5c
fix image not shown
Shahroz16 Feb 17, 2023
6ec31dd
updated comment
Shahroz16 Feb 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions messagingpush/src/main/java/io/customer/messagingpush/CustomerIOPushNotificationHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,13 +168,11 @@ internal class CustomerIOPushNotificationHandler(
.setStyle(NotificationCompat.BigTextStyle().bigText(body))
tintColor?.let { color -> notificationBuilder.setColor(color) }

// This is custom logic to add images to a push notification. Some image URLs have not
// been able to display an image in a push. https://github.com/customerio/issues/issues/7293
// Instead, we recommend customers use `notification.image` in the FCM payload to bypass
// our logic and use the logic from FCM's SDK for image handling instead.
Comment on lines -171 to -174
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was added to help out customers in the past where they would not see an image for some image URLs (the ticket has some example URLs that failed).

That ticket was made a while ago so there is a chance that the customer issue has been resolved and our SDK is able to show images with all URLs now. If that is true, I am OK with removing this comment.

If our SDK still encounters problems with some image URLs, I am still OK with removing this comment as long as we create a new ticket for us to address this problem of increasing image URL compatibility in the SDK in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your concern is very valid Levi,

From LJ's response in the ticket

It ended up not being an issue with the link, but instead an issue with the payload they were using to test. @levibostian any reason to keep this image open?

and my testing, I tested the very same URL you mentioned in the ticket that didn't work,

It works:
Screenshot_1676666457

And for the reasons, that we want to push people to use Rich Push which is data only, hence I decided to remove the comment.

we create a new ticket for us to address this problem of increasing image URL compatibility in the SDK in the future.
Would you advise creating it now or when an issue occurs? because I noticed the PR that adds it just got merged and we didn't get this issue reported for months after we reworked the image handling in SDK and sorted all the edge cases. Happy to create a ticket, if its serves a purpose.

try {
// check for image
val notificationImage = bundle.getString(IMAGE_KEY)
// check for image in data and notification payload to cater for both simple and rich push
// data only payload (foreground and background)
// notification + data payload (foreground)
val notificationImage = bundle.getString(IMAGE_KEY) ?: remoteMessage.notification?.imageUrl?.toString()
if (notificationImage != null) {
addImage(notificationImage, notificationBuilder, body)
}
Expand Down