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.

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

Exposing kv pairs properties #219

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jasper-hsu-kyvo
Copy link

@jasper-hsu-kyvo jasper-hsu-kyvo commented Feb 7, 2025

Description

Added two new extension properties to RemoteMessages:

  1. hasKlaviyoKeyValuePairs which is a boolean (true if message payloads contain kv-pairs, false otherwise)
  2. keyValuePairs optional Map<String,String> which is the dictionary of kv-pairs itself (deserialized from json)

Check List

  • Are you changing anything with the public API?
    No.
  • Are your changes backwards compatible with previous SDK Versions?
  • Have you tested this change on real device?
  • Have you added unit test coverage for your changes?
  • Have you verified that your changes are compatible with all Android versions the SDK currently supports?
    How can I do this?

Changelog / Code Overview

  • Added extension properties for key-value pairs
  • Added new branching logic for silent pushes, and an overridable handler method

Test Plan

  • Tested on the test app - made standard and silent pushes to it, with and without key-value pairs, and then would use extension properties to access the data.
  • Also ensured that the log was being made (for the new conditional logic using the new onSilentPushMessageReceived method)

Related Issues/Tickets

* Determine if the message is bearing key-value pairs
*/
val RemoteMessage.hasKlaviyoKeyValuePairs: Boolean
get() = this.isKlaviyoMessage && (this.data[KlaviyoNotification.KEY_VALUE_PAIRS_KEY]?.let { true } ?: false)
Copy link
Contributor

@evan-masseau evan-masseau Feb 10, 2025

Choose a reason for hiding this comment

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

this.data.containsKey(KlaviyoNotification.KEY_VALUE_PAIRS_KEY) probs cleaner.

Copy link
Author

@jasper-hsu-kyvo jasper-hsu-kyvo Feb 10, 2025

Choose a reason for hiding this comment

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

Wouldn't I have to wrap everything in an if/else then too? (the let statement no longer works)

Copy link
Contributor

Choose a reason for hiding this comment

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

don't need the let i mean, it should just return a bool of whether that key is present

Copy link
Contributor

Choose a reason for hiding this comment

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

same as val RemoteMessage.isKlaviyoMessage: Boolean get() = this.data.containsKey("_k") elsewhere in this file

Copy link
Author

Choose a reason for hiding this comment

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

Oo oops I was looking at the wrong method* (RemoteMessage.keyValuePairs) this makes more sense

/**
* Parse out the key-value pairs into a string:string map
*/
val RemoteMessage.keyValuePairs: Map<String,String>?
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we have a gap in our test coverage, this file has no corresponding test file. Would you mind adding a file in the same dir as KlaviyoPushServiceTest to cover your changes, and we'll backfill tests for other stuff in the future.

@jasper-hsu-kyvo jasper-hsu-kyvo marked this pull request as ready for review February 10, 2025 19:08
@jasper-hsu-kyvo jasper-hsu-kyvo requested a review from a team as a code owner February 10, 2025 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants